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

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart