| Message ID | 20230708190038.24324-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avcodec/decode: move processing discard samples to its own function | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
James Almer: > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/decode.c | 182 +++++++++++++++++++++++--------------------- > 1 file changed, 94 insertions(+), 88 deletions(-) > > diff --git a/libavcodec/decode.c b/libavcodec/decode.c > index 269633ce10..793ab975f6 100644 > --- a/libavcodec/decode.c > +++ b/libavcodec/decode.c > @@ -289,69 +289,14 @@ static int64_t guess_correct_pts(AVCodecContext *ctx, > return pts; > } > > -/* > - * 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); > } 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; > - } > > 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 (avci->skip_samples > 0 && got_frame && > + if (avci->skip_samples > 0 && !ret && > !(avctx->flags2 & AV_CODEC_FLAG2_SKIP_MANUAL)) { > if(frame->nb_samples <= avci->skip_samples){ > - got_frame = 0; > *discarded_samples += frame->nb_samples; > avci->skip_samples -= frame->nb_samples; > av_log(avctx, AV_LOG_DEBUG, "skip whole frame, skip left: %d\n", > avci->skip_samples); > + ret = AVERROR(EAGAIN); > } else { > av_samples_copy(frame->extended_data, frame->extended_data, 0, avci->skip_samples, > frame->nb_samples - avci->skip_samples, avctx->ch_layout.nb_channels, frame->format); > @@ -432,11 +370,11 @@ FF_ENABLE_DEPRECATION_WARNINGS > } > } > > - if (discard_padding > 0 && discard_padding <= frame->nb_samples && got_frame && > + if (discard_padding > 0 && discard_padding <= frame->nb_samples && !ret && > !(avctx->flags2 & AV_CODEC_FLAG2_SKIP_MANUAL)) { > if (discard_padding == frame->nb_samples) { > *discarded_samples += frame->nb_samples; > - got_frame = 0; > + ret = AVERROR(EAGAIN); > } else { > if(avctx->pkt_timebase.num && avctx->sample_rate) { > int64_t diff_ts = av_rescale_q(frame->nb_samples - discard_padding, > @@ -452,7 +390,7 @@ FF_ENABLE_DEPRECATION_WARNINGS > } > } > > - if ((avctx->flags2 & AV_CODEC_FLAG2_SKIP_MANUAL) && got_frame) { > + if ((avctx->flags2 & AV_CODEC_FLAG2_SKIP_MANUAL) && !ret) { > AVFrameSideData *fside = av_frame_new_side_data(frame, AV_FRAME_DATA_SKIP_SAMPLES, 10); > if (fside) { > AV_WL32(fside->data, avci->skip_samples); > @@ -464,15 +402,88 @@ FF_ENABLE_DEPRECATION_WARNINGS > } > } > > - if (!got_frame) > + return ret; > +} > + > +/* > + * 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) > +{ > + AVCodecInternal *avci = avctx->internal; > + AVPacket *const pkt = avci->in_pkt; > + const FFCodec *const codec = ffcodec(avctx->codec); > + int got_frame; > + int consumed, 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) { > + consumed = ff_thread_decode_frame(avctx, frame, &got_frame, pkt); > + } else { > + consumed = 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(); > + > + if (got_frame) > + ret = discard_samples(avctx, frame, discarded_samples); > + else > + ret = AVERROR(EAGAIN); > + > + if (consumed < 0) > + ret = consumed; > + if (consumed >= 0 && avctx->codec->type == AVMEDIA_TYPE_VIDEO) > + consumed = pkt->size; > + > + if (!got_frame || ret == AVERROR(EAGAIN)) > av_frame_unref(frame); > > - if (ret >= 0 && avctx->codec->type == AVMEDIA_TYPE_VIDEO) > - ret = pkt->size; > + if (!ret) > + av_assert0(frame->buf[0]); > + if (ret == AVERROR(EAGAIN)) > + ret = 0; > > - /* do not stop draining when actual_got_frame != 0 or ret < 0 */ > - /* got_frame == 0 but actual_got_frame != 0 when frame is discarded */ > - if (avci->draining && !actual_got_frame) { > + /* do not stop draining when got_frame != 0 or ret < 0 */ > + if (avci->draining && !got_frame) { > if (ret < 0) { > /* prevent infinite loop if a decoder wrongly always return error on draining */ > /* reasonable nb_errors_max = maximum b frames + thread count */ > @@ -490,11 +501,9 @@ FF_ENABLE_DEPRECATION_WARNINGS > } > } > > - if (ret >= pkt->size || ret < 0) { > + if (consumed >= pkt->size || ret < 0) { > av_packet_unref(pkt); > } else { > - int consumed = ret; > - > pkt->data += consumed; > pkt->size -= consumed; > pkt->pts = AV_NOPTS_VALUE; > @@ -509,10 +518,7 @@ FF_ENABLE_DEPRECATION_WARNINGS > } > } > > - if (got_frame) > - av_assert0(frame->buf[0]); > - > - return ret < 0 ? ret : 0; > + return ret; > } > > #if CONFIG_LCMS2 Would using --diff-algorithm=minimal (or patience or histogram) produce a more readable diff (i.e. one where the code that ends up in discard_samples() is moved instead of everything except it)? - Andreas
On 7/8/2023 4:38 PM, Andreas Rheinhardt wrote: > James Almer: >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/decode.c | 182 +++++++++++++++++++++++--------------------- >> 1 file changed, 94 insertions(+), 88 deletions(-) >> >> diff --git a/libavcodec/decode.c b/libavcodec/decode.c >> index 269633ce10..793ab975f6 100644 >> --- a/libavcodec/decode.c >> +++ b/libavcodec/decode.c >> @@ -289,69 +289,14 @@ static int64_t guess_correct_pts(AVCodecContext *ctx, >> return pts; >> } >> >> -/* >> - * 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); >> } 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; >> - } >> >> 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 (avci->skip_samples > 0 && got_frame && >> + if (avci->skip_samples > 0 && !ret && >> !(avctx->flags2 & AV_CODEC_FLAG2_SKIP_MANUAL)) { >> if(frame->nb_samples <= avci->skip_samples){ >> - got_frame = 0; >> *discarded_samples += frame->nb_samples; >> avci->skip_samples -= frame->nb_samples; >> av_log(avctx, AV_LOG_DEBUG, "skip whole frame, skip left: %d\n", >> avci->skip_samples); >> + ret = AVERROR(EAGAIN); >> } else { >> av_samples_copy(frame->extended_data, frame->extended_data, 0, avci->skip_samples, >> frame->nb_samples - avci->skip_samples, avctx->ch_layout.nb_channels, frame->format); >> @@ -432,11 +370,11 @@ FF_ENABLE_DEPRECATION_WARNINGS >> } >> } >> >> - if (discard_padding > 0 && discard_padding <= frame->nb_samples && got_frame && >> + if (discard_padding > 0 && discard_padding <= frame->nb_samples && !ret && >> !(avctx->flags2 & AV_CODEC_FLAG2_SKIP_MANUAL)) { >> if (discard_padding == frame->nb_samples) { >> *discarded_samples += frame->nb_samples; >> - got_frame = 0; >> + ret = AVERROR(EAGAIN); >> } else { >> if(avctx->pkt_timebase.num && avctx->sample_rate) { >> int64_t diff_ts = av_rescale_q(frame->nb_samples - discard_padding, >> @@ -452,7 +390,7 @@ FF_ENABLE_DEPRECATION_WARNINGS >> } >> } >> >> - if ((avctx->flags2 & AV_CODEC_FLAG2_SKIP_MANUAL) && got_frame) { >> + if ((avctx->flags2 & AV_CODEC_FLAG2_SKIP_MANUAL) && !ret) { >> AVFrameSideData *fside = av_frame_new_side_data(frame, AV_FRAME_DATA_SKIP_SAMPLES, 10); >> if (fside) { >> AV_WL32(fside->data, avci->skip_samples); >> @@ -464,15 +402,88 @@ FF_ENABLE_DEPRECATION_WARNINGS >> } >> } >> >> - if (!got_frame) >> + return ret; >> +} >> + >> +/* >> + * 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) >> +{ >> + AVCodecInternal *avci = avctx->internal; >> + AVPacket *const pkt = avci->in_pkt; >> + const FFCodec *const codec = ffcodec(avctx->codec); >> + int got_frame; >> + int consumed, 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) { >> + consumed = ff_thread_decode_frame(avctx, frame, &got_frame, pkt); >> + } else { >> + consumed = 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(); >> + >> + if (got_frame) >> + ret = discard_samples(avctx, frame, discarded_samples); >> + else >> + ret = AVERROR(EAGAIN); >> + >> + if (consumed < 0) >> + ret = consumed; >> + if (consumed >= 0 && avctx->codec->type == AVMEDIA_TYPE_VIDEO) >> + consumed = pkt->size; >> + >> + if (!got_frame || ret == AVERROR(EAGAIN)) >> av_frame_unref(frame); >> >> - if (ret >= 0 && avctx->codec->type == AVMEDIA_TYPE_VIDEO) >> - ret = pkt->size; >> + if (!ret) >> + av_assert0(frame->buf[0]); >> + if (ret == AVERROR(EAGAIN)) >> + ret = 0; >> >> - /* do not stop draining when actual_got_frame != 0 or ret < 0 */ >> - /* got_frame == 0 but actual_got_frame != 0 when frame is discarded */ >> - if (avci->draining && !actual_got_frame) { >> + /* do not stop draining when got_frame != 0 or ret < 0 */ >> + if (avci->draining && !got_frame) { >> if (ret < 0) { >> /* prevent infinite loop if a decoder wrongly always return error on draining */ >> /* reasonable nb_errors_max = maximum b frames + thread count */ >> @@ -490,11 +501,9 @@ FF_ENABLE_DEPRECATION_WARNINGS >> } >> } >> >> - if (ret >= pkt->size || ret < 0) { >> + if (consumed >= pkt->size || ret < 0) { >> av_packet_unref(pkt); >> } else { >> - int consumed = ret; >> - >> pkt->data += consumed; >> pkt->size -= consumed; >> pkt->pts = AV_NOPTS_VALUE; >> @@ -509,10 +518,7 @@ FF_ENABLE_DEPRECATION_WARNINGS >> } >> } >> >> - if (got_frame) >> - av_assert0(frame->buf[0]); >> - >> - return ret < 0 ? ret : 0; >> + return ret; >> } >> >> #if CONFIG_LCMS2 > > Would using --diff-algorithm=minimal (or patience or histogram) produce > a more readable diff (i.e. one where the code that ends up in > discard_samples() is moved instead of everything except it)? No, "git format-patch --diff-algorithm={minimal,patience,histogram}" produces the same output.
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.
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.
Quoting James Almer (2023-07-09 14:11:14) > > 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. The duplicated code would be 2 very simple lines. The cost you're paying for avoiding the duplication seems worse to me.
diff --git a/libavcodec/decode.c b/libavcodec/decode.c index 269633ce10..793ab975f6 100644 --- a/libavcodec/decode.c +++ b/libavcodec/decode.c @@ -289,69 +289,14 @@ static int64_t guess_correct_pts(AVCodecContext *ctx, return pts; } -/* - * 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); } 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; - } 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 (avci->skip_samples > 0 && got_frame && + if (avci->skip_samples > 0 && !ret && !(avctx->flags2 & AV_CODEC_FLAG2_SKIP_MANUAL)) { if(frame->nb_samples <= avci->skip_samples){ - got_frame = 0; *discarded_samples += frame->nb_samples; avci->skip_samples -= frame->nb_samples; av_log(avctx, AV_LOG_DEBUG, "skip whole frame, skip left: %d\n", avci->skip_samples); + ret = AVERROR(EAGAIN); } else { av_samples_copy(frame->extended_data, frame->extended_data, 0, avci->skip_samples, frame->nb_samples - avci->skip_samples, avctx->ch_layout.nb_channels, frame->format); @@ -432,11 +370,11 @@ FF_ENABLE_DEPRECATION_WARNINGS } } - if (discard_padding > 0 && discard_padding <= frame->nb_samples && got_frame && + if (discard_padding > 0 && discard_padding <= frame->nb_samples && !ret && !(avctx->flags2 & AV_CODEC_FLAG2_SKIP_MANUAL)) { if (discard_padding == frame->nb_samples) { *discarded_samples += frame->nb_samples; - got_frame = 0; + ret = AVERROR(EAGAIN); } else { if(avctx->pkt_timebase.num && avctx->sample_rate) { int64_t diff_ts = av_rescale_q(frame->nb_samples - discard_padding, @@ -452,7 +390,7 @@ FF_ENABLE_DEPRECATION_WARNINGS } } - if ((avctx->flags2 & AV_CODEC_FLAG2_SKIP_MANUAL) && got_frame) { + if ((avctx->flags2 & AV_CODEC_FLAG2_SKIP_MANUAL) && !ret) { AVFrameSideData *fside = av_frame_new_side_data(frame, AV_FRAME_DATA_SKIP_SAMPLES, 10); if (fside) { AV_WL32(fside->data, avci->skip_samples); @@ -464,15 +402,88 @@ FF_ENABLE_DEPRECATION_WARNINGS } } - if (!got_frame) + return ret; +} + +/* + * 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) +{ + AVCodecInternal *avci = avctx->internal; + AVPacket *const pkt = avci->in_pkt; + const FFCodec *const codec = ffcodec(avctx->codec); + int got_frame; + int consumed, 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) { + consumed = ff_thread_decode_frame(avctx, frame, &got_frame, pkt); + } else { + consumed = 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(); + + if (got_frame) + ret = discard_samples(avctx, frame, discarded_samples); + else + ret = AVERROR(EAGAIN); + + if (consumed < 0) + ret = consumed; + if (consumed >= 0 && avctx->codec->type == AVMEDIA_TYPE_VIDEO) + consumed = pkt->size; + + if (!got_frame || ret == AVERROR(EAGAIN)) av_frame_unref(frame); - if (ret >= 0 && avctx->codec->type == AVMEDIA_TYPE_VIDEO) - ret = pkt->size; + if (!ret) + av_assert0(frame->buf[0]); + if (ret == AVERROR(EAGAIN)) + ret = 0; - /* do not stop draining when actual_got_frame != 0 or ret < 0 */ - /* got_frame == 0 but actual_got_frame != 0 when frame is discarded */ - if (avci->draining && !actual_got_frame) { + /* do not stop draining when got_frame != 0 or ret < 0 */ + if (avci->draining && !got_frame) { if (ret < 0) { /* prevent infinite loop if a decoder wrongly always return error on draining */ /* reasonable nb_errors_max = maximum b frames + thread count */ @@ -490,11 +501,9 @@ FF_ENABLE_DEPRECATION_WARNINGS } } - if (ret >= pkt->size || ret < 0) { + if (consumed >= pkt->size || ret < 0) { av_packet_unref(pkt); } else { - int consumed = ret; - pkt->data += consumed; pkt->size -= consumed; pkt->pts = AV_NOPTS_VALUE; @@ -509,10 +518,7 @@ FF_ENABLE_DEPRECATION_WARNINGS } } - if (got_frame) - av_assert0(frame->buf[0]); - - return ret < 0 ? ret : 0; + return ret; } #if CONFIG_LCMS2
Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/decode.c | 182 +++++++++++++++++++++++--------------------- 1 file changed, 94 insertions(+), 88 deletions(-)