Re: [RFC] Interactions between camera sensor rotation and flip controls

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Laurent,

On Thu, Jan 26, 2023 at 02:01:43PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Thu, Jan 26, 2023 at 01:46:17PM +0200, Sakari Ailus wrote:
> > On Thu, Jan 26, 2023 at 01:31:39PM +0200, Laurent Pinchart wrote:
> > > On Thu, Jan 26, 2023 at 01:20:11PM +0200, Sakari Ailus wrote:
> > > > On Thu, Jan 26, 2023 at 12:58:08PM +0200, Laurent Pinchart wrote:
> > > > > On Thu, Jan 26, 2023 at 11:16:26AM +0100, Jacopo Mondi wrote:
> > > > > > On Thu, Jan 26, 2023 at 11:48:21AM +0200, Sakari Ailus wrote:
> > > > > > > On Thu, Jan 26, 2023 at 01:18:36AM +0200, Laurent Pinchart wrote:
> > > > > > > > On Thu, Jan 26, 2023 at 01:12:06AM +0200, Laurent Pinchart wrote:
> > > > > > > > > Hello,
> > > > > > > > >
> > > > > > > > > Jacopo, Sakari and I ended up having a long discussion today about the
> > > > > > > > > interactions between sensor rotation (as described in the device tree)
> > > > > > > > > and the V4L2 flip controls. The conversation started from the imx258
> > > > > > > > > series that Jacopo recently posted ([1]) and ended up as an in-depth
> > > > > > > > > analysis of the problem.
> > > > > > > > >
> > > > > > > > > The notes we have taken are copied below. Feedback would be appreciated,
> > > > > > > > > I will then translate that into patches for the kernel documentation.
> > > > > > > > >
> > > > > > > > > [1] https://lore.kernel.org/linux-media/20230117100603.51631-1-jacopo.mondi@xxxxxxxxxxxxxxxx/
> > > > > > > > >
> > > > > > > > > ## Problem description
> > > > > > > > >
> > > > > > > > > V4L2 has five different elements that related to flipping and rotation:
> > > > > > > > >
> > > > > > > > > - Device tree "rotation" property
> > > > > > > > > - V4L2_CID_CAMERA_SENSOR_ROTATION control
> > > > > > > > > - V4L2_CID_ROTATE control
> > > > > > > > > - V4L2_CID_HFLIP and V4L2_CID_VFLIP controls
> > > > > > > > > - Bayer pattern exposed through media bus codes
> > > > > > > > >
> > > > > > > > > While all those components are (more or less) well-defined in the API, their
> > > > > > > > > interactions have never been defined. This has led to different drivers
> > > > > > > > > implementing different behaviours.
> > > > > > > > >
> > > > > > > > > ### Full-featured drivers
> > > > > > > > >
> > > > > > > > > List of drivers that parse the DT rotation property and expose
> > > > > > > > > V4L2_CID_CAMERA_SENSOR_ROTATION, V4L2_CID_HFLIP and V4L2_CID_VFLIP:
> > > > > > > > >
> > > > > > > > > $ git grep -l FLIP $(git grep -l v4l2_ctrl_new_fwnode_properties -- drivers/media/i2c/)
> > > > > > > > > - drivers/media/i2c/imx219.c
> > > > > > > > > - drivers/media/i2c/ov08x40.c
> > > > > > > > > - drivers/media/i2c/ov13b10.c
> > > > > > > > > - drivers/media/i2c/ov5640.c
> > > > > > > > > - drivers/media/i2c/ov5675.c (to be upstreamed)
> > > > > > > > > - drivers/media/i2c/ov5693.c
> > > > > > > > > - drivers/media/i2c/ov8865.c
> > > > > > > > > - drivers/media/i2c/ov9282.c
> > > > > > > > >
> > > > > > > > > All those drivers by ov5640 program the sensor with the HFLIP and VFLIP values
> > > > > > > > > as-is, without taking the rotation property into account. ov5640 inverts the
> > > > > > > > > flipping controls transparently when the rotation is 180, but does still expose
> > > > > > > > > the rotation value to userspace unmodified (commit
> > > > > > > > > 1066fc1c2afdbe5977eae37314f0c21462e82b9a, merged in v6.0).
> > > > > > > > >
> > > > > > > > > ### Flip-enabled drivers
> > > > > > > > >
> > > > > > > > > List of drivers that expose the V4L2_CID_HFLIP and V4L2_CID_VFLIP but not
> > > > > > > > > V4L2_CID_CAMERA_SENSOR_ROTATION:
> > > > > > > > >
> > > > > > > > > $ git grep -vl v4l2_ctrl_new_fwnode_properties $(git grep -l V4L2_CID_HFLIP -- drivers/media/i2c/)
> > > > > > > > > - drivers/media/i2c/ccs/ccs-core.c
> > > > > > > > > - drivers/media/i2c/hi847.c
> > > > > > > > > - drivers/media/i2c/imx208.c
> > > > > > > > > - drivers/media/i2c/imx219.c
> > > > > > > > > - drivers/media/i2c/imx319.c
> > > > > > > > > - drivers/media/i2c/imx355.c
> > > > > > > > > - drivers/media/i2c/mt9m032.c
> > > > > > > > > - drivers/media/i2c/mt9m111.c
> > > > > > > > > - drivers/media/i2c/mt9p031.c
> > > > > > > > > - drivers/media/i2c/mt9v011.c
> > > > > > > > > - drivers/media/i2c/ov08d10.c
> > > > > > > > > - drivers/media/i2c/ov08x40.c
> > > > > > > > > - drivers/media/i2c/ov13b10.c
> > > > > > > > > - drivers/media/i2c/ov2640.c
> > > > > > > > > - drivers/media/i2c/ov2680.c
> > > > > > > > > - drivers/media/i2c/ov5640.c
> > > > > > > > > - drivers/media/i2c/ov5645.c
> > > > > > > > > - drivers/media/i2c/ov5648.c
> > > > > > > > > - drivers/media/i2c/ov5675.c
> > > > > > > > > - drivers/media/i2c/ov5693.c
> > > > > > > > > - drivers/media/i2c/ov6650.c
> > > > > > > > > - drivers/media/i2c/ov7251.c
> > > > > > > > > - drivers/media/i2c/ov7670.c
> > > > > > > > > - drivers/media/i2c/ov772x.c
> > > > > > > > > - drivers/media/i2c/ov7740.c
> > > > > > > > > - drivers/media/i2c/ov8856.c
> > > > > > > > > - drivers/media/i2c/ov8865.c
> > > > > > > > > - drivers/media/i2c/ov9282.c
> > > > > > > > > - drivers/media/i2c/ov9640.c
> > > > > > > > > - drivers/media/i2c/ov9650.c
> > > > > > > > > - drivers/media/i2c/rj54n1cb0c.c
> > > > > > > > > - drivers/media/i2c/s5k5baf.c
> > > > > > > > > - drivers/media/i2c/s5k6aa.c
> > > > > > > > > - drivers/media/i2c/st-vgxy61.c
> > > > > > > > > - drivers/media/i2c/vs6624.c
> > > > > > > > >
> > > > > > > > > Among those, the ccs driver parses the DT rotation property manually and
> > > > > > > > > compensates for it transparently by inverting the flip values. The ov772x and
> > > > > > > > > s5k6aa use a similar mechanism, but based on platform data instead of DT.
> > > > > > > > >
> > > > > > > > > ### Rotation-aware drivers
> > > > > > > > >
> > > > > > > > > List of drivers that parse the DT rotation property manually:
> > > > > > > > >
> > > > > > > > > $ git grep -l '"rotation"' -- drivers/media/i2c/
> > > > > > > > > - drivers/media/i2c/ccs/ccs-core.c
> > > > > > > > > - drivers/media/i2c/imx258.c
> > > > > > > > > - drivers/media/i2c/ov02a10.c
> > > > > > > > >
> > > > > > > > > All those drivers parse the DT rotation property and compensates for it
> > > > > > > > > transparently. The ccs driver inverts the HFLIP and VFLIP controls exposed to
> > > > > > > > > userspace, while the imx258 and ov02a10 flip the image internally but do not
> > > > > > > > > expose the HFLIP and VFLIP controls.
> > > > > > > > >
> > > > > > > > > ## API standardization
> > > > > > > > >
> > > > > > > > > There is a consensus that a standardized API is required. There is also a
> > > > > > > > > consensus that the V4L2_CID_ROTATE control must *not* be used by any sensor
> > > > > > > > > driver. No sensor driver expose that control at the moment, so this shouldn't be
> > > > > > > > > a problem.
> > > > > > > > >
> > > > > > > > > ### API for new drivers
> > > > > > > > >
> > > > > > > > > - Expose the rotation property through V4L2_CID_CAMERA_SENSOR_ROTATION as-is.
> > > > > > > > > - Expose the V4L2_CID_HFLIP and V4L2_CID_VFLIP controls as-is.
> > > > > > > > > - A sensor driver that enables horizontal or vertical flipping *must* expose the
> > > > > > > > >   HFLIP and VFLIP controls. It *should* expose them writable, but *may* expose
> > > > > > > > >   them read-only if not enough information is available to implement them as
> > > > > > > > >   writable in the driver.
> > > > > > > > >
> > > > > > > > > ### Backward-compatibility
> > > > > > > > >
> > > > > > > > > For drivers:
> > > > > > > > >
> > > > > > > > > - We don't care about existing drivers that use platform data (ov772x and
> > > > > > > > >   s5k6aa). The s5k6aa driver requires platform data, so it could be dropped as
> > > > > > > > >   nobody is supplying platform data in mainline.
> > > > > > > > > - The full-featured drivers comply with the API for new drivers except for
> > > > > > > > >   ov5640. Those are thus fine.
> > > > > > > > > - The ov5640 gained V4L2_CID_CAMERA_SENSOR_ROTATION support in v6.0, we should
> > > > > > > > >   fix it, even if it changes the userspace-visible behaviour.
> > > > > > > > >   - Dropping the internal flip has a higher risk of breaking applications.
> > > > > > > > >   - Overriding the V4L2_CID_CAMERA_SENSOR_ROTATION value and setting it
> > > > > > > > >     to 0 when it is 180 is less risky.
> > > > > > > > > - ccs should expose the V4L2_CID_CAMERA_SENSOR_ROTATION control, and modify it
> > > > > > > > >   internally to account the transparent 180° compensation.
> > > > > > > > > - For imx258 and ov02a10, two options are possible:
> > > > > > > > >   - Expose the V4L2_CID_CAMERA_SENSOR_ROTATION control as-is, and expose the
> > > > > > > > >     HFLIP and VFLIP controls read-only and hardcoded to enabled (for imx258) or
> > > > > > > > >     set based on the rotation (for ov02a10). The controls could later be made
> > > > > > > > >     writable. This only risk of userspace breakage would be with applications
> > > > > > > > >     that consider the V4L2_CID_CAMERA_SENSOR_ROTATION control but not the flip
> > > > > > > > >     controls. This is considered to be low-risk.
> > > > > > > > >   - Do as ccs (overriding the V4L2_CID_CAMERA_SENSOR_ROTATION value).
> > > > > > > > >     This is the option preferred by Sakari as it would unify the
> > > > > > > > >     behaviour of the ccs, imx258 and ov02a10 drivers.
> > > > > > > >
> > > > > > > > There is possibly an important user-visible difference between those two
> > > > > > > > options. For rolling-shutter sensors, the motion of objects in the scene
> > > > > > > > will have a different skew effect depending on the sensor rotation. It
> > > > > > > > is thus important for userspace to know the real rotation. For this
> > > > > > >
> > > > > > > How will it be different? The only user-visible difference, as far as I can
> > > > > > > tell, is the order of the Bayer pattern.
> > > > > > 
> > > > > > I expressed a similar concern as the one Laurent has here summarized.
> > > > > > 
> > > > > > I'm thinking about industrial/machine vision applications where knowing the
> > > > > > real mounting rotation and being in control of the row/col read
> > > > > > direction is possibily important with rolling shutters and fast moving
> > > > > > objects? This is mostly speculation, but it might be a valid use case
> > > > > > for very specialized applications..
> > > > > > 
> > > > > > If we compensate V4L2_CID_CAMERA_SENSOR_ROTATION and invert flips to
> > > > > > hide the implicit mirroring that takes place in the driver,
> > > > > > application that expects full control might get confused.
> > > > > 
> > > > > That's my concern too. Imagine an object moving in the top-down
> > > > > direction in front of the sensor. With rolling-shutter sensors, the
> > > > > object will appear stretched vertically if the sensor is mounted
> > > > > upside-up, and squashed if the sensor is mounted upside-down. This may
> > > > > be important information for applications.
> > > > 
> > > > That's not how it works: with the pixel matrix reading direction changed,
> > > > it is read in the same (upright) order than when mounted upright. It
> > > > couldn't be different: the sensor does not have a large buffer where to
> > > > store the data. It's simply sent over the CSI-2 bus in the order it is
> > > > read.
> > > 
> > > Now that you mention it, it's quite obvious indeed. I'm not sure how I
> > > got this wrong.
> > > 
> > > > > > > The users of these drivers have relied on getting upright images without
> > > > > > > further device configuration. The existing user space can be expected to
> > > > > > > break if this is changed.
> > > > > > 
> > > > > > Users that do not manipulate flips doesn't need to be changed, and as
> > > > > > those drivers do not expose flips I don't think there's any issue
> > > > > > here ? IOW current users will continue to work as they do afaict
> > > > > > 
> > > > > > However they will find H/VFLIP enabled by default and (once V/FLIP
> > > > > > controls are registered and made writable) they will have to "disable"
> > > > > > the control to mirror/flip the image if they want to do so.
> > > > > > 
> > > > > > I don't think that's an issue but maybe I'm overlooking it.
> > > > > 
> > > > > For the imx258 and ov02a10, existing users that have the ability to take
> > > > > flip controls or the sensor rotation controls into account, but not
> > > > > both, may be affected. Users that take neither controls into account, or
> > > > > all of them, should be fine. I thus don't expect regressions. For ccs,
> > > > > we may need to check more carefully.
> > > > 
> > > > If the flip control defaults are changed, then there could be fewer
> > > > regressions. But it's hard to tell: the control values are have a
> > > > different meaning after the change.
> > > 
> > > For ccs I agree. Are you concerned about imx258 and ov02a10 ? While we
> > > could handle those two the same way as ccs, handling them the same way
> > > as new drivers but with different defaults for the h/v flip controls has
> > > the advantage of making the implementation simpler, and aligning it with
> > > all the other drivers except ccs. That would be my preference, but if
> > > you strongly prefer otherwise, I'm OK with that as well.
> > 
> > After thinking about this more, I think in the long run it'd be great if we
> > had all the interfaces aligned, including CCS.
> 
> I can't disagree :-)
> 
> > The fallout is likely be
> > very limited, if there is any, even if we change how this works for CCS.
> 
> That's my opinion too.
> 
> > Likely the best way to avoid it would be to default the flipping controls
> > into values that cause the image to be upright with the default
> > configuration, but that, I suppose, would again be a divergence from what
> > the other drivers are doing. Still for applications that already get the
> > CAMERA_SENSOR_ROTATION property, it would be a small step to set the
> > flipping controls as well. Again this not exactly as it has been documented
> > to work, nevertheless still closer to it.
> 
> I'm fine with that.
> 
> > I don't have a wrong opinion about this either way actually. All I want is
> > that we all are aware of the same facts when making the decision.
> > 
> > In fact there are two things that I'm interested in: not breaking existing
> > applications if possible and making this work reasonably well for
> > libcamera.
> 
> We can certainly take care of the latter. As for the former, I'll do my
> best :-)
> 
> Does this mean that you're fine with Jacopo's IMX258 series ?

I replied to the thread.

Beyond these patches, it'd be great if documentation would be written on
how this will work correctly on all sensors. The control documentation
would be probably a good place.

> 
> > > > > > > > reason, I think it would be better to never override the
> > > > > > > > V4L2_CID_CAMERA_SENSOR_ROTATION value exposed to userspace, thus going
> > > > > > > > for the first of the above two options. The ccs driver should ideally do
> > > > > > > > the same.
> > > > > > > >
> > > > > > > > > For userspace:
> > > > > > > > >
> > > > > > > > > - If the V4L2_CID_CAMERA_SENSOR_ROTATION control is not exposed, userspace
> > > > > > > > >   *must* assume that the rotation is 0.
> > > > > > > > > - If the HFLIP and VFLIP controls are not exposed, userspace *must* assume that
> > > > > > > > >   no flipping occurs.
> > > > > > > > > - The captured video is upright if rotation == 0 and both flipping controls are
> > > > > > > > >   disabled or rotation == 180 and both flipping controls are enabled.
> > > > > > > > > - Userspace *must* support read-only HFLIP and VFLIP controls.

-- 
Kind regards,

Sakari Ailus



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux