[FFmpeg-devel] [PATCH 2/4] fftools/ffmpeg: stop using av_stream_get_parser()

[Andreas Rheinhardt]

Anton Khirnov:
> The parser is internal to the demuxer, so its state at any particular
> point is not well-defined for the caller. Additionally, it is being
> accessed from the main thread, while demuxing runs in a separate thread.
> 
> Use a separate parser owned by ffmpeg.c to retrieve the same
> information.
> 
> Fixes races, e.g. in:
> - fate-h264-brokensps-2580
> - fate-h264-extradata-reload
> - fate-iv8-demux
> - fate-m4v-cfr
> - fate-m4v
> ---
>  fftools/ffmpeg.c     | 33 +++++++++++++++++++++++++++++++--
>  fftools/ffmpeg.h     |  9 +++++++++
>  fftools/ffmpeg_opt.c |  9 +++++++++
>  3 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index ef7177fc33..580df0443a 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -610,6 +610,9 @@ static void ffmpeg_cleanup(int ret)
>          avcodec_free_context(&ist->dec_ctx);
>          avcodec_parameters_free(&ist->par);
>  
> +        avcodec_free_context(&ist->parser_dec);
> +        av_parser_close(ist->parser);
> +
>          av_freep(&input_streams[i]);
>      }
>  
> @@ -2421,6 +2424,15 @@ static int process_input_packet(InputStream *ist, const AVPacket *pkt, int no_eo
>          ret = av_packet_ref(avpkt, pkt);
>          if (ret < 0)
>              return ret;
> +
> +        if (ist->parser) {
> +            // we do not use the parsed output, only the
> +            // filled codec context fields
> +            uint8_t *dummy;
> +            int      dummy_size;
> +            av_parser_parse2(ist->parser, ist->parser_dec, &dummy, &dummy_size,
> +                             pkt->data, pkt->size, pkt->pts, pkt->dts, pkt->pos);
> +        }
>      }
>  
>      if (pkt && pkt->dts != AV_NOPTS_VALUE) {
> @@ -2452,7 +2464,8 @@ static int process_input_packet(InputStream *ist, const AVPacket *pkt, int no_eo
>                  if (pkt && pkt->duration) {
>                      duration_dts = av_rescale_q(pkt->duration, ist->st->time_base, AV_TIME_BASE_Q);
>                  } else if(ist->dec_ctx->framerate.num != 0 && ist->dec_ctx->framerate.den != 0) {
> -                    int ticks= av_stream_get_parser(ist->st) ? av_stream_get_parser(ist->st)->repeat_pict+1 : ist->dec_ctx->ticks_per_frame;
> +                    int ticks = ist->parser ? ist->parser->repeat_pict + 1 :
> +                                              ist->dec_ctx->ticks_per_frame;
>                      duration_dts = ((int64_t)AV_TIME_BASE *
>                                      ist->dec_ctx->framerate.den * ticks) /
>                                      ist->dec_ctx->framerate.num / ist->dec_ctx->ticks_per_frame;
> @@ -2555,7 +2568,8 @@ static int process_input_packet(InputStream *ist, const AVPacket *pkt, int no_eo
>              } else if (pkt->duration) {
>                  ist->next_dts += av_rescale_q(pkt->duration, ist->st->time_base, AV_TIME_BASE_Q);
>              } else if(ist->dec_ctx->framerate.num != 0) {
> -                int ticks= av_stream_get_parser(ist->st) ? av_stream_get_parser(ist->st)->repeat_pict + 1 : ist->dec_ctx->ticks_per_frame;
> +                int ticks = ist->parser ? ist->parser->repeat_pict + 1 :
> +                                          ist->dec_ctx->ticks_per_frame;
>                  ist->next_dts += ((int64_t)AV_TIME_BASE *
>                                    ist->dec_ctx->framerate.den * ticks) /
>                                    ist->dec_ctx->framerate.num / ist->dec_ctx->ticks_per_frame;
> @@ -2688,6 +2702,21 @@ static int init_input_stream(int ist_index, char *error, int error_len)
>          assert_avoptions(ist->decoder_opts);
>      }
>  
> +    if (ist->parser) {
> +        AVCodecParameters *par_tmp;
> +
> +        par_tmp = avcodec_parameters_alloc();
> +        if (!par_tmp)
> +            return AVERROR(ENOMEM);
> +
> +        ret = avcodec_parameters_from_context(par_tmp, ist->dec_ctx);
> +        if (ret >= 0)
> +            ret = avcodec_parameters_to_context(ist->parser_dec, par_tmp);
> +        avcodec_parameters_free(&par_tmp);
> +        if (ret < 0)
> +            return ret;
> +    }
> +
>      ist->next_pts = AV_NOPTS_VALUE;
>      ist->next_dts = AV_NOPTS_VALUE;
>  
> diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
> index 44cc23fa84..f67a2f1d1d 100644
> --- a/fftools/ffmpeg.h
> +++ b/fftools/ffmpeg.h
> @@ -334,6 +334,15 @@ typedef struct InputStream {
>      AVFrame *decoded_frame;
>      AVPacket *pkt;
>  
> +    /**
> +     * Parser for timestamp processing.
> +     */
> +    AVCodecParserContext *parser;
> +    /**
> +     * Codec context needed by parsing.
> +     */
> +    AVCodecContext       *parser_dec;
> +
>      int64_t       prev_pkt_pts;
>      int64_t       start;     /* time when read started */
>      /* predicted dts of the next packet read for this stream or (when there are
> diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
> index 30ca5cd609..a505c7b26f 100644
> --- a/fftools/ffmpeg_opt.c
> +++ b/fftools/ffmpeg_opt.c
> @@ -1065,6 +1065,15 @@ static void add_input_streams(OptionsContext *o, AVFormatContext *ic)
>              ist->top_field_first = -1;
>              MATCH_PER_STREAM_OPT(top_field_first, i, ist->top_field_first, ic, st);
>  
> +            ist->parser = av_parser_init(ist->dec->id);
> +            if (ist->parser) {
> +                ist->parser->flags |= PARSER_FLAG_COMPLETE_FRAMES;
> +
> +                ist->parser_dec = avcodec_alloc_context3(NULL);
> +                if (!ist->parser_dec)
> +                    exit_program(1);
> +            }
> +
>              break;
>          case AVMEDIA_TYPE_AUDIO:
>              ist->guess_layout_max = INT_MAX;

Are you aware that some parsers (e.g. the HEVC one) are still slow even
with the PARSER_FLAG_COMPLETE_FRAMES flag and consume non-negligible
amounts of memory?

- Andreas