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.bou...@gmail.com>
> wrote:
>
> > On Thu, Dec 28, 2017 at 05:33:14PM -0800, Aman Gupta wrote:
> > > From: Aman Gupta <a...@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 <a...@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
