Message ID | AM7PR03MB66609E046C99190F0FD113598F709@AM7PR03MB6660.eurprd03.prod.outlook.com |
---|---|
State | Accepted |
Headers | show |
Series | Switch mmaldec to receive_frame API | 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 |
Quoting Andreas Rheinhardt (2021-12-09 14:08:01) > From: Ho Ming Shun <cyph1984@gmail.com> > > 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 a live RTSP source visibly reduced > latency introduced by frames waiting in queue_decoded_frames from > roughly 2s to 0. > > Signed-off-by: Ho Ming Shun <cyph1984@gmail.com> > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavcodec/mmaldec.c | 32 +++++++++++++++++++++++++------- > 1 file changed, 25 insertions(+), 7 deletions(-) > > diff --git a/libavcodec/mmaldec.c b/libavcodec/mmaldec.c > index 8c7d749742..d336f10350 100644 > --- a/libavcodec/mmaldec.c > +++ b/libavcodec/mmaldec.c > @@ -83,6 +83,8 @@ typedef struct MMALDecodeContext { > // libavcodec API can't return new frames, and we have a logical deadlock. > // This is avoided by queuing such buffers here. > FFBufferEntry *waiting_buffers, *waiting_buffers_tail; > + /* Packet used to hold received packets temporarily; not owned by us. */ > + AVPacket *pkt; Why is this needed? Seems to be you'd save three lines of code (and, more importantly, a confusing pointer to an object you don't own) by simply directly accessing avctx->internal->in_pkt in ffmal_receive_frame().
Anton Khirnov: > Quoting Andreas Rheinhardt (2021-12-09 14:08:01) >> From: Ho Ming Shun <cyph1984@gmail.com> >> >> 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 a live RTSP source visibly reduced >> latency introduced by frames waiting in queue_decoded_frames from >> roughly 2s to 0. >> >> Signed-off-by: Ho Ming Shun <cyph1984@gmail.com> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> libavcodec/mmaldec.c | 32 +++++++++++++++++++++++++------- >> 1 file changed, 25 insertions(+), 7 deletions(-) >> >> diff --git a/libavcodec/mmaldec.c b/libavcodec/mmaldec.c >> index 8c7d749742..d336f10350 100644 >> --- a/libavcodec/mmaldec.c >> +++ b/libavcodec/mmaldec.c >> @@ -83,6 +83,8 @@ typedef struct MMALDecodeContext { >> // libavcodec API can't return new frames, and we have a logical deadlock. >> // This is avoided by queuing such buffers here. >> FFBufferEntry *waiting_buffers, *waiting_buffers_tail; >> + /* Packet used to hold received packets temporarily; not owned by us. */ >> + AVPacket *pkt; > > Why is this needed? Seems to be you'd save three lines of code (and, > more importantly, a confusing pointer to an object you don't own) by > simply directly accessing avctx->internal->in_pkt in > ffmal_receive_frame(). > In principle, I don't want to hardcode where the temp packet comes from. It wouldn't make a difference here (as there would only be only place where avctx->internal->in_pkt is accessed), but I nevertheless like it more this way. - Andreas
diff --git a/libavcodec/mmaldec.c b/libavcodec/mmaldec.c index 8c7d749742..d336f10350 100644 --- a/libavcodec/mmaldec.c +++ b/libavcodec/mmaldec.c @@ -83,6 +83,8 @@ typedef struct MMALDecodeContext { // libavcodec API can't return new frames, and we have a logical deadlock. // This is avoided by queuing such buffers here. FFBufferEntry *waiting_buffers, *waiting_buffers_tail; + /* Packet used to hold received packets temporarily; not owned by us. */ + AVPacket *pkt; int64_t packets_sent; atomic_int packets_buffered; @@ -355,6 +357,8 @@ static av_cold int ffmmal_init_decoder(AVCodecContext *avctx) MMAL_COMPONENT_T *decoder; int ret = 0; + ctx->pkt = avctx->internal->in_pkt; + bcm_host_init(); if (mmal_vc_init()) { @@ -570,6 +574,7 @@ static int ffmmal_add_packet(AVCodecContext *avctx, AVPacket *avpkt, done: av_buffer_unref(&buf); + av_packet_unref(avpkt); return ret; } @@ -655,6 +660,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 +774,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; + AVPacket *const avpkt = ctx->pkt; int ret = 0; + int got_frame = 0; if (avctx->extradata_size && !ctx->extradata_sent) { AVPacket pkt = {0}; @@ -780,7 +791,11 @@ static int ffmmal_decode(AVCodecContext *avctx, void *data, int *got_frame, 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) @@ -789,7 +804,7 @@ static int ffmmal_decode(AVCodecContext *avctx, void *data, int *got_frame, 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 @@ -801,7 +816,10 @@ static int ffmmal_decode(AVCodecContext *avctx, void *data, int *got_frame, 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[] = { @@ -833,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, \