[FFmpeg-devel] lavc/mediacodecdec: switch to the new generic filtering mechanism

Submitted by Matthieu Bouron on June 9, 2017, 11:38 p.m.

Details

Message ID 20170609233820.GD12885@tsuri.lan
State New
Headers show

Commit Message

Matthieu Bouron June 9, 2017, 11:38 p.m.
On Fri, Jun 09, 2017 at 08:18:25PM -0300, James Almer wrote:
> On 6/9/2017 7:53 PM, Matthieu Bouron wrote:
> > ---
> >  libavcodec/mediacodecdec.c | 70 ++++++++--------------------------------------
> >  1 file changed, 12 insertions(+), 58 deletions(-)
> > 
> > diff --git a/libavcodec/mediacodecdec.c b/libavcodec/mediacodecdec.c
> > index 5bdeb6c1d7..0c77a78aa9 100644
> > --- a/libavcodec/mediacodecdec.c
> > +++ b/libavcodec/mediacodecdec.c
> > @@ -41,11 +41,9 @@ typedef struct MediaCodecH264DecContext {
> >  
> >      MediaCodecDecContext *ctx;
> >  
> > -    AVBSFContext *bsf;
> > -
> >      AVFifoBuffer *fifo;
> >  
> > -    AVPacket filtered_pkt;
> > +    AVPacket buffered_pkt;
> >  
> >  } MediaCodecH264DecContext;
> >  
> > @@ -58,8 +56,7 @@ static av_cold int mediacodec_decode_close(AVCodecContext *avctx)
> >  
> >      av_fifo_free(s->fifo);
> >  
> > -    av_bsf_free(&s->bsf);
> > -    av_packet_unref(&s->filtered_pkt);
> > +    av_packet_unref(&s->buffered_pkt);
> >  
> >      return 0;
> >  }
> > @@ -312,9 +309,6 @@ static av_cold int mediacodec_decode_init(AVCodecContext *avctx)
> >  
> >      const char *codec_mime = NULL;
> >  
> > -    const char *bsf_name = NULL;
> > -    const AVBitStreamFilter *bsf = NULL;
> > -
> >      FFAMediaFormat *format = NULL;
> >      MediaCodecH264DecContext *s = avctx->priv_data;
> >  
> > @@ -329,7 +323,6 @@ static av_cold int mediacodec_decode_init(AVCodecContext *avctx)
> >  #if CONFIG_H264_MEDIACODEC_DECODER
> >      case AV_CODEC_ID_H264:
> >          codec_mime = "video/avc";
> > -        bsf_name = "h264_mp4toannexb";
> >  
> >          ret = h264_set_extradata(avctx, format);
> >          if (ret < 0)
> > @@ -339,7 +332,6 @@ static av_cold int mediacodec_decode_init(AVCodecContext *avctx)
> >  #if CONFIG_HEVC_MEDIACODEC_DECODER
> >      case AV_CODEC_ID_HEVC:
> >          codec_mime = "video/hevc";
> > -        bsf_name = "hevc_mp4toannexb";
> >  
> >          ret = hevc_set_extradata(avctx, format);
> >          if (ret < 0)
> > @@ -410,25 +402,6 @@ static av_cold int mediacodec_decode_init(AVCodecContext *avctx)
> >          goto done;
> >      }
> >  
> > -    if (bsf_name) {
> > -    bsf = av_bsf_get_by_name(bsf_name);
> > -    if(!bsf) {
> > -        ret = AVERROR_BSF_NOT_FOUND;
> > -        goto done;
> > -    }
> > -
> > -    if ((ret = av_bsf_alloc(bsf, &s->bsf))) {
> > -        goto done;
> > -    }
> > -
> > -    if (((ret = avcodec_parameters_from_context(s->bsf->par_in, avctx)) < 0) ||
> > -        ((ret = av_bsf_init(s->bsf)) < 0)) {
> > -          goto done;
> > -    }
> > -    }
> > -
> > -    av_init_packet(&s->filtered_pkt);
> > -
> >  done:
> >      if (format) {
> >          ff_AMediaFormat_delete(format);
> > @@ -503,10 +476,10 @@ static int mediacodec_decode_frame(AVCodecContext *avctx, void *data,
> >      /* process buffered data */
> >      while (!*got_frame) {
> >          /* prepare the input data -- convert to Annex B if needed */
> 
> The second part of this comment can be removed.

Fixed.

> 
> > -        if (s->filtered_pkt.size <= 0) {
> > -            AVPacket input_pkt = { 0 };
> > +        if (s->buffered_pkt.size <= 0) {
> > +            AVPacket input_pkt;
> >  
> > -            av_packet_unref(&s->filtered_pkt);
> > +            av_packet_unref(&s->buffered_pkt);
> >  
> >              /* no more data */
> >              if (av_fifo_size(s->fifo) < sizeof(AVPacket)) {
> > @@ -514,36 +487,15 @@ static int mediacodec_decode_frame(AVCodecContext *avctx, void *data,
> >                      ff_mediacodec_dec_decode(avctx, s->ctx, frame, got_frame, avpkt);
> >              }
> >  
> > -            av_fifo_generic_read(s->fifo, &input_pkt, sizeof(input_pkt), NULL);
> > -
> > -            if (s->bsf) {
> > -            ret = av_bsf_send_packet(s->bsf, &input_pkt);
> > -            if (ret < 0) {
> > -                return ret;
> > -            }
> > -
> > -            ret = av_bsf_receive_packet(s->bsf, &s->filtered_pkt);
> > -            if (ret == AVERROR(EAGAIN)) {
> > -                goto done;
> > -            }
> > -            } else {
> > -                av_packet_move_ref(&s->filtered_pkt, &input_pkt);
> > -            }
> > -
> > -            /* {h264,hevc}_mp4toannexb are used here and do not require flushing */
> > -            av_assert0(ret != AVERROR_EOF);
> > -
> > -            if (ret < 0) {
> > -                return ret;
> > -            }
> > +            av_fifo_generic_read(s->fifo, &s->buffered_pkt, sizeof(input_pkt), NULL);
> 
> input_pkt is unused aside from this, so why not just do
> sizeof(s->buffered_pkt) instead and remove input_pkt altogether?

Fixed. I removed input_pkt as it was a left-over of the previous code.

> 
> >          }
> >  
> > -        ret = mediacodec_process_data(avctx, frame, got_frame, &s->filtered_pkt);
> > +        ret = mediacodec_process_data(avctx, frame, got_frame, &s->buffered_pkt);
> >          if (ret < 0)
> >              return ret;
> >  
> > -        s->filtered_pkt.size -= ret;
> > -        s->filtered_pkt.data += ret;
> > +        s->buffered_pkt.size -= ret;
> > +        s->buffered_pkt.data += ret;
> >      }
> >  done:

I also removed the above done label, which is now unused.

> >      return avpkt->size;
> > @@ -560,7 +512,7 @@ static void mediacodec_decode_flush(AVCodecContext *avctx)
> >      }
> >      av_fifo_reset(s->fifo);
> >  
> > -    av_packet_unref(&s->filtered_pkt);
> > +    av_packet_unref(&s->buffered_pkt);
> >  
> >      ff_mediacodec_dec_flush(avctx, s->ctx);
> >  }
> > @@ -578,6 +530,7 @@ AVCodec ff_h264_mediacodec_decoder = {
> >      .close          = mediacodec_decode_close,
> >      .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_AVOID_PROBING,
> >      .caps_internal  = FF_CODEC_CAP_SETS_PKT_DTS,
> > +    .bsfs           = "h264_mp4toannexb",
> >  };
> >  #endif
> >  
> > @@ -594,6 +547,7 @@ AVCodec ff_hevc_mediacodec_decoder = {
> >      .close          = mediacodec_decode_close,
> >      .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_AVOID_PROBING,
> >      .caps_internal  = FF_CODEC_CAP_SETS_PKT_DTS,
> > +    .bsfs           = "hevc_mp4toannexb",
> >  };
> >  #endif
> 
> LGTM otherwise if it works (Can't test).

Thanks for the review.

I've done some basic testing and it works but I'll wait a day or two
before pushing it. The updated patch is attached.

[...]

Comments

Matthieu Bouron June 13, 2017, 12:38 p.m.
On Sat, Jun 10, 2017 at 01:38:20AM +0200, Matthieu Bouron wrote:
> On Fri, Jun 09, 2017 at 08:18:25PM -0300, James Almer wrote:
> > On 6/9/2017 7:53 PM, Matthieu Bouron wrote:
> > > ---
> > >  libavcodec/mediacodecdec.c | 70 ++++++++--------------------------------------
> > >  1 file changed, 12 insertions(+), 58 deletions(-)
> > > 
> > > diff --git a/libavcodec/mediacodecdec.c b/libavcodec/mediacodecdec.c
> > > index 5bdeb6c1d7..0c77a78aa9 100644
> > > --- a/libavcodec/mediacodecdec.c
> > > +++ b/libavcodec/mediacodecdec.c
> > > @@ -41,11 +41,9 @@ typedef struct MediaCodecH264DecContext {
> > >  
> > >      MediaCodecDecContext *ctx;
> > >  
> > > -    AVBSFContext *bsf;
> > > -
> > >      AVFifoBuffer *fifo;
> > >  
> > > -    AVPacket filtered_pkt;
> > > +    AVPacket buffered_pkt;
> > >  
> > >  } MediaCodecH264DecContext;
> > >  
> > > @@ -58,8 +56,7 @@ static av_cold int mediacodec_decode_close(AVCodecContext *avctx)
> > >  
> > >      av_fifo_free(s->fifo);
> > >  
> > > -    av_bsf_free(&s->bsf);
> > > -    av_packet_unref(&s->filtered_pkt);
> > > +    av_packet_unref(&s->buffered_pkt);
> > >  
> > >      return 0;
> > >  }
> > > @@ -312,9 +309,6 @@ static av_cold int mediacodec_decode_init(AVCodecContext *avctx)
> > >  
> > >      const char *codec_mime = NULL;
> > >  
> > > -    const char *bsf_name = NULL;
> > > -    const AVBitStreamFilter *bsf = NULL;
> > > -
> > >      FFAMediaFormat *format = NULL;
> > >      MediaCodecH264DecContext *s = avctx->priv_data;
> > >  
> > > @@ -329,7 +323,6 @@ static av_cold int mediacodec_decode_init(AVCodecContext *avctx)
> > >  #if CONFIG_H264_MEDIACODEC_DECODER
> > >      case AV_CODEC_ID_H264:
> > >          codec_mime = "video/avc";
> > > -        bsf_name = "h264_mp4toannexb";
> > >  
> > >          ret = h264_set_extradata(avctx, format);
> > >          if (ret < 0)
> > > @@ -339,7 +332,6 @@ static av_cold int mediacodec_decode_init(AVCodecContext *avctx)
> > >  #if CONFIG_HEVC_MEDIACODEC_DECODER
> > >      case AV_CODEC_ID_HEVC:
> > >          codec_mime = "video/hevc";
> > > -        bsf_name = "hevc_mp4toannexb";
> > >  
> > >          ret = hevc_set_extradata(avctx, format);
> > >          if (ret < 0)
> > > @@ -410,25 +402,6 @@ static av_cold int mediacodec_decode_init(AVCodecContext *avctx)
> > >          goto done;
> > >      }
> > >  
> > > -    if (bsf_name) {
> > > -    bsf = av_bsf_get_by_name(bsf_name);
> > > -    if(!bsf) {
> > > -        ret = AVERROR_BSF_NOT_FOUND;
> > > -        goto done;
> > > -    }
> > > -
> > > -    if ((ret = av_bsf_alloc(bsf, &s->bsf))) {
> > > -        goto done;
> > > -    }
> > > -
> > > -    if (((ret = avcodec_parameters_from_context(s->bsf->par_in, avctx)) < 0) ||
> > > -        ((ret = av_bsf_init(s->bsf)) < 0)) {
> > > -          goto done;
> > > -    }
> > > -    }
> > > -
> > > -    av_init_packet(&s->filtered_pkt);
> > > -
> > >  done:
> > >      if (format) {
> > >          ff_AMediaFormat_delete(format);
> > > @@ -503,10 +476,10 @@ static int mediacodec_decode_frame(AVCodecContext *avctx, void *data,
> > >      /* process buffered data */
> > >      while (!*got_frame) {
> > >          /* prepare the input data -- convert to Annex B if needed */
> > 
> > The second part of this comment can be removed.
> 
> Fixed.
> 
> > 
> > > -        if (s->filtered_pkt.size <= 0) {
> > > -            AVPacket input_pkt = { 0 };
> > > +        if (s->buffered_pkt.size <= 0) {
> > > +            AVPacket input_pkt;
> > >  
> > > -            av_packet_unref(&s->filtered_pkt);
> > > +            av_packet_unref(&s->buffered_pkt);
> > >  
> > >              /* no more data */
> > >              if (av_fifo_size(s->fifo) < sizeof(AVPacket)) {
> > > @@ -514,36 +487,15 @@ static int mediacodec_decode_frame(AVCodecContext *avctx, void *data,
> > >                      ff_mediacodec_dec_decode(avctx, s->ctx, frame, got_frame, avpkt);
> > >              }
> > >  
> > > -            av_fifo_generic_read(s->fifo, &input_pkt, sizeof(input_pkt), NULL);
> > > -
> > > -            if (s->bsf) {
> > > -            ret = av_bsf_send_packet(s->bsf, &input_pkt);
> > > -            if (ret < 0) {
> > > -                return ret;
> > > -            }
> > > -
> > > -            ret = av_bsf_receive_packet(s->bsf, &s->filtered_pkt);
> > > -            if (ret == AVERROR(EAGAIN)) {
> > > -                goto done;
> > > -            }
> > > -            } else {
> > > -                av_packet_move_ref(&s->filtered_pkt, &input_pkt);
> > > -            }
> > > -
> > > -            /* {h264,hevc}_mp4toannexb are used here and do not require flushing */
> > > -            av_assert0(ret != AVERROR_EOF);
> > > -
> > > -            if (ret < 0) {
> > > -                return ret;
> > > -            }
> > > +            av_fifo_generic_read(s->fifo, &s->buffered_pkt, sizeof(input_pkt), NULL);
> > 
> > input_pkt is unused aside from this, so why not just do
> > sizeof(s->buffered_pkt) instead and remove input_pkt altogether?
> 
> Fixed. I removed input_pkt as it was a left-over of the previous code.
> 
> > 
> > >          }
> > >  
> > > -        ret = mediacodec_process_data(avctx, frame, got_frame, &s->filtered_pkt);
> > > +        ret = mediacodec_process_data(avctx, frame, got_frame, &s->buffered_pkt);
> > >          if (ret < 0)
> > >              return ret;
> > >  
> > > -        s->filtered_pkt.size -= ret;
> > > -        s->filtered_pkt.data += ret;
> > > +        s->buffered_pkt.size -= ret;
> > > +        s->buffered_pkt.data += ret;
> > >      }
> > >  done:
> 
> I also removed the above done label, which is now unused.
> 
> > >      return avpkt->size;
> > > @@ -560,7 +512,7 @@ static void mediacodec_decode_flush(AVCodecContext *avctx)
> > >      }
> > >      av_fifo_reset(s->fifo);
> > >  
> > > -    av_packet_unref(&s->filtered_pkt);
> > > +    av_packet_unref(&s->buffered_pkt);
> > >  
> > >      ff_mediacodec_dec_flush(avctx, s->ctx);
> > >  }
> > > @@ -578,6 +530,7 @@ AVCodec ff_h264_mediacodec_decoder = {
> > >      .close          = mediacodec_decode_close,
> > >      .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_AVOID_PROBING,
> > >      .caps_internal  = FF_CODEC_CAP_SETS_PKT_DTS,
> > > +    .bsfs           = "h264_mp4toannexb",
> > >  };
> > >  #endif
> > >  
> > > @@ -594,6 +547,7 @@ AVCodec ff_hevc_mediacodec_decoder = {
> > >      .close          = mediacodec_decode_close,
> > >      .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_AVOID_PROBING,
> > >      .caps_internal  = FF_CODEC_CAP_SETS_PKT_DTS,
> > > +    .bsfs           = "hevc_mp4toannexb",
> > >  };
> > >  #endif
> > 
> > LGTM otherwise if it works (Can't test).
> 
> Thanks for the review.
> 
> I've done some basic testing and it works but I'll wait a day or two
> before pushing it. The updated patch is attached.

Patch applied.

[...]

Patch hide | download patch | download mbox

From 04c46b119891fa4c2f0fe41eb72703cffbbf1ae5 Mon Sep 17 00:00:00 2001
From: Matthieu Bouron <matthieu.bouron@gmail.com>
Date: Sat, 10 Jun 2017 00:41:07 +0200
Subject: [PATCH] lavc/mediacodecdec: switch to the new generic filtering
 mechanism

---
 libavcodec/mediacodecdec.c | 74 ++++++++--------------------------------------
 1 file changed, 13 insertions(+), 61 deletions(-)

diff --git a/libavcodec/mediacodecdec.c b/libavcodec/mediacodecdec.c
index 5bdeb6c1d7..6962ce2474 100644
--- a/libavcodec/mediacodecdec.c
+++ b/libavcodec/mediacodecdec.c
@@ -41,11 +41,9 @@  typedef struct MediaCodecH264DecContext {
 
     MediaCodecDecContext *ctx;
 
-    AVBSFContext *bsf;
-
     AVFifoBuffer *fifo;
 
-    AVPacket filtered_pkt;
+    AVPacket buffered_pkt;
 
 } MediaCodecH264DecContext;
 
@@ -58,8 +56,7 @@  static av_cold int mediacodec_decode_close(AVCodecContext *avctx)
 
     av_fifo_free(s->fifo);
 
-    av_bsf_free(&s->bsf);
-    av_packet_unref(&s->filtered_pkt);
+    av_packet_unref(&s->buffered_pkt);
 
     return 0;
 }
@@ -312,9 +309,6 @@  static av_cold int mediacodec_decode_init(AVCodecContext *avctx)
 
     const char *codec_mime = NULL;
 
-    const char *bsf_name = NULL;
-    const AVBitStreamFilter *bsf = NULL;
-
     FFAMediaFormat *format = NULL;
     MediaCodecH264DecContext *s = avctx->priv_data;
 
@@ -329,7 +323,6 @@  static av_cold int mediacodec_decode_init(AVCodecContext *avctx)
 #if CONFIG_H264_MEDIACODEC_DECODER
     case AV_CODEC_ID_H264:
         codec_mime = "video/avc";
-        bsf_name = "h264_mp4toannexb";
 
         ret = h264_set_extradata(avctx, format);
         if (ret < 0)
@@ -339,7 +332,6 @@  static av_cold int mediacodec_decode_init(AVCodecContext *avctx)
 #if CONFIG_HEVC_MEDIACODEC_DECODER
     case AV_CODEC_ID_HEVC:
         codec_mime = "video/hevc";
-        bsf_name = "hevc_mp4toannexb";
 
         ret = hevc_set_extradata(avctx, format);
         if (ret < 0)
@@ -410,25 +402,6 @@  static av_cold int mediacodec_decode_init(AVCodecContext *avctx)
         goto done;
     }
 
-    if (bsf_name) {
-    bsf = av_bsf_get_by_name(bsf_name);
-    if(!bsf) {
-        ret = AVERROR_BSF_NOT_FOUND;
-        goto done;
-    }
-
-    if ((ret = av_bsf_alloc(bsf, &s->bsf))) {
-        goto done;
-    }
-
-    if (((ret = avcodec_parameters_from_context(s->bsf->par_in, avctx)) < 0) ||
-        ((ret = av_bsf_init(s->bsf)) < 0)) {
-          goto done;
-    }
-    }
-
-    av_init_packet(&s->filtered_pkt);
-
 done:
     if (format) {
         ff_AMediaFormat_delete(format);
@@ -502,11 +475,9 @@  static int mediacodec_decode_frame(AVCodecContext *avctx, void *data,
 
     /* process buffered data */
     while (!*got_frame) {
-        /* prepare the input data -- convert to Annex B if needed */
-        if (s->filtered_pkt.size <= 0) {
-            AVPacket input_pkt = { 0 };
-
-            av_packet_unref(&s->filtered_pkt);
+        /* prepare the input data */
+        if (s->buffered_pkt.size <= 0) {
+            av_packet_unref(&s->buffered_pkt);
 
             /* no more data */
             if (av_fifo_size(s->fifo) < sizeof(AVPacket)) {
@@ -514,38 +485,17 @@  static int mediacodec_decode_frame(AVCodecContext *avctx, void *data,
                     ff_mediacodec_dec_decode(avctx, s->ctx, frame, got_frame, avpkt);
             }
 
-            av_fifo_generic_read(s->fifo, &input_pkt, sizeof(input_pkt), NULL);
-
-            if (s->bsf) {
-            ret = av_bsf_send_packet(s->bsf, &input_pkt);
-            if (ret < 0) {
-                return ret;
-            }
-
-            ret = av_bsf_receive_packet(s->bsf, &s->filtered_pkt);
-            if (ret == AVERROR(EAGAIN)) {
-                goto done;
-            }
-            } else {
-                av_packet_move_ref(&s->filtered_pkt, &input_pkt);
-            }
-
-            /* {h264,hevc}_mp4toannexb are used here and do not require flushing */
-            av_assert0(ret != AVERROR_EOF);
-
-            if (ret < 0) {
-                return ret;
-            }
+            av_fifo_generic_read(s->fifo, &s->buffered_pkt, sizeof(s->buffered_pkt), NULL);
         }
 
-        ret = mediacodec_process_data(avctx, frame, got_frame, &s->filtered_pkt);
+        ret = mediacodec_process_data(avctx, frame, got_frame, &s->buffered_pkt);
         if (ret < 0)
             return ret;
 
-        s->filtered_pkt.size -= ret;
-        s->filtered_pkt.data += ret;
+        s->buffered_pkt.size -= ret;
+        s->buffered_pkt.data += ret;
     }
-done:
+
     return avpkt->size;
 }
 
@@ -560,7 +510,7 @@  static void mediacodec_decode_flush(AVCodecContext *avctx)
     }
     av_fifo_reset(s->fifo);
 
-    av_packet_unref(&s->filtered_pkt);
+    av_packet_unref(&s->buffered_pkt);
 
     ff_mediacodec_dec_flush(avctx, s->ctx);
 }
@@ -578,6 +528,7 @@  AVCodec ff_h264_mediacodec_decoder = {
     .close          = mediacodec_decode_close,
     .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_AVOID_PROBING,
     .caps_internal  = FF_CODEC_CAP_SETS_PKT_DTS,
+    .bsfs           = "h264_mp4toannexb",
 };
 #endif
 
@@ -594,6 +545,7 @@  AVCodec ff_hevc_mediacodec_decoder = {
     .close          = mediacodec_decode_close,
     .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_AVOID_PROBING,
     .caps_internal  = FF_CODEC_CAP_SETS_PKT_DTS,
+    .bsfs           = "hevc_mp4toannexb",
 };
 #endif
 
-- 
2.13.1