[FFmpeg-devel] [PATCH 1/3] avcodec/decode: move processing discard samples to its own function

[James Almer]

On 7/9/2023 6:48 AM, Anton Khirnov wrote:
> Quoting James Almer (2023-07-08 21:00:36)
>> -/*
>> - * The core of the receive_frame_wrapper for the decoders implementing
>> - * the simple API. Certain decoders might consume partial packets without
>> - * returning any output, so this function needs to be called in a loop until it
>> - * returns EAGAIN.
>> - **/
>> -static inline int decode_simple_internal(AVCodecContext *avctx, AVFrame *frame, int64_t *discarded_samples)
>> +static int discard_samples(AVCodecContext *avctx, AVFrame *frame, int64_t *discarded_samples)
>>   {
>> -    AVCodecInternal   *avci = avctx->internal;
>> -    AVPacket     *const pkt = avci->in_pkt;
>> -    const FFCodec *const codec = ffcodec(avctx->codec);
>> -    int got_frame, actual_got_frame;
>> -    int ret;
>> -
>> -    if (!pkt->data && !avci->draining) {
>> -        av_packet_unref(pkt);
>> -        ret = ff_decode_get_packet(avctx, pkt);
>> -        if (ret < 0 && ret != AVERROR_EOF)
>> -            return ret;
>> -    }
>> -
>> -    // Some codecs (at least wma lossless) will crash when feeding drain packets
>> -    // after EOF was signaled.
>> -    if (avci->draining_done)
>> -        return AVERROR_EOF;
>> -
>> -    if (!pkt->data &&
>> -        !(avctx->codec->capabilities & AV_CODEC_CAP_DELAY ||
>> -          avctx->active_thread_type & FF_THREAD_FRAME))
>> -        return AVERROR_EOF;
>> -
>> -    got_frame = 0;
>> -
>> -    if (HAVE_THREADS && avctx->active_thread_type & FF_THREAD_FRAME) {
>> -        ret = ff_thread_decode_frame(avctx, frame, &got_frame, pkt);
>> -    } else {
>> -        ret = codec->cb.decode(avctx, frame, &got_frame, pkt);
>> -
>> -        if (!(codec->caps_internal & FF_CODEC_CAP_SETS_PKT_DTS))
>> -            frame->pkt_dts = pkt->dts;
>> -        if (avctx->codec->type == AVMEDIA_TYPE_VIDEO) {
>> -#if FF_API_FRAME_PKT
>> -FF_DISABLE_DEPRECATION_WARNINGS
>> -            if(!avctx->has_b_frames)
>> -                frame->pkt_pos = pkt->pos;
>> -FF_ENABLE_DEPRECATION_WARNINGS
>> -#endif
>> -            //FIXME these should be under if(!avctx->has_b_frames)
>> -            /* get_buffer is supposed to set frame parameters */
>> -            if (!(avctx->codec->capabilities & AV_CODEC_CAP_DR1)) {
>> -                if (!frame->sample_aspect_ratio.num)  frame->sample_aspect_ratio = avctx->sample_aspect_ratio;
>> -                if (!frame->width)                    frame->width               = avctx->width;
>> -                if (!frame->height)                   frame->height              = avctx->height;
>> -                if (frame->format == AV_PIX_FMT_NONE) frame->format              = avctx->pix_fmt;
>> -            }
>> -        }
>> -    }
>> -    emms_c();
>> -    actual_got_frame = got_frame;
>> +    AVCodecInternal *avci = avctx->internal;
>> +    int ret = 0;
>>   
>>       if (avctx->codec->type == AVMEDIA_TYPE_VIDEO) {
>>           if (frame->flags & AV_FRAME_FLAG_DISCARD)
>> -            got_frame = 0;
>> +            ret = AVERROR(EAGAIN);
> 
> It's quite strange an unexpected for a function called discard_samples()
> to do anything with video. By leaving video processing in the caller you
> also save a level of indentation.

I could call it discard_samples_or_frame(), or discard_frame().
If i process the video in the caller, it will end up being duplicated in 
patch 2, which goes against the point of factoring this out.

> 
>>       } else if (avctx->codec->type == AVMEDIA_TYPE_AUDIO) {
>>           uint8_t *side;
>>           size_t side_size;
>> @@ -359,16 +304,10 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>           uint8_t skip_reason = 0;
>>           uint8_t discard_reason = 0;
>>   
>> -        if (ret >= 0 && got_frame) {
>>               if (frame->format == AV_SAMPLE_FMT_NONE)
>>                   frame->format = avctx->sample_fmt;
>> -            if (!frame->ch_layout.nb_channels) {
>> -                int ret2 = av_channel_layout_copy(&frame->ch_layout, &avctx->ch_layout);
>> -                if (ret2 < 0) {
>> -                    ret = ret2;
>> -                    got_frame = 0;
>> -                }
>> -            }
>> +            if (!frame->ch_layout.nb_channels)
>> +                ret = av_channel_layout_copy(&frame->ch_layout, &avctx->ch_layout);
>>   #if FF_API_OLD_CHANNEL_LAYOUT
>>   FF_DISABLE_DEPRECATION_WARNINGS
>>               if (!frame->channel_layout)
>> @@ -380,7 +319,6 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>   #endif
>>               if (!frame->sample_rate)
>>                   frame->sample_rate = avctx->sample_rate;
>> -        }
> 
> So the rest of the code below was previously executed even when there is
> no frame? The change seems correct, but not I wouldn't expect it in a
> 'move' commit, so perhaps mention it in the commit message.

It still is, at least the block immediately after this, exporting 
avci->skip_samples. I don't know how correct it is to do that, but i did 
not want to change behavior in this set for decode() API decoders.

> 
>>   
>>           side= av_packet_get_side_data(avci->last_pkt_props, AV_PKT_DATA_SKIP_SAMPLES, &side_size);
>>           if(side && side_size>=10) {
>> @@ -393,21 +331,21 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>               discard_reason = AV_RL8(side + 9);
>>           }
>>   
>> -        if ((frame->flags & AV_FRAME_FLAG_DISCARD) && got_frame &&
>> +        if ((frame->flags & AV_FRAME_FLAG_DISCARD) && !ret &&
>>               !(avctx->flags2 & AV_CODEC_FLAG2_SKIP_MANUAL)) {
>>               avci->skip_samples = FFMAX(0, avci->skip_samples - frame->nb_samples);
>> -            got_frame = 0;
>>               *discarded_samples += frame->nb_samples;
>> +            ret = AVERROR(EAGAIN);
> 
> If I'm reading correctly, all the blocks below are conditioned on !ret,
> so you might as well return here and drop those checks.

Ok.