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

[Anton Khirnov <anton@khirnov.net>]

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