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

[Anton Khirnov <anton@khirnov.net>]

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.

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

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

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".