[FFmpeg-devel] [PATCH v2] libavcodec/mediacodecdec: switch to new decoder api

[Matthieu Bouron]

On Wed, Jan 03, 2018 at 04:54:28PM -0800, Aman Gupta wrote:
> On Wed, Jan 3, 2018 at 3:25 AM, Matthieu Bouron <matthieu.bouron at gmail.com>
> wrote:
> 
> > On Thu, Dec 28, 2017 at 05:33:14PM -0800, Aman Gupta wrote:
> > > From: Aman Gupta <aman at tmm1.net>
> > >
> > > Using the new API gives the decoder the ability to produce
> > > N frames per input packet. This is particularly useful with
> > > mpeg2 decoders on some android devices, which automatically
> > > deinterlace video and produce one frame per field.
> > >
> > > Signed-off-by: Aman Gupta <aman at tmm1.net>
> > > ---
> > >  libavcodec/mediacodecdec.c | 77 +++++++++++++++++++++++++++---
> > ----------------
> > >  1 file changed, 45 insertions(+), 32 deletions(-)
> >
> > Hi,
> >
> > Thanks for the patch.
> >
> > I'm attaching a new version of your patch. The new version fixes some
> > issues that appear while seeking or when the decoder reaches EOF.
> >
> > The following comments correspond to the changes I made in the new patch
> > (except for a few minor cosmetics like = {0}; -> = { 0 };).
> >
> > >
> > > diff --git a/libavcodec/mediacodecdec.c b/libavcodec/mediacodecdec.c
> > > index b698ceaef9..90497c64da 100644
> > > --- a/libavcodec/mediacodecdec.c
> > > +++ b/libavcodec/mediacodecdec.c
> > > @@ -31,6 +31,7 @@
> > >  #include "libavutil/pixfmt.h"
> > >
> > >  #include "avcodec.h"
> > > +#include "decode.h"
> > >  #include "h264_parse.h"
> > >  #include "hevc_parse.h"
> > >  #include "hwaccel.h"
> > > @@ -424,29 +425,13 @@ static int mediacodec_process_data(AVCodecContext
> > *avctx, AVFrame *frame,
> > >      return ff_mediacodec_dec_decode(avctx, s->ctx, frame, got_frame,
> > pkt);
> > >  }
> > >
> > > -static int mediacodec_decode_frame(AVCodecContext *avctx, void *data,
> > > -                                   int *got_frame, AVPacket *avpkt)
> > > +static int mediacodec_receive_frame(AVCodecContext *avctx, AVFrame
> > *frame)
> > >  {
> > >      MediaCodecH264DecContext *s = avctx->priv_data;
> > > -    AVFrame *frame    = data;
> > >      int ret;
> > > -
> > > -    /* buffer the input packet */
> > > -    if (avpkt->size) {
> > > -        AVPacket input_pkt = { 0 };
> > > -
> > > -        if (av_fifo_space(s->fifo) < sizeof(input_pkt)) {
> > > -            ret = av_fifo_realloc2(s->fifo,
> > > -                                   av_fifo_size(s->fifo) +
> > sizeof(input_pkt));
> > > -            if (ret < 0)
> > > -                return ret;
> > > -        }
> > > -
> > > -        ret = av_packet_ref(&input_pkt, avpkt);
> > > -        if (ret < 0)
> > > -            return ret;
> > > -        av_fifo_generic_write(s->fifo, &input_pkt, sizeof(input_pkt),
> > NULL);
> > > -    }
> > > +    int got_frame = 0;
> > > +    int is_eof = 0;
> > > +    AVPacket pkt = {0};
> > >
> > >      /*
> > >       * MediaCodec.flush() discards both input and output buffers, thus
> > we
> > > @@ -470,26 +455,51 @@ static int mediacodec_decode_frame(AVCodecContext
> > *avctx, void *data,
> > >       */
> > >      if (ff_mediacodec_dec_is_flushing(avctx, s->ctx)) {
> > >          if (!ff_mediacodec_dec_flush(avctx, s->ctx)) {
> > > -            return avpkt->size;
> > > +            return 0;
> > >          }
> > > +        return AVERROR(EAGAIN);
> >
> > You should only return AVERROR(EAGAIN) if ff_mediacodec_dec_flush returns
> > 0:
> >     if (ff_mediacodec_dec_is_flushing(avctx, s->ctx)) {
> >         if (!ff_mediacodec_dec_flush(avctx, s->ctx)) {
> >             return AVERROR(EAGAIN);
> >         }
> >     }
> >
> > > +    }
> > > +
> > > +    ret = ff_decode_get_packet(avctx, &pkt);
> > > +    if (ret == AVERROR_EOF)
> > > +        is_eof = 1;
> > > +    else if (ret == AVERROR(EAGAIN))
> > > +        ; // no input packet, but fallthrough to check for pending
> > frames
> > > +    else if (ret < 0)
> > > +        return ret;
> > > +
> > > +    /* buffer the input packet */
> > > +    if (pkt.size) {
> > > +        if (av_fifo_space(s->fifo) < sizeof(pkt)) {
> > > +            ret = av_fifo_realloc2(s->fifo,
> > > +                                   av_fifo_size(s->fifo) + sizeof(pkt));
> > > +            if (ret < 0) {
> > > +                av_packet_unref(&pkt);
> > > +                return ret;
> > > +            }
> > > +        }
> > > +        av_fifo_generic_write(s->fifo, &pkt, sizeof(pkt), NULL);
> > >      }
> > >
> > >      /* process buffered data */
> > > -    while (!*got_frame) {
> > > +    while (!got_frame) {
> > >          /* 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)) {
> > > -                return avpkt->size ? avpkt->size :
> > > -                    ff_mediacodec_dec_decode(avctx, s->ctx, frame,
> > got_frame, avpkt);
> > > +                AVPacket null_pkt = {0};
> > > +                if (is_eof)
> > > +                    return ff_mediacodec_dec_decode(avctx, s->ctx,
> > frame,
> > > +                                                    &got_frame,
> > &null_pkt);
> >
> > Returning the return value of ff_mediacodec_dec_decode is not valid. It
> > should be something like:
> >     ret = ff_mediacodec_dec_decode(avctx, s->ctx, frame,
> >                                    &got_frame, &null_pkt);
> >     if (ret < 0)
> >         return ret;
> >     else if (got_frame)
> >         return 0;
> >     else
> >         return AVERROR_EOF;
> >
> > > +                return AVERROR(EAGAIN);
> > >              }
> > >
> > >              av_fifo_generic_read(s->fifo, &s->buffered_pkt,
> > sizeof(s->buffered_pkt), NULL);
> > >          }
> > >
> > > -        ret = mediacodec_process_data(avctx, frame, got_frame,
> > &s->buffered_pkt);
> > > +        ret = mediacodec_process_data(avctx, frame, &got_frame,
> > &s->buffered_pkt);
> > >          if (ret < 0)
> > >              return ret;
> > >
> > > @@ -497,7 +507,10 @@ static int mediacodec_decode_frame(AVCodecContext
> > *avctx, void *data,
> > >          s->buffered_pkt.data += ret;
> > >      }
> > >
> > > -    return avpkt->size;
> > > +    if (got_frame)
> > > +        return 0;
> > > +
> > > +    return AVERROR(EAGAIN);
> >
> > got_frame is always 1 here, returning 0 should be enough.
> >
> > [...]
> >
> > Let me know if the attached updated patch works for you.
> >
> 
> Thanks for the review and fixes. I tested your patch and it works as
> expected.

Patch applied.

-- 
Matthieu B.