[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