Skip to content

Conversation

@daveisfera
Copy link
Contributor

No description provided.

@WyattBlue
Copy link
Member

Issues from least to most important:

  1. If key_frame is going to be settable, it should be settable in the base Frame class.
  2. I'm not sure why None is a special case.
  3. I think this implementation wouldn't work for many video formats
  4. The user could just modify the VideoFrame flags themselves, they don't need a special method

@daveisfera
Copy link
Contributor Author

daveisfera commented Jul 22, 2025

  1. If key_frame is going to be settable, it should be settable in the base Frame class.

I could move this to the base class

  1. I'm not sure why None is a special case.

I was just copying the other setters, but could easily treat None like False

  1. I think this implementation wouldn't work for many video formats

I've only really worked with the H.264 and HEVC code in ffmpeg, so what specifically wouldn't work with many video formats? Isn't flags and pict_type how the key frame information is communicated?

  1. The user could just modify the VideoFrame flags themselves, they don't need a special method

I'm still using 14.4 and it looks like 15 was a pretty big re-structure, but I couldn't access access flags directly when I was testing things. Also, I assumed that have a single property to handle the separate pieces that are implied with key_frame = True was a nice convenience, but I'm glad to directly handle that if it's the better solution.

@WyattBlue
Copy link
Member

Superseded by #1965

@WyattBlue WyattBlue closed this Jul 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants