[PATCH] [media] uvcvideo: Fix clock param realtime setting

[PATCH] [media] uvcvideo: Fix clock param realtime setting

[Olivier Langlois]

timestamps in v4l2 buffers returned to userspace are updated in
uvc_video_clock_update() which uses timestamps fetched from
uvc_video_clock_decode() by calling unconditionally ktime_get_ts().

Hence setting the module clock param to realtime have no effect
before this patch.

This has been tested with ffmpeg:

ffmpeg -y -f v4l2 -input_format yuyv422 -video_size 640x480 -framerate 30 -i /dev/video0 \
 -f alsa -acodec pcm_s16le -ar 16000 -ac 1 -i default \
 -c:v libx264 -preset ultrafast \
 -c:a libfdk_aac \
 out.mkv

and inspecting the v4l2 input starting timestamp.

Signed-off-by: Olivier Langlois <olivier@trillion01.com>
Cc: Stable <stable@vger.kernel.org>
---
 drivers/media/usb/uvc/uvc_video.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 898c208..c79db33 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -361,6 +361,14 @@ static int uvc_commit_video(struct uvc_streaming *stream,
  * Clocks and timestamps
  */
 
+static inline void uvc_video_get_ts(struct timespec *ts)
+{
+	if (uvc_clock_param == CLOCK_MONOTONIC)
+		ktime_get_ts(ts);
+	else
+		ktime_get_real_ts(ts);
+}
+
 static void
 uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
 		       const __u8 *data, int len)
@@ -420,7 +428,7 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
 	stream->clock.last_sof = dev_sof;
 
 	host_sof = usb_get_current_frame_number(stream->dev->udev);
-	ktime_get_ts(&ts);
+	uvc_video_get_ts(&ts);
 
 	/* The UVC specification allows device implementations that can't obtain
 	 * the USB frame number to keep their own frame counters as long as they
@@ -1011,10 +1019,7 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
 			return -ENODATA;
 		}
 
-		if (uvc_clock_param == CLOCK_MONOTONIC)
-			ktime_get_ts(&ts);
-		else
-			ktime_get_real_ts(&ts);
+		uvc_video_get_ts(&ts);
 
 		buf->buf.v4l2_buf.sequence = stream->sequence;
 		buf->buf.v4l2_buf.timestamp.tv_sec = ts.tv_sec;
-- 
1.9.1

Re: [PATCH] [media] uvcvideo: Fix clock param realtime setting

[Laurent Pinchart]

Hi Olivier,

Thank you for the patch.

On Friday 28 March 2014 01:42:38 Olivier Langlois wrote:
> timestamps in v4l2 buffers returned to userspace are updated in
> uvc_video_clock_update() which uses timestamps fetched from
> uvc_video_clock_decode() by calling unconditionally ktime_get_ts().
> 
> Hence setting the module clock param to realtime have no effect
> before this patch.
> 
> This has been tested with ffmpeg:
> 
> ffmpeg -y -f v4l2 -input_format yuyv422 -video_size 640x480 -framerate 30 -i
> /dev/video0 \ -f alsa -acodec pcm_s16le -ar 16000 -ac 1 -i default \
>  -c:v libx264 -preset ultrafast \
>  -c:a libfdk_aac \
>  out.mkv
> 
> and inspecting the v4l2 input starting timestamp.
> 
> Signed-off-by: Olivier Langlois <olivier@trillion01.com>
> Cc: Stable <stable@vger.kernel.org>

Before applying this, I'm curious, do you have a use case for realtime time 
stamps ?

> ---
>  drivers/media/usb/uvc/uvc_video.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index 898c208..c79db33 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -361,6 +361,14 @@ static int uvc_commit_video(struct uvc_streaming
> *stream, * Clocks and timestamps
>   */
> 
> +static inline void uvc_video_get_ts(struct timespec *ts)
> +{
> +	if (uvc_clock_param == CLOCK_MONOTONIC)
> +		ktime_get_ts(ts);
> +	else
> +		ktime_get_real_ts(ts);
> +}
> +
>  static void
>  uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer
> *buf, const __u8 *data, int len)
> @@ -420,7 +428,7 @@ uvc_video_clock_decode(struct uvc_streaming *stream,
> struct uvc_buffer *buf, stream->clock.last_sof = dev_sof;
> 
>  	host_sof = usb_get_current_frame_number(stream->dev->udev);
> -	ktime_get_ts(&ts);
> +	uvc_video_get_ts(&ts);
> 
>  	/* The UVC specification allows device implementations that can't obtain
>  	 * the USB frame number to keep their own frame counters as long as they
> @@ -1011,10 +1019,7 @@ static int uvc_video_decode_start(struct
> uvc_streaming *stream,
> 			return -ENODATA;
>  		}
> 
> -		if (uvc_clock_param == CLOCK_MONOTONIC)
> -			ktime_get_ts(&ts);
> -		else
> -			ktime_get_real_ts(&ts);
> +		uvc_video_get_ts(&ts);
> 
>  		buf->buf.v4l2_buf.sequence = stream->sequence;
>  		buf->buf.v4l2_buf.timestamp.tv_sec = ts.tv_sec;

-- 
Regards,

Laurent Pinchart

Re: [PATCH] [media] uvcvideo: Fix clock param realtime setting

[Olivier Langlois]

Hi Laurent,


On Fri, 2014-03-28 at 17:20 +0100, Laurent Pinchart wrote:
> Hi Olivier,
> 
> Thank you for the patch.
> 
> On Friday 28 March 2014 01:42:38 Olivier Langlois wrote:
> > timestamps in v4l2 buffers returned to userspace are updated in
> > uvc_video_clock_update() which uses timestamps fetched from
> > uvc_video_clock_decode() by calling unconditionally ktime_get_ts().
> > 
> > Hence setting the module clock param to realtime have no effect
> > before this patch.
> > 
> > This has been tested with ffmpeg:
> > 
> > ffmpeg -y -f v4l2 -input_format yuyv422 -video_size 640x480 -framerate 30 -i
> > /dev/video0 \ -f alsa -acodec pcm_s16le -ar 16000 -ac 1 -i default \
> >  -c:v libx264 -preset ultrafast \
> >  -c:a libfdk_aac \
> >  out.mkv
> > 
> > and inspecting the v4l2 input starting timestamp.
> > 
> > Signed-off-by: Olivier Langlois <olivier@trillion01.com>
> > Cc: Stable <stable@vger.kernel.org>
> 
> Before applying this, I'm curious, do you have a use case for realtime time 
> stamps ?
> 

Yes. ffmpeg uses wall clock time to create timestamps for audio packets
from ALSA device.

There is a bug in ffmpeg describing problems to synchronize audio and
the video from a v4l2 webcam.

https://trac.ffmpeg.org/ticket/692

To workaround this issue, ffmpeg devs added a switch to convert back
monotonic to realtime. From ffmpeg/libavdevice/v4l2.c:

  -ts                <int>        .D.... set type of timestamps for
grabbed frames (from 0 to 2) (default 0)
     default                      .D.... use timestamps from the kernel
     abs                          .D.... use absolute timestamps (wall
clock)
     mono2abs                     .D.... force conversion from monotonic
to absolute timestamps

If the v4l2 driver is able to send realtime ts, it is easier synchronize
in userspace if all inputs use the same clock.

Greetings,
Olivier

Re: [PATCH] [media] uvcvideo: Fix clock param realtime setting

[Laurent Pinchart]

Hi Olivier,

On Friday 28 March 2014 17:27:08 Olivier Langlois wrote:
> On Fri, 2014-03-28 at 17:20 +0100, Laurent Pinchart wrote:
> > On Friday 28 March 2014 01:42:38 Olivier Langlois wrote:
> > > timestamps in v4l2 buffers returned to userspace are updated in
> > > uvc_video_clock_update() which uses timestamps fetched from
> > > uvc_video_clock_decode() by calling unconditionally ktime_get_ts().
> > > 
> > > Hence setting the module clock param to realtime have no effect
> > > before this patch.
> > > 
> > > This has been tested with ffmpeg:
> > > 
> > > ffmpeg -y -f v4l2 -input_format yuyv422 -video_size 640x480 -framerate
> > > 30 -i /dev/video0 \ -f alsa -acodec pcm_s16le -ar 16000 -ac 1 -i
> > > default \
> > > -c:v libx264 -preset ultrafast \
> > > -c:a libfdk_aac \
> > > out.mkv
> > > 
> > > and inspecting the v4l2 input starting timestamp.
> > > 
> > > Signed-off-by: Olivier Langlois <olivier@trillion01.com>
> > > Cc: Stable <stable@vger.kernel.org>
> > 
> > Before applying this, I'm curious, do you have a use case for realtime
> > time stamps ?
> 
> Yes. ffmpeg uses wall clock time to create timestamps for audio packets from
> ALSA device.

OK. I suppose I shouldn't drop support for the realtime clock like I wanted to 
then :-)
 
> There is a bug in ffmpeg describing problems to synchronize audio and
> the video from a v4l2 webcam.
> 
> https://trac.ffmpeg.org/ticket/692
> 
> To workaround this issue, ffmpeg devs added a switch to convert back
> monotonic to realtime. From ffmpeg/libavdevice/v4l2.c:
> 
>   -ts                <int>        .D.... set type of timestamps for
> grabbed frames (from 0 to 2) (default 0)
>      default                      .D.... use timestamps from the kernel
>      abs                          .D.... use absolute timestamps (wall
> clock)
>      mono2abs                     .D.... force conversion from monotonic
> to absolute timestamps
> 
> If the v4l2 driver is able to send realtime ts, it is easier synchronize
> in userspace if all inputs use the same clock.

That might be a stupid question, but shouldn't ALSA use the monotonic clock 
instead ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH] [media] uvcvideo: Fix clock param realtime setting

[Olivier Langlois]

> > > 
> > > Before applying this, I'm curious, do you have a use case for realtime
> > > time stamps ?
> > 
> > Yes. ffmpeg uses wall clock time to create timestamps for audio packets from
> > ALSA device.
> 
> OK. I suppose I shouldn't drop support for the realtime clock like I wanted to 
> then :-)
>  
> > There is a bug in ffmpeg describing problems to synchronize audio and
> > the video from a v4l2 webcam.
> > 
> > https://trac.ffmpeg.org/ticket/692
> > 
> > To workaround this issue, ffmpeg devs added a switch to convert back
> > monotonic to realtime. From ffmpeg/libavdevice/v4l2.c:
> > 
> >   -ts                <int>        .D.... set type of timestamps for
> > grabbed frames (from 0 to 2) (default 0)
> >      default                      .D.... use timestamps from the kernel
> >      abs                          .D.... use absolute timestamps (wall
> > clock)
> >      mono2abs                     .D.... force conversion from monotonic
> > to absolute timestamps
> > 
> > If the v4l2 driver is able to send realtime ts, it is easier synchronize
> > in userspace if all inputs use the same clock.
> 
> That might be a stupid question, but shouldn't ALSA use the monotonic clock 
> instead ?
> 

This isn't stupid. I had the same though after replying you.
Intuitively, I would think that monotonic clock is a better choice for
multimedia. I am just speculating but I would say that ffmpeg decided to
use realtime clock as the standard clock throughout the project for
portability purposes since it is a cross-platform project.

Now you know how I ended up trying the clock=realtime option.
IMHO, if the option is there, it should be working but just removing it
could also be a valid option.

I feel that this could bring some new problems if it stays there
because, I'll be honest and say that I didn't test the driver behavior
when the time goes backward....

Re: [PATCH] [media] uvcvideo: Fix clock param realtime setting

[Olivier Langlois]

Hi Laurent,

> > Yes. ffmpeg uses wall clock time to create timestamps for audio packets from
> > ALSA device.
> 
> OK. I suppose I shouldn't drop support for the realtime clock like I wanted to 
> then :-)
>  
> > There is a bug in ffmpeg describing problems to synchronize audio and
> > the video from a v4l2 webcam.
> > 
> > https://trac.ffmpeg.org/ticket/692
> > 
> > To workaround this issue, ffmpeg devs added a switch to convert back
> > monotonic to realtime. From ffmpeg/libavdevice/v4l2.c:
> > 
> >   -ts                <int>        .D.... set type of timestamps for
> > grabbed frames (from 0 to 2) (default 0)
> >      default                      .D.... use timestamps from the kernel
> >      abs                          .D.... use absolute timestamps (wall
> > clock)
> >      mono2abs                     .D.... force conversion from monotonic
> > to absolute timestamps
> > 
> > If the v4l2 driver is able to send realtime ts, it is easier synchronize
> > in userspace if all inputs use the same clock.
> 
> That might be a stupid question, but shouldn't ALSA use the monotonic clock 
> instead ?
> 
I think that I have that answer why ffmpeg use realtime clock for ALSA
data. In fact ffmpeg uses realtime clock for every data coming from
capture devices and the purpose is to be able to seek into the recorded
stream by using the date where the recording occured. Same principle
than a camera recording dates when pictures are taken.

now a tougher question is whether or not it is up to the driver to
provide these realtime ts.

I'm looking forward your verdict.

Re: [PATCH] [media] uvcvideo: Fix clock param realtime setting

[Laurent Pinchart]

Hi Olivier,

On Sunday 30 March 2014 00:23:01 Olivier Langlois wrote:
> Hi Laurent,
> 
> > > Yes. ffmpeg uses wall clock time to create timestamps for audio packets
> > > from ALSA device.
> > 
> > OK. I suppose I shouldn't drop support for the realtime clock like I
> > wanted to then :-)
> > 
> > > There is a bug in ffmpeg describing problems to synchronize audio and
> > > the video from a v4l2 webcam.
> > > 
> > > https://trac.ffmpeg.org/ticket/692
> > > 
> > > To workaround this issue, ffmpeg devs added a switch to convert back
> > > 
> > > monotonic to realtime. From ffmpeg/libavdevice/v4l2.c:
> > >   -ts                <int>        .D.... set type of timestamps for
> > >                                          grabbed frames (from 0 to 2)
> > >                                          (default 0)
> > >      default                      .D.... use timestamps from the kernel
> > >      abs                          .D.... use absolute timestamps (wall
> > >                                          clock)
> > >      mono2abs                     .D.... force conversion from monotonic
> > >                                          to absolute timestamps
> > > 
> > > If the v4l2 driver is able to send realtime ts, it is easier synchronize
> > > in userspace if all inputs use the same clock.
> > 
> > That might be a stupid question, but shouldn't ALSA use the monotonic
> > clock instead ?
> 
> I think that I have that answer why ffmpeg use realtime clock for ALSA data.
> In fact ffmpeg uses realtime clock for every data coming from capture
> devices and the purpose is to be able to seek into the recorded stream by
> using the date where the recording occured. Same principle than a camera
> recording dates when pictures are taken.
>
> now a tougher question is whether or not it is up to the driver to provide
> these realtime ts.

It makes sense to associate a wall time with recorded streams for that 
purpose, but synchronization should in my opinion be performed using the 
monotonic clock, as the wall time can jump around. I believe drivers should 
provide monotonic timestamps only. However, given that the uvcvideo driver has 
the option of providing wall clock timestamps, that option should work, so 
your patch makes sense. I'd still like to remove support for the wall clock 
though, but I don't want to break userspace. ffmpeg should be fixed, 
especially given that most V4L devices provide monotonic timestamps only.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] [media] uvcvideo: Fix clock param realtime setting

[Olivier Langlois]

Hi Laurent,

On Tue, 2014-04-01 at 15:49 +0200, Laurent Pinchart wrote:
> Hi Olivier,
> 
> On Sunday 30 March 2014 00:23:01 Olivier Langlois wrote:
> > Hi Laurent,
> > 
> > > > Yes. ffmpeg uses wall clock time to create timestamps for audio packets
> > > > from ALSA device.
> > > 
> > > OK. I suppose I shouldn't drop support for the realtime clock like I
> > > wanted to then :-)
> > > 
> > > > There is a bug in ffmpeg describing problems to synchronize audio and
> > > > the video from a v4l2 webcam.
> > > > 
> > > > https://trac.ffmpeg.org/ticket/692
> > > > 
> > > > To workaround this issue, ffmpeg devs added a switch to convert back
> > > > 
> > > > monotonic to realtime. From ffmpeg/libavdevice/v4l2.c:
> > > >   -ts                <int>        .D.... set type of timestamps for
> > > >                                          grabbed frames (from 0 to 2)
> > > >                                          (default 0)
> > > >      default                      .D.... use timestamps from the kernel
> > > >      abs                          .D.... use absolute timestamps (wall
> > > >                                          clock)
> > > >      mono2abs                     .D.... force conversion from monotonic
> > > >                                          to absolute timestamps
> > > > 
> > > > If the v4l2 driver is able to send realtime ts, it is easier synchronize
> > > > in userspace if all inputs use the same clock.
> > > 
> > > That might be a stupid question, but shouldn't ALSA use the monotonic
> > > clock instead ?
> > 
> > I think that I have that answer why ffmpeg use realtime clock for ALSA data.
> > In fact ffmpeg uses realtime clock for every data coming from capture
> > devices and the purpose is to be able to seek into the recorded stream by
> > using the date where the recording occured. Same principle than a camera
> > recording dates when pictures are taken.
> >
> > now a tougher question is whether or not it is up to the driver to provide
> > these realtime ts.
> 
> It makes sense to associate a wall time with recorded streams for that 
> purpose, but synchronization should in my opinion be performed using the 
> monotonic clock, as the wall time can jump around. I believe drivers should 
> provide monotonic timestamps only. However, given that the uvcvideo driver has 
> the option of providing wall clock timestamps, that option should work, so 
> your patch makes sense. I'd still like to remove support for the wall clock 
> though, but I don't want to break userspace. ffmpeg should be fixed, 
> especially given that most V4L devices provide monotonic timestamps only.
> 
Please do not stop yourself to remove realtime ts support in your driver
because that would not break ffmpeg, IMHO. It is just me that have tried
to leverage options offered by your driver to remove the need to use
ffmpeg workaround for a sync issue. I apparently have been the first
ffmpeg user to try out!

I am currently in the process to contribute the introduction of using
CLOCK_MONOTONIC inside ffmpeg. CCing their list because I think your
reply is relevant to the discussion we have on the topic there at the
moment.

thank you,
Olivier

Re: [PATCH] [media] uvcvideo: Fix clock param realtime setting

[Laurent Pinchart]

Hi Olivier,

On Wednesday 02 April 2014 00:31:59 Olivier Langlois wrote:
> On Tue, 2014-04-01 at 15:49 +0200, Laurent Pinchart wrote:
> > On Sunday 30 March 2014 00:23:01 Olivier Langlois wrote:
> > > > > Yes. ffmpeg uses wall clock time to create timestamps for audio
> > > > > packets from ALSA device.
> > > > 
> > > > OK. I suppose I shouldn't drop support for the realtime clock like I
> > > > wanted to then :-)
> > > > 
> > > > > There is a bug in ffmpeg describing problems to synchronize audio
> > > > > and the video from a v4l2 webcam.
> > > > > 
> > > > > https://trac.ffmpeg.org/ticket/692
> > > > > 
> > > > > To workaround this issue, ffmpeg devs added a switch to convert back
> > > > > 
> > > > > monotonic to realtime. From ffmpeg/libavdevice/v4l2.c:
> > > > >   -ts                <int>        .D.... set type of timestamps for
> > > > >                                          grabbed frames (from 0 to
> > > > >                                          2) (default 0)
> > > > >      default                      .D.... use timestamps from the
> > > > >                                          kernel
> > > > >      abs                          .D.... use absolute timestamps
> > > > >                                          (wall clock)
> > > > >      mono2abs                     .D.... force conversion from
> > > > >                                          monotonic to absolute
> > > > >                                          timestamps
> > > > > 
> > > > > If the v4l2 driver is able to send realtime ts, it is easier
> > > > > synchronize in userspace if all inputs use the same clock.
> > > > 
> > > > That might be a stupid question, but shouldn't ALSA use the monotonic
> > > > clock instead ?
> > > 
> > > I think that I have that answer why ffmpeg use realtime clock for ALSA
> > > data. In fact ffmpeg uses realtime clock for every data coming from
> > > capture devices and the purpose is to be able to seek into the recorded
> > > stream by using the date where the recording occured. Same principle
> > > than a camera recording dates when pictures are taken.
> > > 
> > > now a tougher question is whether or not it is up to the driver to
> > > provide these realtime ts.
> > 
> > It makes sense to associate a wall time with recorded streams for that
> > purpose, but synchronization should in my opinion be performed using the
> > monotonic clock, as the wall time can jump around. I believe drivers
> > should provide monotonic timestamps only. However, given that the uvcvideo
> > driver has the option of providing wall clock timestamps, that option
> > should work, so your patch makes sense. I'd still like to remove support
> > for the wall clock though, but I don't want to break userspace. ffmpeg
> > should be fixed, especially given that most V4L devices provide monotonic
> > timestamps only.
>
> Please do not stop yourself to remove realtime ts support in your driver
> because that would not break ffmpeg, IMHO. It is just me that have tried
> to leverage options offered by your driver to remove the need to use
> ffmpeg workaround for a sync issue. I apparently have been the first
> ffmpeg user to try out!

Then let's fix it first, and then I'll remove the option :-)

> I am currently in the process to contribute the introduction of using
> CLOCK_MONOTONIC inside ffmpeg. CCing their list because I think your
> reply is relevant to the discussion we have on the topic there at the
> moment.

Very nice. Please keep me informed of the progress.

-- 
Regards,

Laurent Pinchart