[FFmpeg-devel] [PATCH] v4l2: read the correct time per frame from devices that support a standard

[Giorgio Vazzana]

2013/1/25 Stefano Sabatini <stefasab at gmail.com>:
> Bravo! This is what I call a good commit message :-).

Thanks! :D

>> +        if (input.std) {
>> +            av_log(s1, AV_LOG_DEBUG, "Setting V4L2 driver standard: %s\n",
>> +                   s->standard);
>> +            /* set tv standard */
>> +            for(i = 0; ; i++) {
>> +                standard.index = i;
>> +                ret = v4l2_ioctl(s->fd, VIDIOC_ENUMSTD, &standard);
>
> v4l2_ioctl() returns -1 in case of error, to get a meaningful error
> code (which is propagated to the caller function), you need to do:
> ret = AVERROR(errno);
> just after the call.

Fixed.

>> +                if (ret < 0 || !av_strcasecmp(standard.name, s->standard))
>> +                    break;
>> +            }
>> +            if (ret < 0) {
>> +                av_log(s1, AV_LOG_ERROR, "Unknown standard '%s'\n", s->standard);
>> +                return ret;
>> +            }
>> +
>
>> +            if (v4l2_ioctl(s->fd, VIDIOC_S_STD, &standard.id) < 0) {
>> +                av_log(s1, AV_LOG_ERROR, "The V4L2 driver ioctl set standard failed\n");
>> +                return AVERROR(EIO);
>
> Here you can return AVERROR(errno) to provide a more meaningful error
> code (you should also set ret before av_log(), or alternatively:
>
> v4l2_ioctl(s->fd, VIDIOC_S_STD, &standard.id);
> if ((ret = AVERROR(errno)) < 0) {
> ...
> return ret;
> }

Fixed.

>> +            }
>> +        } else {
>> +            av_log(s1, AV_LOG_WARNING,
>> +                   "The V4L2 driver does not allow to change standard\n");
>
> Not sure if we should rather fail here, leave as is if in doubt.

This means the user requested a specific standard, but he is using for
example a webcam, so I'd say we can just print a warning he'll
hopefully notice, and keep going. So, left as it was.

>> +        }
>> +    }
>> +
>> +    /* get standard */
>> +    if (v4l2_ioctl(s->fd, VIDIOC_G_STD, &std_id) == 0) {
>> +        tpf = &standard.frameperiod;
>> +        for (i = 0; ; i++) {
>>              standard.index = i;
>>              ret = v4l2_ioctl(s->fd, VIDIOC_ENUMSTD, &standard);
>> -            if (ret < 0 || !av_strcasecmp(standard.name, s->standard))
>> +            if (ret < 0) {
>> +                av_log(s1, AV_LOG_ERROR, "The V4L2 driver ioctl enum standard failed\n");
>> +                return ret;
>> +            }
>
> ditto for errno

Ok, changed.

>> +            if (standard.id == std_id) {
>> +                av_log(s1, AV_LOG_DEBUG,
>> +                       "Current standard: %s, id: %"PRIu64", frameperiod: %d/%d\n",
>> +                       standard.name, standard.id, tpf->numerator, tpf->denominator);
>>                  break;
>> +            }
>
>> +            standard.index++;
>
> seems redundant

Hmm, if the user does not request a standard on the command line, then
we need to get the current standard, and enumerate the supported
standards one by one until one matches. When we find a match, we can
retrieve the time per frame. So, it should not be redundant.

>>          }
>> -        if (ret < 0) {
>> -            av_log(s1, AV_LOG_ERROR, "Unknown standard '%s'\n", s->standard);
>> -            return ret;
>> -        }
>> +    } else {
>> +        std_id = 0;
>> +        tpf = &streamparm.parm.capture.timeperframe;
>> +    }
>>
>> -        av_log(s1, AV_LOG_DEBUG,
>> -               "The V4L2 driver set standard: %s, id: %"PRIu64"\n",
>> -               s->standard, (uint64_t)standard.id);
>> -        if (v4l2_ioctl(s->fd, VIDIOC_S_STD, &standard.id) < 0) {
>> -            av_log(s1, AV_LOG_ERROR,
>> -                   "The V4L2 driver ioctl set standard(%s) failed\n",
>> -                   s->standard);
>> -            return AVERROR(EIO);
>> -        }
>
>> +    streamparm.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>> +    if (v4l2_ioctl(s->fd, VIDIOC_G_PARM, &streamparm) != 0) {
>> +        av_log(s1, AV_LOG_ERROR, "ioctl(VIDIOC_G_PARM): %s\n", strerror(errno));
>> +        return AVERROR(errno);
>
> usual considerations on errno

Fixed.

>>      }
>>
>>      if (framerate_q.num && framerate_q.den) {
>> -        av_log(s1, AV_LOG_DEBUG, "Setting time per frame to %d/%d\n",
>> -               framerate_q.den, framerate_q.num);
>> -        tpf->numerator   = framerate_q.den;
>> -        tpf->denominator = framerate_q.num;
>> -
>> -        if (v4l2_ioctl(s->fd, VIDIOC_S_PARM, &streamparm) != 0) {
>> -            av_log(s1, AV_LOG_ERROR,
>> -                   "ioctl set time per frame(%d/%d) failed\n",
>> +        if (streamparm.parm.capture.capability & V4L2_CAP_TIMEPERFRAME) {
>> +            tpf = &streamparm.parm.capture.timeperframe;
>> +
>> +            av_log(s1, AV_LOG_DEBUG, "Setting time per frame to %d/%d\n",
>>                     framerate_q.den, framerate_q.num);
>> -            return AVERROR(EIO);
>> -        }
>> +            tpf->numerator   = framerate_q.den;
>> +            tpf->denominator = framerate_q.num;
>>
>> -        if (framerate_q.num != tpf->denominator ||
>> -            framerate_q.den != tpf->numerator) {
>> -            av_log(s1, AV_LOG_INFO,
>> -                   "The driver changed the time per frame from "
>> -                   "%d/%d to %d/%d\n",
>> -                   framerate_q.den, framerate_q.num,
>> -                   tpf->numerator, tpf->denominator);
>> -        }
>> -    } else {
>> -        if (v4l2_ioctl(s->fd, VIDIOC_G_PARM, &streamparm) != 0) {
>> -            av_log(s1, AV_LOG_ERROR, "ioctl(VIDIOC_G_PARM): %s\n",
>> -                   strerror(errno));
>> -            return AVERROR(errno);
>
>> +            if (v4l2_ioctl(s->fd, VIDIOC_S_PARM, &streamparm) != 0) {
>> +                av_log(s1, AV_LOG_ERROR, "ioctl set time per frame failed\n");
>> +                return AVERROR(EIO);
>> +            }
>
> ditto about error code (alternatively return always EIO and we'll fix
> it later globally)

Fixed.

New patch attached.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lavd-v4l2-read-the-correct-time-per-frame-from-devic.patch
Type: application/octet-stream
Size: 7436 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130128/a6a7d1fe/attachment.obj>