diff mbox series

[FFmpeg-devel,v3,1/5] avcodec/mmaldec: use decoupled dataflow

Message ID AM7PR03MB66609E046C99190F0FD113598F709@AM7PR03MB6660.eurprd03.prod.outlook.com
State Accepted
Headers show
Series Switch mmaldec to receive_frame API | expand

Checks

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

Commit Message

Andreas Rheinhardt Dec. 9, 2021, 1:08 p.m. UTC 
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(-)

Comments

Anton Khirnov Dec. 13, 2021, 10:10 a.m. UTC | #1 
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().
Andreas Rheinhardt Dec. 13, 2021, 12:04 p.m. UTC | #2 
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 mbox series

Patch

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, \