[Ffmpeg-devel] [PATCH] support v4l2 video source and normid input

[Luca Abeni]

Hi Limin,

Limin Wang wrote:
> Hi,
> 
> After the patch, ffmpeg don't need depend on other applications like tvtime,
> xawtv etc to set them.
as I said last time a similar change was discussed, I do not like this 
idea too much (I like more the idea of using different applications for 
doing different things). But since everyone seem to want this feature in 
ffmpeg, let's put it in.

I have some comments:

[...]
> Index: ffmpeg.c
> ===================================================================
> --- ffmpeg.c	(revision 8487)
> +++ ffmpeg.c	(working copy)
> @@ -181,6 +181,9 @@
>  static int  video_channel = 0;
>  static char *video_standard = "ntsc";
>  
> +static int  tv_video_input = 0;
> +static int  tv_normid = 0;
I do not like this. Global variables do not need to be initialized to 0 
(yes, I see that the existing code already contains initializations to 0 
for other global variables... But let's try to avoid initializing to 0 
new variables). But Michael is the maintainer, so let's see his opinion.

Also, why do you add a "tv_normid" variable, and a "-normid" option? 
Cannot we use video_standard for this?

[...]
> +    /* set tv video input */
> +    memset (&input, 0, sizeof (input));
> +    input.index = ap->tv_video_input;
> +    if(ioctl (s->fd, VIDIOC_ENUMINPUT, &input) < 0) {
> +        av_log(s1, AV_LOG_ERROR, "The V4L2 driver ioctl enum input failed:\n");
> +        return AVERROR_IO;
> +    }
> +
> +    av_log(s1, AV_LOG_INFO, "The V4L2 driver set input_id: %d(%d), input: %s\n",
> +           ap->tv_video_input, input.index, input.name);
> +    if(ioctl (s->fd, VIDIOC_S_INPUT, &input.index) < 0 ) {
> +        av_log(s1, AV_LOG_ERROR, "The V4L2 driver ioctl set input failed:\n");
> +        return AVERROR_IO;
> +    }
> +
> +    /* set tv standard */
> +    memset (&standard, 0, sizeof (standard));
> +    standard.index = ap->tv_normid;
> +    if (ioctl(s->fd, VIDIOC_ENUMSTD, &standard) < 0) {
> +        av_log(s1, AV_LOG_ERROR, "The V4L2 driver ioctl enum norm failed:\n");
> +        return AVERROR_IO;
> +    }
> +
> +    av_log(s1, AV_LOG_INFO, "The V4L2 driver set normid: %d, norm: %s\n",
> +           ap->tv_normid, standard.name);
> +    if (ioctl(s->fd, VIDIOC_S_STD, &standard.id) < 0) {
> +        av_log(s1, AV_LOG_ERROR, "The V4L2 driver ioctl set norm failed:\n");
> +        return AVERROR_IO;
> +    }
> +
Can you put this code in a separate (static) function, and call it here?

> Index: libavformat/avformat.h
> ===================================================================
> --- libavformat/avformat.h	(revision 8487)
> +++ libavformat/avformat.h	(working copy)
> @@ -144,6 +144,8 @@
>  #if LIBAVFORMAT_VERSION_INT < (52<<16)
>      const char *device; /**< video, audio or DV device */
>  #endif
> +    int tv_video_input; /**< tv video source input */
AVFormatParameters already contains a "channel" field that seems to be 
used only by DV input. Can we reuse it instead of introducing a new 
"tv_video_input" field?

In the same way, the "video_channel" variable and "-vc" option in 
ffmpeg.c can be used for selecting the v4l2 input.
(as I said, I believe that the "tv_normid" field can be avoided too, and 
"standard" can be used instead).


			Thanks,
				Luca