[libcamera-devel] [PATCH 5/9] libcamera: camera_sensor: Validate Transform

[Dave Stevenson]

Hi David & Jacopo

On Mon, 12 Dec 2022 at 11:46, David Plowman via libcamera-devel
<libcamera-devel at lists.libcamera.org> wrote:
>
> Hi Jacopo
>
> Thanks for wading into the sea on this question! (Very appropriate now
> that the stick person example has been turned into a shark...!)
>
> On Fri, 9 Dec 2022 at 12:47, Jacopo Mondi via libcamera-devel
> <libcamera-devel at lists.libcamera.org> wrote:
> >
> > Hi again,
> >
> > On Wed, Dec 07, 2022 at 03:48:40PM +0000, Robert Mader via libcamera-devel wrote:
> > > On 07.12.22 15:28, David Plowman via libcamera-devel wrote:
> > > > Hi Robert
> > > >
> > > > Thanks for prodding us all on this question!
> > >
> > > Hi, thanks for the quick reply! And a pleasure :P
> > >
> > > > I looked back over this again, and found Jacopo's description of the
> > > > V4L2_CID_CAMERA_SENSOR_ROTATION control
> > > > (https://patchwork.kernel.org/project/linux-media/patch/20191204091056.4842-5-jacopo@jmondi.org/
> > > > is what I stumbled into)
> > >
> > > The current kernel docu can be found at
> > >
> > > https://github.com/torvalds/linux/blob/8ed710da2873c2aeb3bb805864a699affaf1d03b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>
> Thanks for the updated reference. The stick person may have turned
> into a shark, but apart from that I think the sense of the
> documentation is unchanged. So if V4L2_CID_CAMERA_SENSOR_ROTATION says
> 90 degrees, then your image in memory looks like it has had a 90
> degree *clockwise* rotation applied.
>
> > >
> > > the libcamera one at
> > >
> > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/transform.cpp#n54
> > >
> > > >  From that, and the nice example with the stick person, I deduce that
> > > > if your camera's V4L2_CID_CAMERA_SENSOR_ROTATION property says 90
> > > > degrees, then the image you capture (if nothing else happens to it)
> > > > will look like it has had a 90 degree clockwise rotation performed.
> > > > Everyone agree with that?
> > > Yep, agreed. That would mean V4L2_CID_CAMERA_SENSOR_ROTATION==90 would
> > > translate to Transform::Rot90.
> >
> > Let's here put some definitions in place
> >
> > V4L2_CID_CAMERA_SENSOR_ROTATION
> > This read-only control describes the rotation correction in degrees in
> > the counter-clockwise direction to be applied to the captured images
> > once captured to memory to compensate for the camera sensor mounting
> > rotation.
> >
> >  \var Transform::Rot270
> >  Rotation by 270 degrees clockwise (90 degrees anticlockwise).
> >
> >  Let's start by saying we did a sub-optimal (to be nice) job in
> >  having Transform and V4L2_CID_ROTATION operate in two different
> >  directions. Before the usage of Transform in libcamera gets widely
> >  adopted sould we align the two ?
>
> I don't mind at all... we just have to decide what we want!
>
> >
> >  Hence if I'm not mistaken, if your camera has Rotation=90 and an
> >  application supplies Transform::Identity in
> >  CameraConfiguration::transform what you should get back is Rot270
> >
> >  Correct ?
>
> The way things are currently, if the camera says Rotation=90, then
> your image in memory looks like it is rotated 90 degrees clockwise, so
> the Transform (if your pipeline and sensor can't apply any
> flips/rotations at all) will come back as Rot90.
>
> That is, the only transform you can specify that won't be adjusted, is
> a 90 degree clockwise rotation, so transform=Rot90 as things stand
> currently.
>
> If we choose to change the sense of the libcamera.Transform rotation,
> then we'd get transform=Rot270 instead.
>
>  (If your pipeline handler can do flips and transposes, it may be able
> to do what you ask and so the transform would come back unchanged and
> would be "Valid".)
>
> >
> > > > So in turn, if you have a camera/ISP that can apply no transforms at
> > > > all, then the only value for the user transform that will not come
> > > > back as adjusted is "rot90", because it looks like it's had a 90
> > > > degree clockwise rotation performed (and libcamera transforms count
> > > > rotations as being clockwise). Does that also make sense?
> >
> > It might be the case today, but I wonder if that's desirable.
> >
> > If an application supplies CameraConfiguration::transform = Identity
> > it means it doesn't want any rotation applied by the library. It will
> > get back 270 to indicate that that's how the image is rotated and will
> > get an Adjusted configuration.
>
> I'm not quite sure I get that, perhaps it depends how we interpret
> "applied by the library". I guess I agree if we mean "applied by the
> library together with any rotation of the sensor"? Maybe a real
> example is helpful:
>
> So for a Raspberry Pi, for example, our sensors are mostly mounted
> upside down (V4L2_CID_CAMERA_SENSOR_ROTATION says "180"). Our users
> say the transform they want is transform=Identity i.e. images come
> back "the right way up". Internally it means that we do apply an extra
> h+v flip to undo the 180 degree rotation of the sensor.
>
> >
> > If instead an application supplies CameraConfiguration::transform = Rot270
> > and your camera -cannot- do any rotation, it will still receive back
> > Rot270 and the state is Valid.
>
> I think that's true if we change the sense of the libcamera.Transform
> rotation. Currently, as I said earlier, I think we get Rot90?
>
> >
> > If it asks for CameraConfiguration::transform = Rot270 and the camera
> > -can- do that, it will clear CameraConfiguration::transform to
> > Identity and return Adjusted ?
>
> I'm not sure there... The CameraConfiguration::transform is the final
> transform you want. If that can be done, it's left alone and you get
> back Valid. It's not telling you what else you would need to do
> afterwards.
>
> (To find out what you need to do afterwards you'd work out
> "inverse(what-you-will-get) * what-you-wanted", or something like
> that.)
>
> >
> > Is this what happens (I'm looking at the above code with David's
> > suggestion to use the inverse of roationTransform).
> >
> >
> > > >
> > > > So by a slightly arbitrary mixture of conventions, it looks to me as
> > > > though the use of the inverse that I queried is indeed wrong, because
> > > > we "need camera 90 degree rotation" => "user transform rot90" in this
> > > > case.
> > > >
> > > > Does that sound plausible... what do folks think?
> > >
> > > Ah, I think the question is whether the adjusted value means: "this is what
> > > you'll get" vs "this is what you have to do yourself".
> > >
> > > Above I argued in favor of the later, but I guess the first one is what the
> > > API is supposed to mean. That would imply that the client has compute the
> > > difference between requested and adjusted value itself.
> >
> > I always presumed the latter was meant to happen..
>
> Yes, I think this is the crux of the matter. I believe it's "what you
> will get". It's not "what you need to do after". (If it was "what you
> need to do after", it would change every time you try to validate the
> configuration!)
>
> >
> > >
> > > In my case that would mean:
> > >
> > > requested CameraConfiguration::transform: Transform::Identity,
> > > V4L2_CID_CAMERA_SENSOR_ROTATION: 270, adjusted
> > > CameraConfiguration::transform: Transform::Rot270
> > >
> >
> > I guess it's Rot90 because of the clockwise/counter-clockwise issue ?
>
> Yes, looks right to me!
>
> >
> > > To compute what the client has to do, it would need to do:
> > > Transform::Identity - Transform::Rot270 = Transform::Rot90. And then apply
> > > that, a 90 degr. rotation clockwise.
>
> Yes, I guess I wrote it above, it's "inverse(what-you-got) *
> what-you-wanted", in this case inverse(rot270) * identity which is
> rot90 (in the current clockwise sense).
>
> > >
> > > Does that sound reasonable? In that case the patch here should indeed be
> > > `*transform = rotationTransform;`, without the inverse.
> > >
>
> Yes, agree!
>
> >
> >         /0\
> >
> > before we plumb this into and make assumptions that will be then
> > set into stone in our adaption layers, can we take a step back and
> > define what kind of API we would like to have ?
> >
> > Let's start by the definition of CameraConfiguration::transform.
> >
> > It's a member of CameraConfiguration, and as by definition it is
> > filled-in by the application and eventually adjusted by the Camera to
> > a value that's acceptable/doable for the current configuration.
> >
> > 1) Application to Camera  = (counter?)clockwise degrees the camera
> > system should rotate the image before presenting it to the user
>
> This may depend on what exactly we mean by "the camera system". I'd
> explain it as
> "the final resulting transform that the application wants to be
> applied to all the output images". After all, this is actually the
> only thing the application knows!
>
> >
> > 2) Camera to application = the rotation the camera can actually do
> > (ie. only sensor flips == no Transpose)
>
> Agree, mostly, though I would say "transform" rather than "rotation"
> because flips aren't rotations, you could in theory have a PH that
> could do transposes (even though we probably don't currently).
>
> >
> > The current documentation doesn't explain how it gets adjusted
>
> This is true, though I also think it's platform dependent. If you had
> a PH that can't do transposes but can do flips, you have a choice how
> you handle (for example) rot90. You could say "oh I can't do that, so
> in this case I won't do anything at all" and leave the application to
> sort everything out. The Pi tries to say "well, I'll do everything
> except for the transpose, so an application only has to worry about
> that one case". But there is a choice...
>
> >
> > /**
> >  * \var CameraConfiguration::transform
> >  * \brief User-specified transform to be applied to the image
> >  *
> >  * The transform is a user-specified 2D plane transform that will be applied
> >  * to the camera images by the processing pipeline before being handed to
> >  * the application. This is subsequent to any transform that is already
> >  * required to fix up any platform-defined rotation.
> >  *
> >  * The usual 2D plane transforms are allowed here (horizontal/vertical
> >  * flips, multiple of 90-degree rotations etc.), but the validate() function
> >  * may adjust this field at its discretion if the selection is not supported.
> >  */
> >
> > The last part in particular, "at its discrection" it's really too
> > generic.
>
> I guess it's hinting at the fact that it's up to the PH, and we don't
> mandate the exact way in which it can fail to do what you asked for!
> But it would be fine to have a discussion on that.
>
> >
> > Let's start from the beginning: what application should use transform
> > for ? I presume they should:
> >
> > 1) Inspect propertis::Rotation
> > 2) Correct the image for the properties::Rotation amount (actually,
> > the inverse because of the clockwise/counter-clockwise thing)
> > 3) Add any additional rotation they would like on top of this
>
> So I think the point of the transform is that you *don't* have to
> worry about the properties::Rotation. It takes care of that for you.
> You say "I want upside down images" and you get them, whatever the
> sensor's rotation.
>
> >
> > How should transform be adjusted ?
> >
> > Identity:
> > If application->identity the camera does not have to apply any
> > transform. Should the transform be adjutes like it happens today to
> > compensate the Rotation (at least that's what I presume happens) ? I
> > presume no, applications can infer rotation from properties and if they
> > chose Identity either other layers above will do the transform or
> > they're fine with images as they are.
>
> If the application says it wants the "identity" transform, then it
> should get output images with no transformation applied. But this does
> mean it will undo the sensor rotation (this being the normal case on
> the Pi).
>
> >
> > A supported/non-supported transform:
> > An application asks for a VFLIP, the camera can do it, transform is
> > not changed. If the camera cannot flip, the transform is set back to
> > Identity and the state is adjusted.
>
> Yes, mostly, except that the sensor may be mounted with a rotation,
> but if it isn't, then I agree with this.
>
> >
> > A partially non-supported transform:
> > An application asks for Rot270 (Transpose | HFLIP) and the camera can
> > only do HFLIP. transform sould be then set to HFLIP and applications
> > know that the remaining transformation should be handled elsewhere.
>
> As I suggested above, I'm not sure whether we want to mandate this. I
> agree it seems useful behaviour, which is why the Pi does it, but I'm
> open minded about requiring it or not.
>
> >
> > Are we missing anything with such behaviour ? I presume it's rather
> > similar to what happens today if not for the fact that the validate()
> > implementation adjusts transform to rotation in case it cannot flip
> >
> >         Transform combined = transform * data_->rotationTransform_;
> >         if (!data_->supportsFlips_ && !!combined)
> >                 transform = -data_->rotationTransform_;
> >
> > Not really sure I got why.
> >
> > All of this, but an even more fundamental question: is it too late/is
> > it worh it to adjust the Transform definition to adopt the same
> > direction as the kernel defined rotation ?
> >
> > Sorry for the long email :/
>
> No problem! I like nothing more than discussing transforms, except
> possibly colour spaces!
>
> More seriously, here are my key points:
>
> 1. The CameraConfiguration::transform field says what the application
> wants the final output images to have.
> 2. It takes account of the sensor rotation. The most obvious example
> is the Pi: if the sensor is mounted upside down, and the applications
> ask for images "the right way up" (i.e. transform = identity), then
> the PH will apply h and v flips to compensate for the sensor mounting.
> 3. If the PH can do what the application requests, it leaves the
> transform alone and your configuration is Valid.
> 4. If the PH can't do what the application wants, it will set the
> value to something that it can do, and the configuration will be
> Adjusted.
>
> If folks wanted a short conference call to iron any of this out, I'd
> be very happy to join in!
>
> Thanks
> David

I'll admit to having only skim read a chunk of this, but it seems
worth throwing in how Android CameraHAL1 dealt with images (largely
from memory of about 10 years ago!).

ALL images would have the native orientation of the sensor.

Preview was told about the appropriate rotation to apply via
setDisplayOrientation().
JPEG and MP4 stored the rotation in the EXIF and Composition Matrix
respectively.
(The bit that many apps forgot about was that it didn't rotate the
pixels in a raw buffer, and they made assumptions. I remember fixing
up the HAL to get around a limitation in the Skype app, and as soon as
I'd done so they released a fixed app).

This obviously puts a requirement on the preview and playback
subsystems, but those are all pretty much mandatory these days.
As long as all the bits of the system know how to orient the images,
then it largely becomes a non-question. As long as libcamera has a
defined way to get all that metadata, then anything can be achieved.

  Dave