Hi Jacob,
I'm very sorry for the (very) late review. I barely had time to work on the
UVC driver for the past 4 weeks. Things will hopefully settle down a bit
soon. Please don't believe your contribution isn't appreciated.
On Saturday 31 October 2009 00:07:51 Jacob Fehr wrote:
> Hey Laurent,
>
> Attached is a changeset that will sync the computer's clock to the camera
> clock.
Thank you very much. I've copied the patch inline for the purpose of
commenting.
> The final result is that uvc_buffer.buf.timestamp is adjusted to compensate
> for the latency that happens on the camera and the latency that happens in
> the USB stack. It seems to work quite nicely. With the patch, I get very
> even timestamp intervals. In fact it's so accurate that I see a 9
> microsecond/frame drift between my camera clock and my computer clock.
Very nice. Do you have a GPS clock to check which one is correct ;-)
> There is still +-105 microseconds of latency that could be tweaked out. But
> for now this is should be accurate enough for even the most demanding
> application... such as watching TV, DVDs, etc. :-) I suspect the remaining
> latency is coming from the interrupt handler and USB stack. Getting that out
> would require that I get down into the HCD code.
>
> Have a look and let me know what you think.
> diff -r 214c94aa62aa -r 4617ec8de827
> linux/drivers/media/video/uvc/uvc_driver.c
> --- a/linux/drivers/media/video/uvc/uvc_driver.c Sat Oct 10 01:55:23
> 2009 +0200
> +++ b/linux/drivers/media/video/uvc/uvc_driver.c Fri Oct 30 16:24:31
> 2009 -0600
> @@ -198,24 +198,23 @@
> kfree(an);
> }
>
> -/* Convert a fraction to a frame interval in 100ns multiples. The idea here
> is
> - * to compute numerator / denominator * 10000000 using 32 bit fixed point
> - * arithmetic only.
> +/* Calculate a crossproduct. The idea here is to compute
> + * numerator / denominator * multiplier using 32 bit fixed point arithmetic
> + * only.
> */
> -uint32_t uvc_fraction_to_interval(uint32_t numerator, uint32_t denominator)
> +uint32_t uvc_cross_product(uint32_t numerator, uint32_t denominator,
> + uint32_t multiplier)
Interesting reuse of the function. Are you sure this is called a cross
product ? Both Wikipedia and my algebra classes fail to give me a definition
for cross product that could apply here.
> {
> - uint32_t multiplier;
>
> /* Saturate the result if the operation would overflow. */
> if (denominator == 0 ||
> - numerator/denominator >= ((uint32_t)-1)/10000000)
> + numerator/denominator >= ((uint32_t)-1)/multiplier)
> return (uint32_t)-1;
>
> /* Divide both the denominator and the multiplier by two until
> * numerator * multiplier doesn't overflow. If anyone knows a better
> * algorithm please let me know.
> */
> - multiplier = 10000000;
> while (numerator > ((uint32_t)-1)/multiplier) {
> multiplier /= 2;
> denominator /= 2;
[snip]
> diff -r 214c94aa62aa -r 4617ec8de827 linux/drivers/media/video/uvc/uvc_video.c
> --- a/linux/drivers/media/video/uvc/uvc_video.c Sat Oct 10 01:55:23
> 2009 +0200
> +++ b/linux/drivers/media/video/uvc/uvc_video.c Fri Oct 30 16:24:31
> 2009 -0600
> @@ -373,6 +373,8 @@
> struct uvc_buffer *buf, const __u8 *data, int len)
> {
> __u8 fid;
> + __u32 pts = 0, stc = 0, pts_stc_delta, device_latency = 0;
> + int i = 0, idx = 2;
>
> /* Sanity checks:
> * - packet must be at least 2 bytes long
> @@ -417,7 +419,44 @@
> return -ENODATA;
> }
>
> - /* TODO: Handle PTS and SCR. */
> + do_gettimeofday(&buf->buf.timestamp);
> +
> + if((data[1] & UVC_STREAM_PTS) &&
> + (data[1] & UVC_STREAM_SCR &&
> + stream->dev->clock_frequency)) {
Please make sure that your patch conforms with Linux kernel coding style
(Documentation/CodingStyle). You can verify patches using the
scripts/checkpatch.pl before submitting them.
> +
> + for(i = 0; i < 4; i++) {
> + pts += data[idx+i] << (8*i);
> + }
> +
> + idx += 4;
> +
> + for(i = 0; i < 4; i++) {
> + stc += data[idx+i] << (8*i);
> + }
You should use the le32_to_cpup macros instead of reinventing them:
pts = le32_to_cpup(&data[2]);
stc = le32_to_cpup(&data[6]);
> +
> + if(stc < pts) {
> + // Camera clock rolled over
> + pts_stc_delta = UINT_MAX-pts+stc;
> + }
> + else {
> + pts_stc_delta = stc-pts;
> + }
You don't need to handle the overflow manually,
pts_stc_delta = stc - pts;
will produce the right value thanks to the integer overflow in all cases.
> + device_latency = uvc_cross_product(
> + pts_stc_delta, stream->dev->clock_frequency,
> + 1000000);
You should use stream->ctrl.dwClockFrequency here. The VC Interface Header
Descriptor dwClockFrequency, stored in stream->dev->clock_frequency, has been
deprecated in UVC 1.1.
> + if(device_latency &&
> + buf->buf.timestamp.tv_usec < device_latency) {
> +
> + // borrow a second
> + buf->buf.timestamp.tv_sec--;
> + buf->buf.timestamp.tv_usec += 1000000;
> + }
> +
> + buf->buf.timestamp.tv_usec -= device_latency;
You're trying to compute device latency by subtracting the PTS field from the
STC field, and then subtracting that latency, converted to microseconds using
the nominal device clock frequency, from the system time acquired when the
first URB of a video frame completes. I'm afraid this is not correct.
First of all, the nominal device clock frequency isn't accurate. Drift and
jitter make the frequency vary over time. Even if it didn't, the device clock
isn't synchronized to the host clock, so you can't use the device clock
frequency to convert a time stamp difference to a host time delta.
The whole purpose of the PTS and SCR fields is to account for the device clock
inaccuracies by offering a mean to match the device and host clock rates over
time in an adaptive way.
The PTS field gives the device time at which the raw frame capture begins. This
is the time that must be copied to the buffer's timestamp after being converted
to host time.
As the device and host clocks are not synchronized and do not slave to a common
clock, conversion from device time to host time requires additional
information. This is the purpose of the SCR field.
The SCR field gives two timestamps sampled from two clocks at the same time.
The 32 low-order bits are sampled from the device clock (System Time Clock or
STC), and the next 10 bits are sampled from the USB bus clock (SOF counter).
As the SOF counter resolution is only 1ms in full-speed USB, an error of up
to 1ms can be present in the SCR field. This error can be mitigated using
statistical analysis on consecutive SCR samples.
Using that information, the relationship between the device clock and the USB
bus clock can be computed, and the PTS field can be converted to USB bus clock
units.
Now, on the host side, a similar process can be done by sampling the USB SOF
counter and the system time at regular intervals, and the relationship between
the USB bus clock and the system clock can be established. The PTS field
converted to USB bus clock units can further be converted to host clock units.
This is what the PTS/SCR support patch should do. With such an algorithm, the
USB latency calculation in uvc_video_decode_isoc() becomes unnecessary. I'm
afraid the version you submitted doesn't perform those operations.
I'm sorry to have to refuse the patch in its current state.
--
Best regards,
Laurent Pinchart
_______________________________________________
Linux-uvc-devel mailing list
Linux-uvc-devel@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/linux-uvc-devel
