| Message ID | 20230417143930.1186-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avutil/frame: use bitfields for some boolean and enum fields | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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.
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.
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
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.
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?
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.
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) /** * @}
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(+)