On Sat, 5 Nov 2011 15:00:38 -0200, Ramiro Polla wrote:

Hi,

On Tue, Oct 18, 2011 at 9:10 PM, Stefano Sabatini <stefasab@gmail.com> wrote:

...

On date Monday 2011-10-17 20:40:03 -0200, Ramiro Polla encoded:

...

diff --git a/doc/indevs.texi b/doc/indevs.texi index 79f786f..1f3739b 100644 --- a/doc/indevs.texi +++ b/doc/indevs.texi @@ -104,6 +104,12 @@ If set to @option{true}, print a list of devices and exit.  If set to @option{true}, print a list of selected device's options  and exit.

+@item video_device_number +Set video device number for devices with same name (starts at 0). + +@item audio_device_number +Set audio device number for devices with same name (starts at 0). +

Maybe add "default to 0".

might be a good idea to put 'write down defaults in documentation' on the patch checklist guide? -compn

On date Saturday 2011-11-05 15:00:38 -0200, Ramiro Polla encoded:

Hi,

On Tue, Oct 18, 2011 at 9:10 PM, Stefano Sabatini <stefasab@gmail.com> wrote:

...

On date Monday 2011-10-17 20:40:03 -0200, Ramiro Polla encoded:

...

From 1184e9336e50fe8adc74d3151244c985f84e886f Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Fri, 30 Sep 2011 17:49:09 -0300 Subject: [PATCH 1/9] dshow: support choosing between devices with same name

---  doc/indevs.texi     |   12 ++++++++++++  libavdevice/dshow.c |    7 +++++++  2 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/doc/indevs.texi b/doc/indevs.texi index 79f786f..1f3739b 100644 --- a/doc/indevs.texi +++ b/doc/indevs.texi @@ -104,6 +104,12 @@ If set to @option{true}, print a list of devices and exit.  If set to @option{true}, print a list of selected device's options  and exit.

+@item video_device_number +Set video device number for devices with same name (starts at 0). + +@item audio_device_number +Set audio device number for devices with same name (starts at 0). +

Maybe add "default to 0".

Done.

...

...

From dcdf0ae08f612d91a6338d02965fb9de936eaece Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Fri, 30 Sep 2011 17:50:00 -0300 Subject: [PATCH 4/9] dshow: properly close device on option listing

---  libavdevice/dshow.c |    2 ++  1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index acfb7e9..2e60efa 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -561,11 +561,13 @@ static int  dshow_list_device_options(AVFormatContext *avctx, ICreateDevEnum *devenum,                            enum dshowDeviceType devtype)  { +    struct dshow_ctx *ctx = avctx->priv_data;      IBaseFilter *device_filter = NULL;      int r;

     if ((r = dshow_cycle_devices(avctx, devenum, devtype, &device_filter)) < 0)          return r; +    ctx->device_filter[devtype] = device_filter;      if ((r = dshow_cycle_pins(avctx, devtype, device_filter, NULL)) < 0)          return r;

Seems OK, but the log is a bit backward, I suppose the logic is something along the lines of:

|In function dshow_list_device_options(), set created device_filter in |ctx->device_filter[devtype], so its reference can be released later.

Slightly reworded.

...

...

From 56716cdbc1b35dbd6b0bae2ecbcb3ef50ebef0a0 Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Sat, 8 Oct 2011 15:00:00 -0300 Subject: [PATCH 6/9] dshow: support BI_BITFIELDS

---  libavdevice/dshow.c |    3 ++-  1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index 8681fa9..666e186 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -72,6 +72,7 @@ static enum PixelFormat dshow_pixfmt(DWORD biCompression, WORD biBitCount)          return PIX_FMT_YUYV422;      case MKTAG('I', '4', '2', '0'):          return PIX_FMT_YUV420P; +    case BI_BITFIELDS:      case BI_RGB:          switch(biBitCount) { /* 1-8 are untested */              case 1: @@ -710,7 +711,7 @@ dshow_add_device(AVFormatContext *avctx, AVFormatParameters *ap,              codec->bits_per_coded_sample = bih->biBitCount;          } else {              codec->codec_id = CODEC_ID_RAWVIDEO; -            if (bih->biCompression == BI_RGB) { +            if (bih->biCompression == BI_RGB || bih->biCompression == BI_BITFIELDS) {                  codec->bits_per_coded_sample = bih->biBitCount;                  codec->extradata = av_malloc(9 + FF_INPUT_BUFFER_PADDING_SIZE);                  if (codec->extradata) { -- 1.7.4.1

OK, just please provide some more information related to the meaning of BI_BITFIELDS.

Slightly reworded.

...

...

From a565deeba59d0f180872c15f8b3a8125bd7985a1 Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Sat, 8 Oct 2011 15:02:09 -0300 Subject: [PATCH 7/9] dshow: support video codec and pixel format selection

---  libavdevice/dshow.c |   47 +++++++++++++++++++++++++++++++++++++++++++++--  1 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index 666e186..d227539 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -20,6 +20,7 @@   */

 #include "libavutil/parseutils.h" +#include "libavutil/pixdesc.h"  #include "libavutil/opt.h"

 #include "avdevice.h" @@ -51,6 +52,9 @@ struct dshow_ctx {

     IMediaControl *control;

+    char *pixel_format; +    enum PixelFormat pix_fmt;

nit: pixel_format and pixel_format_str to make clear the relation between the two variables.

Done.

...

...

+    enum CodecID video_codec_id;      char *video_size;      char *framerate;

@@ -370,13 +374,35 @@ dshow_cycle_formats(AVFormatContext *avctx, enum dshowDeviceType devtype,                  goto next;              }              if (!pformat_set) { -                av_log(avctx, AV_LOG_INFO, "  min s=%ldx%ld fps=%g max s=%ldx%ld fps=%g\n", +                enum PixelFormat pix_fmt = dshow_pixfmt(bih->biCompression, bih->biBitCount); +                av_log(avctx, AV_LOG_INFO, "  min s=%ldx%ld fps=%g max s=%ldx%ld fps=%g",                         vcaps->MinOutputSize.cx, vcaps->MinOutputSize.cy,                         1e7 / vcaps->MaxFrameInterval,                         vcaps->MaxOutputSize.cx, vcaps->MaxOutputSize.cy,                         1e7 / vcaps->MinFrameInterval); +                if (pix_fmt == PIX_FMT_NONE) { +                    enum CodecID codec_id = dshow_codecid(bih->biCompression); +                    AVCodec *c = (codec_id == CODEC_ID_NONE) ? +                                 NULL : avcodec_find_decoder(codec_id); +                    if (codec_id == CODEC_ID_NONE || !c) { +                        av_log(avctx, AV_LOG_INFO, " (unknown compression type)"); +                    } else { +                        av_log(avctx, AV_LOG_INFO, " (video_codec %s)", c->name); +                    }

nit: simpler: av_log(avctx, AV_LOG_INFO, " video_codec=%s", c ? c->name, "unknown");

I prefer this way. The value in the struct is "compression type", which maps either to a pix_fmt or a codec_id in FFmpeg, so I test for pix_fmt, then codec_id, and if none are found it says "unknown compression type".

...

...

+                } else {

...

+                    av_log(avctx, AV_LOG_INFO, " (pix_fmt %s)", av_pix_fmt_descriptors[pix_fmt].name);

nit: slightly shorter: av_get_pix_fmt_name(pix_fmt)

Done.

...

for consistency: pix_fmt=%s

Added = to both pix_fmt and video_codec.

...

...

+                } +                av_log(avctx, AV_LOG_INFO, "\n");                  continue;              }

...

+            if (ctx->video_codec_id != CODEC_ID_RAWVIDEO) { +                if (ctx->video_codec_id != dshow_codecid(bih->biCompression)) +                    goto next; +            } +            if (ctx->pixel_format) { +                if (ctx->pix_fmt != dshow_pixfmt(bih->biCompression, bih->biBitCount)) +                    goto next; +            }

nit+: here you could merge the two if conditions: if (A) { if (B) ...  -> if (A && B) { ...

Done.

...

...

+    ctx->video_codec_id = avctx->video_codec_id ? avctx->video_codec_id +                                                : CODEC_ID_RAWVIDEO; +    if (ctx->pixel_format) { +        if (ctx->video_codec_id != CODEC_ID_RAWVIDEO) { +            av_log(avctx, AV_LOG_ERROR, "Pixel format may only be set when " +                              "video codec is not set or set to rawvideo.\n"); +            goto error; +        } +        ctx->pix_fmt = av_get_pix_fmt(ctx->pixel_format); +        if (ctx->pix_fmt == PIX_FMT_NONE) { +            av_log(avctx, AV_LOG_ERROR, "No such pixel format '%s'.\n", ctx->pixel_format); +            goto error; +        } +    }

missing ret = AVERROR(EINVAL) (this is supposed to be an user configuration error)

Fixed, and added a new patch to fix more error codes.

...

...

From b1bc428df2ef85b13de09d7267200da13841b848 Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Mon, 17 Oct 2011 12:17:58 -0200 Subject: [PATCH 8/9] dshow: fix return code when opening device fails

---  libavdevice/dshow.c |   18 ++++++++----------  1 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index d227539..48c2ffd 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -889,20 +889,18 @@ static int dshow_read_header(AVFormatContext *avctx, AVFormatParameters *ap)      }

     if (ctx->device_name[VideoDevice]) { -        ret = dshow_open_device(avctx, devenum, VideoDevice); -        if (ret < 0) -            goto error; -        ret = dshow_add_device(avctx, ap, VideoDevice); -        if (ret < 0) +        if ((r = dshow_open_device(avctx, devenum, VideoDevice)) < 0 || +            (r = dshow_add_device(avctx, ap, VideoDevice) < 0)) { +            ret = r;              goto error; +        }      }

     if (ctx->device_name[AudioDevice]) { -        ret = dshow_open_device(avctx, devenum, AudioDevice); -        if (ret < 0) -            goto error; -        ret = dshow_add_device(avctx, ap, AudioDevice); -        if (ret < 0) +        if ((r = dshow_open_device(avctx, devenum, AudioDevice)) < 0 || +            (r = dshow_add_device(avctx, ap, AudioDevice) < 0)) { +            ret = r;              goto error; +        }      }

Isn't dshow_open/add_device() already returning an AVERROR() error code?

Yes, but the error code of 0 would remain in 'ret' afterwards.

...

...

     ctx->mutex = CreateMutex(NULL, 0, NULL); -- 1.7.4.1

...

From c28b467bcefd771d03663a4acc01f0c41a86e8be Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Mon, 17 Oct 2011 20:25:27 -0200 Subject: [PATCH 9/9] dshow: set frame rate to prevent fps analysis code from running

---  libavdevice/dshow.c |    6 ++++++  1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index 48c2ffd..284e3bf 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -710,12 +710,15 @@ dshow_add_device(AVFormatContext *avctx, AVFormatParameters *ap,      codec = st->codec;      if (devtype == VideoDevice) {          BITMAPINFOHEADER *bih = NULL; +        AVRational frame_rate;

         if (IsEqualGUID(&type.formattype, &FORMAT_VideoInfo)) {              VIDEOINFOHEADER *v = (void *) type.pbFormat; +            frame_rate = av_d2q(10000000. / v->AvgTimePerFrame, INT_MAX);              bih = &v->bmiHeader;          } else if (IsEqualGUID(&type.formattype, &FORMAT_VideoInfo2)) {              VIDEOINFOHEADER2 *v = (void *) type.pbFormat; +            frame_rate = av_d2q(10000000. / v->AvgTimePerFrame, INT_MAX);              bih = &v->bmiHeader;          }          if (!bih) { @@ -723,6 +726,9 @@ dshow_add_device(AVFormatContext *avctx, AVFormatParameters *ap,              goto error;          }

+        st->avg_frame_rate = frame_rate; +        st->r_frame_rate = frame_rate; +          codec->time_base  = ap->time_base;          codec->codec_type = AVMEDIA_TYPE_VIDEO;          codec->width      = bih->biWidth;

Looks sane (but I'm not really sure about what "fps analysis code") refers to.

It's some code that reads a few frames before determining the fps. This makes it look like ffmpeg has stalled for a few seconds on startup. With this patch, it only reads one frame, so it stalls for much less time. I'm not sure this is the best solution, but it's what Michael suggested...

All patches attached again.

Ramiro

From fef213c926d0fde5e624d2ced2003c5b4d9e0313 Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Fri, 30 Sep 2011 17:49:09 -0300 Subject: [PATCH 01/10] dshow: support choosing between devices with same name

--- doc/indevs.texi | 14 ++++++++++++++ libavdevice/dshow.c | 7 +++++++ 2 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/doc/indevs.texi b/doc/indevs.texi index 3aa8f2f..90ae29c 100644 --- a/doc/indevs.texi +++ b/doc/indevs.texi @@ -104,6 +104,14 @@ If set to @option{true}, print a list of devices and exit. If set to @option{true}, print a list of selected device's options and exit.

+@item video_device_number +Set video device number for devices with same name (starts at 0, +defaults to 0). + +@item audio_device_number +Set audio device number for devices with same name (starts at 0, +defaults to 0). + @end table

@subsection Examples @@ -123,6 +131,12 @@ $ ffmpeg -f dshow -i video="Camera" @end example

@item +Open second video device with name @var{Camera}: +@example +$ ffmpeg -f dshow -video_device_number 1 -i video="Camera" +@end example + +@item Open video device @var{Camera} and audio device @var{Microphone}: @example $ ffmpeg -f dshow -i video="Camera":audio="Microphone" diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index ab5f7e9..a5263df 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -31,6 +31,8 @@ struct dshow_ctx { IGraphBuilder *graph;

char *device_name[2]; + int video_device_number; + int audio_device_number;

int list_options; int list_devices; @@ -249,6 +251,8 @@ dshow_cycle_devices(AVFormatContext *avctx, ICreateDevEnum *devenum, IEnumMoniker *classenum = NULL; IMoniker *m = NULL; const char *device_name = ctx->device_name[devtype]; + int skip = (devtype == VideoDevice) ? ctx->video_device_number + : ctx->audio_device_number; int r;

const GUID *device_guid[2] = { &CLSID_VideoInputDeviceCategory, @@ -283,6 +287,7 @@ dshow_cycle_devices(AVFormatContext *avctx, ICreateDevEnum *devenum, if (strcmp(device_name, buf)) goto fail1;

+ if (!skip--) IMoniker_BindToObject(m, 0, 0, &IID_IBaseFilter, (void *) &device_filter); } else { av_log(avctx, AV_LOG_INFO, " \"%s\"\n", buf); @@ -938,6 +943,8 @@ static const AVOption options[] = { { "list_options", "list available options for specified device", OFFSET(list_options), AV_OPT_TYPE_INT, {.dbl=0}, 0, 1, DEC, "list_options" }, { "true", "", 0, AV_OPT_TYPE_CONST, {.dbl=1}, 0, 0, DEC, "list_options" }, { "false", "", 0, AV_OPT_TYPE_CONST, {.dbl=0}, 0, 0, DEC, "list_options" }, + { "video_device_number", "set video device number for devices with same name (starts at 0)", OFFSET(video_device_number), AV_OPT_TYPE_INT, {.dbl = 0}, 0, INT_MAX, DEC }, + { "audio_device_number", "set audio device number for devices with same name (starts at 0)", OFFSET(audio_device_number), AV_OPT_TYPE_INT, {.dbl = 0}, 0, INT_MAX, DEC }, { NULL }, };

-- 1.7.4.1

LGTM.

From 27b3aaf6e9f16aa3138234caf147c3ccafa0d400 Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Fri, 30 Sep 2011 17:49:22 -0300 Subject: [PATCH 02/10] dshow: indent

--- libavdevice/dshow.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index a5263df..4f641e7 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -288,7 +288,7 @@ dshow_cycle_devices(AVFormatContext *avctx, ICreateDevEnum *devenum, goto fail1;

if (!skip--) - IMoniker_BindToObject(m, 0, 0, &IID_IBaseFilter, (void *) &device_filter); + IMoniker_BindToObject(m, 0, 0, &IID_IBaseFilter, (void *) &device_filter); } else { av_log(avctx, AV_LOG_INFO, " \"%s\"\n", buf); } -- 1.7.4.1

OkkeyOfCourse.

From 9d98663d6ecf66ebc58c74305bdfbfe7e8b948af Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Fri, 30 Sep 2011 17:49:43 -0300 Subject: [PATCH 03/10] dshow: release filter reference obtained from enumeration

--- libavdevice/dshow.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index 4f641e7..0191d51 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -121,10 +121,12 @@ dshow_read_close(AVFormatContext *s) if (r == S_OK) { IBaseFilter *f; IEnumFilters_Reset(fenum); - while (IEnumFilters_Next(fenum, 1, &f, NULL) == S_OK) + while (IEnumFilters_Next(fenum, 1, &f, NULL) == S_OK) { if (IGraphBuilder_RemoveFilter(ctx->graph, f) == S_OK) IEnumFilters_Reset(fenum); /* When a filter is removed, * the list must be reset. */ + IBaseFilter_Release(f); + } IEnumFilters_Release(fenum); } IGraphBuilder_Release(ctx->graph); -- 1.7.4.1

Should be fine.

From 16287b44c4faf7e6ef08d7d72dc73620d8b9854a Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Fri, 30 Sep 2011 17:50:00 -0300 Subject: [PATCH 04/10] dshow: save opened device reference so it may be properly closed

--- libavdevice/dshow.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index 0191d51..af2cf90 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -561,11 +561,13 @@ static int dshow_list_device_options(AVFormatContext *avctx, ICreateDevEnum *devenum, enum dshowDeviceType devtype) { + struct dshow_ctx *ctx = avctx->priv_data; IBaseFilter *device_filter = NULL; int r;

if ((r = dshow_cycle_devices(avctx, devenum, devtype, &device_filter)) < 0) return r; + ctx->device_filter[devtype] = device_filter; if ((r = dshow_cycle_pins(avctx, devtype, device_filter, NULL)) < 0) return r;

-- 1.7.4.1

Yes, much better commit log.

From 967242707d72581ab1dafb2dd501f0572e3dd400 Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Fri, 30 Sep 2011 18:10:30 -0300 Subject: [PATCH 05/10] dshow: don't print min/max values for fps the wrong way around

--- libavdevice/dshow.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index af2cf90..354e663 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -371,9 +371,9 @@ dshow_cycle_formats(AVFormatContext *avctx, enum dshowDeviceType devtype, if (!pformat_set) { av_log(avctx, AV_LOG_INFO, " min s=%ldx%ld fps=%g max s=%ldx%ld fps=%g\n", vcaps->MinOutputSize.cx, vcaps->MinOutputSize.cy, - 1e7 / vcaps->MinFrameInterval, + 1e7 / vcaps->MaxFrameInterval, vcaps->MaxOutputSize.cx, vcaps->MaxOutputSize.cy, - 1e7 / vcaps->MaxFrameInterval); + 1e7 / vcaps->MinFrameInterval); continue; } if (ctx->framerate) { -- 1.7.4.1

Looks fine.

From 0dfa169600f1f11951d4f32f24bbd3f9fec510cb Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Sat, 8 Oct 2011 15:00:00 -0300 Subject: [PATCH 06/10] dshow: support BI_BITFIELDS compression type

--- libavdevice/dshow.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index 354e663..bba1bba 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -72,6 +72,7 @@ static enum PixelFormat dshow_pixfmt(DWORD biCompression, WORD biBitCount) return PIX_FMT_YUYV422; case MKTAG('I', '4', '2', '0'): return PIX_FMT_YUV420P; + case BI_BITFIELDS: case BI_RGB: switch(biBitCount) { /* 1-8 are untested */ case 1: @@ -711,7 +712,7 @@ dshow_add_device(AVFormatContext *avctx, AVFormatParameters *ap, codec->bits_per_coded_sample = bih->biBitCount; } else { codec->codec_id = CODEC_ID_RAWVIDEO; - if (bih->biCompression == BI_RGB) { + if (bih->biCompression == BI_RGB || bih->biCompression == BI_BITFIELDS) { codec->bits_per_coded_sample = bih->biBitCount; codec->extradata = av_malloc(9 + FF_INPUT_BUFFER_PADDING_SIZE); if (codec->extradata) { -- 1.7.4.1

After reading this: http://www.virtualdub.org/blog/pivot/entry.php?id=177 http://msdn.microsoft.com/en-us/library/dd183376%28v=vs.85%29.aspx I wonder if the right format to select with biCompression = BI_BITFIELDS should be detected against BITMAPINFO.bmiColors (this is somehow similar to the way the pixel format is set in fbdev.c, you may borrow some code from there).

From 05cd8d1609ff06719524bc12bd8db91a9e0df127 Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Sat, 8 Oct 2011 15:02:09 -0300 Subject: [PATCH 07/10] dshow: support video codec and pixel format selection

--- libavdevice/dshow.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index bba1bba..3bdf38c 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -20,6 +20,7 @@ */

#include "libavutil/parseutils.h" +#include "libavutil/pixdesc.h" #include "libavutil/opt.h"

#include "avdevice.h" @@ -51,6 +52,9 @@ struct dshow_ctx {

IMediaControl *control;

+ char *pixel_format_str; + enum PixelFormat pixel_format; + enum CodecID video_codec_id; char *video_size; char *framerate;

@@ -370,13 +374,35 @@ dshow_cycle_formats(AVFormatContext *avctx, enum dshowDeviceType devtype, goto next; } if (!pformat_set) { - av_log(avctx, AV_LOG_INFO, " min s=%ldx%ld fps=%g max s=%ldx%ld fps=%g\n", + enum PixelFormat pix_fmt = dshow_pixfmt(bih->biCompression, bih->biBitCount); + av_log(avctx, AV_LOG_INFO, " min s=%ldx%ld fps=%g max s=%ldx%ld fps=%g", vcaps->MinOutputSize.cx, vcaps->MinOutputSize.cy, 1e7 / vcaps->MaxFrameInterval, vcaps->MaxOutputSize.cx, vcaps->MaxOutputSize.cy, 1e7 / vcaps->MinFrameInterval); + if (pix_fmt == PIX_FMT_NONE) { + enum CodecID codec_id = dshow_codecid(bih->biCompression); + AVCodec *c = (codec_id == CODEC_ID_NONE) ? + NULL : avcodec_find_decoder(codec_id); + if (codec_id == CODEC_ID_NONE || !c) { + av_log(avctx, AV_LOG_INFO, " (unknown compression type)"); + } else { + av_log(avctx, AV_LOG_INFO, " (video_codec=%s)", c->name); + } + } else { + av_log(avctx, AV_LOG_INFO, " (pix_fmt=%s)", av_get_pix_fmt_name(pix_fmt));

Nit: I still find the surrounding parentheses a bit weird...

+ } + av_log(avctx, AV_LOG_INFO, "\n"); continue; } + if (ctx->video_codec_id != CODEC_ID_RAWVIDEO) { + if (ctx->video_codec_id != dshow_codecid(bih->biCompression)) + goto next; + } + if (ctx->pixel_format_str && + ctx->pixel_format != dshow_pixfmt(bih->biCompression, bih->biBitCount)) { + goto next; + } if (ctx->framerate) { int64_t framerate = ((int64_t) ctx->requested_framerate.den*10000000) / ctx->requested_framerate.num; @@ -465,7 +491,9 @@ 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)) + int set_format = (devtype == VideoDevice && (ctx->video_size || ctx->framerate || + ctx->pixel_format_str || + ctx->video_codec_id != CODEC_ID_RAWVIDEO)) || (devtype == AudioDevice && (ctx->channels || ctx->sample_rate)); int format_set = 0;

@@ -798,6 +826,22 @@ static int dshow_read_header(AVFormatContext *avctx, AVFormatParameters *ap) goto error; }

+ ctx->video_codec_id = avctx->video_codec_id ? avctx->video_codec_id + : CODEC_ID_RAWVIDEO; + if (ctx->pixel_format_str) { + if (ctx->video_codec_id != CODEC_ID_RAWVIDEO) { + av_log(avctx, AV_LOG_ERROR, "Pixel format may only be set when " + "video codec is not set or set to rawvideo.\n"); + ret = AVERROR(EINVAL); + goto error; + } + ctx->pixel_format = av_get_pix_fmt(ctx->pixel_format_str); + if (ctx->pixel_format == PIX_FMT_NONE) { + av_log(avctx, AV_LOG_ERROR, "No such pixel format '%s'.\n", ctx->pixel_format_str); + ret = AVERROR(EINVAL); + goto error; + } + } if (ctx->video_size) { r = av_parse_video_size(&ctx->requested_width, &ctx->requested_height, ctx->video_size); if (r < 0) { @@ -938,6 +982,7 @@ static int dshow_read_packet(AVFormatContext *s, AVPacket *pkt) #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), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, DEC }, + { "pixel_format", "set video pixel format", OFFSET(pixel_format_str), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, DEC }, { "framerate", "set video frame rate", OFFSET(framerate), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, DEC }, { "sample_rate", "set audio sample rate", OFFSET(sample_rate), AV_OPT_TYPE_INT, {.dbl = 0}, 0, INT_MAX, DEC }, { "sample_size", "set audio sample size", OFFSET(sample_size), AV_OPT_TYPE_INT, {.dbl = 0}, 0, 16, DEC }, -- 1.7.4.1

Looks nice otherwise (although missing docs, if you prefer you can do it in another patch or I'll fill it in when committing).

From da98929b19a22d85c2f5342a945c5aca62c1518f Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Mon, 17 Oct 2011 12:17:58 -0200 Subject: [PATCH 08/10] dshow: fix return code when opening device fails

--- libavdevice/dshow.c | 18 ++++++++---------- 1 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index 3bdf38c..c12847c 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -892,20 +892,18 @@ static int dshow_read_header(AVFormatContext *avctx, AVFormatParameters *ap) }

if (ctx->device_name[VideoDevice]) { - ret = dshow_open_device(avctx, devenum, VideoDevice); - if (ret < 0) - goto error; - ret = dshow_add_device(avctx, ap, VideoDevice); - if (ret < 0) + if ((r = dshow_open_device(avctx, devenum, VideoDevice)) < 0 || + (r = dshow_add_device(avctx, ap, VideoDevice) < 0)) { + ret = r; goto error; + } } if (ctx->device_name[AudioDevice]) { - ret = dshow_open_device(avctx, devenum, AudioDevice); - if (ret < 0) - goto error; - ret = dshow_add_device(avctx, ap, AudioDevice); - if (ret < 0) + if ((r = dshow_open_device(avctx, devenum, AudioDevice)) < 0 || + (r = dshow_add_device(avctx, ap, AudioDevice) < 0)) { + ret = r; goto error; + } }

OK, although I'd still prefer to always set explicitely the error code.

ctx->mutex = CreateMutex(NULL, 0, NULL); -- 1.7.4.1

From 1e688cb216636689f3bf5372189eaa7bbc92c9df Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Mon, 17 Oct 2011 20:25:27 -0200 Subject: [PATCH 09/10] dshow: set frame rate to prevent fps analysis code from running

--- libavdevice/dshow.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index c12847c..09c6511 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -711,12 +711,15 @@ dshow_add_device(AVFormatContext *avctx, AVFormatParameters *ap, codec = st->codec; if (devtype == VideoDevice) { BITMAPINFOHEADER *bih = NULL; + AVRational frame_rate;

if (IsEqualGUID(&type.formattype, &FORMAT_VideoInfo)) { VIDEOINFOHEADER *v = (void *) type.pbFormat; + frame_rate = av_d2q(10000000. / v->AvgTimePerFrame, INT_MAX); bih = &v->bmiHeader; } else if (IsEqualGUID(&type.formattype, &FORMAT_VideoInfo2)) { VIDEOINFOHEADER2 *v = (void *) type.pbFormat; + frame_rate = av_d2q(10000000. / v->AvgTimePerFrame, INT_MAX); bih = &v->bmiHeader; } if (!bih) { @@ -724,6 +727,9 @@ dshow_add_device(AVFormatContext *avctx, AVFormatParameters *ap, goto error; }

+ st->avg_frame_rate = frame_rate; + st->r_frame_rate = frame_rate; + codec->time_base = ap->time_base; codec->codec_type = AVMEDIA_TYPE_VIDEO; codec->width = bih->biWidth; -- 1.7.4.1

Looks fine. I'd add this to the commit message to clarify the rationale: |fps analysis makes it look like ffmpeg has stalled for a few seconds |on startup when opening the dshow device. With this patch, it only |reads one frame, so it stalls for much less time.

From bfda37ef5babebb04166faebdc6acf00bee3f5ea Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Sat, 5 Nov 2011 14:50:18 -0200 Subject: [PATCH 10/10] dshow: separate user and device configuration errors

--- libavdevice/dshow.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index 09c6511..39f312e 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -824,7 +824,8 @@ static int dshow_read_header(AVFormatContext *avctx, AVFormatParameters *ap) IGraphBuilder *graph = NULL; ICreateDevEnum *devenum = NULL; IMediaControl *control = NULL; - int ret = AVERROR(EIO); + /* Initial errors are related to user configuration. */ + int ret = AVERROR(EINVAL); int r;

if (!ctx->list_devices && !parse_device_name(avctx)) { @@ -838,13 +839,11 @@ static int dshow_read_header(AVFormatContext *avctx, AVFormatParameters *ap) if (ctx->video_codec_id != CODEC_ID_RAWVIDEO) { av_log(avctx, AV_LOG_ERROR, "Pixel format may only be set when " "video codec is not set or set to rawvideo.\n"); - ret = AVERROR(EINVAL); goto error; } ctx->pixel_format = av_get_pix_fmt(ctx->pixel_format_str); if (ctx->pixel_format == PIX_FMT_NONE) { av_log(avctx, AV_LOG_ERROR, "No such pixel format '%s'.\n", ctx->pixel_format_str); - ret = AVERROR(EINVAL); goto error; } } @@ -863,6 +862,9 @@ static int dshow_read_header(AVFormatContext *avctx, AVFormatParameters *ap) } }

+ /* From now on, errors are related to device configuration. */ + ret = AVERROR(EIO); + CoInitialize(0);

r = CoCreateInstance(&CLSID_FilterGraph, NULL, CLSCTX_INPROC_SERVER,

Fine, or alternatively always set explicitely the error code (more robust to changes, more predictable). I'll apply the patches which LG(TM) in a day or so if I read no more comments (and you don't beat me at it assuming you want to commit them yourself), thanks. -- FFmpeg = Friendly & Friendly Mastering Patchable Energized Glue

On Sat, Nov 5, 2011 at 9:49 PM, Stefano Sabatini <stefasab@gmail.com> wrote:

On date Saturday 2011-11-05 15:00:38 -0200, Ramiro Polla encoded:

...

From 0dfa169600f1f11951d4f32f24bbd3f9fec510cb Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Sat, 8 Oct 2011 15:00:00 -0300 Subject: [PATCH 06/10] dshow: support BI_BITFIELDS compression type

---  libavdevice/dshow.c |    3 ++-  1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index 354e663..bba1bba 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -72,6 +72,7 @@ static enum PixelFormat dshow_pixfmt(DWORD biCompression, WORD biBitCount)          return PIX_FMT_YUYV422;      case MKTAG('I', '4', '2', '0'):          return PIX_FMT_YUV420P; +    case BI_BITFIELDS:      case BI_RGB:          switch(biBitCount) { /* 1-8 are untested */              case 1: @@ -711,7 +712,7 @@ dshow_add_device(AVFormatContext *avctx, AVFormatParameters *ap,              codec->bits_per_coded_sample = bih->biBitCount;          } else {              codec->codec_id = CODEC_ID_RAWVIDEO; -            if (bih->biCompression == BI_RGB) { +            if (bih->biCompression == BI_RGB || bih->biCompression == BI_BITFIELDS) {                  codec->bits_per_coded_sample = bih->biBitCount;                  codec->extradata = av_malloc(9 + FF_INPUT_BUFFER_PADDING_SIZE);                  if (codec->extradata) { -- 1.7.4.1

After reading this: http://www.virtualdub.org/blog/pivot/entry.php?id=177 http://msdn.microsoft.com/en-us/library/dd183376%28v=vs.85%29.aspx

I wonder if the right format to select with biCompression = BI_BITFIELDS should be detected against BITMAPINFO.bmiColors (this is somehow similar to the way the pixel format is set in fbdev.c, you may borrow some code from there).

Hm, this is too much work for such a rare case. If a bug report ever pops up I'll look at it.

...

From 05cd8d1609ff06719524bc12bd8db91a9e0df127 Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Sat, 8 Oct 2011 15:02:09 -0300 Subject: [PATCH 07/10] dshow: support video codec and pixel format selection

---  libavdevice/dshow.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++--  1 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index bba1bba..3bdf38c 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -20,6 +20,7 @@   */

 #include "libavutil/parseutils.h" +#include "libavutil/pixdesc.h"  #include "libavutil/opt.h"

 #include "avdevice.h" @@ -51,6 +52,9 @@ struct dshow_ctx {

     IMediaControl *control;

+    char *pixel_format_str; +    enum PixelFormat pixel_format; +    enum CodecID video_codec_id;      char *video_size;      char *framerate;

@@ -370,13 +374,35 @@ dshow_cycle_formats(AVFormatContext *avctx, enum dshowDeviceType devtype,                  goto next;              }              if (!pformat_set) { -                av_log(avctx, AV_LOG_INFO, "  min s=%ldx%ld fps=%g max s=%ldx%ld fps=%g\n", +                enum PixelFormat pix_fmt = dshow_pixfmt(bih->biCompression, bih->biBitCount); +                av_log(avctx, AV_LOG_INFO, "  min s=%ldx%ld fps=%g max s=%ldx%ld fps=%g",                         vcaps->MinOutputSize.cx, vcaps->MinOutputSize.cy,                         1e7 / vcaps->MaxFrameInterval,                         vcaps->MaxOutputSize.cx, vcaps->MaxOutputSize.cy,                         1e7 / vcaps->MinFrameInterval); +                if (pix_fmt == PIX_FMT_NONE) { +                    enum CodecID codec_id = dshow_codecid(bih->biCompression); +                    AVCodec *c = (codec_id == CODEC_ID_NONE) ? +                                 NULL : avcodec_find_decoder(codec_id); +                    if (codec_id == CODEC_ID_NONE || !c) { +                        av_log(avctx, AV_LOG_INFO, " (unknown compression type)"); +                    } else { +                        av_log(avctx, AV_LOG_INFO, " (video_codec=%s)", c->name); +                    } +                } else { +                    av_log(avctx, AV_LOG_INFO, " (pix_fmt=%s)", av_get_pix_fmt_name(pix_fmt));

Nit: I still find the surrounding parentheses a bit weird...

What I thought was that all other parameters have a range, but this one is fixed, it's just there to show what compression type it is. For some reason I thought surrounding it in parentheses was a good idea.

...

+                } +                av_log(avctx, AV_LOG_INFO, "\n");                  continue;              } +            if (ctx->video_codec_id != CODEC_ID_RAWVIDEO) { +                if (ctx->video_codec_id != dshow_codecid(bih->biCompression)) +                    goto next; +            } +            if (ctx->pixel_format_str && +                ctx->pixel_format != dshow_pixfmt(bih->biCompression, bih->biBitCount)) { +                goto next; +            }              if (ctx->framerate) {                  int64_t framerate = ((int64_t) ctx->requested_framerate.den*10000000)                                              /  ctx->requested_framerate.num; @@ -465,7 +491,9 @@ 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)) +    int set_format = (devtype == VideoDevice && (ctx->video_size || ctx->framerate || +                                                 ctx->pixel_format_str || +                                                 ctx->video_codec_id != CODEC_ID_RAWVIDEO))                    || (devtype == AudioDevice && (ctx->channels || ctx->sample_rate));      int format_set = 0;

@@ -798,6 +826,22 @@ static int dshow_read_header(AVFormatContext *avctx, AVFormatParameters *ap)          goto error;      }

+    ctx->video_codec_id = avctx->video_codec_id ? avctx->video_codec_id +                                                : CODEC_ID_RAWVIDEO; +    if (ctx->pixel_format_str) { +        if (ctx->video_codec_id != CODEC_ID_RAWVIDEO) { +            av_log(avctx, AV_LOG_ERROR, "Pixel format may only be set when " +                              "video codec is not set or set to rawvideo.\n"); +            ret = AVERROR(EINVAL); +            goto error; +        } +        ctx->pixel_format = av_get_pix_fmt(ctx->pixel_format_str); +        if (ctx->pixel_format == PIX_FMT_NONE) { +            av_log(avctx, AV_LOG_ERROR, "No such pixel format '%s'.\n", ctx->pixel_format_str); +            ret = AVERROR(EINVAL); +            goto error; +        } +    }      if (ctx->video_size) {          r = av_parse_video_size(&ctx->requested_width, &ctx->requested_height, ctx->video_size);          if (r < 0) { @@ -938,6 +982,7 @@ static int dshow_read_packet(AVFormatContext *s, AVPacket *pkt)  #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), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, DEC }, +    { "pixel_format", "set video pixel format", OFFSET(pixel_format_str), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, DEC },      { "framerate", "set video frame rate", OFFSET(framerate), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, DEC },      { "sample_rate", "set audio sample rate", OFFSET(sample_rate), AV_OPT_TYPE_INT, {.dbl = 0}, 0, INT_MAX, DEC },      { "sample_size", "set audio sample size", OFFSET(sample_size), AV_OPT_TYPE_INT, {.dbl = 0}, 0, 16, DEC }, -- 1.7.4.1

Looks nice otherwise (although missing docs, if you prefer you can do it in another patch or I'll fill it in when committing).

Isn't pixel_format a normal option that should also be accepted by libavformat. I thought documentation was unnecessary in that case.

...

From da98929b19a22d85c2f5342a945c5aca62c1518f Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Mon, 17 Oct 2011 12:17:58 -0200 Subject: [PATCH 08/10] dshow: fix return code when opening device fails

---  libavdevice/dshow.c |   18 ++++++++----------  1 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index 3bdf38c..c12847c 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -892,20 +892,18 @@ static int dshow_read_header(AVFormatContext *avctx, AVFormatParameters *ap)      }

     if (ctx->device_name[VideoDevice]) { -        ret = dshow_open_device(avctx, devenum, VideoDevice); -        if (ret < 0) -            goto error; -        ret = dshow_add_device(avctx, ap, VideoDevice); -        if (ret < 0) +        if ((r = dshow_open_device(avctx, devenum, VideoDevice)) < 0 || +            (r = dshow_add_device(avctx, ap, VideoDevice) < 0)) { +            ret = r;              goto error; +        }      }      if (ctx->device_name[AudioDevice]) { -        ret = dshow_open_device(avctx, devenum, AudioDevice); -        if (ret < 0) -            goto error; -        ret = dshow_add_device(avctx, ap, AudioDevice); -        if (ret < 0) +        if ((r = dshow_open_device(avctx, devenum, AudioDevice)) < 0 || +            (r = dshow_add_device(avctx, ap, AudioDevice) < 0)) { +            ret = r;              goto error; +        }      }

OK, although I'd still prefer to always set explicitely the error code.

...

     ctx->mutex = CreateMutex(NULL, 0, NULL); -- 1.7.4.1

...

From 1e688cb216636689f3bf5372189eaa7bbc92c9df Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Mon, 17 Oct 2011 20:25:27 -0200 Subject: [PATCH 09/10] dshow: set frame rate to prevent fps analysis code from running

---  libavdevice/dshow.c |    6 ++++++  1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index c12847c..09c6511 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -711,12 +711,15 @@ dshow_add_device(AVFormatContext *avctx, AVFormatParameters *ap,      codec = st->codec;      if (devtype == VideoDevice) {          BITMAPINFOHEADER *bih = NULL; +        AVRational frame_rate;

         if (IsEqualGUID(&type.formattype, &FORMAT_VideoInfo)) {              VIDEOINFOHEADER *v = (void *) type.pbFormat; +            frame_rate = av_d2q(10000000. / v->AvgTimePerFrame, INT_MAX);              bih = &v->bmiHeader;          } else if (IsEqualGUID(&type.formattype, &FORMAT_VideoInfo2)) {              VIDEOINFOHEADER2 *v = (void *) type.pbFormat; +            frame_rate = av_d2q(10000000. / v->AvgTimePerFrame, INT_MAX);              bih = &v->bmiHeader;          }          if (!bih) { @@ -724,6 +727,9 @@ dshow_add_device(AVFormatContext *avctx, AVFormatParameters *ap,              goto error;          }

+        st->avg_frame_rate = frame_rate; +        st->r_frame_rate = frame_rate; +          codec->time_base  = ap->time_base;          codec->codec_type = AVMEDIA_TYPE_VIDEO;          codec->width      = bih->biWidth; -- 1.7.4.1

Looks fine. I'd add this to the commit message to clarify the rationale:

|fps analysis makes it look like ffmpeg has stalled for a few seconds |on startup when opening the dshow device. With this patch, it only |reads one frame, so it stalls for much less time.

Looks good.

...

From bfda37ef5babebb04166faebdc6acf00bee3f5ea Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Sat, 5 Nov 2011 14:50:18 -0200 Subject: [PATCH 10/10] dshow: separate user and device configuration errors

---  libavdevice/dshow.c |    8 +++++---  1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index 09c6511..39f312e 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -824,7 +824,8 @@ static int dshow_read_header(AVFormatContext *avctx, AVFormatParameters *ap)      IGraphBuilder *graph = NULL;      ICreateDevEnum *devenum = NULL;      IMediaControl *control = NULL; -    int ret = AVERROR(EIO); +    /* Initial errors are related to user configuration. */ +    int ret = AVERROR(EINVAL);      int r;

     if (!ctx->list_devices && !parse_device_name(avctx)) { @@ -838,13 +839,11 @@ static int dshow_read_header(AVFormatContext *avctx, AVFormatParameters *ap)          if (ctx->video_codec_id != CODEC_ID_RAWVIDEO) {              av_log(avctx, AV_LOG_ERROR, "Pixel format may only be set when "                                "video codec is not set or set to rawvideo.\n"); -            ret = AVERROR(EINVAL);              goto error;          }          ctx->pixel_format = av_get_pix_fmt(ctx->pixel_format_str);          if (ctx->pixel_format == PIX_FMT_NONE) {              av_log(avctx, AV_LOG_ERROR, "No such pixel format '%s'.\n", ctx->pixel_format_str); -            ret = AVERROR(EINVAL);              goto error;          }      } @@ -863,6 +862,9 @@ static int dshow_read_header(AVFormatContext *avctx, AVFormatParameters *ap)          }      }

+    /* From now on, errors are related to device configuration. */ +    ret = AVERROR(EIO); +      CoInitialize(0);

     r = CoCreateInstance(&CLSID_FilterGraph, NULL, CLSCTX_INPROC_SERVER,

Fine, or alternatively always set explicitely the error code (more robust to changes, more predictable).

More lines of code too, but it seems like a better idea. Drop this last patch and I'll look at it later.

I'll apply the patches which LG(TM) in a day or so if I read no more comments (and you don't beat me at it assuming you want to commit them yourself), thanks.

Ramiro

On Sun, Nov 06, 2011 at 12:22:14PM +0100, Stefano Sabatini wrote:

On date Saturday 2011-11-05 22:18:42 -0200, Ramiro Polla encoded:

...

On Sat, Nov 5, 2011 at 9:49 PM, Stefano Sabatini <stefasab@gmail.com> wrote:

...

On date Saturday 2011-11-05 15:00:38 -0200, Ramiro Polla encoded:

...

From 0dfa169600f1f11951d4f32f24bbd3f9fec510cb Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Sat, 8 Oct 2011 15:00:00 -0300 Subject: [PATCH 06/10] dshow: support BI_BITFIELDS compression type

---  libavdevice/dshow.c |    3 ++-  1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index 354e663..bba1bba 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -72,6 +72,7 @@ static enum PixelFormat dshow_pixfmt(DWORD biCompression, WORD biBitCount)          return PIX_FMT_YUYV422;      case MKTAG('I', '4', '2', '0'):          return PIX_FMT_YUV420P; +    case BI_BITFIELDS:      case BI_RGB:          switch(biBitCount) { /* 1-8 are untested */              case 1: @@ -711,7 +712,7 @@ dshow_add_device(AVFormatContext *avctx, AVFormatParameters *ap,              codec->bits_per_coded_sample = bih->biBitCount;          } else {              codec->codec_id = CODEC_ID_RAWVIDEO; -            if (bih->biCompression == BI_RGB) { +            if (bih->biCompression == BI_RGB || bih->biCompression == BI_BITFIELDS) {                  codec->bits_per_coded_sample = bih->biBitCount;                  codec->extradata = av_malloc(9 + FF_INPUT_BUFFER_PADDING_SIZE);                  if (codec->extradata) { -- 1.7.4.1

After reading this: http://www.virtualdub.org/blog/pivot/entry.php?id=177 http://msdn.microsoft.com/en-us/library/dd183376%28v=vs.85%29.aspx

I wonder if the right format to select with biCompression = BI_BITFIELDS should be detected against BITMAPINFO.bmiColors (this is somehow similar to the way the pixel format is set in fbdev.c, you may borrow some code from there).

Hm, this is too much work for such a rare case. If a bug report ever pops up I'll look at it.

Fine with me.

so patch 6 is LGTY ? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Observe your enemies, for they first find out your faults. -- Antisthenes

On Sun, Nov 06, 2011 at 02:35:01PM +0100, Stefano Sabatini wrote:

On date Sunday 2011-11-06 13:16:17 +0100, Michael Niedermayer encoded:

...

On Sun, Nov 06, 2011 at 12:22:14PM +0100, Stefano Sabatini wrote:

...

On date Saturday 2011-11-05 22:18:42 -0200, Ramiro Polla encoded:

...

On Sat, Nov 5, 2011 at 9:49 PM, Stefano Sabatini <stefasab@gmail.com> wrote:

...

On date Saturday 2011-11-05 15:00:38 -0200, Ramiro Polla encoded:

...

From 0dfa169600f1f11951d4f32f24bbd3f9fec510cb Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Sat, 8 Oct 2011 15:00:00 -0300 Subject: [PATCH 06/10] dshow: support BI_BITFIELDS compression type

---  libavdevice/dshow.c |    3 ++-  1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index 354e663..bba1bba 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -72,6 +72,7 @@ static enum PixelFormat dshow_pixfmt(DWORD biCompression, WORD biBitCount)          return PIX_FMT_YUYV422;      case MKTAG('I', '4', '2', '0'):          return PIX_FMT_YUV420P; +    case BI_BITFIELDS:      case BI_RGB:          switch(biBitCount) { /* 1-8 are untested */              case 1: @@ -711,7 +712,7 @@ dshow_add_device(AVFormatContext *avctx, AVFormatParameters *ap,              codec->bits_per_coded_sample = bih->biBitCount;          } else {              codec->codec_id = CODEC_ID_RAWVIDEO; -            if (bih->biCompression == BI_RGB) { +            if (bih->biCompression == BI_RGB || bih->biCompression == BI_BITFIELDS) {                  codec->bits_per_coded_sample = bih->biBitCount;                  codec->extradata = av_malloc(9 + FF_INPUT_BUFFER_PADDING_SIZE);                  if (codec->extradata) { -- 1.7.4.1

After reading this: http://www.virtualdub.org/blog/pivot/entry.php?id=177 http://msdn.microsoft.com/en-us/library/dd183376%28v=vs.85%29.aspx

I wonder if the right format to select with biCompression = BI_BITFIELDS should be detected against BITMAPINFO.bmiColors (this is somehow similar to the way the pixel format is set in fbdev.c, you may borrow some code from there).

Hm, this is too much work for such a rare case. If a bug report ever pops up I'll look at it.

Fine with me.

so patch 6 is LGTY ?

Yes, a comment in the source may be helpful but the patch is fine as is.

ok, #6 applied then [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Incandescent light bulbs waste a lot of energy as heat so the EU forbids them. Their replacement, compact fluorescent lamps, much more expensive, dont fit in many old lamps, flicker, contain toxic mercury, produce a fraction of the light that is claimed and in a unnatural spectrum rendering colors different than in natural light. Ah and we now need to turn the heaters up more in winter to compensate the lower wasted heat. Who wins? Not the environment, thats for sure

On Thu, Aug 23, 2012 at 4:45 PM, Ramiro Polla <ramiro.polla@gmail.com> wrote:

On Sat, Nov 5, 2011 at 8:49 PM, Stefano Sabatini <stefasab@gmail.com> wrote:

...

On date Saturday 2011-11-05 15:00:38 -0200, Ramiro Polla encoded: [...]

...

From 05cd8d1609ff06719524bc12bd8db91a9e0df127 Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Sat, 8 Oct 2011 15:02:09 -0300 Subject: [PATCH 07/10] dshow: support video codec and pixel format selection

--- libavdevice/dshow.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index bba1bba..3bdf38c 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -20,6 +20,7 @@ */

#include "libavutil/parseutils.h" +#include "libavutil/pixdesc.h" #include "libavutil/opt.h"

#include "avdevice.h" @@ -51,6 +52,9 @@ struct dshow_ctx {

IMediaControl *control;

+ char *pixel_format_str; + enum PixelFormat pixel_format; + enum CodecID video_codec_id; char *video_size; char *framerate;

@@ -370,13 +374,35 @@ dshow_cycle_formats(AVFormatContext *avctx, enum dshowDeviceType devtype, goto next; } if (!pformat_set) { - av_log(avctx, AV_LOG_INFO, " min s=%ldx%ld fps=%g max s=%ldx%ld fps=%g\n", + enum PixelFormat pix_fmt = dshow_pixfmt(bih->biCompression, bih->biBitCount); + av_log(avctx, AV_LOG_INFO, " min s=%ldx%ld fps=%g max s=%ldx%ld fps=%g", vcaps->MinOutputSize.cx, vcaps->MinOutputSize.cy, 1e7 / vcaps->MaxFrameInterval, vcaps->MaxOutputSize.cx, vcaps->MaxOutputSize.cy, 1e7 / vcaps->MinFrameInterval); + if (pix_fmt == PIX_FMT_NONE) { + enum CodecID codec_id = dshow_codecid(bih->biCompression); + AVCodec *c = (codec_id == CODEC_ID_NONE) ? + NULL : avcodec_find_decoder(codec_id); + if (codec_id == CODEC_ID_NONE || !c) { + av_log(avctx, AV_LOG_INFO, " (unknown compression type)"); + } else { + av_log(avctx, AV_LOG_INFO, " (video_codec=%s)", c->name); + } + } else { + av_log(avctx, AV_LOG_INFO, " (pix_fmt=%s)", av_get_pix_fmt_name(pix_fmt));

Nit: I still find the surrounding parentheses a bit weird...

Changed to something else.

...

...

+ } + av_log(avctx, AV_LOG_INFO, "\n"); continue; } + if (ctx->video_codec_id != CODEC_ID_RAWVIDEO) { + if (ctx->video_codec_id != dshow_codecid(bih->biCompression)) + goto next; + } + if (ctx->pixel_format_str && + ctx->pixel_format != dshow_pixfmt(bih->biCompression, bih->biBitCount)) { + goto next; + } if (ctx->framerate) { int64_t framerate = ((int64_t) ctx->requested_framerate.den*10000000) / ctx->requested_framerate.num; @@ -465,7 +491,9 @@ 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)) + int set_format = (devtype == VideoDevice && (ctx->video_size || ctx->framerate || + ctx->pixel_format_str || + ctx->video_codec_id != CODEC_ID_RAWVIDEO)) || (devtype == AudioDevice && (ctx->channels || ctx->sample_rate)); int format_set = 0;

@@ -798,6 +826,22 @@ static int dshow_read_header(AVFormatContext *avctx, AVFormatParameters *ap) goto error; }

+ ctx->video_codec_id = avctx->video_codec_id ? avctx->video_codec_id + : CODEC_ID_RAWVIDEO; + if (ctx->pixel_format_str) { + if (ctx->video_codec_id != CODEC_ID_RAWVIDEO) { + av_log(avctx, AV_LOG_ERROR, "Pixel format may only be set when " + "video codec is not set or set to rawvideo.\n"); + ret = AVERROR(EINVAL); + goto error; + } + ctx->pixel_format = av_get_pix_fmt(ctx->pixel_format_str); + if (ctx->pixel_format == PIX_FMT_NONE) { + av_log(avctx, AV_LOG_ERROR, "No such pixel format '%s'.\n", ctx->pixel_format_str); + ret = AVERROR(EINVAL); + goto error; + } + } if (ctx->video_size) { r = av_parse_video_size(&ctx->requested_width, &ctx->requested_height, ctx->video_size); if (r < 0) { @@ -938,6 +982,7 @@ static int dshow_read_packet(AVFormatContext *s, AVPacket *pkt) #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), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, DEC }, + { "pixel_format", "set video pixel format", OFFSET(pixel_format_str), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, DEC }, { "framerate", "set video frame rate", OFFSET(framerate), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, DEC }, { "sample_rate", "set audio sample rate", OFFSET(sample_rate), AV_OPT_TYPE_INT, {.dbl = 0}, 0, INT_MAX, DEC }, { "sample_size", "set audio sample size", OFFSET(sample_size), AV_OPT_TYPE_INT, {.dbl = 0}, 0, 16, DEC }, -- 1.7.4.1

Looks nice otherwise (although missing docs, if you prefer you can do it in another patch or I'll fill it in when committing).

Updated, assuming you fill in the docs the way you want.

...

...

From da98929b19a22d85c2f5342a945c5aca62c1518f Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Mon, 17 Oct 2011 12:17:58 -0200 Subject: [PATCH 08/10] dshow: fix return code when opening device fails

--- libavdevice/dshow.c | 18 ++++++++---------- 1 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index 3bdf38c..c12847c 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -892,20 +892,18 @@ static int dshow_read_header(AVFormatContext *avctx, AVFormatParameters *ap) }

if (ctx->device_name[VideoDevice]) { - ret = dshow_open_device(avctx, devenum, VideoDevice); - if (ret < 0) - goto error; - ret = dshow_add_device(avctx, ap, VideoDevice); - if (ret < 0) + if ((r = dshow_open_device(avctx, devenum, VideoDevice)) < 0 || + (r = dshow_add_device(avctx, ap, VideoDevice) < 0)) { + ret = r; goto error; + } } if (ctx->device_name[AudioDevice]) { - ret = dshow_open_device(avctx, devenum, AudioDevice); - if (ret < 0) - goto error; - ret = dshow_add_device(avctx, ap, AudioDevice); - if (ret < 0) + if ((r = dshow_open_device(avctx, devenum, AudioDevice)) < 0 || + (r = dshow_add_device(avctx, ap, AudioDevice) < 0)) { + ret = r; goto error; + } }

OK, although I'd still prefer to always set explicitely the error code.

Updated.

...

...

From 1e688cb216636689f3bf5372189eaa7bbc92c9df Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Mon, 17 Oct 2011 20:25:27 -0200 Subject: [PATCH 09/10] dshow: set frame rate to prevent fps analysis code from running

--- libavdevice/dshow.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index c12847c..09c6511 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -711,12 +711,15 @@ dshow_add_device(AVFormatContext *avctx, AVFormatParameters *ap, codec = st->codec; if (devtype == VideoDevice) { BITMAPINFOHEADER *bih = NULL; + AVRational frame_rate;

if (IsEqualGUID(&type.formattype, &FORMAT_VideoInfo)) { VIDEOINFOHEADER *v = (void *) type.pbFormat; + frame_rate = av_d2q(10000000. / v->AvgTimePerFrame, INT_MAX); bih = &v->bmiHeader; } else if (IsEqualGUID(&type.formattype, &FORMAT_VideoInfo2)) { VIDEOINFOHEADER2 *v = (void *) type.pbFormat; + frame_rate = av_d2q(10000000. / v->AvgTimePerFrame, INT_MAX); bih = &v->bmiHeader; } if (!bih) { @@ -724,6 +727,9 @@ dshow_add_device(AVFormatContext *avctx, AVFormatParameters *ap, goto error; }

+ st->avg_frame_rate = frame_rate; + st->r_frame_rate = frame_rate; + codec->time_base = ap->time_base; codec->codec_type = AVMEDIA_TYPE_VIDEO; codec->width = bih->biWidth; -- 1.7.4.1

Looks fine. I'd add this to the commit message to clarify the rationale:

|fps analysis makes it look like ffmpeg has stalled for a few seconds |on startup when opening the dshow device. With this patch, it only |reads one frame, so it stalls for much less time.

I am no longer able to reproduce the issue this patch was trying to fix. It's either because I forgot how to reproduce it or some change regarding time_base some time ago fixed it.

ping

On Tue, Sep 4, 2012 at 1:25 PM, Stefano Sabatini <stefasab@gmail.com> wrote:

On date Thursday 2012-08-23 16:45:47 -0300, Ramiro Polla encoded:

...

On Sat, Nov 5, 2011 at 8:49 PM, Stefano Sabatini <stefasab@gmail.com> wrote: [...]

...

Looks fine. I'd add this to the commit message to clarify the rationale:

|fps analysis makes it look like ffmpeg has stalled for a few seconds |on startup when opening the dshow device. With this patch, it only |reads one frame, so it stalls for much less time.

I am no longer able to reproduce the issue this patch was trying to fix. It's either because I forgot how to reproduce it or some change regarding time_base some time ago fixed it.

Ramiro

...

From 68bf7d131ada9357108d31d7f18f7008319c8102 Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Thu, 23 Aug 2012 16:35:55 -0300 Subject: [PATCH 1/2] dshow: support video codec and pixel format selection

--- libavdevice/dshow.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index 93bca1d..d3b5d8f 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -20,6 +20,7 @@ */

#include "libavutil/parseutils.h" +#include "libavutil/pixdesc.h" #include "libavutil/opt.h" #include "libavformat/internal.h" #include "avdevice.h" @@ -51,6 +52,9 @@ struct dshow_ctx {

IMediaControl *control;

+ char *pixel_format_str; + enum PixelFormat pixel_format; + enum AVCodecID video_codec_id; char *video_size; char *framerate;

@@ -370,13 +374,35 @@ dshow_cycle_formats(AVFormatContext *avctx, enum dshowDeviceType devtype, goto next; } if (!pformat_set) { - av_log(avctx, AV_LOG_INFO, " min s=%ldx%ld fps=%g max s=%ldx%ld fps=%g\n", + enum PixelFormat pix_fmt = dshow_pixfmt(bih->biCompression, bih->biBitCount); + if (pix_fmt == PIX_FMT_NONE) { + enum AVCodecID codec_id = dshow_codecid(bih->biCompression);

...

+ AVCodec *c = (codec_id == AV_CODEC_ID_NONE) ? + NULL : avcodec_find_decoder(codec_id);

Nit: codec

Also simpler: AVCodec *codec = avcodec_find_decoder(codec_id);

as codec will be NULL if codec_id == NONE.

Ok.

...

+ if (codec_id == AV_CODEC_ID_NONE || !c) { + av_log(avctx, AV_LOG_INFO, " unknown compression type"); + } else { + av_log(avctx, AV_LOG_INFO, " video_codec=%s", c->name); + }

I wonder if these logs don't really belong to VERBOSE.

No, these will only print out when the user asks for them.

...

+ } else { + av_log(avctx, AV_LOG_INFO, " pix_fmt=%s", av_get_pix_fmt_name(pix_fmt)); + } + av_log(avctx, AV_LOG_INFO, " min s=%ldx%ld fps=%g max s=%ldx%ld fps=%g", vcaps->MinOutputSize.cx, vcaps->MinOutputSize.cy, 1e7 / vcaps->MaxFrameInterval, vcaps->MaxOutputSize.cx, vcaps->MaxOutputSize.cy, 1e7 / vcaps->MinFrameInterval);

...

+ av_log(avctx, AV_LOG_INFO, "\n");

Why a separate log line for this?

...

continue; } + if (ctx->video_codec_id != AV_CODEC_ID_RAWVIDEO) { + if (ctx->video_codec_id != dshow_codecid(bih->biCompression)) + goto next; + } + if (ctx->pixel_format_str && + ctx->pixel_format != dshow_pixfmt(bih->biCompression, bih->biBitCount)) { + goto next; + } if (ctx->framerate) { int64_t framerate = ((int64_t) ctx->requested_framerate.den*10000000) / ctx->requested_framerate.num; @@ -465,7 +491,9 @@ 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)) + int set_format = (devtype == VideoDevice && (ctx->video_size || ctx->framerate || + ctx->pixel_format_str ||

ctx->pixel_format != PIX_FMT_NONE for consistency?

...

+ ctx->video_codec_id != AV_CODEC_ID_RAWVIDEO)) || (devtype == AudioDevice && (ctx->channels || ctx->sample_rate)); int format_set = 0;

@@ -801,6 +829,22 @@ static int dshow_read_header(AVFormatContext *avctx) goto error; }

+ ctx->video_codec_id = avctx->video_codec_id ? avctx->video_codec_id + : AV_CODEC_ID_RAWVIDEO; + if (ctx->pixel_format_str) { + if (ctx->video_codec_id != AV_CODEC_ID_RAWVIDEO) { + av_log(avctx, AV_LOG_ERROR, "Pixel format may only be set when " + "video codec is not set or set to rawvideo.\n"); + ret = AVERROR(EINVAL); + goto error; + }

...

+ ctx->pixel_format = av_get_pix_fmt(ctx->pixel_format_str); + if (ctx->pixel_format == PIX_FMT_NONE) { + av_log(avctx, AV_LOG_ERROR, "No such pixel format '%s'.\n", ctx->pixel_format_str); + ret = AVERROR(EINVAL); + goto error; + } + } if (ctx->video_size) { r = av_parse_video_size(&ctx->requested_width, &ctx->requested_height, ctx->video_size); if (r < 0) { @@ -941,6 +985,7 @@ static int dshow_read_packet(AVFormatContext *s, AVPacket *pkt) #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), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, DEC },

Unrelated: you may use AV_OPT_VIDEO_SIZE

I wasn't aware of those AV_OPT_TYPEs. I might get around to updating them once the next issue is fixed.

...

+ { "pixel_format", "set video pixel format", OFFSET(pixel_format_str), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, DEC },

You may use AV_OPT_PIXEL_FMT

It is currently not possible to set the default to PIX_FMT_NONE. I've left it checking for pixel_format_str as was in the previous patch until it is possible to set this default.

...

{ "framerate", "set video frame rate", OFFSET(framerate), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, DEC }, { "sample_rate", "set audio sample rate", OFFSET(sample_rate), AV_OPT_TYPE_INT, {.dbl = 0}, 0, INT_MAX, DEC }, { "sample_size", "set audio sample size", OFFSET(sample_size), AV_OPT_TYPE_INT, {.dbl = 0}, 0, 16, DEC }, -- 1.7.4.1

...

From f480590b658f36d3ca84c57111f4bd2af09c1fe3 Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Thu, 23 Aug 2012 16:36:36 -0300 Subject: [PATCH 2/2] dshow: fix return code when opening device fails

--- libavdevice/dshow.c | 18 ++++++++---------- 1 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index d3b5d8f..05e1bbe 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -895,20 +895,18 @@ static int dshow_read_header(AVFormatContext *avctx) }

if (ctx->device_name[VideoDevice]) { - ret = dshow_open_device(avctx, devenum, VideoDevice); - if (ret < 0) - goto error; - ret = dshow_add_device(avctx, VideoDevice); - if (ret < 0) + if ((r = dshow_open_device(avctx, devenum, VideoDevice)) < 0 || + (r = dshow_add_device(avctx, VideoDevice) < 0)) { + ret = r; goto error; + } } if (ctx->device_name[AudioDevice]) { - ret = dshow_open_device(avctx, devenum, AudioDevice); - if (ret < 0) - goto error; - ret = dshow_add_device(avctx, AudioDevice); - if (ret < 0) + if ((r = dshow_open_device(avctx, devenum, AudioDevice)) < 0 || + (r = dshow_add_device(avctx, AudioDevice) < 0)) { + ret = r; goto error; + } }

Uhm, this double r/ret business is confusing. Maybe it would be simpler to separate normal terminating logic from error code logic.

error code is ret, normal terminating logic is ir. Isn't that the way it is now?

Also after several minutes spent looking at the patch and at the code I can't still figure out why this is necessary.

I remember it being necessary at the time, but I don't remember how to reproduce and I can't figure it out as well. Ramiro

On date Tuesday 2012-09-04 21:28:49 -0300, Ramiro Polla encoded:

On Tue, Sep 4, 2012 at 1:25 PM, Stefano Sabatini <stefasab@gmail.com> wrote: [...] From 773200980d70569fa25475052ea4061a9672aba5 Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Tue, 4 Sep 2012 20:59:39 -0300 Subject: [PATCH] dshow: support video codec and pixel format selection

--- libavdevice/dshow.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 44 insertions(+), 1 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index 18adf8f..0beb186 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -20,6 +20,7 @@ */

#include "libavutil/parseutils.h" +#include "libavutil/pixdesc.h" #include "libavutil/opt.h" #include "libavformat/internal.h" #include "avdevice.h" @@ -52,6 +53,9 @@ struct dshow_ctx {

IMediaControl *control;

+ char *pixel_format_str; + enum PixelFormat pixel_format; + enum AVCodecID video_codec_id; char *video_size; char *framerate;

@@ -371,6 +375,18 @@ dshow_cycle_formats(AVFormatContext *avctx, enum dshowDeviceType devtype, goto next; } if (!pformat_set) { + enum PixelFormat pix_fmt = dshow_pixfmt(bih->biCompression, bih->biBitCount); + if (pix_fmt == PIX_FMT_NONE) { + enum AVCodecID codec_id = dshow_codecid(bih->biCompression); + AVCodec *codec = avcodec_find_decoder(codec_id); + if (codec_id == AV_CODEC_ID_NONE || !codec) { + av_log(avctx, AV_LOG_INFO, " unknown compression type"); + } else { + av_log(avctx, AV_LOG_INFO, " video_codec=%s", codec->name); + } + } else { + av_log(avctx, AV_LOG_INFO, " pix_fmt=%s", av_get_pix_fmt_name(pix_fmt)); + } av_log(avctx, AV_LOG_INFO, " min s=%ldx%ld fps=%g max s=%ldx%ld fps=%g\n", vcaps->MinOutputSize.cx, vcaps->MinOutputSize.cy, 1e7 / vcaps->MaxFrameInterval, @@ -378,6 +394,14 @@ dshow_cycle_formats(AVFormatContext *avctx, enum dshowDeviceType devtype, 1e7 / vcaps->MinFrameInterval); continue; } + if (ctx->video_codec_id != AV_CODEC_ID_RAWVIDEO) { + if (ctx->video_codec_id != dshow_codecid(bih->biCompression)) + goto next; + } + if (ctx->pixel_format_str && + ctx->pixel_format != dshow_pixfmt(bih->biCompression, bih->biBitCount)) { + goto next; + } if (ctx->framerate) { int64_t framerate = ((int64_t) ctx->requested_framerate.den*10000000) / ctx->requested_framerate.num; @@ -511,7 +535,9 @@ 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)) + int set_format = (devtype == VideoDevice && (ctx->video_size || ctx->framerate || + ctx->pixel_format_str || + ctx->video_codec_id != AV_CODEC_ID_RAWVIDEO)) || (devtype == AudioDevice && (ctx->channels || ctx->sample_rate)); int format_set = 0;

@@ -851,6 +877,22 @@ static int dshow_read_header(AVFormatContext *avctx) goto error; }

+ ctx->video_codec_id = avctx->video_codec_id ? avctx->video_codec_id + : AV_CODEC_ID_RAWVIDEO; + if (ctx->pixel_format_str) { + if (ctx->video_codec_id != AV_CODEC_ID_RAWVIDEO) { + av_log(avctx, AV_LOG_ERROR, "Pixel format may only be set when " + "video codec is not set or set to rawvideo.\n");

Nit: ending "." is inconsistent with the prevailing convention in the codebase

+ ret = AVERROR(EINVAL); + goto error; + } + ctx->pixel_format = av_get_pix_fmt(ctx->pixel_format_str); + if (ctx->pixel_format == PIX_FMT_NONE) { + av_log(avctx, AV_LOG_ERROR, "No such pixel format '%s'.\n", ctx->pixel_format_str);

Nit: same (or you could use AV_OPT_TYPE_PIXEL_FMT)

+ ret = AVERROR(EINVAL); + goto error; + } + } if (ctx->video_size) { r = av_parse_video_size(&ctx->requested_width, &ctx->requested_height, ctx->video_size); if (r < 0) { @@ -990,6 +1032,7 @@ static int dshow_read_packet(AVFormatContext *s, AVPacket *pkt) #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), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, DEC }, + { "pixel_format", "set video pixel format", OFFSET(pixel_format_str), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, DEC },

Nit: pixel_format for consistency with the other options, which are also taking a string.

{ "framerate", "set video frame rate", OFFSET(framerate), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, DEC }, { "sample_rate", "set audio sample rate", OFFSET(sample_rate), AV_OPT_TYPE_INT, {.dbl = 0}, 0, INT_MAX, DEC }, { "sample_size", "set audio sample size", OFFSET(sample_size), AV_OPT_TYPE_INT, {.dbl = 0}, 0, 16, DEC },

Also: missing docs in indevs.texi for pixel_format option. Should be good otherwise if tested (I'll apply with the nits fixed if you won't send an updated patch in a few days). -- FFmpeg = Frightening & Fancy Mortal Pacific Elected Gigant

On Thu, Sep 6, 2012 at 8:52 PM, Ramiro Polla <ramiro.polla@gmail.com> wrote:

On Wed, Sep 5, 2012 at 11:34 AM, Stefano Sabatini <stefasab@gmail.com> wrote:

...

On date Tuesday 2012-09-04 21:28:49 -0300, Ramiro Polla encoded:

...

On Tue, Sep 4, 2012 at 1:25 PM, Stefano Sabatini <stefasab@gmail.com> wrote: [...] From 773200980d70569fa25475052ea4061a9672aba5 Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla@gmail.com> Date: Tue, 4 Sep 2012 20:59:39 -0300 Subject: [PATCH] dshow: support video codec and pixel format selection

--- libavdevice/dshow.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 44 insertions(+), 1 deletions(-)

diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index 18adf8f..0beb186 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -20,6 +20,7 @@ */

#include "libavutil/parseutils.h" +#include "libavutil/pixdesc.h" #include "libavutil/opt.h" #include "libavformat/internal.h" #include "avdevice.h" @@ -52,6 +53,9 @@ struct dshow_ctx {

IMediaControl *control;

+ char *pixel_format_str; + enum PixelFormat pixel_format; + enum AVCodecID video_codec_id; char *video_size; char *framerate;

@@ -371,6 +375,18 @@ dshow_cycle_formats(AVFormatContext *avctx, enum dshowDeviceType devtype, goto next; } if (!pformat_set) { + enum PixelFormat pix_fmt = dshow_pixfmt(bih->biCompression, bih->biBitCount); + if (pix_fmt == PIX_FMT_NONE) { + enum AVCodecID codec_id = dshow_codecid(bih->biCompression); + AVCodec *codec = avcodec_find_decoder(codec_id); + if (codec_id == AV_CODEC_ID_NONE || !codec) { + av_log(avctx, AV_LOG_INFO, " unknown compression type"); + } else { + av_log(avctx, AV_LOG_INFO, " video_codec=%s", codec->name); + } + } else { + av_log(avctx, AV_LOG_INFO, " pix_fmt=%s", av_get_pix_fmt_name(pix_fmt)); + } av_log(avctx, AV_LOG_INFO, " min s=%ldx%ld fps=%g max s=%ldx%ld fps=%g\n", vcaps->MinOutputSize.cx, vcaps->MinOutputSize.cy, 1e7 / vcaps->MaxFrameInterval, @@ -378,6 +394,14 @@ dshow_cycle_formats(AVFormatContext *avctx, enum dshowDeviceType devtype, 1e7 / vcaps->MinFrameInterval); continue; } + if (ctx->video_codec_id != AV_CODEC_ID_RAWVIDEO) { + if (ctx->video_codec_id != dshow_codecid(bih->biCompression)) + goto next; + } + if (ctx->pixel_format_str && + ctx->pixel_format != dshow_pixfmt(bih->biCompression, bih->biBitCount)) { + goto next; + } if (ctx->framerate) { int64_t framerate = ((int64_t) ctx->requested_framerate.den*10000000) / ctx->requested_framerate.num; @@ -511,7 +535,9 @@ 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)) + int set_format = (devtype == VideoDevice && (ctx->video_size || ctx->framerate || + ctx->pixel_format_str || + ctx->video_codec_id != AV_CODEC_ID_RAWVIDEO)) || (devtype == AudioDevice && (ctx->channels || ctx->sample_rate)); int format_set = 0;

@@ -851,6 +877,22 @@ static int dshow_read_header(AVFormatContext *avctx) goto error; }

+ ctx->video_codec_id = avctx->video_codec_id ? avctx->video_codec_id + : AV_CODEC_ID_RAWVIDEO; + if (ctx->pixel_format_str) { + if (ctx->video_codec_id != AV_CODEC_ID_RAWVIDEO) { + av_log(avctx, AV_LOG_ERROR, "Pixel format may only be set when " + "video codec is not set or set to rawvideo.\n");

Nit: ending "." is inconsistent with the prevailing convention in the codebase

Do you mean all ending '.'s should be removed? If so, feel free to do it or I can send a patch later.

...

...

+ ret = AVERROR(EINVAL); + goto error; + } + ctx->pixel_format = av_get_pix_fmt(ctx->pixel_format_str); + if (ctx->pixel_format == PIX_FMT_NONE) { + av_log(avctx, AV_LOG_ERROR, "No such pixel format '%s'.\n", ctx->pixel_format_str);

Nit: same (or you could use AV_OPT_TYPE_PIXEL_FMT)

Thanks for fixing this. Changed to AV_OPT_TYPE_PIXEL_FMT and removed pixel_format_str variable.

...

...

+ ret = AVERROR(EINVAL); + goto error; + } + } if (ctx->video_size) { r = av_parse_video_size(&ctx->requested_width, &ctx->requested_height, ctx->video_size); if (r < 0) { @@ -990,6 +1032,7 @@ static int dshow_read_packet(AVFormatContext *s, AVPacket *pkt) #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), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, DEC }, + { "pixel_format", "set video pixel format", OFFSET(pixel_format_str), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, DEC },

Nit: pixel_format for consistency with the other options, which are also taking a string.

...

{ "framerate", "set video frame rate", OFFSET(framerate), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, DEC }, { "sample_rate", "set audio sample rate", OFFSET(sample_rate), AV_OPT_TYPE_INT, {.dbl = 0}, 0, INT_MAX, DEC }, { "sample_size", "set audio sample size", OFFSET(sample_size), AV_OPT_TYPE_INT, {.dbl = 0}, 0, 16, DEC },

Also: missing docs in indevs.texi for pixel_format option.

Added. It would be nice if I could use the existing -pix_fmt option though.

...

Should be good otherwise if tested (I'll apply with the nits fixed if you won't send an updated patch in a few days).

Added another patch which uses AV_OPT_TYPE_IMAGE_SIZE for video_size.

I forgot how to send patches. Now patches are attached =)