[FFmpeg-devel] [PATCH 3/4] avdevice/dshow: implement capabilities API

[Andreas Rheinhardt]

Diederick C. Niehorster:
> On Sat, Jun 5, 2021 at 2:25 PM Andreas Rheinhardt
> <andreas.rheinhardt at outlook.com> wrote:
>> Diederick Niehorster:
>>> This implements avdevice_capabilities_create and avdevice_capabilities_free for the dshow device.> > +
>>> +            if (ranges) {
>>> +                if (query_type == CAP_QUERY_FRAME_SIZE) {
>>> +                    for (int j = 0; j < 3; j++) {
>>> +                        range = av_mallocz(sizeof(AVOptionRange));
>>
>> It seems that you are allocating individual AVOptionRange structures.
>> This is bad, because that is not how av_opt_freep_ranges() treats them:
>> They are supposed to be pointers into one big array of AVOptionRange
>> structures.
> 
> av_opt_freep_ranges() does this (NB: ranges->range is an AVOptionRange **):
> for (i = 0; i < ranges->nb_ranges * ranges->nb_components; i++) {
>         AVOptionRange *range = ranges->range[i];
>         if (range) {
>             av_freep(&range->str);
>             av_freep(&ranges->range[i]); // <--
>         }
>     }
>     av_freep(&ranges->range);
> 
> Note line highlighted by <--:
> it frees each individual element, one at a time, and only after that
> deletes the whole array. So i think it is correct that i am allocating
> them one at a time? Am i misreading/-understanding this?
> 

No, I was wrong.

>>> +    const AVFormatContext *avctx = caps->device_context;
>>
>> avctx is only used for AVCodecContexts by convention.
> 
> the dshow device (almost) consistently uses avctx as name for the
> AVFormatContext. Should i change that everywhere (what is the
> convention name for a AVFormatContext?), or is it ok if i remain
> consistent with that here?
> 

Normally we use 's' for AVFormatContext; but it seems that several
devices use avctx. Probably best to stick with what the rest of this
file uses then.

>>> +    if (ctx->device_name[0]) {
>>> +        av_log(avctx, AV_LOG_ERROR, "You cannot query device capabilities on an opened device\n");
>>
>> Is this supposed to be a limitation of the capabilities API in general
>> or only of the implementation here in for dshow?
> 
> See other mails: the bit of documentation in avdevice.h (lines
> 334--402) suggest the capabilities API is supposed to be used on an
> unopened device. It would make sense: probe the device for what it can
> do, decide what settings you want to use, then open device). In case
> of the dshow device, I think i cannot avoid potential false positives
> (entries in the query output that shouldn't be there) if the device is
> already opened. I'll experiment, perhaps this is only a theoretical
> worry.
> 
And is the AVFormatContext that has been used for probing then supposed
to be reused for actually using the device? (You are calling
dshow_read_close() manually, but our cleanup functions typically only
clean up things that need to be freed; they e.g. don't reset (say)
ordinary ints to zero, although the code may rely upon this.)

- Andreas