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

[Aman Gupta]

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.

Aman


>
> Best regards,
>
> --
> Matthieu B.
>