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

[Luca Abeni]

Hi,

On Mon, 2012-03-26 at 15:14 +0200, Nicolas George wrote:
[...]
> > 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, ?).
Yes, this part I knew :)

[...]
> V4L_TS_* are the values for the to-user timestamp.
Ok, this is part of my misunderstanding. I assumed that "user"
timestamps are wall-clock (what libavdevice always used), and that
V4L_TS_* values simply controlled the "convert from monotonic to
wall-clock" or "do not convert" behaviour (small difference respect to
the real meaning of this parameter).

> As you can notice, for V4L_TS_RAW, no conversion is done, so no detection
> is necessary.
Ok, I see I am not the one being confused :) It seems to me that there
is no V4L_TS_RAW value (I think you mean V4L_TS_DEFAULT, right? BTW. I
think _RAW is more understandable)

One of the things that initially confused me is that convert_timestamp()
is called also in the V4L_TS_DEFAULT case... This is of course ok, it
just confused me when reading the patch (I think it would have been
simpler to call convert_timestamp() only if ts_mode !=
V4L_TS_DEFAULT/RAW)

> 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.
Yes, this originally confused me too... Maybe a comment in the code
would help.


[...]
> > And I'd also offer an option to always perform the conversion, without any kind
> > of autodetection.
> 
> How do you mean exactly?
Just provide a V4L_TS_* value which always asks for a conversion
(without detecting if the kernel timestamps are monotonic or something
else... When I specify this value, I say "I know kernel timestamps are
monotonic, please convert them to wall-clock").


> > 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.
Ok, then.


> > 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?
Well, it could just fail, or return the wall clock... Anyway, this can
be done later.

I think the patch looks ok, and IMHO it can be applied (if you can
address my small doubts highlighted in these emails, that would be good.
But these doubts are not stoppers).


				Thanks,
					Luca