[FFmpeg-devel] [PATCH 1/2] doc/examples/demuxing: show how to use the reference counting system.

[wm4]

On Wed, 30 Oct 2013 16:28:51 +0100
Clément Bœsch <u at pkh.me> wrote:

> From: Clément Bœsch <clement at stupeflix.com>
> 
> ---
>  doc/examples/demuxing.c | 44 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/doc/examples/demuxing.c b/doc/examples/demuxing.c
> index 7ae3654..379b1ea 100644
> --- a/doc/examples/demuxing.c
> +++ b/doc/examples/demuxing.c
> @@ -53,6 +53,11 @@ static AVPacket pkt;
>  static int video_frame_count = 0;
>  static int audio_frame_count = 0;
>  
> +/* This flag is used to make decoding using ref counting or not. In practice,
> + * you choose one way or another; ref counting notably allowing you to keep the
> + * data as long as you wish. */
> +static int use_ref_counting = 0;

Why confuse people with providing both? That's just confusing. In fact
I'd expect that the API without reference counting would be removed
sooner or later.

>  static int decode_packet(int *got_frame, int cached)
>  {
>      int ret = 0;
> @@ -115,6 +120,11 @@ static int decode_packet(int *got_frame, int cached)
>          }
>      }
>  
> +    /* If we use the reference counter API, we own the data and need to
> +     * de-reference it when we don't use it anymore */
> +    if (*got_frame && use_ref_counting)
> +        av_frame_unref(frame);

Checking got_frame shouldn't really be needed, or is it?

> +
>      return decoded;
>  }
>  
> @@ -125,6 +135,7 @@ static int open_codec_context(int *stream_idx,
>      AVStream *st;
>      AVCodecContext *dec_ctx = NULL;
>      AVCodec *dec = NULL;
> +    AVDictionary *opts = NULL;
>  
>      ret = av_find_best_stream(fmt_ctx, type, -1, -1, NULL, 0);
>      if (ret < 0) {
> @@ -144,7 +155,10 @@ static int open_codec_context(int *stream_idx,
>              return ret;
>          }
>  
> -        if ((ret = avcodec_open2(dec_ctx, dec, NULL)) < 0) {
> +        /* Init the decoders, with or without reference counting */
> +        if (use_ref_counting)
> +            av_dict_set(&opts, "refcounted_frames", "1", 0);
> +        if ((ret = avcodec_open2(dec_ctx, dec, &opts)) < 0) {
>              fprintf(stderr, "Failed to open %s codec\n",
>                      av_get_media_type_string(type));
>              return ret;
> @@ -187,18 +201,27 @@ int main (int argc, char **argv)
>  {
>      int ret = 0, got_frame;
>  
> -    if (argc != 4) {
> -        fprintf(stderr, "usage: %s input_file video_output_file audio_output_file\n"
> +    if (argc != 4 && argc != 5) {
> +        fprintf(stderr, "usage: %s [-refcount] input_file video_output_file audio_output_file\n"
>                  "API example program to show how to read frames from an input file.\n"
>                  "This program reads frames from a file, decodes them, and writes decoded\n"
>                  "video frames to a rawvideo file named video_output_file, and decoded\n"
> -                "audio frames to a rawaudio file named audio_output_file.\n"
> +                "audio frames to a rawaudio file named audio_output_file.\n\n"
> +                "If the -refcount option is specified, the program use the\n"
> +                "reference counting frame system which allows keeping a copy of\n"
> +                "the data for longer than one decode call. If unset, it's using\n"
> +                "the classic old method.\n"

This is confusing. Users will have no clue what this ref counting stuff
is about, whether it has advantages or disadvantages, or whether it
influences decoding in any way. And a command line user of a program
certainly does not have to care at all. (Yes, this is an example, but
it makes it look like changing this behavior from command line makes
sense.)

Let's just make it clear: not doing refcounting is a compatibility
thing.

>                  "\n", argv[0]);
>          exit(1);
>      }
> +    if (argc == 5) {
> +        use_ref_counting = strcmp(argv[1], "refcount");
> +        argv++;
> +    }
>      src_filename = argv[1];
>      video_dst_filename = argv[2];
>      audio_dst_filename = argv[3];
> +    printf("Use reference counting API: %s\n", use_ref_counting ? "YES" : "NO");
>  
>      /* register all formats and codecs */
>      av_register_all();
> @@ -257,7 +280,13 @@ int main (int argc, char **argv)
>          goto end;
>      }
>  
> -    frame = avcodec_alloc_frame();
> +    /* When using the ref counting system, you need to use the
> +     * libavutil/frame.h API, while the classic frame management is available
> +     * in libavcodec */
> +    if (use_ref_counting)
> +        frame = av_frame_alloc();
> +    else
> +        frame = avcodec_alloc_frame();

Why use avcodec_alloc_frame()? Either function should work for both
cases. (At least personally, I'm using avcodec_alloc_frame() in both
cases, because it spares me 1 ifdef, but new code should use
av_frame_alloc.)

>      if (!frame) {
>          fprintf(stderr, "Could not allocate frame\n");
>          ret = AVERROR(ENOMEM);
> @@ -336,7 +365,10 @@ end:
>          fclose(video_dst_file);
>      if (audio_dst_file)
>          fclose(audio_dst_file);
> -    av_free(frame);
> +    if (use_ref_counting)
> +        av_frame_free(&frame);
> +    else
> +        avcodec_free_frame(&frame);

Reading the sources, using the "right" function is actually required
here. Neither function can deal with the other case. (I'm going to fix
my own code...)

Again, just make the example use always reference counting, instead of
making people use deprecated and incredibly sketchy functions. (The
avcodec_ function blatantly violate what AVFrame documents.)

Maybe avcodec_free_frame should even be fixed to handle the ref-counted
case. It's easy to detect whether an AVFrame is refcounted or not.

>      av_free(video_dst_data[0]);
>  
>      return ret < 0;