Message-ID: <03ac47742945cc04e4663b87563b47a96ed3ec1f.camel@ndufresne.ca>
Date:   Wed, 08 Nov 2023 15:32:23 -0500
From:   nicolas@...fresne.ca
To:     Ricardo Ribalda <ribalda@...omium.org>,
        Esker Wong <esker@...gle.com>,
        Kieran Bingham <kieran.bingham@...asonboard.com>
Cc:     Sakari Ailus <sakari.ailus@....fi>,
        Esker Wong <esker@...omium.org>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] media: uvcvideo: Implement V4L2_EVENT_FRAME_SYNC

Le mercredi 08 novembre 2023 à 08:04 +0100, Ricardo Ribalda a écrit :
> Hi Esker
> 
> On Wed, 8 Nov 2023 at 07:54, Esker Wong <esker@...gle.com> wrote:
> > 
> > Hi Nicholas and Sakari,
> > 
> > We need it as precise as possible. Currently the earliest time of a
> > frame we can have in userspace  is the dqbuf.
> > 
> > And for UVC timestamp, it is somewhat awkward for us to use. Since
> > other functions in our stacks do not necessarily contain such
> > timestamps. So we want some event to be trigger and we can get the
> > system time directly.

The fact that you interpret the time from FRAME_SYNC to DQBUF (well the
READ IO notification) as the actual latency is yours of course. It
assumes that the camera on the other end does not introduce other
source of latency (or that these are negligible). You are also going to
introduce a lot of jitter, since it relies on when the OS decides to
wake up your process.

I think my opinion resides in if you can accurately *enough* implement
what the spec says for FRAME_SYNC then do it, otherwise just don't lie.
I think for ISO, "after the first chunk" i a small lie, but acceptable.
But for BULK, the way it was explained is that it will be always very
close to DQBUF time. and it should not emit FRAME_SYNC for this type of
UVC device. If it fits other events fine of course, I'm just making a
judgment on if its fits V4L2_EVENT_FRAME_SYNC or not.

In term of accuracy, if timestamp was passed with the FRAME_SYNC event,
it would not matter how fast your process the event anymore and greatly
improve accuracy.

> 
> Not to mention that the UVC timestamping requires a bit of love.
> 
> @Laurent Pinchart, @Kieran Bingham  any progress reviewing :P :
> https://patchwork.linuxtv.org/project/linux-media/list/?series=10083

Thanks for working on this by the way, hope someone will find the time
to review this. The timestamps should in theory provide a jitter free
measurement of the delay Esker is trying to measure, and if it wasn't
of bugs (and crazy complexity) it would in the worst case match the
transfer time.

Nicolas

> Esker
> 
> 
> > 
> > If the V4L2_EVENT_FRAME_SYNC will be earlier then V4L2_EVENT_VSYNC,
> > then it has value. We would want to know the delay of a frame being
> > captured to the time it is displayed.
> > 
> > I'm not sure for bulk is the V4L2_EVENT_VSYNC more accurate?
> 
>  V4L2_EVENT_VSYNC wont be more accurate than V4L2_EVENT_FRAME_SYNC.
> 
> My understanding is that Sakari thinks that the description of
> V4L2_EVENT_FRAME_SYNC
> https://www.kernel.org/doc/html/v4.9/media/uapi/v4l/vidioc-dqevent.html#description
>  does not match the current implementation, and suggests using
> V4L2_EVENT_VSYNC instead.
> 
> 
> > 
> > Esker
> > 
> > 
> > On Wed, Nov 8, 2023 at 3:27 AM <nicolas@...fresne.ca> wrote:
> > > 
> > > Hi,
> > > 
> > > Le mardi 07 novembre 2023 à 13:06 +0800, Esker Wong a écrit :
> > > > [send again in text mode]
> > > > Hi Sakari,
> > > > 
> > > > Sequence number is important to us. We need it to measure the latency
> > > > from this event to the time we display the frame.
> > > 
> > > how much precision do you expect, because as described, this number
> > > will be completely false for bulk.
> > > 
> > > Aren't UVC timestamp support to allow measuring latency properly ?
> > > 
> > > Nicolas
> > > 
> > > > 
> > > > Regards,
> > > > Esker
> > > > 
> > > > 
> > > > On Mon, Nov 6, 2023 at 7:06 PM Sakari Ailus <sakari.ailus@....fi> wrote:
> > > > > 
> > > > > Hi Ricardo,
> > > > > 
> > > > > On Mon, Nov 06, 2023 at 10:52:27AM +0000, Ricardo Ribalda wrote:
> > > > > > Add support for the frame_sync event, so user-space can become aware
> > > > > > earlier of new frames.
> > > > > > 
> > > > > > Suggested-by: Esker Wong <esker@...omium.org>
> > > > > > Tested-by: Esker Wong <esker@...omium.org>
> > > > > > Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> > > > > > ---
> > > > > > We have measured a latency of around 30msecs between frame sync
> > > > > > and dqbuf.
> > > > > > ---
> > > > > > Changes in v2:
> > > > > > - Suggested by Laurent. Split sequence++ and event init.
> > > > > > - Link to v1: https://lore.kernel.org/r/20231020-uvc-event-v1-1-3baa0e9f6952@chromium.org
> > > > > > ---
> > > > > >  drivers/media/usb/uvc/uvc_v4l2.c  | 2 ++
> > > > > >  drivers/media/usb/uvc/uvc_video.c | 7 +++++++
> > > > > >  2 files changed, 9 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> > > > > > index f4988f03640a..9f3fb5fd2375 100644
> > > > > > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > > > > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > > > > > @@ -1352,6 +1352,8 @@ static int uvc_ioctl_subscribe_event(struct v4l2_fh *fh,
> > > > > >       switch (sub->type) {
> > > > > >       case V4L2_EVENT_CTRL:
> > > > > >               return v4l2_event_subscribe(fh, sub, 0, &uvc_ctrl_sub_ev_ops);
> > > > > > +     case V4L2_EVENT_FRAME_SYNC:
> > > > > > +             return v4l2_event_subscribe(fh, sub, 0, NULL);
> > > > > >       default:
> > > > > >               return -EINVAL;
> > > > > >       }
> > > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > > > > index 28dde08ec6c5..4f3a510ca4fe 100644
> > > > > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > > > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > > > > @@ -1073,9 +1073,16 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
> > > > > >        * that discontinuous sequence numbers always indicate lost frames.
> > > > > >        */
> > > > > >       if (stream->last_fid != fid) {
> > > > > > +             struct v4l2_event event = {
> > > > > > +                     .type = V4L2_EVENT_FRAME_SYNC,
> > > > > > +             };
> > > > > > +
> > > > > >               stream->sequence++;
> > > > > >               if (stream->sequence)
> > > > > >                       uvc_video_stats_update(stream);
> > > > > > +
> > > > > > +             event.u.frame_sync.frame_sequence = stream->sequence,
> > > > > > +             v4l2_event_queue(&stream->vdev, &event);
> > > > > 
> > > > > uvc_video_decode_start() is called when the reception of the entire frame
> > > > > has been completed. However, the documentation for V4L2_EVENT_FRAME_SYNC
> > > > > says that the event is "Triggered immediately when the reception of a frame
> > > > > has begun.". The functionality here doesn't seem to fit to this patch.
> > > > > 
> > > > > Wouldn't V4L2_EVENT_VSYNC be a better fit, even if we don't really have a
> > > > > concept of vertical sync in the case of USB? That event doesn't have the
> > > > > sequence though but I guess it's not an issue at least if your case.
> > > > > 
> > > > > Another technically correct option could be to create a new event for this
> > > > > but I'm not sure it's worth it.
> > > > > 
> > > > > >       }
> > > > > > 
> > > > > >       uvc_video_clock_decode(stream, buf, data, len);
> > > > > > 
> > > > > 
> > > > > --
> > > > > Regards,
> > > > > 
> > > > > Sakari Ailus
> > > 
> 
> 
> 

Powered by blists - more mailing lists