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

This implements avdevice_capabilities_create and avdevice_capabilities_free for the dshow device.

Signed-off-by: Diederick Niehorster <dcnieho@gmail.com>
---
 libavdevice/dshow.c | 498 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 462 insertions(+), 36 deletions(-)

Diederick Niehorster:
> This implements avdevice_capabilities_create and avdevice_capabilities_free for the dshow device.
> 
> Signed-off-by: Diederick Niehorster <dcnieho@gmail.com>
> ---
>  libavdevice/dshow.c | 498 ++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 462 insertions(+), 36 deletions(-)
> 
> diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c
> index 8d0a6fcc09..c65492119e 100644
> --- a/libavdevice/dshow.c
> +++ b/libavdevice/dshow.c
> @@ -30,6 +30,48 @@
>  #include "objidl.h"
>  #include "shlwapi.h"
>  
> +enum DshowCapQueryType {
> +    CAP_QUERY_NONE = 0,
> +    CAP_QUERY_SAMPLE_FORMAT,
> +    CAP_QUERY_SAMPLE_RATE,
> +    CAP_QUERY_CHANNELS,
> +    CAP_QUERY_CODEC,
> +    CAP_QUERY_PIXEL_FORMAT,
> +    CAP_QUERY_FRAME_SIZE,
> +    CAP_QUERY_FPS
> +};
> +typedef struct DshowCapQueryTypeEntry {
> +    const char*             name;
> +    enum DshowCapQueryType  query_type;
> +} DshowCapQueryTypeEntry;
> +
> +static const DshowCapQueryTypeEntry query_table[] = {
> +    { "sample_format",  CAP_QUERY_SAMPLE_FORMAT },
> +    { "sample_rate",    CAP_QUERY_SAMPLE_RATE },
> +    { "channels",       CAP_QUERY_CHANNELS },
> +    { "codec",          CAP_QUERY_CODEC },
> +    { "pixel_format",   CAP_QUERY_PIXEL_FORMAT },
> +    { "frame_size",     CAP_QUERY_FRAME_SIZE },
> +    { "fps",            CAP_QUERY_FPS },
> +};
> +
> +static enum DshowCapQueryType dshow_get_query_type(const char* option_name)
> +{
> +    for (int i = 0; i < FF_ARRAY_ELEMS(query_table); ++i) {
> +        if (!strcmp(query_table[i].name, option_name))
> +            return query_table[i].query_type;
> +    }
> +    return CAP_QUERY_NONE;
> +}
> +
> +static const char* dshow_get_query_type_string(enum DshowCapQueryType query_type)
> +{
> +    for (int i = 0; i < FF_ARRAY_ELEMS(query_table); ++i) {
> +        if (query_table[i].query_type == query_type)
> +            return query_table[i].name;
> +    }
> +    return NULL;
> +}
>  
>  static enum AVPixelFormat dshow_pixfmt(DWORD biCompression, WORD biBitCount)
>  {
> @@ -54,6 +96,26 @@ static enum AVPixelFormat dshow_pixfmt(DWORD biCompression, WORD biBitCount)
>      return avpriv_find_pix_fmt(avpriv_get_raw_pix_fmt_tags(), biCompression); // all others
>  }
>  
> +static enum AVCodecID waveform_codec_id(enum AVSampleFormat sample_fmt)
> +{
> +    switch (sample_fmt) {
> +    case AV_SAMPLE_FMT_U8:  return AV_CODEC_ID_PCM_U8;
> +    case AV_SAMPLE_FMT_S16: return AV_CODEC_ID_PCM_S16LE;
> +    case AV_SAMPLE_FMT_S32: return AV_CODEC_ID_PCM_S32LE;
> +    default:                return AV_CODEC_ID_NONE; /* Should never happen. */
> +    }
> +}
> +
> +static enum AVSampleFormat sample_fmt_bits_per_sample(int bits)
> +{
> +    switch (bits) {
> +    case 8:  return AV_SAMPLE_FMT_U8;
> +    case 16: return AV_SAMPLE_FMT_S16;
> +    case 32: return AV_SAMPLE_FMT_S32;
> +    default: return AV_SAMPLE_FMT_NONE; /* Should never happen. */
> +    }
> +}
> +
>  static int
>  dshow_read_close(AVFormatContext *s)
>  {
> @@ -317,10 +379,13 @@ fail1:
>   * try to set parameters specified through AVOptions and if successful
>   * return 1 in *pformat_set.
>   * If pformat_set is NULL, list all pin capabilities.
> + * When listing pin capabilities, if ranges is NULL, output to log,
> + * else store capabilities in ranges.
>   */
>  static void
>  dshow_cycle_formats(AVFormatContext *avctx, enum dshowDeviceType devtype,
> -                    IPin *pin, int *pformat_set)
> +                    IPin *pin, int *pformat_set,
> +                    AVOptionRanges *ranges, enum DshowCapQueryType query_type)
>  {
>      struct dshow_ctx *ctx = avctx->priv_data;
>      IAMStreamConfig *config = NULL;
> @@ -338,7 +403,11 @@ dshow_cycle_formats(AVFormatContext *avctx, enum dshowDeviceType devtype,
>      if (!caps)
>          goto end;
>  
> -    for (i = 0; i < n && !format_set; i++) {
> +    for (i = 0; i < n && (!format_set || ranges); i++) {
> +        AVOptionRange *range = NULL;
> +        AVOptionRange **range_list = NULL;
> +        int nb_range = 0;
> +
>          r = IAMStreamConfig_GetStreamCaps(config, i, &type, (void *) caps);
>          if (r != S_OK)
>              goto next;
> @@ -365,7 +434,7 @@ dshow_cycle_formats(AVFormatContext *avctx, enum dshowDeviceType devtype,
>              } else {
>                  goto next;
>              }
> -            if (!pformat_set) {
> +            if (!pformat_set && !ranges) {
>                  enum AVPixelFormat pix_fmt = dshow_pixfmt(bih->biCompression, bih->biBitCount);
>                  if (pix_fmt == AV_PIX_FMT_NONE) {
>                      enum AVCodecID codec_id = av_codec_get_id(tags, bih->biCompression);
> @@ -410,6 +479,81 @@ dshow_cycle_formats(AVFormatContext *avctx, enum dshowDeviceType devtype,
>                  bih->biWidth  = ctx->requested_width;
>                  bih->biHeight = ctx->requested_height;
>              }
> +
> +            if (ranges) {
> +                if (query_type == CAP_QUERY_FRAME_SIZE) {
> +                    for (int j = 0; j < 3; j++) {
> +                        range = av_mallocz(sizeof(AVOptionRange));
> +                        if (!range)
> +                            goto next;
> +                        range->str = av_strdup(j == 0 ? "pixel_count" : (j == 1 ? "width" : (j == 2 ? "height" : "")));
> +                        if (!range->str)
> +                            goto next;
> +                        switch (j)
> +                        {
> +                        case 0:
> +                            range->value_min = vcaps->MinOutputSize.cx * vcaps->MinOutputSize.cy;
> +                            range->value_max = vcaps->MaxOutputSize.cx * vcaps->MaxOutputSize.cy;
> +                            break;
> +                        case 1:
> +                            range->value_min = vcaps->MinOutputSize.cx;
> +                            range->value_max = vcaps->MaxOutputSize.cx;
> +                            break;
> +                        case 2:
> +                            range->value_min = vcaps->MinOutputSize.cy;
> +                            range->value_max = vcaps->MaxOutputSize.cy;
> +                            break;
> +                        }
> +                        range->is_range = range->value_min != range->value_max;
> +
> +                        if (av_dynarray_add_nofree(&range_list,
> +                                                   &nb_range, range) < 0)
> +                            goto next;
> +                        range = NULL;  // copied into array, make sure not freed below
> +                    }
> +                }
> +                else {
> +                    range = av_mallocz(sizeof(AVOptionRange));
> +                    if (!range)
> +                        goto next;
> +                    range->str = av_strdup(dshow_get_query_type_string(query_type));
> +                    if (!range->str)
> +                        goto next;
> +
> +                    switch (query_type)
> +                    {
> +                    case CAP_QUERY_CODEC:
> +                        if (dshow_pixfmt(bih->biCompression, bih->biBitCount) == AV_PIX_FMT_NONE) {
> +                            const AVCodecTag* const tags[] = { avformat_get_riff_video_tags(), NULL };
> +                            range->value_min = av_codec_get_id(tags, bih->biCompression);
> +                        }
> +                        else
> +                            range->value_min = AV_CODEC_ID_RAWVIDEO;
> +                        range->value_max = range->value_min;
> +                        break;
> +                    case CAP_QUERY_PIXEL_FORMAT:
> +                        range->value_min = range->value_max = dshow_pixfmt(bih->biCompression, bih->biBitCount);
> +                        range->value_min;
> +                        break;
> +                    case CAP_QUERY_FPS:
> +                        range->value_min = 1e7 / vcaps->MaxFrameInterval;
> +                        range->value_max = 1e7 / vcaps->MinFrameInterval;
> +                        break;
> +
> +                    default:
> +                        // an audio property is being queried, output 0
> +                        range->value_min = range->value_max = 0;
> +                        break;
> +                    }
> +
> +                    range->is_range = range->value_min != range->value_max;
> +
> +                    if (av_dynarray_add_nofree(&range_list,
> +                                               &nb_range, range) < 0)
> +                        goto next;
> +                    range = NULL;  // copied into array, make sure not freed below
> +                }
> +            }
>          } else {
>              AUDIO_STREAM_CONFIG_CAPS *acaps = caps;
>              WAVEFORMATEX *fx;
> @@ -421,7 +565,7 @@ dshow_cycle_formats(AVFormatContext *avctx, enum dshowDeviceType devtype,
>              } else {
>                  goto next;
>              }
> -            if (!pformat_set) {
> +            if (!pformat_set && !ranges) {
>                  av_log(avctx, AV_LOG_INFO, "  min ch=%lu bits=%lu rate=%6lu max ch=%lu bits=%lu rate=%6lu\n",
>                         acaps->MinimumChannels, acaps->MinimumBitsPerSample, acaps->MinimumSampleFrequency,
>                         acaps->MaximumChannels, acaps->MaximumBitsPerSample, acaps->MaximumSampleFrequency);
> @@ -445,11 +589,90 @@ dshow_cycle_formats(AVFormatContext *avctx, enum dshowDeviceType devtype,
>                      goto next;
>                  fx->nChannels = ctx->channels;
>              }
> +
> +            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.

Anyway, there is quite a lot of code duplication (mallocz, strdup,
av_dynarray_add_nofree) here. Could this be avoided?

> +                        if (!range)
> +                            goto next;
> +                        range->str = av_strdup(j == 0 ? "pixel_count" : (j == 1 ? "width" : (j == 2 ? "height" : "")));
> +                        if (!range->str)
> +                            goto next;
> +                        // an video property is being queried, output 0
> +                        range->value_min = range->value_max = 0;
> +                        range->is_range = 0;
> +
> +                        if (av_dynarray_add_nofree(&range_list,
> +                                                   &nb_range, range) < 0)
> +                            goto next;
> +                        range = NULL;  // copied into array, make sure not freed below
> +                    }
> +                }
> +                else {
> +                    range = av_mallocz(sizeof(AVOptionRange));
> +                    if (!range)
> +                        goto next;
> +                    range->str = av_strdup(dshow_get_query_type_string(query_type));
> +                    if (!range->str)
> +                        goto next;
> +
> +                    switch (query_type)
> +                    {
> +                    case CAP_QUERY_SAMPLE_FORMAT:
> +                        range->value_min = sample_fmt_bits_per_sample(acaps->MinimumBitsPerSample);
> +                        range->value_max = sample_fmt_bits_per_sample(acaps->MaximumBitsPerSample);
> +                        break;
> +                    case CAP_QUERY_SAMPLE_RATE:
> +                        range->value_min = acaps->MinimumSampleFrequency;
> +                        range->value_max = acaps->MaximumSampleFrequency;
> +                        break;
> +                    case CAP_QUERY_CHANNELS:
> +                        range->value_min = acaps->MinimumChannels;
> +                        range->value_max = acaps->MaximumChannels;
> +                        break;
> +
> +                    default:
> +                        // an video property is being queried, output 0
> +                        range->value_min = range->value_max = 0;
> +                        break;
> +                    }
> +
> +                    range->is_range = range->value_min != range->value_max;
> +
> +                    if (av_dynarray_add_nofree(&range_list,
> +                                               &nb_range, range) < 0)
> +                        goto next;
> +                    range = NULL;  // copied into array, make sure not freed below
> +                }
> +            }
>          }
>          if (IAMStreamConfig_SetFormat(config, type) != S_OK)
>              goto next;
> +        else if (ranges) {
> +            // format matched and could be set successfully. Add capabilities to ranges output
> +            for (int j=0; j< nb_range; ++j) {
> +                if (av_dynarray_add_nofree(&ranges->range,
> +                                           &ranges->nb_ranges, range_list[j]) < 0)
> +                    goto next;

The way you are doing it here has two drawbacks:
1. av_dynarray_add_nofree() overallocates, but it only stores the amount
of currently valid entries, not the amount of allocated entries. To
achieve this, it simply presumes that the array has already been
allocated by av_dynarray_add_nofree (it overallocates to a power-of-two
and it presumes the array to be so allocated, so if range->nb_ranges is
(say) 9, it just presumes that there is enough space for the next
element). I don't know whether this is always fulfilled in this code,
but I don't like this implicit presumption.
2. You actually know how many elements your array will have at the end:
ranges->nb_ranges + nb_range. Why don't you reallocate the array once?

> +                range_list[j] = NULL;  // copied into array, make sure not freed below
> +            }
> +        }
>          format_set = 1;
>  next:
> +        if (range) {
> +            av_freep(&range->str);
> +            av_freep(&range);
> +        }
> +        if (range_list) {
> +            for (int j = 0; j < nb_range; ++j) {
> +                if (range_list[j]) {
> +                    av_freep(&range_list[j]->str);
> +                    av_freep(&range_list[j]);
> +                }
> +            }
> +            av_freep(&range_list);
> +        }
>          if (type->pbFormat)
>              CoTaskMemFree(type->pbFormat);
>          CoTaskMemFree(type);
> @@ -558,10 +781,13 @@ end:
>   * devtype, retrieve the first output pin and return the pointer to the
>   * object found in *ppin.
>   * If ppin is NULL, cycle through all pins listing audio/video capabilities.
> + * If ppin is not NULL and ranges is also not null, enumerate all formats
> + * supported by the selected pin.
>   */
>  static int
>  dshow_cycle_pins(AVFormatContext *avctx, enum dshowDeviceType devtype,
> -                 enum dshowSourceFilterType sourcetype, IBaseFilter *device_filter, IPin **ppin)
> +                 enum dshowSourceFilterType sourcetype, IBaseFilter *device_filter,
> +                 IPin **ppin, AVOptionRanges *ranges, enum DshowCapQueryType query_type)
>  {
>      struct dshow_ctx *ctx = avctx->priv_data;
>      IEnumPins *pins = 0;
> @@ -630,7 +856,7 @@ dshow_cycle_pins(AVFormatContext *avctx, enum dshowDeviceType devtype,
>  
>          if (!ppin) {
>              av_log(avctx, AV_LOG_INFO, " Pin \"%s\" (alternative pin name \"%s\")\n", name_buf, pin_buf);
> -            dshow_cycle_formats(avctx, devtype, pin, NULL);
> +            dshow_cycle_formats(avctx, devtype, pin, NULL, NULL, CAP_QUERY_NONE);
>              goto next;
>          }
>  
> @@ -642,13 +868,13 @@ dshow_cycle_pins(AVFormatContext *avctx, enum dshowDeviceType devtype,
>              }
>          }
>  
> -        if (set_format) {
> -            dshow_cycle_formats(avctx, devtype, pin, &format_set);
> +        if (set_format || ranges) {
> +            dshow_cycle_formats(avctx, devtype, pin, &format_set, ranges, query_type);
>              if (!format_set) {
>                  goto next;
>              }
>          }
> -        if (devtype == AudioDevice && ctx->audio_buffer_size) {
> +        if (devtype == AudioDevice && ctx->audio_buffer_size && !ranges) {
>              if (dshow_set_audio_buffer_size(avctx, pin) < 0) {
>                  av_log(avctx, AV_LOG_ERROR, "unable to set audio buffer size %d to pin, using pin anyway...", ctx->audio_buffer_size);
>              }
> @@ -673,8 +899,22 @@ next:
>              IEnumMediaTypes_Release(types);
>          if (p)
>              IKsPropertySet_Release(p);
> -        if (device_pin != pin)
> +        if (device_pin != pin) {
>              IPin_Release(pin);
> +            // clear all ranges info again, wrong pin
> +            if (ranges) {
> +                for (int i = 0; i < ranges->nb_ranges; i++) {
> +                    AVOptionRange* range = ranges->range[i];
> +                    if (range) {
> +                        av_freep(&range->str);
> +                        av_freep(&ranges->range[i]);
> +                    }
> +                }
> +                av_freep(&ranges->range);
> +                ranges->nb_ranges = 0;
> +            }
> +            device_pin = NULL;
> +        }
>          av_free(name_buf);
>          av_free(pin_buf);
>          if (pin_id)
> @@ -706,17 +946,19 @@ next:
>   */
>  static int
>  dshow_list_device_options(AVFormatContext *avctx, ICreateDevEnum *devenum,
> -                          enum dshowDeviceType devtype, enum dshowSourceFilterType sourcetype)
> +                          enum dshowDeviceType devtype, enum dshowSourceFilterType sourcetype,
> +                          AVOptionRanges *ranges, enum DshowCapQueryType query_type)
>  {
>      struct dshow_ctx *ctx = avctx->priv_data;
>      IBaseFilter *device_filter = NULL;
> +    IPin *device_pin = NULL;
>      char *device_unique_name = NULL;
>      int r;
>  
>      if ((r = dshow_cycle_devices(avctx, devenum, devtype, sourcetype, &device_filter, &device_unique_name)) < 0)
>          return r;
>      ctx->device_filter[devtype] = device_filter;
> -    if ((r = dshow_cycle_pins(avctx, devtype, sourcetype, device_filter, NULL)) < 0)
> +    if ((r = dshow_cycle_pins(avctx, devtype, sourcetype, device_filter, ranges ? &device_pin : NULL, ranges, query_type)) < 0)
>          return r;
>      av_freep(&device_unique_name);
>      return 0;
> @@ -800,7 +1042,7 @@ dshow_open_device(AVFormatContext *avctx, ICreateDevEnum *devenum,
>          goto error;
>      }
>  
> -    if ((r = dshow_cycle_pins(avctx, devtype, sourcetype, device_filter, &device_pin)) < 0) {
> +    if ((r = dshow_cycle_pins(avctx, devtype, sourcetype, device_filter, &device_pin, NULL, CAP_QUERY_NONE)) < 0) {
>          ret = r;
>          goto error;
>      }
> @@ -912,26 +1154,6 @@ error:
>      return ret;
>  }
>  
> -static enum AVCodecID waveform_codec_id(enum AVSampleFormat sample_fmt)
> -{
> -    switch (sample_fmt) {
> -    case AV_SAMPLE_FMT_U8:  return AV_CODEC_ID_PCM_U8;
> -    case AV_SAMPLE_FMT_S16: return AV_CODEC_ID_PCM_S16LE;
> -    case AV_SAMPLE_FMT_S32: return AV_CODEC_ID_PCM_S32LE;
> -    default:                return AV_CODEC_ID_NONE; /* Should never happen. */
> -    }
> -}
> -
> -static enum AVSampleFormat sample_fmt_bits_per_sample(int bits)
> -{
> -    switch (bits) {
> -    case 8:  return AV_SAMPLE_FMT_U8;
> -    case 16: return AV_SAMPLE_FMT_S16;
> -    case 32: return AV_SAMPLE_FMT_S32;
> -    default: return AV_SAMPLE_FMT_NONE; /* Should never happen. */
> -    }
> -}
> -
>  static int
>  dshow_add_device(AVFormatContext *avctx,
>                   enum dshowDeviceType devtype)
> @@ -1138,14 +1360,14 @@ static int dshow_read_header(AVFormatContext *avctx)
>      }
>      if (ctx->list_options) {
>          if (ctx->device_name[VideoDevice])
> -            if ((r = dshow_list_device_options(avctx, devenum, VideoDevice, VideoSourceDevice))) {
> +            if ((r = dshow_list_device_options(avctx, devenum, VideoDevice, VideoSourceDevice, NULL, CAP_QUERY_NONE))) {
>                  ret = r;
>                  goto error;
>              }
>          if (ctx->device_name[AudioDevice]) {
> -            if (dshow_list_device_options(avctx, devenum, AudioDevice, AudioSourceDevice)) {
> +            if (dshow_list_device_options(avctx, devenum, AudioDevice, AudioSourceDevice, NULL, CAP_QUERY_NONE)) {
>                  /* show audio options from combined video+audio sources as fallback */
> -                if ((r = dshow_list_device_options(avctx, devenum, AudioDevice, VideoSourceDevice))) {
> +                if ((r = dshow_list_device_options(avctx, devenum, AudioDevice, VideoSourceDevice, NULL, CAP_QUERY_NONE))) {
>                      ret = r;
>                      goto error;
>                  }
> @@ -1289,6 +1511,208 @@ static int dshow_read_packet(AVFormatContext *s, AVPacket *pkt)
>      return ctx->eof ? AVERROR(EIO) : pkt->size;
>  }
>  
> +// TODO: how to expose extra info? If no field found, check if it in a set of extra keys (like color_range)?
> +static int dshow_query_ranges_func(AVOptionRanges** ranges_arg, void* obj, const char* key, int flags)
> +{
> +    AVDeviceCapabilitiesQuery *caps = obj;
> +    const AVFormatContext *avctx = caps->device_context;

avctx is only used for AVCodecContexts by convention.

> +    struct dshow_ctx *ctx = avctx->priv_data;
> +
> +    int backup_sample_size;
> +    int backup_sample_rate;
> +    int backup_channels;
> +    enum AVCodecID backup_video_codec_id;
> +    enum AVPixelFormat backup_pixel_format;
> +    int backup_requested_width;
> +    int backup_requested_height;
> +    char* backup_framerate = NULL;
> +
> +    enum DshowCapQueryType query_type = CAP_QUERY_NONE;
> +
> +    AVOptionRanges *ranges = av_mallocz(sizeof(AVOptionRanges));

I have to admit that my first instinct here was to mention that this
can't be legal because it would make sizeof(AVOptionRanges) part of the
public ABI, but given that av_opt_query_ranges() does indeed not
allocate the AVOptionRange for us this is how it seems to be.

> +    const AVOption *field = av_opt_find(obj, key, NULL, 0, flags);
> +    int ret;
> +
> +    ICreateDevEnum *devenum = NULL;
> +
> +    *ranges_arg = NULL;
> +
> +    if (!ranges) {
> +        ret = AVERROR(ENOMEM);
> +        goto fail1;
> +    }
> +
> +    if (!field) {
> +        ret = AVERROR_OPTION_NOT_FOUND;
> +        goto fail1;
> +    }
> +
> +    // turn option name into cap query
> +    query_type = dshow_get_query_type(field->name);
> +
> +    if (query_type == CAP_QUERY_NONE) {
> +        av_log(avctx, AV_LOG_ERROR, "Querying the option %s is not supported for a dshow device\n",field->name);
> +        ret = AVERROR(EINVAL);
> +        goto fail1;
> +    }
> +
> +    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?

> +        ret = AVERROR(EIO);
> +        goto fail1;
> +    }
> +
> +    if (!parse_device_name(avctx)) {
> +        av_log(avctx, AV_LOG_ERROR, "You must set a device name (AVFormatContext url) to specify which device to query capabilities from\n");
> +        ret = AVERROR(EINVAL);
> +        goto fail1;
> +    }
> +
> +    // take backup of dshow parameters/options
> +    // audio
> +    backup_sample_size = ctx->sample_size;
> +    backup_sample_rate = ctx->sample_rate;
> +    backup_channels = ctx->channels;
> +    // video
> +    backup_video_codec_id = ctx->video_codec_id;
> +    backup_pixel_format = ctx->pixel_format;
> +    backup_requested_width = ctx->requested_width;
> +    backup_requested_height= ctx->requested_height;
> +    backup_framerate = ctx->framerate;
> +
> +
> +    // set format constraints set in AVDeviceCapabilitiesQuery
> +    // audio (NB: channel_layout not used)
> +    ctx->sample_size = av_get_bytes_per_sample(caps->sample_format) << 3;
> +    ctx->sample_rate = (caps->sample_rate == -1) ? 0 : caps->sample_rate;
> +    ctx->channels = (caps->channels == -1) ? 0 : caps->channels;
> +    // video (NB: window_width and window_height not used)
> +    ctx->video_codec_id = caps->codec;
> +    ctx->pixel_format = caps->pixel_format;
> +    ctx->requested_width = caps->frame_width;
> +    ctx->requested_height = caps->frame_height;
> +    // checking whether requested framerate is set is done by !ctx->framerate
> +    if (caps->fps.num > 0 && caps->fps.den > 0) {
> +        ctx->requested_framerate = caps->fps;
> +        ctx->framerate = av_strdup("dummy");    // just make sure its non-zero
> +        if (!ctx->framerate) {
> +            ret = AVERROR(ENOMEM);
> +            goto fail2;
> +        }
> +    }
> +    else
> +        ctx->framerate = NULL;  // make sure its NULL (if it wasn't, we already have a backup of the pointer to restore later)
> +
> +    // now iterate matching format of pin that would be selected when device
> +    // is opened with options currently in effect.
> +    // for each matching format, output its parameter range, also if that same
> +    // range already returned for another format. That way, user can reconstruct
> +    // possible valid combinations by av_opt_query_ranges() for each of the
> +    // format options and matching returned values by sequence number.
> +    CoInitialize(0);
> +
> +    if (CoCreateInstance(&CLSID_SystemDeviceEnum, NULL, CLSCTX_INPROC_SERVER,
> +                         &IID_ICreateDevEnum, (void **) &devenum)) {
> +        av_log(avctx, AV_LOG_ERROR, "Could not enumerate system devices.\n");
> +        goto fail2;

You forgot to set ret.

> +    }
> +    ctx->video_codec_id = avctx->video_codec_id ? avctx->video_codec_id
> +                                                : AV_CODEC_ID_RAWVIDEO;
> +
> +    ranges->nb_components = field->type == AV_OPT_TYPE_IMAGE_SIZE ? 3 : 1;
> +    if (ctx->device_name[VideoDevice])
> +        if ((ret = dshow_list_device_options(avctx, devenum, VideoDevice, VideoSourceDevice, ranges, query_type)) < 0)
> +            goto fail2;
> +    if (ctx->device_name[AudioDevice]) {
> +        if (dshow_list_device_options(avctx, devenum, AudioDevice, AudioSourceDevice, ranges, query_type) < 0) {
> +            /* show audio options from combined video+audio sources as fallback */
> +            if ((ret = dshow_list_device_options(avctx, devenum, AudioDevice, VideoSourceDevice, ranges, query_type)) < 0)
> +                goto fail2;
> +        }
> +    }
> +    ret = ranges->nb_ranges ? ranges->nb_components : 0;
> +
> +    // when dealing with a multi-component item (regardless of whether
> +    // AV_OPT_MULTI_COMPONENT_RANGE is set or not), we need to reorganize the
> +    // output range array from [r1_c1 r1_c2 r1_c3 r2_c1 r2_c2 r2_c3 ...] to
> +    // [r1_c1 r2_c1 ... r1_c2 r2_c2 ... r1_c3 r2_c3 ...] to be consistent with
> +    // documentation of AVOptionRanges in libavutil/opt.h
> +    // this is only the case for a AV_OPT_TYPE_IMAGE_SIZE option.
> +    if (ranges->nb_ranges && ranges->nb_components>1) {
> +        AVOptionRanges* old_ranges;

Actually, all you need is a second range array (which could even be
avoided if one performed the transpose in-place, but I don't know how
much effort it would be to implement this).

> +        ranges->nb_ranges /= ranges->nb_components;
> +        old_ranges = ranges;
> +        ranges = av_mallocz(sizeof(AVOptionRanges));
> +        if (!ranges) {
> +            ranges = old_ranges;    // for cleanup
> +            ret = AVERROR(ENOMEM);
> +            goto fail2;
> +        }
> +        ranges->nb_components = old_ranges->nb_components;
> +        ranges->nb_ranges = old_ranges->nb_ranges;
> +        ranges->range = av_malloc(ranges->nb_components * ranges->nb_ranges * sizeof(AVOptionRange*));

Missing check; also, using sizeof(*ranges->range) increases clarity (I
already wanted to complain about you using the wrong type here until I
noticed that it is an array of arrays).

> +
> +        for (int n = 0; n < ranges->nb_components * ranges->nb_ranges; n++) {
> +            int i = n / ranges->nb_components;
> +            int j = n % ranges->nb_components;
> +            ranges->range[ranges->nb_ranges * j + i] = old_ranges->range[n];
> +            old_ranges->range[n] = NULL; // don't keep double references
> +        }
> +
> +        av_opt_freep_ranges(&old_ranges);
> +    }
> +
> +    // success, set output
> +    *ranges_arg = ranges;
> +
> +fail2:
> +    // set dshow parameters/options back to original values
> +    // audio
> +    ctx->sample_size = backup_sample_size;
> +    ctx->sample_rate = backup_sample_rate;
> +    ctx->channels = backup_channels;
> +    // video
> +    ctx->video_codec_id = backup_video_codec_id;
> +    ctx->pixel_format = backup_pixel_format;
> +    ctx->requested_width = backup_requested_width;
> +    ctx->requested_height = backup_requested_height;
> +    if (ctx->framerate)
> +        av_free(ctx->framerate);
> +    ctx->framerate = backup_framerate;
> +
> +    if (devenum)
> +        ICreateDevEnum_Release(devenum);
> +
> +    dshow_read_close(avctx); // clears filenames and removes device_filters or other state variables that may have been set
> +
> +fail1:
> +    if (ret < 0)
> +        av_opt_freep_ranges(&ranges);
> +
> +    return ret;
> +}
> +
> +// fake class to point av_opt_query_ranges to our query_ranges function
> +static const AVClass dshow_dev_caps_class = {
> +    .class_name = "",
> +    .item_name = av_default_item_name,
> +    .option = av_device_capabilities,
> +    .version = LIBAVDEVICE_VERSION_INT,

LIBAVUTIL_VERSION_INT (this is meant to make AVClass extensible even
without a major bump).

> +    .query_ranges = dshow_query_ranges_func,

We typically don't add "_func" to function pointers (as you can see from
the other function pointers set above).

Furthermore, aligning this on '=' improves legibility.

> +};
> +
> +static int dshow_create_device_capabilities(struct AVFormatContext* s, struct AVDeviceCapabilitiesQuery* caps)
> +{
> +    caps->av_class = &dshow_dev_caps_class;
> +    return 0;
> +}
> +
> +static int dshow_free_device_capabilities(struct AVFormatContext* s, struct AVDeviceCapabilitiesQuery* caps)
> +{
> +    // nothing to clean up

Then this function should not exist at all (after all,
avdevice_capabilities_free() checks for whether this function exists, it
is not called unconditionally).

> +    return 0;
> +}
> +
>  #define OFFSET(x) offsetof(struct dshow_ctx, x)
>  #define DEC AV_OPT_FLAG_DECODING_PARAM
>  static const AVOption options[] = {
> @@ -1335,6 +1759,8 @@ const AVInputFormat ff_dshow_demuxer = {
>      .read_header    = dshow_read_header,
>      .read_packet    = dshow_read_packet,
>      .read_close     = dshow_read_close,
> +    .create_device_capabilities = dshow_create_device_capabilities,
> +    .free_device_capabilities = dshow_free_device_capabilities,
>      .flags          = AVFMT_NOFILE,
>      .priv_class     = &dshow_class,
>  };
>

On Sat, Jun 5, 2021 at 2:25 PM Andreas Rheinhardt
<andreas.rheinhardt@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?

> > +    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?

> > +    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.

Thanks!
Dee

Diederick C. Niehorster:
> On Sat, Jun 5, 2021 at 2:25 PM Andreas Rheinhardt
> <andreas.rheinhardt@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

On Sun, Jun 6, 2021 at 6:15 AM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
> Diederick C. Niehorster:
> > 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.)

It is not supposed to, but does work. However, as user you still need
to set the device options in the opts dict passed to
avformat_open_input() even after narrowing down the format you want to
use through the capabilities API. I'll document this. NB: reusing the
AVFormatContext currently leaks a little bit of memory as, e.g.,
avformat_open_input() set url without checking and freeing if its
already set. And maybe more. I'll just add a patch with my proposed
API extension avformat_alloc_input_context() and needed extra checks
to avformat_open_input(), so we can discuss it.

All the best,
Dee