[FFmpeg-devel] [PATCH] avutil/frame: use bitfields for some boolean and enum fields

[Lynne]

Apr 17, 2023, 19:27 by jamrial at gmail.com:

> On 4/17/2023 2:25 PM, Lynne wrote:
>
>> Apr 17, 2023, 17:15 by jamrial at gmail.com:
>>
>>> On 4/17/2023 12:00 PM, Lynne wrote:
>>>
>>>> Apr 17, 2023, 16:40 by jamrial at gmail.com:
>>>>
>>>>> Should reduce the size of AVFrame in the next major bump without changing the API.
>>>>>
>>>>> Suggested-by: Anton Khirnov <anton at khirnov.net>
>>>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>>>> ---
>>>>> This supersedes "avutil/frame: add new interlaced and top_field_first flags"
>>>>> and "avutil/frame: add a keyframe flag to AVFrame".
>>>>>
>>>>>  libavutil/frame.h   | 56 +++++++++++++++++++++++++++++++++++++++++++++
>>>>>  libavutil/version.h |  1 +
>>>>>  2 files changed, 57 insertions(+)
>>>>>
>>>>> diff --git a/libavutil/frame.h b/libavutil/frame.h
>>>>> index f85d630c5c..c26067f383 100644
>>>>> --- a/libavutil/frame.h
>>>>> +++ b/libavutil/frame.h
>>>>> @@ -416,6 +416,7 @@ typedef struct AVFrame {
>>>>>  */
>>>>>  int format;
>>>>>  +#if FF_API_BITFIELDS
>>>>>  /**
>>>>>  * 1 -> keyframe, 0-> not
>>>>>  */
>>>>> @@ -425,6 +426,57 @@ typedef struct AVFrame {
>>>>>  * Picture type of the frame.
>>>>>  */
>>>>>  enum AVPictureType pict_type;
>>>>> +#else
>>>>> +    /**
>>>>> +     * 1 -> keyframe, 0-> not
>>>>> +     */
>>>>> +    unsigned int key_frame: 1;
>>>>> +
>>>>> +    /**
>>>>> +     * The content of the picture is interlaced.
>>>>> +     */
>>>>> +    unsigned int interlaced_frame: 1;
>>>>> +
>>>>> +    /**
>>>>> +     * If the content is interlaced, is top field displayed first.
>>>>> +     */
>>>>> +    unsigned int top_field_first: 1;
>>>>> +
>>>>> +    /**
>>>>> +     * Tell user application that palette has changed from previous frame.
>>>>> +     */
>>>>> +    unsigned int palette_has_changed: 1;
>>>>> +
>>>>> +    /**
>>>>> +     * Reserved. Must not be touched.
>>>>> +     */
>>>>> +    unsigned int reserved_bitfield: (sizeof(unsigned int) * 8) - 9;
>>>>> +
>>>>> +    /**
>>>>> +     * MPEG vs JPEG YUV range.
>>>>> +     * - encoding: Set by user
>>>>> +     * - decoding: Set by libavcodec
>>>>> +     */
>>>>> +    enum AVColorRange color_range: 2;
>>>>> +
>>>>> +    enum AVChromaLocation chroma_location: 3;
>>>>>
>>>>
>>>> Definitely disagree on all non-8bit field limits.
>>>> The reserved_bitfield is especially ugly.
>>>> A few wasted bits wouldn't affect much, we don't even support building on 6502s.
>>>> Use bools, or limit them to 8bits so we can use bools when we bump?
>>>> The rest can be limited to 8bits.
>>>>
>>>
>>> I added reserved_bitfield to turn what otherwise will be compiler-injected padding into something that can be reused if we were to add new fields here instead of at the end of the struct. I can remove it if you prefer, and make color_range and chroma_location into :8.
>>> Like i told you on IRC, i want to keep these as enum and not change their type to uint8_t, _Bool, or anything like that. Also, i wouldn't be surprised if using _Bool breaks some old weird compilers.
>>>
>>> With this change, I'm replacing 40 bytes worth of fields with 8 bytes worth of fields with no API break.
>>>
>>
>> I'm fine with enums staying as enums, and limiting them to 8 bits.
>> I'm not fine with limiting flags to 1 bit or 2/3 bits. I'd like for them to
>> be limited to 8 bits, and changing their type to bool or uint8_t at the
>> bump. You're still saving at least 21 bytes.
>>
>
> But why use 8 bits when we can use 1?
>

No need for explicit padding. They're able to be set via pointers, simplifying FFI.
They're faster to access. We could need to extend color_range and chroma_location
in the future. It's not worth saving a few tens of bits in a large struct that's going
to have to grow larger if we want to support e.g. subtitles in avframes.