| Message ID | 20220613103528.226610-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 1c9dc0fd89cfb32d31e25c4d14501c891d329307 |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran On Mon, 13 Jun 2022 at 11:34, Kieran Bingham via libcamera-devel <libcamera-devel@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? (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 [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@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. > > 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 >
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@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@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 > >
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@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@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)
Hi Kieran On Mon, Jun 13, 2022 at 12:35:28PM +0200, 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. > > Signed-off-by: Kieran Bingham <kieran.bingham@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. > Let's keep the warning! As the requirement is documented, as reported by Dave, I wonder if we should not "fix your kernel driver" as we do for missing sensor driver features. Anyway, the patch is good Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > 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 >
Hi Laurent, On Mon, Jun 20, 2022 at 11:59:39AM +0300, Laurent Pinchart wrote: > 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. How many sensors really need that nowadays? I think some ten years ago it was thought to be mainly a property of old, old stuff. Well, surprises always tend to happen when it comes to hardware.
Hi Sakari, On Wed, Jul 13, 2022 at 08:46:14PM +0300, Sakari Ailus wrote: > On Mon, Jun 20, 2022 at 11:59:39AM +0300, Laurent Pinchart wrote: > > 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. > > How many sensors really need that nowadays? I think some ten years ago it > was thought to be mainly a property of old, old stuff. Well, surprises > always tend to happen when it comes to hardware. It's hard to come up with an exact number. I'm sure there are newer sensors whose first images are bad. I also wouldn't be surprised if the number of bad frames could depend on the sensor configuration in some cases. That's why I think the kernel shouldn't be involved here.
Hi Laurent, On Wed, Jul 13, 2022 at 08:52:42PM +0300, Laurent Pinchart wrote: > On Wed, Jul 13, 2022 at 08:46:14PM +0300, Sakari Ailus wrote: > > On Mon, Jun 20, 2022 at 11:59:39AM +0300, Laurent Pinchart wrote: > > > 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. > > > > How many sensors really need that nowadays? I think some ten years ago it > > was thought to be mainly a property of old, old stuff. Well, surprises > > always tend to happen when it comes to hardware. > > It's hard to come up with an exact number. I'm sure there are newer > sensors whose first images are bad. I also wouldn't be surprised if the > number of bad frames could depend on the sensor configuration in some > cases. That's why I think the kernel shouldn't be involved here. Would you store this information outside the kernel, bound to the sensor somehow? But I agree the g_skip_frames shouldn't be visible outside the kernel. The number indicates frames that are really broken, i.e. not just e.g. unusually dark (most applies to SoC cameras).
Hi Sakari, On Wed, Jul 13, 2022 at 09:05:39PM +0300, Sakari Ailus wrote: > On Wed, Jul 13, 2022 at 08:52:42PM +0300, Laurent Pinchart wrote: > > On Wed, Jul 13, 2022 at 08:46:14PM +0300, Sakari Ailus wrote: > > > On Mon, Jun 20, 2022 at 11:59:39AM +0300, Laurent Pinchart wrote: > > > > 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. > > > > > > How many sensors really need that nowadays? I think some ten years ago it > > > was thought to be mainly a property of old, old stuff. Well, surprises > > > always tend to happen when it comes to hardware. > > > > It's hard to come up with an exact number. I'm sure there are newer > > sensors whose first images are bad. I also wouldn't be surprised if the > > number of bad frames could depend on the sensor configuration in some > > cases. That's why I think the kernel shouldn't be involved here. > > Would you store this information outside the kernel, bound to the sensor > somehow? Yes, I'd store it in libcamera, and we actually already do for some sensors. > But I agree the g_skip_frames shouldn't be visible outside the kernel. The > number indicates frames that are really broken, i.e. not just e.g. > unusually dark (most applies to SoC cameras). Just to make sure we're on the same page, I'd deprecated .g_skip_frames() in the kernel, and provide all frames to userspace, unconditionally.
Hi Laurent, On Wed, Jul 13, 2022 at 09:23:00PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Wed, Jul 13, 2022 at 09:05:39PM +0300, Sakari Ailus wrote: > > On Wed, Jul 13, 2022 at 08:52:42PM +0300, Laurent Pinchart wrote: > > > On Wed, Jul 13, 2022 at 08:46:14PM +0300, Sakari Ailus wrote: > > > > On Mon, Jun 20, 2022 at 11:59:39AM +0300, Laurent Pinchart wrote: > > > > > 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. > > > > > > > > How many sensors really need that nowadays? I think some ten years ago it > > > > was thought to be mainly a property of old, old stuff. Well, surprises > > > > always tend to happen when it comes to hardware. > > > > > > It's hard to come up with an exact number. I'm sure there are newer > > > sensors whose first images are bad. I also wouldn't be surprised if the > > > number of bad frames could depend on the sensor configuration in some > > > cases. That's why I think the kernel shouldn't be involved here. > > > > Would you store this information outside the kernel, bound to the sensor > > somehow? > > Yes, I'd store it in libcamera, and we actually already do for some > sensors. > > > But I agree the g_skip_frames shouldn't be visible outside the kernel. The > > number indicates frames that are really broken, i.e. not just e.g. > > unusually dark (most applies to SoC cameras). > > Just to make sure we're on the same page, I'd deprecated > .g_skip_frames() in the kernel, and provide all frames to userspace, > unconditionally. Would you do that also for SoC cameras or just raw ones? The former may well be used without libcamera and so keeping this information in the kernel is somehow justified. I agree moving this out of the kernel has its merits --- whether a frame is going to be usable could be up to user space, too. g_skip_frames() was always a very coarse way to tell the frame was broken.
Hi Sakari, On Wed, Jul 13, 2022 at 09:49:36PM +0300, Sakari Ailus wrote: > On Wed, Jul 13, 2022 at 09:23:00PM +0300, Laurent Pinchart wrote: > > On Wed, Jul 13, 2022 at 09:05:39PM +0300, Sakari Ailus wrote: > > > On Wed, Jul 13, 2022 at 08:52:42PM +0300, Laurent Pinchart wrote: > > > > On Wed, Jul 13, 2022 at 08:46:14PM +0300, Sakari Ailus wrote: > > > > > On Mon, Jun 20, 2022 at 11:59:39AM +0300, Laurent Pinchart wrote: > > > > > > 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. > > > > > > > > > > How many sensors really need that nowadays? I think some ten years ago it > > > > > was thought to be mainly a property of old, old stuff. Well, surprises > > > > > always tend to happen when it comes to hardware. > > > > > > > > It's hard to come up with an exact number. I'm sure there are newer > > > > sensors whose first images are bad. I also wouldn't be surprised if the > > > > number of bad frames could depend on the sensor configuration in some > > > > cases. That's why I think the kernel shouldn't be involved here. > > > > > > Would you store this information outside the kernel, bound to the sensor > > > somehow? > > > > Yes, I'd store it in libcamera, and we actually already do for some > > sensors. > > > > > But I agree the g_skip_frames shouldn't be visible outside the kernel. The > > > number indicates frames that are really broken, i.e. not just e.g. > > > unusually dark (most applies to SoC cameras). > > > > Just to make sure we're on the same page, I'd deprecated > > .g_skip_frames() in the kernel, and provide all frames to userspace, > > unconditionally. > > Would you do that also for SoC cameras or just raw ones? Despite the removal of soc_camera, the name sticks :-) I would do it for raw sensors for sure, and I'm tempted to do it for sensors with an on-board ISP as well. This operation is a bit ill-defined I think, and most receiver drivers don't use it. On the sensor side it's implemented by adv7180, ccs, ov13858 and ov5670 only, where ccs sets it to 1 for JT8EW9 only, and the three other drivers set it to 2. On the receiver side, it's only used by camss, omap3isp and omap4iss (I've left atomisp2 out as it's in staging and in a bad shape). It's thus fairly useless, as on most systems the "bad frames" will still be provided to userspace. I'd rather drop this on the kernel side instead of pretending we handle it correctly. Another reason why it's better done in userspace is that it will allow better control over exposure time and analog gain. Sensors often have an internal delay of one or two frames for those controls, which means per-frame control of the exposure time can often not be guaranteed for the first few frames. If we know the first or first couple of frames are bad, we can still start setting exposure times to prime the per-frame control mechanism and get it ready by the time the first good frame is captured. > The former may well be used without libcamera and so keeping this > information in the kernel is somehow justified. I understand your concern there though, but the mechanism is currently broken, so I think we should remove it, or fix it for real and use it in all drivers. > I agree moving this out of the kernel has its merits --- whether a frame is > going to be usable could be up to user space, too. g_skip_frames() was > always a very coarse way to tell the frame was broken. For what it's worth, the OV5640 seems to produce a garbage frame when it starts, and it doesn't implement .g_skip_frames(). I think it's a lost battle :-)
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)
V4L2 Video devices should always start streaming from index zero. Identify and report a warning if they don't, and correct for any offset. Signed-off-by: Kieran Bingham <kieran.bingham@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. include/libcamera/internal/v4l2_videodevice.h | 1 + src/libcamera/v4l2_videodevice.cpp | 15 +++++++++++++++ 2 files changed, 16 insertions(+)