[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:35 PM, Andreas Rheinhardt wrote:
> 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.

I don't consider this a problem, but doing so would make patch 5/5 not 
possible, so i guess it's fine as is.

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

Ah, I did, yes. My bad.

> 
>> 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;
>>>
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>