[FFmpeg-devel] [RFC] libavfilter query_format() related functions error handling

Clément Bœsch u at pkh.me
Sat Mar 14 12:18:51 CET 2015


Hi,

I'm adding error handling in the following functions:

  ff_formats_ref()
  ff_channel_layouts_ref()
  ff_set_common_formats()
  ff_set_common_samplerates()
  ff_set_common_channel_layouts()

(I may have forgotten some)

So basically these functions would return AVERROR(ENOMEM) (or whatever
else) in case of memory allocation.

The problem I'm having is that query_formats() functions gets annoyingly
verbose. I will take adelay filter as an example in the following.

So this is a typical query_format() callback:

    static int query_formats(AVFilterContext *ctx)
    {
        AVFilterChannelLayouts *layouts;
        AVFilterFormats *formats;
        static const enum AVSampleFormat sample_fmts[] = {
            AV_SAMPLE_FMT_U8P, AV_SAMPLE_FMT_S16P, AV_SAMPLE_FMT_S32P,
            AV_SAMPLE_FMT_FLTP, AV_SAMPLE_FMT_DBLP,
            AV_SAMPLE_FMT_NONE
        };

        layouts = ff_all_channel_layouts();
        if (!layouts)
            return AVERROR(ENOMEM);
        ff_set_common_channel_layouts(ctx, layouts);

        formats = ff_make_format_list(sample_fmts);
        if (!formats)
            return AVERROR(ENOMEM);
        ff_set_common_formats(ctx, formats);

        formats = ff_all_samplerates();
        if (!formats)
            return AVERROR(ENOMEM);
        ff_set_common_samplerates(ctx, formats);

        return 0;
    }

It's already quite verbose, but with proper error handling, it would look like
this:

    static int query_formats(AVFilterContext *ctx)
    {
        int ret;
        AVFilterChannelLayouts *layouts;
        AVFilterFormats *formats;
        static const enum AVSampleFormat sample_fmts[] = {
            AV_SAMPLE_FMT_U8P, AV_SAMPLE_FMT_S16P, AV_SAMPLE_FMT_S32P,
            AV_SAMPLE_FMT_FLTP, AV_SAMPLE_FMT_DBLP,
            AV_SAMPLE_FMT_NONE
        };

        layouts = ff_all_channel_layouts();
        if (!layouts)
            return AVERROR(ENOMEM);
        ret = ff_set_common_channel_layouts(ctx, layouts);
        if (ret < 0)
            return ret;

        formats = ff_make_format_list(sample_fmts);
        if (!formats)
            return AVERROR(ENOMEM);
        ret = ff_set_common_formats(ctx, formats);
        if (ret < 0)
            return ret;

        formats = ff_all_samplerates();
        if (!formats)
            return AVERROR(ENOMEM);
        ret = ff_set_common_samplerates(ctx, formats);
        if (ret < 0)
            return ret;

        return 0;
    }

This is generally one of the most straightforward query_formats(), so it's
generally even more annoying.

One alternative I had in mind was to make the ff_set_common_*() functions
return AVERROR(ENOMEM) if the second argument was NULL. This would lead to
something like this:

    static int query_formats(AVFilterContext *ctx)
    {
        int ret;
        static const enum AVSampleFormat sample_fmts[] = {
            AV_SAMPLE_FMT_U8P, AV_SAMPLE_FMT_S16P, AV_SAMPLE_FMT_S32P,
            AV_SAMPLE_FMT_FLTP, AV_SAMPLE_FMT_DBLP,
            AV_SAMPLE_FMT_NONE
        };

        ret = ff_set_common_channel_layouts(ctx, ff_all_channel_layouts());
        if (ret < 0)
            return ret;

        ret = ff_set_common_formats(ctx, ff_make_format_list(sample_fmts));
        if (ret < 0)
            return ret;

        ret = ff_set_common_samplerates(ctx, ff_all_samplerates());
        if (ret < 0)
            return ret;

        return 0;
    }

This sounds mostly OK, even though returning AVERROR(ENOMEM) in case of NULL is
kind of hacky, since a NULL could be instead be used to signal "accept
everything":

    static int query_formats(AVFilterContext *ctx)
    {
        int ret;
        AVFilterFormats *formats;
        static const enum AVSampleFormat sample_fmts[] = {
            AV_SAMPLE_FMT_U8P, AV_SAMPLE_FMT_S16P, AV_SAMPLE_FMT_S32P,
            AV_SAMPLE_FMT_FLTP, AV_SAMPLE_FMT_DBLP,
            AV_SAMPLE_FMT_NONE
        };

        ret = ff_set_common_channel_layouts(ctx, NULL);
        if (ret < 0)
            return ret;

        formats = ff_make_format_list(sample_fmts);
        if (!formats)
            return AVERROR(ENOMEM);
        ret = ff_set_common_formats(ctx, formats);
        if (ret < 0)
            return ret;

        ret = ff_set_common_samplerates(ctx, NULL);
        if (ret < 0)
            return ret;

        return 0;
    }

Notice here that I needed to put back the formats check, because if it fails it
would make the filter accepts every format instead of a limited list, causing
many bugs or security issues. This last solution looked fine to me at first but
it is unfortunately pretty dangerous, so I'd better not pick it.

Last solution I had in mind was to just make the ff_set_common_*() functions
not do anything if the argument is NULL (because of memory allocation failure
in the creation of the list, or another bug), which would make the usage the
same as the 3rd proposition:


    static int query_formats(AVFilterContext *ctx)
    {
        int ret;
        static const enum AVSampleFormat sample_fmts[] = {
            AV_SAMPLE_FMT_U8P, AV_SAMPLE_FMT_S16P, AV_SAMPLE_FMT_S32P,
            AV_SAMPLE_FMT_FLTP, AV_SAMPLE_FMT_DBLP,
            AV_SAMPLE_FMT_NONE
        };

        ret = ff_set_common_channel_layouts(ctx, ff_all_channel_layouts());
        if (ret < 0)
            return ret;

        ret = ff_set_common_formats(ctx, ff_make_format_list(sample_fmts));
        if (ret < 0)
            return ret;

        ret = ff_set_common_samplerates(ctx, ff_all_samplerates());
        if (ret < 0)
            return ret;

        return 0;
    }

The difference in behaviour will be that these functions will just not enable
the common channel layouts/formats/samples rates in case the second argument is
NULL, which will (I suppose) make the configuration of the filter fails one way
or another.

What do you think?

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150314/88727094/attachment.asc>


More information about the ffmpeg-devel mailing list