[FFmpeg-devel] [PATCH] More DirectShow patches

Ramiro Polla ramiro.polla at gmail.com
Sun Nov 6 01:18:42 CET 2011


On Sat, Nov 5, 2011 at 9:49 PM, Stefano Sabatini <stefasab at 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 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).

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 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...

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 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.

Looks good.

>> 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).

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


More information about the ffmpeg-devel mailing list