[FFmpeg-devel] [PATCH 3/4] avcodec/decode: Set KEY flag+pict_type generically for intra-only codecs

[Tomas Härdin]

mån 2024-05-13 klockan 10:54 +0200 skrev Andreas Rheinhardt:
> Tomas Härdin:
> > tor 2024-05-09 klockan 04:04 +0200 skrev Andreas Rheinhardt:
> > > This commit is the analog of
> > > 3f11eac75741888c7b2b6f93c458766f2613bab5
> > > for decoding: It sets the AV_FRAME_FLAG_KEY and (for video
> > > decoders)
> > > also pict_type to AV_PICTURE_TYPE_I. It furthermore stops setting
> > > audio frames as always being key frames -- it is wrong for e.g.
> > > TrueHD/MLP. The latter also affects TAK and DFPWM.
> > > 
> > > The change already improves output for several decoders where
> > > it has been forgotten to set e.g. pict_type like speedhq, wnv1
> > > or tiff. The latter is the reason for the change to the exif-
> > > image-
> > > tiff
> > > FATE test reference file.
> > > 
> > > Signed-off-by: Andreas Rheinhardt
> > > <andreas.rheinhardt at outlook.com>
> > > ---
> > >  libavcodec/decode.c            | 29 +++++++++++++++++++++++++++-
> > > -
> > >  libavcodec/pthread_frame.c     | 17 ++++++++++++++---
> > >  tests/ref/fate/exif-image-tiff |  2 +-
> > >  3 files changed, 42 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> > > index d031b1ca17..0ca5344ef5 100644
> > > --- a/libavcodec/decode.c
> > > +++ b/libavcodec/decode.c
> > > @@ -57,6 +57,20 @@
> > >  typedef struct DecodeContext {
> > >      AVCodecInternal avci;
> > >  
> > > +    /**
> > > +     * This is set to AV_FRAME_FLAG_KEY for decoders of intra-
> > > only
> > > formats
> > > +     * (those whose codec descriptor has
> > > AV_CODEC_PROP_INTRA_ONLY
> > > set)
> > > +     * to set the flag generically.
> > > +     */
> > > +    int intra_only_flag;
> > > +
> > > +    /**
> > > +     * This is set to AV_PICTURE_TYPE_I for intra only video
> > > decoders
> > > +     * and to AV_PICTURE_TYPE_NONE for other decoders. It is
> > > used to
> > > set
> > > +     * the AVFrame's pict_type before the decoder receives it.
> > > +     */
> > > +    enum AVPictureType initial_pict_type;
> > 
> > Carrying this around as state seems unnecessary when a small static
> > function could do the same?
> > 
> 
> The aim of this is to avoid branches for every frame.

Checking a single value that will likely reside in cache once per frame
is hardly expensive, especially when the branch predictor will predict
it correctly.

When you carry state around you're carrying around multiple sources of
truth, which is something that tends to cause bugs..

/Tomas