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

[Andreas Rheinhardt]

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

That would add a dereference and a branch more for every packet; the
above avoids this.

>>       }
>>         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?
> 

You overlooked my above comment, I presume.

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

Agreed.

>> +        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;
>>
>