[FFmpeg-devel] [PATCH] Add support to video4linux2 framerate auto-set.

[Luca Abeni]

On 01/05/2011 12:26 PM, Stefano Sabatini wrote:
> On date Wednesday 2011-01-05 08:46:05 +0100, Luca Abeni encoded:
>> Hi Stefano,
>>
>> On 01/04/2011 09:52 PM, Stefano Sabatini wrote:
>>> In particular, fix ffmpeg grabbing timestamps when the timebase value
>>> is not set through the CLI.
>>
>> The idea looks good, but, can you move the frame rate autodetection to
>> v4l2_set_parameters(), after the call to VIDIOC_S_PARM? (more precisely,
>> after the "if (ap->time_base.num&&  ap->time_base.den)" block)
>> Does it work, with such a change? (I think so, but, can you verify?)
>
> This would require another change:
> if (ap->time_base.num&&  ap->time_base.den) {
>     // set the timebase in the driver
> }
> if (!ap->time_base.num || !ap->time_base.den) {
>     // auto-select timebase and set it in the driver

Sorry, but why the "set" part? AFAIK, VIDIOC_G_PARM returns the current
frame period, and VIDIOC_S_PARM sets it. So, there is no point in reading
a value and then setting it.

My point is that we want to read the current frame period only if we did
not set it through VIDIOC_S_PARM.
So, I'd do
	if (ap->time_base.num&&  ap->time_base.den) {
		// set the timebase in the driver
	} else {
		// read the current frame period, and use it as a timebase
	}

BTW, I am wondering if VIDIOC_G_PARM is the right ioctl, here, or if the
frame rate should be detected by looking at the video standard description.

> I put the framerate auto-select code just after the size auto-select
> code, as that looked more symmetrical and sounded more consistent with
> the overall design.
>
> As for v4l2_set_parameters(), I suppose it should be used for setting
> parameters, so the auto-selection phase should be already done when
> the function is called. Currently the logic implemented is:
>
> v4l2_read_header()
>    1. check the size and frame rate values, if unset (i.e. if they are
>       set to 0) then use the auto-select feature of the v4l driver for
>       selecting the values to set
>
> v4l2_set_parameters()
>    2. set the size and frame rate values (they need to be non-0 values)

v4l2_set_parameters() can set the video standard, which can change the
video size and frame rate... So, it should be done before autodetecting
the values. And, yes, looking at the code this seems to be currently
broken (regarding the frame size).

I suppose we should first call v4l2_set_parameters(), and then do the
rest of the stuff...


				Luca