[FFmpeg-devel] [PATCH 4/5] avcodec/encode: Set AV_PKT_FLAG_KEY based upon AV_CODEC_PROP_INTRA_ONLY

[James Almer]

On 9/21/2021 7:18 PM, Andreas Rheinhardt wrote:
> Currently, the AV_PKT_FLAG_KEY is automatically set for audio encoders;
> yet this is wrong, as both MLP and TrueHD have non-keyframes. So set it
> based upon AV_CODEC_PROP_INTRA_ONLY (from the corresponding
> AVCodecDescriptor) instead. This also sets it for some video codecs,
> which is intended.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
> ---
> I was surprised that this did not necessitate FATE changes.
> 
> Btw: AVCodecContext.codec_descriptor is always set by avcodec_open2(),
> yet marked as unused for encoding in its doxy. Shall I make document
> the current behaviour?
> 
>   libavcodec/encode.c   | 7 +++----
>   libavcodec/internal.h | 7 +++++++
>   2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/libavcodec/encode.c b/libavcodec/encode.c
> index 98dfbfdff3..dd25cf999b 100644
> --- a/libavcodec/encode.c
> +++ b/libavcodec/encode.c
> @@ -235,12 +235,9 @@ static int encode_simple_internal(AVCodecContext *avctx, AVPacket *avpkt)
>               }
>           }
>           if (avctx->codec->type == AVMEDIA_TYPE_AUDIO) {
> -            /* NOTE: if we add any audio encoders which output non-keyframe packets,
> -             *       this needs to be moved to the encoders, but for now we can do it
> -             *       here to simplify things */
> -            avpkt->flags |= AV_PKT_FLAG_KEY;
>               avpkt->dts = avpkt->pts;
>           }
> +        avpkt->flags |= avci->intra_only_flag;

You could do the check you added to ff_encode_preinit() here, inside the 
audio media type check, instead of adding another single use field to 
AVCodecInternal.

>       }
>   
>       if (avci->draining && !got_packet)
> @@ -553,6 +550,8 @@ int ff_encode_preinit(AVCodecContext *avctx)
>           }
>           avctx->sw_pix_fmt = frames_ctx->sw_format;
>       }
> +    if (avctx->codec_descriptor->props & AV_CODEC_PROP_INTRA_ONLY)

fwiw, the doxy for AVCodecContext says codec_descriptor is unused for 
encoding, but i see it's set unconditionally in avcodec_open2(). Should 
the doxy be amended?

It also kinda feels like a superfluous field. Anyone can do 
avcodec_descriptor_get(avctx->codec_id) if required.

> +        avctx->internal->intra_only_flag = AV_PKT_FLAG_KEY;
>   
>       return 0;
>   }
> diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> index dc60e4bf08..8df622968c 100644
> --- a/libavcodec/internal.h
> +++ b/libavcodec/internal.h
> @@ -155,6 +155,13 @@ typedef struct AVCodecInternal {
>       uint8_t *byte_buffer;
>       unsigned int byte_buffer_size;
>   
> +    /**
> +     * This is set to AV_PKT_FLAG_KEY for encoders that encode intra-only
> +     * formats (i.e. whose codec descriptor has AV_CODEC_PROP_INTRA_ONLY set).
> +     * This is used to set said flag generically for said encoders.
> +     */
> +    int intra_only_flag;
> +
>       void *frame_thread_encoder;
>   
>       EncodeSimpleContext es;
>