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

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

[James Almer]

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;
+
+    /**
+     * Picture type of the frame.
+     */
+    enum AVPictureType pict_type: 8;
+
+    enum AVColorPrimaries color_primaries: 8;
+
+    enum AVColorTransferCharacteristic color_trc: 8;
+
+    /**
+     * YUV colorspace type.
+     * - encoding: Set by user
+     * - decoding: Set by libavcodec
+     */
+    enum AVColorSpace colorspace: 8;
+#endif
 
     /**
      * Sample aspect ratio for the video frame, 0/1 if unknown/unspecified.
@@ -491,6 +543,7 @@ typedef struct AVFrame {
      */
     int repeat_pict;
 
+#if FF_API_BITFIELDS
     /**
      * The content of the picture is interlaced.
      */
@@ -505,6 +558,7 @@ typedef struct AVFrame {
      * Tell user application that palette has changed from previous frame.
      */
     int palette_has_changed;
+#endif
 
 #if FF_API_REORDERED_OPAQUE
     /**
@@ -595,6 +649,7 @@ typedef struct AVFrame {
      */
     int flags;
 
+#if FF_API_BITFIELDS
     /**
      * MPEG vs JPEG YUV range.
      * - encoding: Set by user
@@ -614,6 +669,7 @@ typedef struct AVFrame {
     enum AVColorSpace colorspace;
 
     enum AVChromaLocation chroma_location;
+#endif
 
     /**
      * frame timestamp estimated using various heuristics, in stream time base
diff --git a/libavutil/version.h b/libavutil/version.h
index 40f92af055..23cad31b46 100644
--- a/libavutil/version.h
+++ b/libavutil/version.h
@@ -115,6 +115,7 @@
 #define FF_API_FRAME_PICTURE_NUMBER     (LIBAVUTIL_VERSION_MAJOR < 59)
 #define FF_API_HDR_VIVID_THREE_SPLINE   (LIBAVUTIL_VERSION_MAJOR < 59)
 #define FF_API_FRAME_PKT                (LIBAVUTIL_VERSION_MAJOR < 59)
+#define FF_API_BITFIELDS                (LIBAVUTIL_VERSION_MAJOR < 59)
 
 /**
  * @}
-- 
2.40.0

_______________________________________________
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".

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

[Lynne]

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.
_______________________________________________
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".

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

[James Almer]

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".

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

[Lynne]

Apr 17, 2023, 17:15 by 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.
>

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.
_______________________________________________
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".

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

[James Almer]

On 4/17/2023 2:25 PM, Lynne wrote:
> Apr 17, 2023, 17:15 by 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.
>>
> 
> 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?
_______________________________________________
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".

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

[Lynne]

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

> On 4/17/2023 2:25 PM, Lynne wrote:
>
>> Apr 17, 2023, 17:15 by 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.
>>>
>>
>> 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.
_______________________________________________
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".

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

[Anton Khirnov]

Quoting Lynne (2023-04-17 17:00:27)
> 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'm not seeing any arguments for what exactly is improved by wasting
almost 90% of bits in those fields. "Ugly" is not an argument, this is
not a beauty pageant.

The patch looks generally good, except for a few remarks:
* why not merge the color bitfields with the interlacing ones
* weren't we going to deprecate palette_has_changed? there's no use case
  for it.
* only one free value for AVColorRange and AVChromaLocation, yet
  pict_type is 8 bits out of which most are unused and that doesn't seem
  likely to change; I'd leave space to double the currently defined
  range for each of those

-- 
Anton Khirnov
_______________________________________________
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".