[FFmpeg-devel,v2] avfilter/src_movie: change the deprecate API to new decode api

Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
---
 libavfilter/src_movie.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

Steven Liu (12019-03-23):
> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> ---
>  libavfilter/src_movie.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/libavfilter/src_movie.c b/libavfilter/src_movie.c
> index bcabfcc4c2..65561a3959 100644
> --- a/libavfilter/src_movie.c
> +++ b/libavfilter/src_movie.c
> @@ -172,8 +172,6 @@ static int open_stream(void *log, MovieStream *st)
>      if (ret < 0)
>          return ret;
>  
> -    st->codec_ctx->refcounted_frames = 1;
> -
>      if ((ret = avcodec_open2(st->codec_ctx, codec, NULL)) < 0) {
>          av_log(log, AV_LOG_ERROR, "Failed to open codec\n");
>          return ret;
> @@ -524,17 +522,22 @@ static int movie_push_frame(AVFilterContext *ctx, unsigned out_id)
>          return AVERROR(ENOMEM);
>  
>      frame_type = st->st->codecpar->codec_type;
> -    switch (frame_type) {
> -    case AVMEDIA_TYPE_VIDEO:
> -        ret = avcodec_decode_video2(st->codec_ctx, frame, &got_frame, pkt);
> -        break;
> -    case AVMEDIA_TYPE_AUDIO:
> -        ret = avcodec_decode_audio4(st->codec_ctx, frame, &got_frame, pkt);
> -        break;
> -    default:
> +    if (frame_type == AVMEDIA_TYPE_VIDEO || frame_type == AVMEDIA_TYPE_AUDIO) {

> +        ret = avcodec_send_packet(st->codec_ctx, pkt);
> +        if (ret == AVERROR(EAGAIN) || ret == AVERROR_EOF) {
> +            ret = 0;
> +        }
> +        if (ret >= 0) {
> +            ret = avcodec_receive_frame(st->codec_ctx, frame);

This is doing one avcodec_receive_frame() for each
avcodec_send_packet(): this is not how the new API is supposed to be
used. If it was, there would not have been the need for a new API in the
first place, would it?

> +            if (ret >= 0)
> +                got_frame = 1;
> +            if (ret == AVERROR(EAGAIN) || ret == AVERROR_EOF)
> +                ret = 0;
> +        }
> +    } else {
>          ret = AVERROR(ENOSYS);
> -        break;
>      }
> +
>      if (ret < 0) {
>          av_log(ctx, AV_LOG_WARNING, "Decode error: %s\n", av_err2str(ret));
>          av_frame_free(&frame);

Regards,

> > @@ -524,17 +522,22 @@ static int movie_push_frame(AVFilterContext *ctx, unsigned out_id)
> >          return AVERROR(ENOMEM);
> >
> >      frame_type = st->st->codecpar->codec_type;
> > -    switch (frame_type) {
> > -    case AVMEDIA_TYPE_VIDEO:
> > -        ret = avcodec_decode_video2(st->codec_ctx, frame, &got_frame, pkt);
> > -        break;
> > -    case AVMEDIA_TYPE_AUDIO:
> > -        ret = avcodec_decode_audio4(st->codec_ctx, frame, &got_frame, pkt);
> > -        break;
> > -    default:
> > +    if (frame_type == AVMEDIA_TYPE_VIDEO || frame_type == AVMEDIA_TYPE_AUDIO) {
>
> > +        ret = avcodec_send_packet(st->codec_ctx, pkt);
> > +        if (ret == AVERROR(EAGAIN) || ret == AVERROR_EOF) {
> > +            ret = 0;
> > +        }
> > +        if (ret >= 0) {
> > +            ret = avcodec_receive_frame(st->codec_ctx, frame);
>
> This is doing one avcodec_receive_frame() for each
> avcodec_send_packet(): this is not how the new API is supposed to be
> used. If it was, there would not have been the need for a new API in the
> first place, would it?
>

Its indeed not ideal, and instead it should loop over receive_frame
until EAGAIN is returned. Otherwise you might lose frames when
send_packet doesnt actually accept a new one yet.

Additionally, the patch could still use quite some cleanup in the
AVPacket handling, since the new API will always consume a full
AVPacket, and any manual handling of partial packets is no longer
required, so this all can be removed as well further down in that
function.

- Hendrik