[FFmpeg-devel] [PATCH 2/4] lavd: add device capabilities API

Lukasz Marek lukasz.m.luki at gmail.com
Tue Feb 4 22:11:35 CET 2014


>> +const AVOption av_device_capabilities[] = {
>
>> +    { "__device_name",    "device name", offsetof(AVDeviceCapabilities, device_name), AV_OPT_TYPE_STRING,
>> +        {.str = NULL}, 0, 0, AVDEVICE_ALL_PARAM },
>
> Are you sure about the "__" prefix as a namespace isolation? Something more
> explicit, like, maybe "device." or "device/", or "devcap.", seems nicer.

Ok, I will change to devcap.

>
>> +    { "__device_context",  "device context", offsetof(AVDeviceCapabilities, device_context), AV_OPT_TYPE_POINTER,
>> +        {.str = NULL}, 0, 0, AVDEVICE_ALL_PARAM },
>> +    { "__codec",          "codec", offsetof(AVDeviceCapabilities, codec), AV_OPT_TYPE_INT,
>> +        {.i64 = -1}, -1, INT_MAX, AVDEVICE_ALL_PARAM },
>> +    { "__format",         "format", offsetof(AVDeviceCapabilities, format), AV_OPT_TYPE_INT,
>> +        {.i64 = -1}, -1, INT_MAX, AVDEVICE_ALL_PARAM },
>> +
>> +    { "__sample_rate",    "sample rate", offsetof(AVDeviceCapabilities, sample_rate), AV_OPT_TYPE_INT,
>> +        {.i64 = -1}, -1, INT_MAX, AV_OPT_FLAG_AUDIO_PARAM | AVDEVICE_DECENC_PARAM },
>> +    { "__channels",       "channels", offsetof(AVDeviceCapabilities, channels), AV_OPT_TYPE_INT,
>> +        {.i64 = -1}, -1, INT_MAX, AV_OPT_FLAG_AUDIO_PARAM | AVDEVICE_DECENC_PARAM },
>
>> +    { "__channel_layout", "channel layout", offsetof(AVDeviceCapabilities, channel_layout), AV_OPT_TYPE_INT,
>> +        {.i64 = -1}, -1, INT_MAX, AV_OPT_FLAG_AUDIO_PARAM | AVDEVICE_DECENC_PARAM },
>
> Should be INT64, no?

Yes, thx.

>> +
>> +    { "__window_width",  "window width", offsetof(AVDeviceCapabilities, window_width), AV_OPT_TYPE_INT,
>> +        {.i64 = -1}, -1, INT_MAX, AV_OPT_FLAG_VIDEO_PARAM | AVDEVICE_DECENC_PARAM },
>> +    { "__window_height", "window height", offsetof(AVDeviceCapabilities, window_height), AV_OPT_TYPE_INT,
>> +        {.i64 = -1}, -1, INT_MAX, AV_OPT_FLAG_VIDEO_PARAM | AVDEVICE_DECENC_PARAM },
>
>> +    { "__frame_width",   "frame width", offsetof(AVDeviceCapabilities, frame_width), AV_OPT_TYPE_INT,
>> +        {.i64 = -1}, -1, INT_MAX, AV_OPT_FLAG_VIDEO_PARAM | AVDEVICE_DECENC_PARAM },
>> +    { "__frame_height",  "frame height", offsetof(AVDeviceCapabilities, frame_height), AV_OPT_TYPE_INT,
>> +        {.i64 = -1}, -1, INT_MAX, AV_OPT_FLAG_VIDEO_PARAM | AVDEVICE_DECENC_PARAM },
>
> How does it handle situations where a small number frame sizes are possible?

I don't understand.

>> +int avdevice_finish_device_capabilities(AVFormatContext *s,
>> +                                        AVDeviceCapabilities **spec,
>> +                                        enum AVDeviceApplyStrategy strategy)
>> +{
>> +    if (!s->oformat || !s->oformat->apply_configuration)
>> +        return AVERROR(ENOSYS);
>
>> +    return s->oformat->apply_configuration(s, (void **)spec, strategy);
>
> Why the cast to void?

I used void** in lavf to not forward declare structs from lavd, and I 
got a warning here as I remeber. I will recheck it later.

>> +void avdevice_free_device_capabilities(AVDeviceCapabilities **spec)
>> +{
>> +    if (!spec || !(*spec))
>> +        return;
>> +    av_free((*spec)->device_name);
>> +    av_freep(spec);
>> +}
>
> I do not see the corresponding alloc function, is it normal?

It is allocated by AVOption API.

>
>> +
>> +int avdevice_get_device_capability(AVFormatContext *s, enum AVDeviceCapability capability,
>> +                                   AVOptionRanges **allowed_values)
>> +{
>> +    const char *opt_name;
>
>> +    if (!s || !allowed_values ||
>> +        !(opt_name = get_opt_name_from_cap_enum(capability)))
>> +        return AVERROR(EINVAL);
>
> IMHO, since these errors are obviously invalid use of the API, an assert
> failure is more adapted.
>
>> +    return av_opt_query_ranges(allowed_values, s->priv_data, opt_name, AV_OPT_SEARCH_CHILDREN);
>
> Here and in the following functions, it seems you are just wrapping /
> duplicating the options code. Why not just allow to use the options API
> directly? Less code for you, less new API to learn for the others.
>
> Maybe I am missing something, but the API could be something like that:
>
>      AVDeviceCapability *cap;
>      avdevice_capability_create(&cap, fmt_ctx, options);
>      av_opt_set_type(cap, "fps", 30);
>      avdevice_capability_compute(cap);
>      av_opt_query_ranges(cap, "frame_width", &width);
>      avdevice_capability_free(&cap);

I'm not sure what avdevice_capability_compute should do?

In this case calling av_opt_set_ is pointless. You can just set fps in 
cap structure directly.

I had something similar earlier in mind, but there are at least 2 
disadvantages:
1.  query_ranges implementation require device's AVFormatContext (for 
options, for control message API app callback, maybe other reasons). In 
your solution user may free this context and still use 
avdevice_capablity* which will usually lead to use freed pointer.
All function in the sample above may need context, but it is not visible 
in code.
2. I'm not sure extracting variable names as strings in public API is 
good idea. Typo in its name may be hard to notice, when you have type in 
enum you get compilation error.

I also wanted to add possibility to set up "any" configuration and use 
API to adjust it or just validate some configuration.

I will rethink it, but I still prefer mine solution over this.

-- 
Best Regards,
Lukasz Marek

Microsoft isn't evil, they just make really crappy operating systems. - 
Linus Torvalds


More information about the ffmpeg-devel mailing list