[FFmpeg-devel] Fw: [PATCH] DirectShow patches
Ramiro Polla
ramiro.polla at gmail.com
Tue Sep 13 20:14:04 CEST 2011
On Fri, Sep 9, 2011 at 7:08 PM, Stefano Sabatini
<stefano.sabatini-lala at poste.it> wrote:
> On date Friday 2011-09-09 00:29:47 -0300, Ramiro Polla encoded:
>> From dfda276cd1458fbd9409dd7284ada7c97ac734bb Mon Sep 17 00:00:00 2001
>> From: Ramiro Polla <ramiro.polla at gmail.com>
>> Date: Fri, 9 Sep 2011 00:10:07 -0300
>> Subject: [PATCH 04/13] dshow: factorise cycling through pins
>>
>> ---
>> libavdevice/dshow.c | 73 ++++++++++++++++++++++++++++++++------------------
>> 1 files changed, 47 insertions(+), 26 deletions(-)
>>
>> diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c
>> index 7035337..a64e645 100644
>> --- a/libavdevice/dshow.c
>> +++ b/libavdevice/dshow.c
>> @@ -295,42 +295,27 @@ fail1:
>> return 0;
>> }
>>
>> +/**
>> + * Cycle through available pins using the device_filter device, retrieve
>> + * the first output pin and return the pointer to the object found in
>> + * *ppin.
>> + */
>
> Please also mention the devtype parameter: maybe something like:
>
> |retrieve the first output pin with media type specified by devtype,
> |and return ...
Done.
>> From 986ff7585bbb6485e00eb9f6085e6f6e2cb7cfde Mon Sep 17 00:00:00 2001
>> From: Ramiro Polla <ramiro.polla at gmail.com>
>> Date: Fri, 9 Sep 2011 00:12:42 -0300
>> Subject: [PATCH 06/13] dshow: add audio/video options
>>
>> ---
>> libavdevice/dshow.c | 159 ++++++++++++++++++++++++++++++++++++++++++++
>> libavdevice/dshow.h | 2 +
>> libavdevice/dshow_common.c | 49 ++++++++++++++
>> 3 files changed, 210 insertions(+), 0 deletions(-)
>>
>> diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c
>> index fe1598a..9159ba6 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)
>> @@ -296,6 +308,117 @@ fail1:
>> }
>>
>
>> /**
>> + * Cycle through available formats using the specified pin,
>> + * try to set parameters specified through AVOptions and if successful
>> + * return 1 in *pformat_set.
>> + */
>> +static void
>> +dshow_cycle_formats(AVFormatContext *avctx, enum dshowDeviceType devtype,
>> + IPin *pin, AM_MEDIA_TYPE *type, int *pformat_set)
>> +{
>
> Please also document what AM_MEDIA_TYPE type is good for.
It was good for nothing =). Added a local variable instead.
>> + struct dshow_ctx *ctx = avctx->priv_data;
>> + IAMStreamConfig *config = NULL;
>> + int format_set = 0;
>> + void *caps = NULL;
>> + int i, n, size;
>> +
>> + if (IPin_QueryInterface(pin, &IID_IAMStreamConfig, (void **) &config) != S_OK)
>> + return;
>> + if (IAMStreamConfig_GetNumberOfCapabilities(config, &n, &size) != S_OK)
>> + goto end;
>> +
>> + caps = av_malloc(size);
>> + if (!caps)
>> + goto end;
>> +
>> + for (i = 0; i < n && !format_set; i++) {
>> + IAMStreamConfig_GetStreamCaps(config, i, &type, (void *) caps);
>> +
>> +#if DSHOWDEBUG
>> + ff_print_AM_MEDIA_TYPE(type);
>> +#endif
>> +
>> + if (devtype == VideoDevice) {
>> + VIDEO_STREAM_CONFIG_CAPS *vcaps = caps;
>> + BITMAPINFOHEADER *bih;
>> + int64_t *fr;
>> +#if DSHOWDEBUG
>> + ff_print_VIDEO_STREAM_CONFIG_CAPS(vcaps);
>> +#endif
>> + if (IsEqualGUID(&type->formattype, &FORMAT_VideoInfo)) {
>> + VIDEOINFOHEADER *v = (void *) type->pbFormat;
>> + fr = &v->AvgTimePerFrame;
>> + bih = &v->bmiHeader;
>> + } else if (IsEqualGUID(&type->formattype, &FORMAT_VideoInfo2)) {
>> + VIDEOINFOHEADER2 *v = (void *) type->pbFormat;
>> + fr = &v->AvgTimePerFrame;
>> + bih = &v->bmiHeader;
>> + } else {
>> + goto next;
>> + }
>
>> + if (ctx->framerate) {
>> + int64_t framerate = ((int64_t) ctx->requested_framerate.den*10000000)
>> + / ctx->requested_framerate.num;
>> + if (framerate > vcaps->MaxFrameInterval ||
>> + framerate < vcaps->MinFrameInterval)
>> + goto next;
>> + *fr = framerate;
>> + }
>> + if (ctx->video_size) {
>> + if (ctx->requested_width > vcaps->MaxOutputSize.cx ||
>> + ctx->requested_width < vcaps->MinOutputSize.cx ||
>> + ctx->requested_height > vcaps->MaxOutputSize.cy ||
>> + ctx->requested_height < vcaps->MinOutputSize.cy)
>> + goto next;
>> + bih->biWidth = ctx->requested_width;
>> + bih->biHeight = ctx->requested_height;
>> + }
>
> Note: in case framerate is set and video_size is invalid, framerate is
> set but then code fails and goes to the next capability, not sure this
> is a problem though. Maybe you may set both fr and bih only when both
> checks have been passed.
Those values get set in a temporary variable that gets freed at the
end of the loop, so it's ok to change its contents like this.
>> @@ -594,6 +733,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;
>> + }
>
> nit: "Could not parse video size '%s'.\n"
>
> Rationale: contractions are better avoided (easier on non fluent
> readers), pointing the invalid string improves feedback.
Copy&paste, it's this way in other places. Changed anyways.
>> + }
>> + 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,
>> @@ -710,6 +864,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", "set video size given a string such as 640x480 or hd720.", OFFSET(video_size), FF_OPT_TYPE_STRING, {.str = NULL}, 0, 0, DEC },
>> + { "framerate", "set video frame rate", OFFSET(framerate), FF_OPT_TYPE_STRING, {.str = NULL}, 0, 0, DEC },
>> + { "sample_rate", "set audio sample rate, such as 8000 or 44100", OFFSET(sample_rate), FF_OPT_TYPE_INT, {.dbl = 0}, 0, INT_MAX, DEC },
>> + { "sample_size", "set audio sample size, such as 8 or 16", OFFSET(sample_size), FF_OPT_TYPE_INT, {.dbl = 0}, 0, 16, DEC },
>> + { "channels", "set number of audio channels, such as 1 or 2", OFFSET(channels), FF_OPT_TYPE_INT, {.dbl = 0}, 0, INT_MAX, DEC },
>> { "list_devices", "list available devices", OFFSET(list_devices), FF_OPT_TYPE_INT, {.dbl=0}, 0, 1, DEC, "list_devices" },
>> { "true", "", 0, FF_OPT_TYPE_CONST, {.dbl=1}, 0, 0, DEC, "list_devices" },
>> { "false", "", 0, FF_OPT_TYPE_CONST, {.dbl=0}, 0, 0, DEC, "list_devices" },
>> diff --git a/libavdevice/dshow.h b/libavdevice/dshow.h
>> index 4e79680..83c71c4 100644
>> --- a/libavdevice/dshow.h
>> +++ b/libavdevice/dshow.h
>> @@ -29,6 +29,8 @@
>> #include <dvdmedia.h>
>>
>> long ff_copy_dshow_media_type(AM_MEDIA_TYPE *dst, const AM_MEDIA_TYPE *src);
>> +void ff_print_VIDEO_STREAM_CONFIG_CAPS(const VIDEO_STREAM_CONFIG_CAPS *caps);
>> +void ff_print_AUDIO_STREAM_CONFIG_CAPS(const AUDIO_STREAM_CONFIG_CAPS *caps);
>> void ff_print_AM_MEDIA_TYPE(const AM_MEDIA_TYPE *type);
>> void ff_printGUID(const GUID *g);
>>
>> diff --git a/libavdevice/dshow_common.c b/libavdevice/dshow_common.c
>> index c813dc1..8fe2f77 100644
>> --- a/libavdevice/dshow_common.c
>> +++ b/libavdevice/dshow_common.c
>> @@ -82,6 +82,55 @@ static void dump_bih(void *s, BITMAPINFOHEADER *bih)
>> }
>> #endif
>>
>> +void ff_print_VIDEO_STREAM_CONFIG_CAPS(const VIDEO_STREAM_CONFIG_CAPS *caps)
>> +{
>> +#if DSHOWDEBUG
>> + dshowdebug(" VIDEO_STREAM_CONFIG_CAPS\n");
>> + dshowdebug(" guid\t");
>> + ff_printGUID(&caps->guid);
>> + dshowdebug("\n");
>> + dshowdebug(" VideoStandard\t%lu\n", caps->VideoStandard);
>> + dshowdebug(" InputSize %ld\t%ld\n", caps->InputSize.cx, caps->InputSize.cy);
>> + dshowdebug(" MinCroppingSize %ld\t%ld\n", caps->MinCroppingSize.cx, caps->MinCroppingSize.cy);
>> + dshowdebug(" MaxCroppingSize %ld\t%ld\n", caps->MaxCroppingSize.cx, caps->MaxCroppingSize.cy);
>> + dshowdebug(" CropGranularityX\t%d\n", caps->CropGranularityX);
>> + dshowdebug(" CropGranularityY\t%d\n", caps->CropGranularityY);
>> + dshowdebug(" CropAlignX\t%d\n", caps->CropAlignX);
>> + dshowdebug(" CropAlignY\t%d\n", caps->CropAlignY);
>> + dshowdebug(" MinOutputSize %ld\t%ld\n", caps->MinOutputSize.cx, caps->MinOutputSize.cy);
>> + dshowdebug(" MaxOutputSize %ld\t%ld\n", caps->MaxOutputSize.cx, caps->MaxOutputSize.cy);
>> + dshowdebug(" OutputGranularityX\t%d\n", caps->OutputGranularityX);
>> + dshowdebug(" OutputGranularityY\t%d\n", caps->OutputGranularityY);
>> + dshowdebug(" StretchTapsX\t%d\n", caps->StretchTapsX);
>> + dshowdebug(" StretchTapsY\t%d\n", caps->StretchTapsY);
>> + dshowdebug(" ShrinkTapsX\t%d\n", caps->ShrinkTapsX);
>> + dshowdebug(" ShrinkTapsY\t%d\n", caps->ShrinkTapsY);
>> + dshowdebug(" MinFrameInterval\t%"PRId64"\n", caps->MinFrameInterval);
>> + dshowdebug(" MaxFrameInterval\t%"PRId64"\n", caps->MaxFrameInterval);
>> + dshowdebug(" MinBitsPerSecond\t%ld\n", caps->MinBitsPerSecond);
>> + dshowdebug(" MaxBitsPerSecond\t%ld\n", caps->MaxBitsPerSecond);
>> +#endif
>> +}
>> +
>> +void ff_print_AUDIO_STREAM_CONFIG_CAPS(const AUDIO_STREAM_CONFIG_CAPS *caps)
>> +{
>> +#if DSHOWDEBUG
>> + dshowdebug(" AUDIO_STREAM_CONFIG_CAPS\n");
>> + dshowdebug(" guid\t");
>> + ff_printGUID(&caps->guid);
>> + dshowdebug("\n");
>> + dshowdebug(" MinimumChannels\t%lu\n", caps->MinimumChannels);
>> + dshowdebug(" MaximumChannels\t%lu\n", caps->MaximumChannels);
>> + dshowdebug(" ChannelsGranularity\t%lu\n", caps->ChannelsGranularity);
>> + dshowdebug(" MinimumBitsPerSample\t%lu\n", caps->MinimumBitsPerSample);
>> + dshowdebug(" MaximumBitsPerSample\t%lu\n", caps->MaximumBitsPerSample);
>> + dshowdebug(" BitsPerSampleGranularity\t%lu\n", caps->BitsPerSampleGranularity);
>> + dshowdebug(" MinimumSampleFrequency\t%lu\n", caps->MinimumSampleFrequency);
>> + dshowdebug(" MaximumSampleFrequency\t%lu\n", caps->MaximumSampleFrequency);
>> + dshowdebug(" SampleFrequencyGranularity\t%lu\n", caps->SampleFrequencyGranularity);
>> +#endif
>> +}
>
> Unrelated: this use of dshowdebug() is a bit weird, maybe you may
> replace DSHOWDEBUG -> DEBUG, and dshowdebug() with av_dlog().
This way whenever DEBUG is set it will spew a bunch of log from dshow.
These are useful only if you're debugging dshow indev itself.
>> From 3e405fb74a1bfa2eaff677ea85712e0d4ba14464 Mon Sep 17 00:00:00 2001
>> From: Ramiro Polla <ramiro.polla at gmail.com>
>> Date: Fri, 9 Sep 2011 00:16:06 -0300
>> Subject: [PATCH 11/13] dshow: cleanup filters and pins on capture interface
>>
>> ---
>> libavdevice/dshow_enumpins.c | 8 +++++++-
>> libavdevice/dshow_filter.c | 8 +++++++-
>> 2 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavdevice/dshow_enumpins.c b/libavdevice/dshow_enumpins.c
>> index 97890fb..02e967a 100644
>> --- a/libavdevice/dshow_enumpins.c
>> +++ b/libavdevice/dshow_enumpins.c
>> @@ -94,6 +94,12 @@ libAVEnumPins_Setup(libAVEnumPins *this, libAVPin *pin, libAVFilter *filter)
>>
>> return 1;
>> }
>> +static int
>> +libAVEnumPins_Cleanup(libAVEnumPins *this)
>> +{
>> + libAVFilter_Release(this->filter);
>> + return 1;
>> +}
>> DECLARE_CREATE(libAVEnumPins, libAVEnumPins_Setup(this, pin, filter),
>> libAVPin *pin, libAVFilter *filter)
>> -DECLARE_DESTROY(libAVEnumPins, nothing)
>> +DECLARE_DESTROY(libAVEnumPins, libAVEnumPins_Cleanup)
>> diff --git a/libavdevice/dshow_filter.c b/libavdevice/dshow_filter.c
>> index e5a3be8..64e8306 100644
>> --- a/libavdevice/dshow_filter.c
>> +++ b/libavdevice/dshow_filter.c
>> @@ -191,6 +191,12 @@ libAVFilter_Setup(libAVFilter *this, void *priv_data, void *callback,
>>
>> return 1;
>> }
>> +static int
>> +libAVFilter_Cleanup(libAVFilter *this)
>> +{
>> + libAVPin_Release(this->pin);
>> + return 1;
>> +}
>> DECLARE_CREATE(libAVFilter, libAVFilter_Setup(this, priv_data, callback, type),
>> void *priv_data, void *callback, enum dshowDeviceType type)
>> -DECLARE_DESTROY(libAVFilter, nothing)
>> +DECLARE_DESTROY(libAVFilter, libAVFilter_Cleanup)
>> --
>> 1.7.4.1
>
> OK, I tried to understand the code in dshow* but this is beyond my
> capabilities without a serious study, but I trust you on this (well
> you're the supposed dshow maintainer afterall). If you can say a few
> words about how all this is supposed to work that would be helpful.
I reworded the commit message, see if it makes more sense now. The
code would keep some internal references to some objects and would not
release them.
>> From e880c249258b24fb4455e2b3ac14f834394ae734 Mon Sep 17 00:00:00 2001
>> From: Ramiro Polla <ramiro.polla at gmail.com>
>> Date: Fri, 9 Sep 2011 00:16:28 -0300
>> Subject: [PATCH 13/13] dshow: remove filters directly instead of enumerating them first
>>
>> ---
>> libavdevice/dshow.c | 26 ++++++++++++--------------
>> 1 files changed, 12 insertions(+), 14 deletions(-)
>>
>> diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c
>> index d830786..be194cb 100644
>> --- a/libavdevice/dshow.c
>> +++ b/libavdevice/dshow.c
>> @@ -112,6 +112,18 @@ dshow_read_close(AVFormatContext *s)
>> IMediaControl_Release(ctx->control);
>> }
>>
>> + if (ctx->graph) {
>> + if (ctx->capture_filter[VideoDevice])
>> + IGraphBuilder_RemoveFilter(ctx->graph, (IBaseFilter *) ctx->capture_filter[VideoDevice]);
>> + if (ctx->capture_filter[AudioDevice])
>> + IGraphBuilder_RemoveFilter(ctx->graph, (IBaseFilter *) ctx->capture_filter[AudioDevice]);
>> + if (ctx->device_filter[VideoDevice])
>> + IGraphBuilder_RemoveFilter(ctx->graph, ctx->device_filter[VideoDevice]);
>> + if (ctx->device_filter[AudioDevice])
>> + IGraphBuilder_RemoveFilter(ctx->graph, ctx->device_filter[AudioDevice]);
>> + IGraphBuilder_Release(ctx->graph);
>> + }
>> +
>> if (ctx->capture_pin[VideoDevice])
>> libAVPin_Release(ctx->capture_pin[VideoDevice]);
>> if (ctx->capture_pin[AudioDevice])
>> @@ -130,20 +142,6 @@ dshow_read_close(AVFormatContext *s)
>> if (ctx->device_filter[AudioDevice])
>> IBaseFilter_Release(ctx->device_filter[AudioDevice]);
>>
>> - if (ctx->graph) {
>> - IEnumFilters *fenum;
>> - int r;
>> - r = IGraphBuilder_EnumFilters(ctx->graph, &fenum);
>> - if (r == S_OK) {
>> - IBaseFilter *f;
>> - IEnumFilters_Reset(fenum);
>> - while (IEnumFilters_Next(fenum, 1, &f, NULL) == S_OK)
>> - IGraphBuilder_RemoveFilter(ctx->graph, f);
>> - IEnumFilters_Release(fenum);
>> - }
>> - IGraphBuilder_Release(ctx->graph);
>> - }
>> -
>> if (ctx->device_name[0])
>> av_free(ctx->device_name[0]);
>> if (ctx->device_name[1])
>
> The enumerative method seems more generic/robust to me (no need to
> explicitely enumerate the created filters), but that's bikeshed so I
> leave this to you.
I've done this last part differently in two commits. The enumeration
method was wrong the way it was because whenever a filter was removed,
the other filters would go down in the list. So when we got filter 1,
and didn't free it, we would get filter 2 on *next. When we freed
filter 1, we would get what was supposed to be filter 3 on *next.
Resetting the list fixes it.
All patches attached again (though only the ones commented have changed).
Ramiro
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-dshow-factorise-cycling-through-pins.patch
Type: text/x-patch
Size: 3717 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110913/123b212d/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-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/20110913/123b212d/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-dshow-add-audio-video-options.patch
Type: text/x-patch
Size: 12574 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110913/123b212d/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-dshow-add-option-to-list-audio-video-options.patch
Type: text/x-patch
Size: 6043 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110913/123b212d/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-dshow-indent.patch
Type: text/x-patch
Size: 1477 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110913/123b212d/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-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/20110913/123b212d/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-dshow-release-pin-on-disconnect.patch
Type: text/x-patch
Size: 695 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110913/123b212d/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0008-dshow-cleanup-internal-references-on-capture-interfa.patch
Type: text/x-patch
Size: 1647 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110913/123b212d/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0009-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/20110913/123b212d/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0010-dshow-reset-list-for-each-filter-removed.patch
Type: text/x-patch
Size: 1033 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110913/123b212d/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0011-dshow-remove-filters-from-graph-before-releasing-the.patch
Type: text/x-patch
Size: 2262 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110913/123b212d/attachment-0010.bin>
More information about the ffmpeg-devel
mailing list