[FFmpeg-devel] [PATCH 2/2] v4l2: allow to convert monotonic timestamps.

[Nicolas George]

Le septidi 7 germinal, an CCXX, Luca Abeni a écrit :
> I finally found the time to look at this patch... I found it on the mailing list
> archives, here:
> http://ffmpeg.org/pipermail/ffmpeg-devel/2012-March/121685.html
> (please correct me if I looked at the wrong version of the patch).

Thanks. That is the right (and only, at this time) version.

> I think the concept (allowing to convert from monotonic timestamps to
> gettimeofday() times) is very good; I just have some questions on the
> implementation:
> It seems to me that detect_timestamp() is invoked only when ts_mode == V4L_TS_ABS,
> (use absolute timestamps) and it will either set ts_mode = V4L_TS_DEFAULT or fail...
> What is the exact meaning of V4L_TS_ABS? Based on the source code, I'd call it
> V4L_TS_AUTODETECT (isn't this what it is really doing?).

There are two kinds of timestamps to consider: the ones we get from the
kernel and  the ones we return to the user.

The ones we get from the kernel can be either wall-clock (old kernels or
recent kernels with a special option), monotonic-clock (recent kernels by
default) or something-else (bogus drivers, ?).

The ones we return to the user can be either same-as-kernel (the current
code), wall-clock (what this patch intends to add), or anything else in a
future patch.

If I put that in a small ASCII-art tableau:

                ,-------- to user: --------.
from kernel:    same-as-kernel    wall-clock
 |      wall    no conversion     no conversion
 | monotonic    no conversion     CONVERSION
 \   strange    no conversion     impossible

V4L_TS_* are the values for the to-user timestamp.

As you can notice, for V4L_TS_RAW, no conversion is done, so no detection
is necessary.

Also, I am somewhat abusing the option field: if the init code detects that
the kernel is returning wall-clock timestamps, then V4L_TS_RAW and
V4L_TS_ABS are synonyms.

If the init code detects that the kernel is returning monotonic-clock
timestamps, the option field is set to V4L_TS_RAW as a means to say
"detection is already done", and the timefilter field is what really matter.

What I could do without making the code more complex is define
V4L_TS_AUTODETECTION_DONE as an alias to V4L_TS_RAW and add what I just
explained as a comment.

> And I'd also offer an option to always perform the conversion, without any kind
> of autodetection.

How do you mean exactly?

> Regarding the timestamps filtering code, I think it is ok if tested.

For me it works, famous last words.

> Also, it seems to me that the patch is adding two options (-ts and -timestamps) to
> do the same thing... Why?

-timestamps is long, -ts is not very explicit; there are already several
places where synonyms for options are offered, especially in lavfi.

> Finally, maybe (but I am not sure about this) av_gettime_monotonic() can be moved
> to libavformat/utils.c, where av_gettime() already is.

I named the function so that it could be easily moved later, but there are
difficult considerations about that: some crappy OSes do not offer the
clock_gettime function or miss the CLOCK_MONOTONIC clock source. What should
av_gettime_monotonic() return on those OSes?

Regards,

-- 
  Nicolas George