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

[Sakari Ailus]

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.

-- 
Regards,

Sakari Ailus