[FFmpeg-devel] [PATCH] v4l2: Select input immediately after opening the device

[Stefano Sabatini]

On date Friday 2013-01-25 13:12:51 +0100, Giorgio Vazzana encoded:
> 2013/1/25 Stefano Sabatini <stefasab at gmail.com>:
> > On date Thursday 2013-01-24 16:58:59 +0100, Giorgio Vazzana encoded:
> >> -    /* set tv video input */
> >> +    /* enum tv video input */
> >>      input.index = s->channel;
> >>      if (v4l2_ioctl(s->fd, VIDIOC_ENUMINPUT, &input) < 0) {
> >>          av_log(s1, AV_LOG_ERROR, "The V4L2 driver ioctl enum input failed:\n");
> >>          return AVERROR(EIO);
> >>      }
> >
> > maybe use:
> >         av_log(ctx, AV_LOG_ERROR, "ioctl(VIDIOC_ENUMINPUT): %s\n", strerror(errno)
> >
> > like in the other places for consistency. Also you should return
> > AVERROR(errno) in order to provide more info to the user.
> 
> Fixed.
> 
> >> @@ -801,6 +793,21 @@ static int v4l2_read_header(AVFormatContext *s1)
> >>      if (s->fd < 0)
> >>          return s->fd;
> >>
> >> +    /* set tv video input */
> >> +    av_log(s1, AV_LOG_DEBUG, "Selecting V4L2 input_id: %d\n", s->channel);
> >> +    if (v4l2_ioctl(s->fd, VIDIOC_S_INPUT, &s->channel) < 0) {
> >> +        av_log(s1, AV_LOG_ERROR, "The V4L2 driver ioctl set input failed\n");
> >> +        return AVERROR(EIO);
> >> +    }
> >
> > same: unrelated but returning AVERROR(errno) is favored
> 
> Changed.
> 
> >> +    input.index = s->channel;
> >> +    if (v4l2_ioctl(s->fd, VIDIOC_ENUMINPUT, &input) < 0) {
> >> +        av_log(s1, AV_LOG_ERROR, "The V4L2 driver ioctl enum input failed\n");
> >> +        return AVERROR(EIO);
> >> +    }
> >
> > ditto about return error code
> 
> Ok, done.
> 
> >> +    av_log(s1, AV_LOG_DEBUG, "The V4L2 driver set input_id: %d, input: %s\n",
> >> +            s->channel, input.name);
> >> +
> >
> > Nit: all this "V4L2 driver" stuff is redundant and a bit annoying,
> > especially in DEBUG log messages, I suggest something along the line:
> > input_channel:%d input_name:%s
> 
> Changed. There are other places where "V4L2 driver" is used in the log
> messages and/or AVERROR(errno) is not returned, I suggest we fix them
> all later with another patch.
> 
> Thanks for the review, new patch attached.
> 
> Giorgio Vazzana

> From e4434e5bbec6728b04c6c51dc06a63ee3de09a6e Mon Sep 17 00:00:00 2001
> From: Giorgio Vazzana <mywing81 at gmail.com>
> Date: Fri, 25 Jan 2013 13:01:29 +0100
> Subject: [PATCH] v4l2: Select input immediately after opening the device
> 
> After opening the device, the first thing we should do is selecting the input. This is
> because the image formats (VIDIOC_ENUM_FMT ioctl) and the standards (VIDIOC_ENUMSTD ioctl)
> supported may depend on the selected input ([1] and [2]).
> 
> [1] http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-enum-fmt.html
> [2] http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-enumstd.html
> ---
>  libavdevice/v4l2.c |   31 +++++++++++++++++++------------
>  1 files changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c
> index 4ac6967..df904d2 100644
> --- a/libavdevice/v4l2.c
> +++ b/libavdevice/v4l2.c
> @@ -671,20 +671,11 @@ static int v4l2_set_parameters(AVFormatContext *s1)
>          return ret;
>      }
>  
> -    /* set tv video input */
> +    /* enum tv video input */
>      input.index = s->channel;
>      if (v4l2_ioctl(s->fd, VIDIOC_ENUMINPUT, &input) < 0) {
> -        av_log(s1, AV_LOG_ERROR, "The V4L2 driver ioctl enum input failed:\n");
> -        return AVERROR(EIO);
> -    }
> -
> -    av_log(s1, AV_LOG_DEBUG, "The V4L2 driver set input_id: %d, input: %s\n",
> -            s->channel, input.name);
> -    if (v4l2_ioctl(s->fd, VIDIOC_S_INPUT, &input.index) < 0) {
> -        av_log(s1, AV_LOG_ERROR,
> -               "The V4L2 driver ioctl set input(%d) failed\n",
> -                s->channel);
> -        return AVERROR(EIO);
> +        av_log(s1, AV_LOG_ERROR, "ioctl(VIDIOC_ENUMINPUT): %s\n", strerror(errno));
> +        return AVERROR(errno);

av_log() may change errno as a side effect. Thus a safer approach:
v4l2_ioctl(s->fd, VIDIOC_S_INPUT, &input.index);
ret = AVERROR(errno);
if (ret < 0) {
...
}

BTW strerror() is not thread safe and we should rather use
av_err2str(), but this is an unrelated issue.

Note: or you can keep the current error reporting logic in order to
minimize the diff (keeping the first v4l2_ioctl(ENUMINPUT) as is) and
we will fix it later.

Also: given that you're doing ENUMINPUT and S_INPUT in
v4l2_read_header(), isn't this ioclt(ENUMINPUT) redundant here?

>      }
>  
>      if (s->standard) {
> @@ -792,6 +783,7 @@ static int v4l2_read_header(AVFormatContext *s1)
>      uint32_t desired_format;
>      enum AVCodecID codec_id = AV_CODEC_ID_NONE;
>      enum AVPixelFormat pix_fmt = AV_PIX_FMT_NONE;
> +    struct v4l2_input input = { 0 };
>  
>      st = avformat_new_stream(s1, NULL);
>      if (!st)
> @@ -801,6 +793,21 @@ static int v4l2_read_header(AVFormatContext *s1)
>      if (s->fd < 0)
>          return s->fd;
>  
> +    /* set tv video input */
> +    av_log(s1, AV_LOG_DEBUG, "Selecting input_channel: %d\n", s->channel);
> +    if (v4l2_ioctl(s->fd, VIDIOC_S_INPUT, &s->channel) < 0) {
> +        av_log(s1, AV_LOG_ERROR, "ioctl(VIDIOC_S_INPUT): %s\n", strerror(errno));
> +        return AVERROR(errno);
> +    }

ditto about error code issues

> +
> +    input.index = s->channel;
> +    if (v4l2_ioctl(s->fd, VIDIOC_ENUMINPUT, &input) < 0) {
> +        av_log(s1, AV_LOG_ERROR, "ioctl(VIDIOC_ENUMINPUT): %s\n", strerror(errno));
> +        return AVERROR(errno);
> +    }
> +    av_log(s1, AV_LOG_DEBUG, "input_channel: %d, input_name: %s\n",
> +           s->channel, input.name);

ditto

(or you can simply move the code from one place to another, as you prefer).
[...]
-- 
FFmpeg = Free Funny Magnificient Prodigious Educated Guru