[FFmpeg-devel] [PATCH] Set frame rate on v4l2 devices

[jose.goncalves at inov.pt]

M?ns,

Most of your comments are regarding with coding style. I agree with 
most of them (not all). But... if you take a look to the 
v4l2_set_parameters() function you will see that I followed the coding 
style previously used, what seemed the correct thing to do.
FFmpeg maintainers are free to use any coding style but, to make the 
changes that you suggest to my patch, you should previously revise all 
the source code in v4l2.c.

Regards,
Jos? Gon?alves

Quoting M?ns Rullg?rd <mans at mansr.com>:

> jose.goncalves at inov.pt writes:
>
>> Using ffmpeg to stream a webcam MJPEG feed, I found that it can't set
>> the webcam frame rate. For that we must use the VIDIOC_S_PARM
>> ioctl. Please check the attached patch that fixes this.
>>
>> Jos? Gon?alves
>>
>> Index: libavdevice/v4l2.c
>> ===================================================================
>> --- libavdevice/v4l2.c	(revision 25082)
>> +++ libavdevice/v4l2.c	(working copy)
>> @@ -501,6 +501,8 @@
>>      struct video_data *s = s1->priv_data;
>>      struct v4l2_input input;
>>      struct v4l2_standard standard;
>> +    struct v4l2_streamparm streamparm;
>> +    struct v4l2_fract *tpf;
>
> Those can go inside the if() block.
>
>>      int i;
>>
>>      if(ap->channel>=0) {
>> @@ -548,6 +550,33 @@
>>          }
>>      }
>>
>> +    if(ap->time_base.num && ap->time_base.den) {
>> +        av_log(s1, AV_LOG_DEBUG, "The V4L2 driver set time per 
>> frame: %d/%d\n",
>> +               ap->time_base.num, ap->time_base.den);
>> +        /* set frame rate */
>
> Useless comment.  The log line above it says the same thing, and it's
> pretty clear anyway.
>
>> +        memset (&streamparm, 0, sizeof (streamparm));
>
> Initialise the struct with = { 0 } instead.
>
>> +        streamparm.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>> +        tpf = &streamparm.parm.capture.timeperframe;
>
> Initialise tpf in the declaration.
>
>> +        tpf->numerator = ap->time_base.num;
>> +        tpf->denominator = ap->time_base.den;
>> +        if (ioctl(s->fd, VIDIOC_S_PARM, &streamparm) < 0) {
>
> ioctl() returns exactly -1 on error.
>
>> +            av_log(s1, AV_LOG_ERROR,
>> +                   "The V4L2 driver ioctl set time per frame(%d/%d) 
>> failed\n",
>> +                   ap->time_base.num, ap->time_base.den);
>> +            return AVERROR(EIO);
>
> I'm not certain EIO is the best error code here.
>
>> +        }
>> +
>> +        if (ap->time_base.den != tpf->denominator ||
>> +            ap->time_base.num != tpf->numerator) {
>
> Can the ioctl succeed even if a different frame rate was set?  Weird
> interface...
>
>> +            av_log(s1, AV_LOG_INFO,
>> +                   "The V4L2 driver changed the time per frame from 
>> %d/%d to %d/%d\n",
>> +                   ap->time_base.num, ap->time_base.den,
>> +                   tpf->numerator, tpf->denominator);
>> +            ap->time_base.num = tpf->numerator;
>> +            ap->time_base.den = tpf->denominator;
>> +        }
>> +    }
>
> There is no need to mention v4l2 in the messages as they will be
> automatically tagged.
>
> --
> M?ns Rullg?rd
> mans at mansr.com
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
>