[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 17:55 +0200 skrev Andreas Rheinhardt:
> 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..
> > 
> 
> Given that this is immutable after init, it is not really "state".

State tends to live in memory..

Perhaps this borders on bikeshedding, but I have said my peace and look
forward to cashing in accrued I-told-you-so's in the future should my
fears come to pass.

/Tomas