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

[Kieran Bingham]

Quoting Dave Stevenson (2022-06-13 11:52:47)
> Hi Kieran
> 
> On Mon, 13 Jun 2022 at 11:34, Kieran Bingham via libcamera-devel
> <libcamera-devel at lists.libcamera.org> 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?


> 
> (I probably ought to add support for g_skip_frames to the Unicam
> driver at some point).
> 
>   Dave
> 
> [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'.

--
Kieran

> >
> >  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;
> > +       }
> > +       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)
> > --
> > 2.25.1
> >