[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