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

[James Almer <jamrial@gmail.com>]

On 4/17/2023 12:00 PM, Lynne wrote:
> Apr 17, 2023, 16:40 by jamrial@gmail.com:
> 
>> Should reduce the size of AVFrame in the next major bump without changing the API.
>>
>> Suggested-by: Anton Khirnov <anton@khirnov.net>
>> Signed-off-by: James Almer <jamrial@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".