diff mbox series

[FFmpeg-devel] avcodec/mmaldec: use decoupled dataflow

Message ID 20210923183735.27400-1-cyph1984@gmail.com
State New
Headers show
Series [FFmpeg-devel] avcodec/mmaldec: use decoupled dataflow
Related show

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

Ming Shun Ho Sept. 23, 2021, 6:37 p.m. UTC
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(-)

Comments

Andreas Rheinhardt Sept. 23, 2021, 7:40 p.m. UTC | #1
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, \
>
Ming Shun Ho Sept. 24, 2021, 6:29 a.m. UTC | #2
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 mbox series

Patch

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