Re: [FFmpeg-devel] [PATCH v5 07/21] avdevice/avdevice: Revert "Deprecate AVDevice Capabilities API"

["Diederick C. Niehorster" <dcnieho@gmail.com>]

On Tue, May 10, 2022 at 7:25 PM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
>
> Diederick Niehorster:
> > This reverts commit 4f49ca7bbc75a9db4cdf93f27f95a668c751f160. The next
> > few patches clean up the API and implement this capability for
> > avdevice/dshow.
> >
> > Bumping avformat and avdevice version.
> >
> > Signed-off-by: Diederick Niehorster <dcnieho@gmail.com>
> > ---
> >  libavdevice/avdevice.c | 71 ++++++++++++++++++++++++++++++++++++++----
> >  libavdevice/avdevice.h |  5 ---
> >  libavdevice/version.h  |  4 +--
> >  libavformat/avformat.h | 21 +++++++++++++
> >  libavformat/version.h  |  2 +-
> >  5 files changed, 89 insertions(+), 14 deletions(-)
> >
> > diff --git a/libavdevice/avdevice.c b/libavdevice/avdevice.c
> > index 43d3406670..d367189532 100644
> > --- a/libavdevice/avdevice.c
> > +++ b/libavdevice/avdevice.c
> > @@ -28,11 +28,39 @@
> >  #include "libavutil/ffversion.h"
> >  const char av_device_ffversion[] = "FFmpeg version " FFMPEG_VERSION;
> >
> > -#if FF_API_DEVICE_CAPABILITIES
> > +#define E AV_OPT_FLAG_ENCODING_PARAM
> > +#define D AV_OPT_FLAG_DECODING_PARAM
> > +#define A AV_OPT_FLAG_AUDIO_PARAM
> > +#define V AV_OPT_FLAG_VIDEO_PARAM
> > +#define OFFSET(x) offsetof(AVDeviceCapabilitiesQuery, x)
> > +
> >  const AVOption av_device_capabilities[] = {
> > +    { "codec", "codec", OFFSET(codec), AV_OPT_TYPE_INT,
> > +        {.i64 = AV_CODEC_ID_NONE}, AV_CODEC_ID_NONE, INT_MAX, E|D|A|V },
> > +    { "sample_format", "sample format", OFFSET(sample_format), AV_OPT_TYPE_SAMPLE_FMT,
> > +        {.i64 = AV_SAMPLE_FMT_NONE}, AV_SAMPLE_FMT_NONE, INT_MAX, E|D|A },
> > +    { "sample_rate", "sample rate", OFFSET(sample_rate), AV_OPT_TYPE_INT,
> > +        {.i64 = -1}, -1, INT_MAX, E|D|A },
> > +    { "channels", "channels", OFFSET(channels), AV_OPT_TYPE_INT,
> > +        {.i64 = -1}, -1, INT_MAX, E|D|A },
> > +    { "channel_layout", "channel layout", OFFSET(channel_layout), AV_OPT_TYPE_CHANNEL_LAYOUT,
> > +        {.i64 = -1}, -1, INT_MAX, E|D|A },
> > +    { "pixel_format", "pixel format", OFFSET(pixel_format), AV_OPT_TYPE_PIXEL_FMT,
> > +        {.i64 = AV_PIX_FMT_NONE}, AV_PIX_FMT_NONE, INT_MAX, E|D|V },
> > +    { "window_size", "window size", OFFSET(window_width), AV_OPT_TYPE_IMAGE_SIZE,
> > +        {.str = NULL}, -1, INT_MAX, E|D|V },
> > +    { "frame_size", "frame size", OFFSET(frame_width), AV_OPT_TYPE_IMAGE_SIZE,
> > +        {.str = NULL}, -1, INT_MAX, E|D|V },
> > +    { "fps", "fps", OFFSET(fps), AV_OPT_TYPE_RATIONAL,
> > +        {.dbl = -1}, -1, INT_MAX, E|D|V },
> >      { NULL }
> >  };
> > -#endif
> > +
> > +#undef E
> > +#undef D
> > +#undef A
> > +#undef V
> > +#undef OFFSET
> >
> >  unsigned avdevice_version(void)
> >  {
> > @@ -81,18 +109,49 @@ int avdevice_dev_to_app_control_message(struct AVFormatContext *s, enum AVDevToA
> >      return s->control_message_cb(s, type, data, data_size);
> >  }
> >
> > -#if FF_API_DEVICE_CAPABILITIES
> >  int avdevice_capabilities_create(AVDeviceCapabilitiesQuery **caps, AVFormatContext *s,
> >                                   AVDictionary **device_options)
> >  {
> > -    return AVERROR(ENOSYS);
> > +    int ret;
> > +    av_assert0(s && caps);
> > +    av_assert0(s->iformat || s->oformat);
> > +    if ((s->oformat && !s->oformat->create_device_capabilities) ||
> > +        (s->iformat && !s->iformat->create_device_capabilities))
> > +        return AVERROR(ENOSYS);
> > +    *caps = av_mallocz(sizeof(**caps));
> > +    if (!(*caps))
> > +        return AVERROR(ENOMEM);
> > +    (*caps)->device_context = s;
> > +    if (((ret = av_opt_set_dict(s->priv_data, device_options)) < 0))
> > +        goto fail;
> > +    if (s->iformat) {
> > +        if ((ret = s->iformat->create_device_capabilities(s, *caps)) < 0)
> > +            goto fail;
> > +    } else {
> > +        if ((ret = s->oformat->create_device_capabilities(s, *caps)) < 0)
> > +            goto fail;
> > +    }
> > +    av_opt_set_defaults(*caps);
> > +    return 0;
> > +  fail:
> > +    av_freep(caps);
> > +    return ret;
> >  }
> >
> >  void avdevice_capabilities_free(AVDeviceCapabilitiesQuery **caps, AVFormatContext *s)
> >  {
> > -    return;
> > +    if (!s || !caps || !(*caps))
> > +        return;
> > +    av_assert0(s->iformat || s->oformat);
> > +    if (s->iformat) {
> > +        if (s->iformat->free_device_capabilities)
> > +            s->iformat->free_device_capabilities(s, *caps);
> > +    } else {
> > +        if (s->oformat->free_device_capabilities)
> > +            s->oformat->free_device_capabilities(s, *caps);
> > +    }
> > +    av_freep(caps);
> >  }
> > -#endif
> >
> >  int avdevice_list_devices(AVFormatContext *s, AVDeviceInfoList **device_list)
> >  {
> > diff --git a/libavdevice/avdevice.h b/libavdevice/avdevice.h
> > index 74e9518a8e..9724e7edf5 100644
> > --- a/libavdevice/avdevice.h
> > +++ b/libavdevice/avdevice.h
> > @@ -347,7 +347,6 @@ int avdevice_dev_to_app_control_message(struct AVFormatContext *s,
> >                                          enum AVDevToAppMessageType type,
> >                                          void *data, size_t data_size);
> >
> > -#if FF_API_DEVICE_CAPABILITIES
> >  /**
> >   * Following API allows user to probe device capabilities (supported codecs,
> >   * pixel formats, sample formats, resolutions, channel counts, etc).
> > @@ -443,7 +442,6 @@ typedef struct AVDeviceCapabilitiesQuery {
> >  /**
> >   * AVOption table used by devices to implement device capabilities API. Should not be used by a user.
> >   */
> > -attribute_deprecated
> >  extern const AVOption av_device_capabilities[];
> >
> >  /**
> > @@ -463,7 +461,6 @@ extern const AVOption av_device_capabilities[];
> >   *
> >   * @return >= 0 on success, negative otherwise.
> >   */
> > -attribute_deprecated
> >  int avdevice_capabilities_create(AVDeviceCapabilitiesQuery **caps, AVFormatContext *s,
> >                                   AVDictionary **device_options);
> >
> > @@ -473,9 +470,7 @@ int avdevice_capabilities_create(AVDeviceCapabilitiesQuery **caps, AVFormatConte
> >   * @param caps Device capabilities data to be freed.
> >   * @param s    Context of the device.
> >   */
> > -attribute_deprecated
> >  void avdevice_capabilities_free(AVDeviceCapabilitiesQuery **caps, AVFormatContext *s);
> > -#endif
> >
> >  /**
> >   * Structure describes basic parameters of the device.
> > diff --git a/libavdevice/version.h b/libavdevice/version.h
> > index e2c8757b27..7c132f2174 100644
> > --- a/libavdevice/version.h
> > +++ b/libavdevice/version.h
> > @@ -30,8 +30,8 @@
> >
> >  #include "version_major.h"
> >
> > -#define LIBAVDEVICE_VERSION_MINOR   8
> > -#define LIBAVDEVICE_VERSION_MICRO 101
> > +#define LIBAVDEVICE_VERSION_MINOR   9
> > +#define LIBAVDEVICE_VERSION_MICRO 100
> >
> >  #define LIBAVDEVICE_VERSION_INT AV_VERSION_INT(LIBAVDEVICE_VERSION_MAJOR, \
> >                                                 LIBAVDEVICE_VERSION_MINOR, \
> > diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> > index 06db024559..027a914e13 100644
> > --- a/libavformat/avformat.h
> > +++ b/libavformat/avformat.h
> > @@ -617,6 +617,16 @@ typedef struct AVOutputFormat {
> >       * @see avdevice_list_devices() for more details.
> >       */
> >      int (*get_device_list)(struct AVFormatContext *s, struct AVDeviceInfoList *device_list);
> > +    /**
> > +     * Initialize device capabilities submodule.
> > +     * @see avdevice_capabilities_create() for more details.
> > +     */
> > +    int (*create_device_capabilities)(struct AVFormatContext *s, struct AVDeviceCapabilitiesQuery *caps);
> > +    /**
> > +     * Free device capabilities submodule.
> > +     * @see avdevice_capabilities_free() for more details.
> > +     */
> > +    int (*free_device_capabilities)(struct AVFormatContext *s, struct AVDeviceCapabilitiesQuery *caps);
> >      enum AVCodecID data_codec; /**< default data codec */
> >      /**
> >       * Initialize format. May allocate data here, and set any AVFormatContext or
> > @@ -797,6 +807,17 @@ typedef struct AVInputFormat {
> >       */
> >      int (*get_device_list)(struct AVFormatContext *s, struct AVDeviceInfoList *device_list);
> >
> > +    /**
> > +     * Initialize device capabilities submodule.
> > +     * @see avdevice_capabilities_create() for more details.
> > +     */
> > +    int (*create_device_capabilities)(struct AVFormatContext *s, struct AVDeviceCapabilitiesQuery *caps);
> > +
> > +    /**
> > +     * Free device capabilities submodule.
> > +     * @see avdevice_capabilities_free() for more details.
> > +     */
> > +    int (*free_device_capabilities)(struct AVFormatContext *s, struct AVDeviceCapabilitiesQuery *caps);
> >  } AVInputFormat;
> >  /**
> >   * @}
> > diff --git a/libavformat/version.h b/libavformat/version.h
> > index d195704a31..88d6f750a2 100644
> > --- a/libavformat/version.h
> > +++ b/libavformat/version.h
> > @@ -32,7 +32,7 @@
> >
> >  #include "version_major.h"
> >
> > -#define LIBAVFORMAT_VERSION_MINOR  22
> > +#define LIBAVFORMAT_VERSION_MINOR  23
> >  #define LIBAVFORMAT_VERSION_MICRO 100
> >
> >  #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \
>
> I have not really looked at the actual content of your dshow patches (as
> I know nothing about dshow). But I really don't like the thought that
> something is added to AVInput/OutputFormat that only a tiny fraction of
> all (de)muxers will likely ever use.

No worries, its this higher level discussion we should have first.

> I think it would be more efficient
> to add a single callback for the seldomly used types of callback like this:

I don't think we should introduce a kind of second-class citizens who
are not to be seen and harder to interact with. Either implement a
functionality properly without extra unnecessary indirection, or
decide its not worth it.

> int (*generic_callback)(struct AVFormatContext *s, int type, void
> *priv1, void *priv2, int64_t num)
>
> The uses of priv1, priv2 (and whether they are used at all) would depend
> upon the type; they can be pointers to structures (which structures
> depends upon type) to allow passing more arguments than otherwise
> possible by the signature of generic_callback. type should actually be
> an enum, but one can't forward declare an enum, so as long as the
> internals are in public headers it won't be an enum.
>
> The typical implementation of such a callback would be with a switch:
>
> static int generic_callback(AVFormatContext *s, int type, void *priv1,
> void *priv2, int64_t num)
> {
>     switch (type) {
>     case FOO:
>         return handle_foo(s, priv1, priv2, num);
>     case BAR:
>         return handle_bar(s, priv1, priv2, num);
>     default:
>         return AVERROR(ENOSYS);
> }
>
> The typical calling code would look like this:
>
> {
>     if (!s->oformat->generic_callback)
>         AVERROR(ENOSYS);
>     /* In case this type needs so many arguments that
>      * a structure for them is used they should be prepared here. */
>     ret = s->oformat->generic_callback(s, type_foo, /* other arguments);
>     /* Inspect ret or just return it; notice that ret may very well be
> AVERROR(ENOSYS) again, because the existence of the generic callback
> does not imply that every type is supported. */
> }
>
> There are more callbacks that should be moved into this generic callback
> besides the callbacks that you intend to add: The read_play and
> read_pause callbacks which are only supported by a single demuxer. E.g.
> av_read_play would look like this:
>
> int av_read_play(AVFormatContext *s)
> {
>     if (s->iformat->generic_callback) {
>         int ret = s->iformat->generic_callback(s, FF_CB_TYPE_READ_PLAY,
> NULL, NULL, 0);
>         if (ret != AVERROR(ENOSYS))
>             return ret;
>     }
>     if (s->pb)
>         return avio_pause(s->pb, 0);
>     return AVERROR(ENOSYS);
> }

Hi Andreas,

Thanks for chiming in! I have to agree with Hendrik, to me this is a
rather opaque, and thus hard to use, API.

https://ffmpeg.org/pipermail/ffmpeg-devel/2022-March/294747.html
provides another solution to the avformat/avdevice marriage, as based
on previous discussions (see links in
https://ffmpeg.org/pipermail/ffmpeg-devel/2022-March/294746.html).

Would yet another alternative be to have only a single field in
AVInput/OutputFormat, an opaque pointer void *avdevicecallbacks to a
new AVInputDevice/AVOutputDevice struct that is set when a format is a
device. These new types would live in avdevice and contain the
callback pointers that now live in AVInput/OutputFormat (and possibly
later other relevant things, perhaps a more generic name like void
*priv_avdevice would be good). (With my patches,) these callbacks are:
AVInputFormat:
int (*control_message)(struct AVFormatContext *s, int type,
                           void *data, size_t data_size);
int (*get_device_list)(struct AVFormatContext *s, struct
AVDeviceInfoList *device_list);
int (*create_device_capabilities)(struct AVFormatContext *s, struct
AVDeviceCapabilitiesQuery *caps);
int (*free_device_capabilities)(struct AVFormatContext *s, struct
AVDeviceCapabilitiesQuery *caps);

AVOutputFormat:
void (*get_output_timestamp)(struct AVFormatContext *s, int stream,
                                 int64_t *dts, int64_t *wall);
int (*control_message)(struct AVFormatContext *s, int type,
                           void *data, size_t data_size);
int (*get_device_list)(struct AVFormatContext *s, struct
AVDeviceInfoList *device_list);
int (*create_device_capabilities)(struct AVFormatContext *s, struct
AVDeviceCapabilitiesQuery *caps);
int (*free_device_capabilities)(struct AVFormatContext *s, struct
AVDeviceCapabilitiesQuery *caps);

Please let me know if i missed some.

That way any extension to avdevices would occur only in libavdevice,
while avdevices can still (__very conveniently!__) be pretend
formats--something that i think must be preserved.

This looks simple, am I forgetting something obvious?

(This does not remove HLSs read_play and read_pause callbacks, but
those aren't my concern ;) ).

The current patchset no longer applies to HEAD by the way, I'll send a
rebased one once we get a bit further in this discussion.

All the best,
Dee
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".