[FFmpeg-devel] [PATCH] More DirectShow patches

[Stefano Sabatini]

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 at gmail.com> wrote:
[...]
> >> @@ -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.

Now it is.

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

In this case then maybe better to drop the patch, than to apply a
patch which we don't know what's useful for.
-- 
FFmpeg = Foolish Fancy Majestic Philosophical Elaborated Gigant