[FFmpeg-devel] [PATCH] More DirectShow patches

Ramiro Polla ramiro.polla at gmail.com
Wed Sep 5 02:28:49 CEST 2012


On Tue, Sep 4, 2012 at 1:25 PM, Stefano Sabatini <stefasab at 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 at 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 at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-dshow-support-video-codec-and-pixel-format-selection.patch
Type: application/octet-stream
Size: 5262 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120904/a2522aa3/attachment.obj>


More information about the ffmpeg-devel mailing list