[FFmpeg-devel] [PATCH] More DirectShow patches

Stefano Sabatini stefasab at gmail.com
Sun Nov 6 00:49:52 CET 2011


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 at 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 at 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.
> >>
> >> + at item video_device_number
> >> +Set video device number for devices with same name (starts at 0).
> >> +
> >> + at 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 at 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 at 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 at 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 at 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 at 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 at 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.
>  
> + at item video_device_number
> +Set video device number for devices with same name (starts at 0,
> +defaults to 0).
> +
> + at 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}:
> + at example
> +$ ffmpeg -f dshow -video_device_number 1 -i video="Camera"
> + at end example
> +
> + at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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


More information about the ffmpeg-devel mailing list