[FFmpeg-devel] Fw: [PATCH] DirectShow patches

Ramiro Polla ramiro.polla at gmail.com
Fri Sep 9 05:29:47 CEST 2011


Hi,

On Thu, Sep 8, 2011 at 11:55 AM, Stefano Sabatini
<stefano.sabatini-lala at poste.it> wrote:
> On date Wednesday 2011-09-07 21:34:58 -0300, Ramiro Polla encoded:

>> From 72bf72ce30c09dff4977ef3b4f12f80caff931e4 Mon Sep 17 00:00:00 2001
>> From: Ramiro Polla <ramiro.polla at gmail.com>
>> Date: Wed, 7 Sep 2011 21:09:31 -0300
>> Subject: [PATCH 1/9] dshow: factorise cycling through devices
>>
>> ---
>>  libavdevice/dshow.c |   48 ++++++++++++++++++++++++++++++++++--------------
>>  1 files changed, 34 insertions(+), 14 deletions(-)
>>
>> diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c
>> index 10e3d4f..aa29b6b 100644
>> --- a/libavdevice/dshow.c
>> +++ b/libavdevice/dshow.c
>> @@ -214,35 +214,26 @@ fail:
>>  }
>>
>>  static int
>> -dshow_open_device(AVFormatContext *avctx, ICreateDevEnum *devenum,
>> -                  enum dshowDeviceType devtype)
>> +dshow_cycle_devices(AVFormatContext *avctx, ICreateDevEnum *devenum,
>> +                    enum dshowDeviceType devtype, IBaseFilter **pfilter)
>
> Please add a short doxy, it is not that easy to understand what the
> code does.
>
> From my understanding:
>
> |Cycle through available devices using the device enumerator devenum,
> |retrieve the one with type specified by devtype and return the
> |pointer to the found object in *pfilter.

Done, and for all other functions that have similar functionality too.

>> From 142f9aaa7c1511b769d5ebbba16279cba164f987 Mon Sep 17 00:00:00 2001
>> From: Ramiro Polla <ramiro.polla at gmail.com>
>> Date: Wed, 7 Sep 2011 21:09:41 -0300
>> Subject: [PATCH 2/9] dshow: add option to list devices
>>
>> ---
>>  libavdevice/dshow.c |   40 +++++++++++++++++++++++++++++++++++++++-
>>  1 files changed, 39 insertions(+), 1 deletions(-)
>>
>> diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c
>> index aa29b6b..45137d5 100644
>> --- a/libavdevice/dshow.c
>> +++ b/libavdevice/dshow.c
>> @@ -19,14 +19,20 @@
>>   * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>>   */
>>
>> +#include "libavutil/opt.h"
>> +
>>  #include "avdevice.h"
>>  #include "dshow.h"
>>
>>  struct dshow_ctx {
>> +    AVClass *class;
>
> const?

Copied from some other place that didn't have const. Note that they're
in consistent in libavfilter.

>> From 203b67b6e3c7348e973772ccc7ea30d3ab679e0e Mon Sep 17 00:00:00 2001
>> From: Ramiro Polla <ramiro.polla at gmail.com>
>> Date: Wed, 7 Sep 2011 21:11:32 -0300
>> Subject: [PATCH 6/9] dshow: add audio/video options
>>
>> ---
>>  libavdevice/dshow.c        |  154 ++++++++++++++++++++++++++++++++++++++++++++
>>  libavdevice/dshow.h        |    2 +
>>  libavdevice/dshow_common.c |   49 ++++++++++++++
>>  3 files changed, 205 insertions(+), 0 deletions(-)
>>
>> diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c
>> index 0d8042c..27bbeaf 100644
>> --- a/libavdevice/dshow.c
>> +++ b/libavdevice/dshow.c
>> @@ -19,6 +19,7 @@
>>   * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>>   */
>>
>> +#include "libavutil/parseutils.h"
>>  #include "libavutil/opt.h"
>>
>>  #include "avdevice.h"
>> @@ -46,6 +47,17 @@ struct dshow_ctx {
>>      unsigned int video_frame_num;
>>
>>      IMediaControl *control;
>> +
>> +    char *video_size;
>> +    char *framerate;
>> +
>> +    int requested_width;
>> +    int requested_height;
>> +    AVRational requested_framerate;
>> +
>> +    int sample_rate;
>> +    int sample_size;
>> +    int channels;
>>  };
>>
>>  static enum PixelFormat dshow_pixfmt(DWORD biCompression, WORD biBitCount)
>> @@ -289,10 +301,117 @@ fail1:
>>      return 0;
>>  }
>>
>> +static void
>> +dshow_cycle_formats(AVFormatContext *avctx, enum dshowDeviceType devtype,
>> +                    IPin *pin, AM_MEDIA_TYPE *type, int *pset)
>
> Again: a doxy may help here, especially given the funny names MS devs
> like to use for their APIs.
>
> Nit: pset -> pformat_set should be more clear/less ambiguous

Done.

>> @@ -301,6 +420,10 @@ dshow_cycle_pins(AVFormatContext *avctx, enum dshowDeviceType devtype,
>>      const GUID *mediatype[2] = { &MEDIATYPE_Video, &MEDIATYPE_Audio };
>>      const char *devtypename = (devtype == VideoDevice) ? "video" : "audio";
>>
>> +    int set_format = (devtype == VideoDevice && (ctx->video_size || ctx->framerate))
>> +                  || (devtype == AudioDevice && (ctx->channels || ctx->sample_rate));
>> +    int format_set = !set_format;
>
> ugh, this is confusing, I'd prefer to keep format_set to 0.
>
>> +
>>      r = IBaseFilter_EnumPins(device_filter, &pins);
>>      if (r != S_OK) {
>>          av_log(avctx, AV_LOG_ERROR, "Could not enumerate pins.\n");
>> @@ -328,6 +451,13 @@ dshow_cycle_pins(AVFormatContext *avctx, enum dshowDeviceType devtype,
>>          if (!IsEqualGUID(&category, &PIN_CATEGORY_CAPTURE))
>>              goto next;
>>
>> +        if (set_format) {
>> +            dshow_cycle_formats(avctx, devtype, pin, type, &format_set);
>> +            if (!format_set) {
>> +                goto next;
>> +            }
>> +        }
>> +
>>          if (IPin_EnumMediaTypes(pin, &types) != S_OK)
>>              goto next;
>>
>> @@ -351,6 +481,10 @@ next:
>>
>>      IEnumPins_Release(pins);
>>
>
>> +    if (!format_set) {
>> +        av_log(avctx, AV_LOG_ERROR, "Could not set %s options\n", devtypename);
>> +        return AVERROR(EIO);
>> +    }
>
> and change the check here to:
> if (set_format && !format_set) // it was requested to set format, but no format could be set

Ok.

>>      if (!device_pin) {
>>          av_log(avctx, AV_LOG_ERROR,
>>                 "Could not find output pin from %s capture device.\n", devtypename);
>> @@ -583,6 +717,21 @@ static int dshow_read_header(AVFormatContext *avctx, AVFormatParameters *ap)
>>          goto error;
>>      }
>>
>> +    if (ctx->video_size) {
>> +        r = av_parse_video_size(&ctx->requested_width, &ctx->requested_height, ctx->video_size);
>> +        if (r < 0) {
>> +            av_log(avctx, AV_LOG_ERROR, "Couldn't parse video size.\n");
>> +            goto error;
>> +        }
>> +    }
>> +    if (ctx->framerate) {
>> +        r = av_parse_video_rate(&ctx->requested_framerate, ctx->framerate);
>> +        if (r < 0) {
>> +            av_log(avctx, AV_LOG_ERROR, "Could not parse framerate '%s'.\n", ctx->framerate);
>> +            goto error;
>> +        }
>> +    }
>> +
>>      CoInitialize(0);
>>
>>      r = CoCreateInstance(&CLSID_FilterGraph, NULL, CLSCTX_INPROC_SERVER,
>> @@ -699,6 +848,11 @@ static int dshow_read_packet(AVFormatContext *s, AVPacket *pkt)
>>  #define OFFSET(x) offsetof(struct dshow_ctx, x)
>>  #define DEC AV_OPT_FLAG_DECODING_PARAM
>>  static const AVOption options[] = {
>> +    { "video_size", "A string describing frame size, such as 640x480 or hd720.", OFFSET(video_size), FF_OPT_TYPE_STRING, {.str = NULL}, 0, 0, DEC },
>
> Nit: "set video size given a string such as  ..."

Again, it was copied from some other place =).

>> From 0bec081602d583b915d88a831384db251869c137 Mon Sep 17 00:00:00 2001
>> From: Ramiro Polla <ramiro.polla at gmail.com>
>> Date: Wed, 7 Sep 2011 21:14:59 -0300
>> Subject: [PATCH 7/9] dshow: add option to list audio/video options
>>
>> ---
>>  libavdevice/dshow.c |   55 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 55 insertions(+), 0 deletions(-)
>>
>> diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c
>> index 27bbeaf..ba08e31 100644
>> --- a/libavdevice/dshow.c
>> +++ b/libavdevice/dshow.c
>> @@ -32,6 +32,7 @@ struct dshow_ctx {
>>
>>      char *device_name[2];
>>
>> +    int   list_options;
>>      int   list_devices;
>>
>>      IBaseFilter *device_filter[2];
>> @@ -345,6 +346,14 @@ dshow_cycle_formats(AVFormatContext *avctx, enum dshowDeviceType devtype,
>>              } else {
>>                  goto next;
>>              }
>> +            if (!pset) {
>> +                av_log(avctx, AV_LOG_INFO, "  min %ldx%ld %gfps max %ldx%ld %gfps\n",
>
> uhm... still a bit strange, I suggest:
> min s=WxH fps=N max s=WxH fps=N
>
> easier to parse, for both men and machines.
>
>> +                       vcaps->MinOutputSize.cx, vcaps->MinOutputSize.cy,
>> +                       1e7 / vcaps->MinFrameInterval,
>> +                       vcaps->MaxOutputSize.cx, vcaps->MaxOutputSize.cy,
>> +                       1e7 / vcaps->MaxFrameInterval);
>> +                continue;
>> +            }
>>              if (ctx->framerate) {
>>                  int64_t framerate = ((int64_t) ctx->requested_framerate.den*10000000)
>>                                              /  ctx->requested_framerate.num;
>> @@ -373,6 +382,12 @@ dshow_cycle_formats(AVFormatContext *avctx, enum dshowDeviceType devtype,
>>              } else {
>>                  goto next;
>>              }
>
>> +            if (!pset) {
>> +                av_log(avctx, AV_LOG_INFO, "  min %luch %lu-bit %6luHz max %luch %lu-bit %6luHz\n",
>
> Same here, I suggest:
>   min ch=N bps=B rate=RHz max ch=N bps=B rate=RHz

bps reminds me of bits per second, so I kept that as bits.

>> @@ -491,6 +519,22 @@ next:
>>          return AVERROR(EIO);
>>      }
>>      *ppin = device_pin;
>> +    }
>> +
>> +    return 0;
>> +}
>
> In general I'm not very fond of this kind of code, indeed it took me a
> bit of time to understand it, and I feel it can be made more clear by
> creating specialized functions with no "hybrid" behavior depending on
> the passed parameters.

I fear they would lead to much more code duplication, and that's what
I'm trying to avoid.

New patches attached, with a few more for proper cleanup. I believe
you may directly apply approved patches.

Ramiro
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-dshow-factorise-cycling-through-devices.patch
Type: text/x-patch
Size: 3696 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110909/2d9550e7/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-dshow-add-option-to-list-devices.patch
Type: text/x-patch
Size: 3954 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110909/2d9550e7/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-dshow-indent.patch
Type: text/x-patch
Size: 1526 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110909/2d9550e7/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-dshow-factorise-cycling-through-pins.patch
Type: text/x-patch
Size: 3700 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110909/2d9550e7/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-dshow-initialize-variable-to-prevent-releasing-rando.patch
Type: text/x-patch
Size: 830 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110909/2d9550e7/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-dshow-add-audio-video-options.patch
Type: text/x-patch
Size: 12551 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110909/2d9550e7/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-dshow-add-option-to-list-audio-video-options.patch
Type: text/x-patch
Size: 6038 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110909/2d9550e7/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0008-dshow-indent.patch
Type: text/x-patch
Size: 1477 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110909/2d9550e7/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0009-doc-add-documentation-for-dshow-indev.patch
Type: text/x-patch
Size: 2471 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110909/2d9550e7/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0010-dshow-release-pin-on-disconnect.patch
Type: text/x-patch
Size: 695 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110909/2d9550e7/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0011-dshow-cleanup-filters-and-pins-on-capture-interface.patch
Type: text/x-patch
Size: 1644 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110909/2d9550e7/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0012-dshow-invert-condition-to-avoid-leaking-objects.patch
Type: text/x-patch
Size: 1754 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110909/2d9550e7/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0013-dshow-remove-filters-directly-instead-of-enumerating.patch
Type: text/x-patch
Size: 2114 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110909/2d9550e7/attachment-0012.bin>


More information about the ffmpeg-devel mailing list