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

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

- Andreas