[FFmpeg-devel] [PATCH 1/2] doc/examples/demuxing: show how to use the reference counting system.
Clément Bœsch
u at pkh.me
Thu Oct 31 11:13:30 CET 2013
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?
> > 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?
> 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?
> > "\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.
> > 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?
> 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.
--
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20131031/a0eb0215/attachment.asc>
More information about the ffmpeg-devel
mailing list