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

[Laurent Pinchart]

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 :-)

-- 
Regards,

Laurent Pinchart