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.
Aman
>
> Best regards,
>
> --
> Matthieu B.
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
