[libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Identify non-zero stream starts

[Laurent Pinchart]

Hi Kieran,

(CC'ing Sakari)

On Mon, Jun 13, 2022 at 12:54:14PM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Dave Stevenson (2022-06-13 11:52:47)
> > On Mon, 13 Jun 2022 at 11:34, Kieran Bingham via libcamera-devel wrote:
> > >
> > > V4L2 Video devices should always start streaming from index zero.
> > > Identify and report a warning if they don't, and correct for any offset.
> > 
> > [1] sequence
> > "The count starts at zero and includes dropped or repeated frames"
> > 
> > So if the sensor driver has set a value via subdev sensor ops
> > g_skip_frames[2], shouldn't it include that count? Or is skipping
> > considered different from dropping?
> 
> Well - from my perspective, if the frames are skipped at the beginning
> of the stream 'consistently', and didn't even require a buffer - that's
> like they never existed, so I would say they are excluded.
> 
> I also presume though that this would mean userspace has no ability to
> set controls during those frames (as there are no SoF events?). And if
> so - then I think it's right to exclude.
> 
> Of course then to counter-argument myself, could knowing those two frames are
> skipped - provide time to set controls in advance? Or do we then assume
> that controls set from before starting the stream are always fully
> active at the 'first' received real frame?

.g_skip_frames() is an internal kernel API, so I think its result should
not be visible to userspace when it comes to the sequence number.
Otherwise that would effectively say that the sequence number must
always start at zero except when it can start at a different value :-)

I'd go one step further and argue that .g_frame_skip() should be
considered legacy, it's better handled in userspace these days.

> > (I probably ought to add support for g_skip_frames to the Unicam
> > driver at some point).
> > 
> > [1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/buffer.html#struct-v4l2-buffer
> 
> Aha thanks, I'm sure I looked there, and somehow must have missed that
> text. I think I saw the 'In V4L2_FIELD_ALTERNATE_MODE ...' and believed
> the following text was irrelevant.
> 
> > [2] https://www.kernel.org/doc/html/latest/driver-api/media/v4l2-subdev.html#c.v4l2_subdev_sensor_ops
> > 
> > > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > ---
> > >
> > > I'm not yet sure if the Warning print is perhaps just unhelpful.
> > > It doesn't really affect us if we have this work around in - so we can
> > > be silent, and I'm not yet sure if V4L2 has any 'requirement' that it
> > > should start at 0 at every stream on.
> 
> Sounds like this print is actually a valid warning and should stay to
> report kernel 'bugs'.

Any progress updating the kernel documentation to make this requirement
explicit ?

> > >  include/libcamera/internal/v4l2_videodevice.h |  1 +
> > >  src/libcamera/v4l2_videodevice.cpp            | 15 +++++++++++++++
> > >  2 files changed, 16 insertions(+)
> > >
> > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> > > index 23f37f83f8e2..8525acbc558d 100644
> > > --- a/include/libcamera/internal/v4l2_videodevice.h
> > > +++ b/include/libcamera/internal/v4l2_videodevice.h
> > > @@ -276,6 +276,7 @@ private:
> > >         EventNotifier *fdBufferNotifier_;
> > >
> > >         State state_;
> > > +       std::optional<unsigned int> firstFrame_;
> > >
> > >         Timer watchdog_;
> > >         utils::Duration watchdogDuration_;
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > index 430715afd554..63911339f96e 100644
> > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > @@ -1771,6 +1771,19 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()
> > >         if (V4L2_TYPE_IS_OUTPUT(buf.type))
> > >                 return buffer;
> > >
> > > +       /*
> > > +        * Detect kernel drivers which do not reset the sequence number to zero
> > > +        * on stream start.
> > > +        */
> > > +       if (!firstFrame_) {
> > > +               if (buf.sequence)
> > > +                       LOG(V4L2, Warning)
> > > +                               << "Zero sequence expected for first frame (got "
> > > +                               << buf.sequence << ")";
> > > +               firstFrame_ = buf.sequence;
> > > +       }

I'd add a blank line here.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> > > +       buffer->metadata_.sequence -= firstFrame_.value();
> > > +
> > >         unsigned int numV4l2Planes = multiPlanar ? buf.length : 1;
> > >         FrameMetadata &metadata = buffer->metadata_;
> > >
> > > @@ -1847,6 +1860,8 @@ int V4L2VideoDevice::streamOn()
> > >  {
> > >         int ret;
> > >
> > > +       firstFrame_.reset();
> > > +
> > >         ret = ioctl(VIDIOC_STREAMON, &bufferType_);
> > >         if (ret < 0) {
> > >                 LOG(V4L2, Error)

-- 
Regards,

Laurent Pinchart