| Message ID | 20210923183735.27400-1-cyph1984@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/mmaldec: use decoupled dataflow | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
Ho Ming Shun: > MMAL is an fundamentally an asynchronous decoder, which was a bad fit > for the legacy dataflow API. Often multiple packets are enqueued before > a flood of frames are returned from MMAL. > > The previous lockstep dataflow meant that any delay in returning packets > from the VPU would cause ctx->queue_decoded_frames to grow with no way > of draining the queue. > > Testing this with mpv streaming from an RTSP source reduced decode > latency from 2s to about 0.2s. > > Signed-off-by: Ho Ming Shun <cyph1984@gmail.com> > --- > libavcodec/mmaldec.c | 30 +++++++++++++++++++++++------- > 1 file changed, 23 insertions(+), 7 deletions(-) > > diff --git a/libavcodec/mmaldec.c b/libavcodec/mmaldec.c > index 96140bf53d..3d7cc90cd2 100644 > --- a/libavcodec/mmaldec.c > +++ b/libavcodec/mmaldec.c > @@ -570,6 +570,7 @@ static int ffmmal_add_packet(AVCodecContext *avctx, AVPacket *avpkt, > > done: > av_buffer_unref(&buf); > + av_packet_unref(avpkt); > return ret; > } > > @@ -655,6 +656,12 @@ static int ffmal_copy_frame(AVCodecContext *avctx, AVFrame *frame, > avctx->pix_fmt, avctx->width, avctx->height); > } > > + frame->sample_aspect_ratio = avctx->sample_aspect_ratio; > + frame->width = avctx->width; > + frame->width = avctx->width; > + frame->height = avctx->height; > + frame->format = avctx->pix_fmt; > + > frame->pts = buffer->pts == MMAL_TIME_UNKNOWN ? AV_NOPTS_VALUE : buffer->pts; > frame->pkt_dts = AV_NOPTS_VALUE; > > @@ -763,12 +770,12 @@ done: > return ret; > } > > -static int ffmmal_decode(AVCodecContext *avctx, void *data, int *got_frame, > - AVPacket *avpkt) > +static int ffmmal_receive_frame(AVCodecContext *avctx, AVFrame *frame) > { > MMALDecodeContext *ctx = avctx->priv_data; > - AVFrame *frame = data; > int ret = 0; > + AVPacket avpkt; You are adding a new AVPacket; and you are not even zeroing it. This is even worse than the current code (and it might even be dangerous: ff_decode_get_packet() expects initialized, blank packets, not uninitialized ones; what you are doing only works because this decoder does not have an automatically inserted bitstream filter). You will have to add an actually allocated packet for this; or one could reuse the spare packet of the DecodeSimpleContext that is unused for decoders implementing the receive_frame API. It is easy to fix the deprecation issue if one already has a spare packet: Just put the extradata into said packet. My guess that your patch does not exhibit any deep conflicts with mine turned out to be correct: the only part where there is a real conflict is in the fact that it doesn't make any sense any more to treat the packet as const, given that a decoder implementing the receive_frame API is supposed to unref the packets it receives on its own. While I regard not wrapping the extradata in a packet as cleaner, the code actually becomes simpler if one does so (as I will demonstrate lateron). In other words: I drop my patches. > + int got_frame = 0; > > if (avctx->extradata_size && !ctx->extradata_sent) { > AVPacket pkt = {0}; > @@ -782,7 +789,11 @@ FF_ENABLE_DEPRECATION_WARNINGS > return ret; > } > > - if ((ret = ffmmal_add_packet(avctx, avpkt, 0)) < 0) > + ret = ff_decode_get_packet(avctx, &avpkt); > + if(ret == 0) { > + if ((ret = ffmmal_add_packet(avctx, &avpkt, 0)) < 0) > + return ret; > + } else if(ret < 0 && !(ret == AVERROR(EAGAIN))) > return ret; > > if ((ret = ffmmal_fill_input_port(avctx)) < 0) > @@ -791,7 +802,7 @@ FF_ENABLE_DEPRECATION_WARNINGS > if ((ret = ffmmal_fill_output_port(avctx)) < 0) > return ret; > > - if ((ret = ffmmal_read_frame(avctx, frame, got_frame)) < 0) > + if ((ret = ffmmal_read_frame(avctx, frame, &got_frame)) < 0) > return ret; > > // ffmmal_read_frame() can block for a while. Since the decoder is > @@ -803,7 +814,12 @@ FF_ENABLE_DEPRECATION_WARNINGS > if ((ret = ffmmal_fill_input_port(avctx)) < 0) > return ret; > > - return ret; > + if(!got_frame && ret == 0) > + return AVERROR(EAGAIN); > + else > + return ret; > + > + Unnecessary newlines. > } > > static const AVCodecHWConfigInternal *const mmal_hw_configs[] = { > @@ -835,7 +851,7 @@ static const AVOption options[]={ > .priv_data_size = sizeof(MMALDecodeContext), \ > .init = ffmmal_init_decoder, \ > .close = ffmmal_close_decoder, \ > - .decode = ffmmal_decode, \ > + .receive_frame = ffmmal_receive_frame, \ > .flush = ffmmal_flush, \ > .priv_class = &ffmmal_##NAME##_dec_class, \ > .capabilities = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE, \ >
On Fri, Sep 24, 2021 at 3:40 AM Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote: > > Ho Ming Shun: > > MMAL is an fundamentally an asynchronous decoder, which was a bad fit > > for the legacy dataflow API. Often multiple packets are enqueued before > > a flood of frames are returned from MMAL. > > > > The previous lockstep dataflow meant that any delay in returning packets > > from the VPU would cause ctx->queue_decoded_frames to grow with no way > > of draining the queue. > > > > Testing this with mpv streaming from an RTSP source reduced decode > > latency from 2s to about 0.2s. > > > > Signed-off-by: Ho Ming Shun <cyph1984@gmail.com> > > --- > > libavcodec/mmaldec.c | 30 +++++++++++++++++++++++------- > > 1 file changed, 23 insertions(+), 7 deletions(-) > > > > diff --git a/libavcodec/mmaldec.c b/libavcodec/mmaldec.c > > index 96140bf53d..3d7cc90cd2 100644 > > --- a/libavcodec/mmaldec.c > > +++ b/libavcodec/mmaldec.c > > @@ -570,6 +570,7 @@ static int ffmmal_add_packet(AVCodecContext *avctx, AVPacket *avpkt, > > > > done: > > av_buffer_unref(&buf); > > + av_packet_unref(avpkt); > > return ret; > > } > > > > @@ -655,6 +656,12 @@ static int ffmal_copy_frame(AVCodecContext *avctx, AVFrame *frame, > > avctx->pix_fmt, avctx->width, avctx->height); > > } > > > > + frame->sample_aspect_ratio = avctx->sample_aspect_ratio; > > + frame->width = avctx->width; > > + frame->width = avctx->width; > > + frame->height = avctx->height; > > + frame->format = avctx->pix_fmt; > > + > > frame->pts = buffer->pts == MMAL_TIME_UNKNOWN ? AV_NOPTS_VALUE : buffer->pts; > > frame->pkt_dts = AV_NOPTS_VALUE; > > > > @@ -763,12 +770,12 @@ done: > > return ret; > > } > > > > -static int ffmmal_decode(AVCodecContext *avctx, void *data, int *got_frame, > > - AVPacket *avpkt) > > +static int ffmmal_receive_frame(AVCodecContext *avctx, AVFrame *frame) > > { > > MMALDecodeContext *ctx = avctx->priv_data; > > - AVFrame *frame = data; > > int ret = 0; > > + AVPacket avpkt; > > You are adding a new AVPacket; and you are not even zeroing it. This is > even worse than the current code (and it might even be dangerous: > ff_decode_get_packet() expects initialized, blank packets, not > uninitialized ones; what you are doing only works because this decoder > does not have an automatically inserted bitstream filter). Ah thanks for that. > > You will have to add an actually allocated packet for this; or one could > reuse the spare packet of the DecodeSimpleContext that is unused for > decoders implementing the receive_frame API. Ok. Kind of scary because I don't see anyone else doing this. > > It is easy to fix the deprecation issue if one already has a spare > packet: Just put the extradata into said packet. I realized I do not need another spare packet. Previous decode based API needed another AVPacket for extra_data because avpkt was passed in as a function parameter. Will post another version of this series to fix all warnings and switch to receive_frame (for latency reasons most importantly). > My guess that your patch does not exhibit any deep conflicts with mine > turned out to be correct: the only part where there is a real conflict > is in the fact that it doesn't make any sense any more to treat the > packet as const, given that a decoder implementing the receive_frame API > is supposed to unref the packets it receives on its own. > While I regard not wrapping the extradata in a packet as cleaner, the > code actually becomes simpler if one does so (as I will demonstrate > lateron). In other words: I drop my patches. > > > + int got_frame = 0; > > > > if (avctx->extradata_size && !ctx->extradata_sent) { > > AVPacket pkt = {0}; > > @@ -782,7 +789,11 @@ FF_ENABLE_DEPRECATION_WARNINGS > > return ret; > > } > > > > - if ((ret = ffmmal_add_packet(avctx, avpkt, 0)) < 0) > > + ret = ff_decode_get_packet(avctx, &avpkt); > > + if(ret == 0) { > > + if ((ret = ffmmal_add_packet(avctx, &avpkt, 0)) < 0) > > + return ret; > > + } else if(ret < 0 && !(ret == AVERROR(EAGAIN))) > > return ret; > > > > if ((ret = ffmmal_fill_input_port(avctx)) < 0) > > @@ -791,7 +802,7 @@ FF_ENABLE_DEPRECATION_WARNINGS > > if ((ret = ffmmal_fill_output_port(avctx)) < 0) > > return ret; > > > > - if ((ret = ffmmal_read_frame(avctx, frame, got_frame)) < 0) > > + if ((ret = ffmmal_read_frame(avctx, frame, &got_frame)) < 0) > > return ret; > > > > // ffmmal_read_frame() can block for a while. Since the decoder is > > @@ -803,7 +814,12 @@ FF_ENABLE_DEPRECATION_WARNINGS > > if ((ret = ffmmal_fill_input_port(avctx)) < 0) > > return ret; > > > > - return ret; > > + if(!got_frame && ret == 0) > > + return AVERROR(EAGAIN); > > + else > > + return ret; > > + > > + > > Unnecessary newlines. > > > } > > > > static const AVCodecHWConfigInternal *const mmal_hw_configs[] = { > > @@ -835,7 +851,7 @@ static const AVOption options[]={ > > .priv_data_size = sizeof(MMALDecodeContext), \ > > .init = ffmmal_init_decoder, \ > > .close = ffmmal_close_decoder, \ > > - .decode = ffmmal_decode, \ > > + .receive_frame = ffmmal_receive_frame, \ > > .flush = ffmmal_flush, \ > > .priv_class = &ffmmal_##NAME##_dec_class, \ > > .capabilities = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE, \ > > > > _______________________________________________ > 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".
diff --git a/libavcodec/mmaldec.c b/libavcodec/mmaldec.c index 96140bf53d..3d7cc90cd2 100644 --- a/libavcodec/mmaldec.c +++ b/libavcodec/mmaldec.c @@ -570,6 +570,7 @@ static int ffmmal_add_packet(AVCodecContext *avctx, AVPacket *avpkt, done: av_buffer_unref(&buf); + av_packet_unref(avpkt); return ret; } @@ -655,6 +656,12 @@ static int ffmal_copy_frame(AVCodecContext *avctx, AVFrame *frame, avctx->pix_fmt, avctx->width, avctx->height); } + frame->sample_aspect_ratio = avctx->sample_aspect_ratio; + frame->width = avctx->width; + frame->width = avctx->width; + frame->height = avctx->height; + frame->format = avctx->pix_fmt; + frame->pts = buffer->pts == MMAL_TIME_UNKNOWN ? AV_NOPTS_VALUE : buffer->pts; frame->pkt_dts = AV_NOPTS_VALUE; @@ -763,12 +770,12 @@ done: return ret; } -static int ffmmal_decode(AVCodecContext *avctx, void *data, int *got_frame, - AVPacket *avpkt) +static int ffmmal_receive_frame(AVCodecContext *avctx, AVFrame *frame) { MMALDecodeContext *ctx = avctx->priv_data; - AVFrame *frame = data; int ret = 0; + AVPacket avpkt; + int got_frame = 0; if (avctx->extradata_size && !ctx->extradata_sent) { AVPacket pkt = {0}; @@ -782,7 +789,11 @@ FF_ENABLE_DEPRECATION_WARNINGS return ret; } - if ((ret = ffmmal_add_packet(avctx, avpkt, 0)) < 0) + ret = ff_decode_get_packet(avctx, &avpkt); + if(ret == 0) { + if ((ret = ffmmal_add_packet(avctx, &avpkt, 0)) < 0) + return ret; + } else if(ret < 0 && !(ret == AVERROR(EAGAIN))) return ret; if ((ret = ffmmal_fill_input_port(avctx)) < 0) @@ -791,7 +802,7 @@ FF_ENABLE_DEPRECATION_WARNINGS if ((ret = ffmmal_fill_output_port(avctx)) < 0) return ret; - if ((ret = ffmmal_read_frame(avctx, frame, got_frame)) < 0) + if ((ret = ffmmal_read_frame(avctx, frame, &got_frame)) < 0) return ret; // ffmmal_read_frame() can block for a while. Since the decoder is @@ -803,7 +814,12 @@ FF_ENABLE_DEPRECATION_WARNINGS if ((ret = ffmmal_fill_input_port(avctx)) < 0) return ret; - return ret; + if(!got_frame && ret == 0) + return AVERROR(EAGAIN); + else + return ret; + + } static const AVCodecHWConfigInternal *const mmal_hw_configs[] = { @@ -835,7 +851,7 @@ static const AVOption options[]={ .priv_data_size = sizeof(MMALDecodeContext), \ .init = ffmmal_init_decoder, \ .close = ffmmal_close_decoder, \ - .decode = ffmmal_decode, \ + .receive_frame = ffmmal_receive_frame, \ .flush = ffmmal_flush, \ .priv_class = &ffmmal_##NAME##_dec_class, \ .capabilities = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE, \
MMAL is an fundamentally an asynchronous decoder, which was a bad fit for the legacy dataflow API. Often multiple packets are enqueued before a flood of frames are returned from MMAL. The previous lockstep dataflow meant that any delay in returning packets from the VPU would cause ctx->queue_decoded_frames to grow with no way of draining the queue. Testing this with mpv streaming from an RTSP source reduced decode latency from 2s to about 0.2s. Signed-off-by: Ho Ming Shun <cyph1984@gmail.com> --- libavcodec/mmaldec.c | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-)