[FFmpeg-devel] [PATCH 1/2] doc/examples/demuxing: show how to use the reference counting system.
wm4
nfxjfg at googlemail.com
Thu Oct 31 11:50:14 CET 2013
On Thu, 31 Oct 2013 11:13:30 +0100
Clément Bœsch <u at pkh.me> wrote:
> On Thu, Oct 31, 2013 at 10:53:00AM +0100, wm4 wrote:
> > 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.
> >
>
> I'm assuming it could be helpful when moving code from the old to the new
> API. Also, the new one requires more non-intuitive code (setting an
> obscure option).
>
> Are you sure the API without ref counting is supposed to be dropped? Where
> is that mentioned?
Libav basically said "eventually, probably", so at least there are no
definitive plans to this, but it might happen.
In general, whether the API does refcounting or not doesn't matter
for an API user who is not interested in refcounting. The user can
just use av_frame_alloc and av_frame_free at start and end of decoding,
and ignore the fact that they're really reference counted.
The exception is when the user overrides get_buffer/reget_buffer.
These functions are already deprecated. The replacement, get_buffer2,
mandates reference counting. So, if the deprecated functions are
removed, "refcounted_frames" can be turned into a NOP kept only for ABI
compatibility.
> > > 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?
> >
>
> Mmh probably not but I'll check.
>
> > > +
> > > 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.
>
> "... which allows keeping a copy of the data for longer than one decode
> call"
>
> Are there other benefits?
No, but removing the non-refcounted stuff has a benefit: simplicity,
and not requiring making the user choices that don't matter (for him).
> > 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.)
> >
>
> See on top the following comment: "In practice, you choose one way or
> another"
>
> > Let's just make it clear: not doing refcounting is a compatibility
> > thing.
> >
>
> Maybe I should explicit more that this flag is to show the differences
> required to move from !use_ref_counting to use_ref_counting?
It would be an improvement. I think this patch will be ok if the
warning uses enough capital letters, and if it's clear that refcounting
is preferred.
> > > "\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.)
> >
>
> They should, but it wasn't obvious at first glance if the function were
> identical. Again, it's all about from before to now: how your old code
> looks like vs how it should look like.
I know they use separate code, but looking at the debugger, all fields
are set to the same values (except different malloc'ed pointer for
extended_data).
> > > 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...)
> >
>
> If I understand you well, seeing that explicit difference before/after
> helped you fixing your code?
I don't admit that. I just double-checked that both functions to
different things, while I assumed they're equivalent before. It really
isn't obvious that it does (although the doxy comments say so).
> > 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.)
> >
>
> See previous comments.
>
> > 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.
> >
>
> Please submit a patch, I'm not yet comfortable with that API as you saw by
> yourself.
>
> Thanks for your feedback.
>
More information about the ffmpeg-devel
mailing list