[FFmpeg-devel] [PATCH 1/4] lavf: add probe device API
Michael Niedermayer
michaelni at gmx.at
Thu Jan 30 04:17:14 CET 2014
On Thu, Jan 30, 2014 at 01:56:39AM +0100, Lukasz Marek wrote:
> On 26.01.2014 21:27, Michael Niedermayer wrote:
> >On Sun, Jan 26, 2014 at 09:06:48PM +0100, Lukasz Marek wrote:
> >>On 26.01.2014 20:32, Michael Niedermayer wrote:
> >>>On Sun, Jan 26, 2014 at 07:40:56PM +0100, Nicolas George wrote:
> >>>>Le sextidi 6 pluviôse, an CCXXII, Lukasz Marek a écrit :
> >>>>>>From c1e66d75f698fbd301743cd0664733a5d48f03e8 Mon Sep 17 00:00:00 2001
> >>>>>From: Lukasz Marek <lukasz.m.luki at gmail.com>
> >>>>>Date: Sat, 25 Jan 2014 22:46:03 +0100
> >>>>>Subject: [PATCH] lavd: add probe device API
> >>>>>
> >>>>>---
> >>>>> libavformat/avformat.h | 103 +++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>> 1 file changed, 103 insertions(+)
> >>>>>
> >>>>>diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> >>>>>index a495ee0..0ff4560 100644
> >>>>>--- a/libavformat/avformat.h
> >>>>>+++ b/libavformat/avformat.h
> >>>>>@@ -323,6 +323,101 @@ typedef struct AVFrac {
> >>>>> int64_t val, num, den;
> >>>>> } AVFrac;
> >>>>>
> >>>>
> >>>>>+//TODO: Move this stuff to libavdevice.
> >>>>
> >>>>I wonder. Being able to query the list of codecs supported by RTP, for
> >>>>example, would be nice.
> >>>
> >>>also its a bit redudant with query_codec()
> >>
> >>You refer to API or Nicolas' comment?
> >
> >API but iam quite aware that query_codec() is insufficient
> >
> >
> >>If API then you may see (and other entries around it)
> >>http://ffmpeg.org/pipermail/ffmpeg-devel/2013-November/150496.html
> >
> >yes, query_codec() sucks
> >
> >
> >>
> >>>>
> >>>>>+
> >>>>>+/**
> >>>>>+ * TODO: desc
> >>>>>+ */
> >>>>>+typedef struct AVDeviceFormat
> >>>>>+{
> >>>>>+ enum AVCodecID codec; /**< codec */
> >>>>>+ int *formats; /**< list of formats supported with codec.
> >>>>>+ AVPixelFormat/AVSampleFormat terminated with -1 */
> >>>>>+} AVDeviceFomat;
> >>>>
> >>>>First, I suggest, here and everywhere, to replace "terminated with X" lists
> >>>>with a count indicator: "int *formats; unsigned nb_formats;". I believe this
> >>>>is more practical for everyone. Also, some list are terminated by -1, some
> >>>>by 0, some by NULL, that takes effort to remember.
> >>>>
> >>>
> >>>>Second, I am not sure whether codec/format is the only pair that needs to be
> >>>>linked. That is the most obvious one, but I can easily imagine a camera with
> >>>>limited bandwidth supporting 50 FPS in MJPEG mode but only 30 FPS in
> >>>>RAWVIDEO mode.
> >>>
> >>>+1
> >>>
> >>>also see AVClass.query_ranges() and av_opt_query_ranges()
> >>>its more generic and simpler
> >>
> >>Not obvious goal I want to archive is to make use of it simple.
> >>You call a function and you get configuration you may pass to filter
> >>chain sink for example. I agree this API is not enough because of
> >>issues Nicolas pointed. I would tend to add some flags like "pick
> >>the best configuration", "pick the nearest to wanted spec", "pick
> >>matching wanted spec". When user gets spammed with large amount of
> >>supported configurations, it will require possibly a lot of work to
> >>pick the correct. This "pick" algorithm will need to be repeated in
> >>every code that uses the device. Also, the device may possibly take
> >>device specific factors when deciding which is the best, that are
> >>not known to user.
> >>
> >>You mention before about AVOptionRange, I had a short look on it,
> >>and I don't see how it could make it simpler? Maybe some hint,
> >>because I may miss something.
> >>
> >>>
> >>>
> >>>>
> >>>>Basically, we have the full Cartesian product:
> >>>>CODECS × PIXEL_FORMAT × RESOLUTIONS × FRAME_RATES
> >>>>CODECS × SAMPLE_FORMAT × CHANNEL_COUNTS × SAMPLE_RATES
> >>>>and we need to be able to express a part of the set.
> >>>>
> >>>>The obvious simple idea is to consider that the subset is itself a Cartesian
> >>>>product:
> >>>>
> >>>>SUPPORTED_FORMATS × SUPPORTED_RESOLUTIONS × SUPPORTED_FRAME_RATES \subset
> >>>>PIXEL_FORMAT × RESOLUTIONS × FRAME_RATES
> >>>>
> >>>>Except for CODECS and FORMAT, because they always are strongly linked.
> >>>>
> >>>>I can suggest two solutions, one simple and one powerful.
> >>>>
> >>>>The simple one: each device can return a small list of AVDeviceCapabilities.
> >>>>Each device expands its list the way it is most convenient. For example:
> >>>>
> >>>>[
> >>>> { codec = MJPEG, format = YUV420, list of supported resolutions and rates },
> >>>> { codec = MJPEG, format = YUV422, list of supported resolutions and rates },
> >>>> { codec = RAWVIDEO, format = RGB, list of supported resolutions and rates },
> >>>>]
> >>>>
> >>>>The powerful one: the AVDeviceCapabilities can leave fields unset and point
> >>>>to a list of AVDeviceCapabilities to define them. Something like that:
> >>>>
> >>>>[
> >>>> { codec = MJPEG, pointer to sublist by format for MJPEG },
> >>>> { codec = RAWVIDEO, idem },
> >>>>]
> >>>>
> >>>>sublist = [
> >>>> { format = YUV420, pointer to sublist of frame sizes and rates },
> >>>> { format = YUV422, pointer to sublist of frame sizes and rates },
> >>>>]
> >>>
> >>>even more complex and still a subset of the existing
> >>>AVOptionRange(s) API
> >>
> >>I would appreciate some more information how you see it.
> >
> >well it simply allows querrying a device (or other AVClass struct)
> >about what it supports so for example it could be querried about
> >supported frame rates and that could produce a different list
> >depending on if width/height have been already set or which
> >actual hardware was detected.
> >
> >it doesnt support picking the closest valid configuration.
> >Though that could be implemented on top of it
> >
> >like
> >if current configuration that has been set is invalid
> > for each parameter
> > get list of valid choices (this depends on other already set ones)
> > find choice that is closest to what the user did set
> > return to the user the smallest change needed (name of option and value)
> >
> >The user app could then just call av_opt_set() with this to make things
> >consistent
> >
> >Also you can extend AVOptionRange(s) if needed and add new
> >functionality if that what exists isnt doing the job you need but
> >it does fall in the area of use cases that AVOptionRange(s) should
> >cover
>
> I attached another draft. This base on AVOption API and it seems to
> be good direction. Please have a look on it, just want to make sure
> I do what you had in mind. I made some dummy implementation for
> opengl device and simple test app.
>
> In simple words I added new AVOption struct that should be used as
> nested structure to device private context structure. It is very
> convenient all devices have the same names of the params. The only
> disadvantage of this are options that may get duplicated with
> already existing options. Input devices probably provide most of
> them, but output devices usually take them from the stream. I'm not
> sure it can cause some problems. Some method to link them would be
> nice.
you mean something like "w" and "width" ?
they should both point to the same index in the structure and be
one after the other in a AVOption array, so detecting them shouldnt
be hard.
>
> maybe AVClass can be extended by following callbacks:
> - option set - device may get informed duplicated value is changed
> and copy value of duplicated param to second one)
> - validation - device (or any other class in fact) may validate new
> value before it is set. av_opt_set* would return AVERROR(EINVAL) if
> rejected by device.
this could make sense, yes
>
>
> Other problem I notice there is no way to free nested structure.
> Following code will leave a leak:
>
> avformat_alloc_output_context2(&oc, NULL, device_name, NULL);
> av_opt_set_int(oc->priv_data, "width", 100, AV_OPT_SEARCH_CHILDREN);
> avformat_free_context(oc);
normally after allocating a output context one would use it, in
which case the private structure gets freed by the muxers code at the
end (the muxer would also potentially put all kinds of other
allocated things in its private structure that it has to free at the
end)
the obvious solution would be to free it in avformat_free_context()
this would need to be tested though
>
> I left avdevice_list_devices() method with change suggested by Nicolas.
> So far AVDeviceInfo have only 2 fields, but there may be more and I
> think they should be obtained together. In other way application may
> get for example device_name of 2 devices and in-between time user
> connects headset that adds 3rd device, and list of
> device_description would contains 3 items.
>
>
> --
> Best Regards,
> Lukasz Marek
>
> You can avoid reality, but you cannot avoid the consequences of
> avoiding reality. - Ayn Rand
> doc/examples/Makefile | 1
> doc/examples/device_parameters.c | 110 +++++++++++++++++++++++++++++++++++++++
> libavdevice/avdevice.c | 35 ++++++++++++
> libavdevice/avdevice.h | 49 +++++++++++++++++
> libavdevice/opengl_enc.c | 80 +++++++++++++++++++++++++++-
> 5 files changed, 273 insertions(+), 2 deletions(-)
> 3ab15e71878a4a235ee70facdaaf0fbe1a1582c0 0001-lavd-probe-api-draft.patch
> From 8102fe71d055353deed0038a340f2022d68a2793 Mon Sep 17 00:00:00 2001
> From: Lukasz Marek <lukasz.m.luki at gmail.com>
> Date: Wed, 29 Jan 2014 18:11:14 +0100
> Subject: [PATCH] lavd: probe api draft
>
> ---
> doc/examples/Makefile | 1 +
> doc/examples/device_parameters.c | 110 +++++++++++++++++++++++++++++++++++++++
> libavdevice/avdevice.c | 35 +++++++++++++
> libavdevice/avdevice.h | 49 +++++++++++++++++
> libavdevice/opengl_enc.c | 80 +++++++++++++++++++++++++++-
> 5 files changed, 273 insertions(+), 2 deletions(-)
> create mode 100644 doc/examples/device_parameters.c
>
> diff --git a/doc/examples/Makefile b/doc/examples/Makefile
> index a25455e..7c93b50 100644
> --- a/doc/examples/Makefile
> +++ b/doc/examples/Makefile
> @@ -21,6 +21,7 @@ EXAMPLES= decoding_encoding \
> resampling_audio \
> scaling_video \
> transcode_aac \
> + device_parameters \
>
> OBJS=$(addsuffix .o,$(EXAMPLES))
>
> diff --git a/doc/examples/device_parameters.c b/doc/examples/device_parameters.c
> new file mode 100644
> index 0000000..590b89f
> --- /dev/null
> +++ b/doc/examples/device_parameters.c
> @@ -0,0 +1,110 @@
> +
> +#include <libavutil/log.h>
> +#include <libavutil/opt.h>
> +#include <libavformat/avformat.h>
> +#include <libavdevice/avdevice.h>
> +
> +void print_options(void *device_class)
> +{
> + const AVOption *opt = NULL;
> + void *next;
> +
> + opt = av_opt_next(device_class, NULL);
> + while(opt) {
> + av_log(device_class, AV_LOG_INFO, "option name: %s\n", opt->name);
> + opt = av_opt_next(device_class, opt);
> + }
> +
> + next = av_opt_child_next(device_class, NULL);
> + while(next) {
> + print_options(next);
> + next = av_opt_child_next(device_class, next);
> + }
> +
> +}
> +
> +void print_ranges(void *device_class, const AVOptionRanges *ranges, const char *name)
> +{
> + int i;
> + AVOptionRange *range;
> +
> + for(i = 0; i < ranges->nb_ranges; i++) {
> + range = ranges->range[i];
> + if (range->is_range)
> + av_log(device_class, AV_LOG_INFO, "%s: value: %f - %f\n", name,
> + range->value_min, range->value_max);
> + else
> + av_log(device_class, AV_LOG_INFO, "%s: value: %f\n", name,
> + range->value_min);
> + }
> +}
> +
> +int test_video_configuration(void *device_class)
> +{
> + AVOptionRanges *ranges;
> +
> + //print_options(device_class);
> +
> + av_opt_query_ranges(&ranges, device_class, "width", AV_OPT_SEARCH_CHILDREN);
> + print_ranges(device_class, ranges, "width");
> + av_opt_freep_ranges(&ranges);
> + av_opt_query_ranges(&ranges, device_class, "height", AV_OPT_SEARCH_CHILDREN);
> + print_ranges(device_class, ranges, "height");
> + av_opt_freep_ranges(&ranges);
AV_OPT_TYPE_IMAGE_SIZE might allow these 2 to be printed as pairs
somehow, which probably is a more natural representation
> +
> + av_opt_set_int(device_class, "width", 100, AV_OPT_SEARCH_CHILDREN);
> + av_opt_set_int(device_class, "width", 100, AV_OPT_SEARCH_CHILDREN);
> +
> + av_opt_query_ranges(&ranges, device_class, "width", AV_OPT_SEARCH_CHILDREN);
> + print_ranges(device_class, ranges, "width");
> + av_opt_freep_ranges(&ranges);
> + av_opt_query_ranges(&ranges, device_class, "height", AV_OPT_SEARCH_CHILDREN);
> + print_ranges(device_class, ranges, "height");
> + av_opt_freep_ranges(&ranges);
> +
> + /* returns just defaults */
> + av_opt_query_ranges(&ranges, device_class, "fps", AV_OPT_SEARCH_CHILDREN);
> + print_ranges(device_class, ranges, "fps");
> + av_opt_freep_ranges(&ranges);
> +
> + av_opt_query_ranges(&ranges, device_class, "codec", AV_OPT_SEARCH_CHILDREN);
> + print_ranges(device_class, ranges, "codec");
> + av_opt_freep_ranges(&ranges);
> +
> + av_opt_query_ranges(&ranges, device_class, "format", AV_OPT_SEARCH_CHILDREN);
> + print_ranges(device_class, ranges, "format");
> + av_opt_freep_ranges(&ranges);
the code before and after print_ranges() looks like it could possibly
be moved into print_ranges, simplifying the code
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
No human being will ever know the Truth, for even if they happen to say it
by chance, they would not even known they had done so. -- Xenophanes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140130/7887185a/attachment.asc>
More information about the ffmpeg-devel
mailing list