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

[Jacopo Mondi]

Hi David,

On Tue, Dec 13, 2022 at 02:30:24PM +0000, David Plowman wrote:
> Hi Jacopo
>
> Thanks for the reply. I won't write a great deal this time round, I
> agree that possibly a call would be a good idea.
>

Sure thanks

[snip]


> >
> > What I would like is to make as easier as possible for applications
> > to know what they have to do to have the image rotated in the way they
> > expect. There are several "transform" steps on the image, the first
> > one happens in libcamera, all the other ones are left to the
> > appplication.
>
> I think this is exactly what I want, namely "to make it as easy as
> possible for applications".
>
> On Raspberry Pi - and this has been the case for a couple of years
> now! - if a user wants images the "normal" way up then they simply
> pass Transform::Identity. If they want upside down images they pass
> Transform::Rot180. It's as easy as that, there is simply nothing else
> to do.
>
> Requiring all applications to look at the sensor rotation and then
> combine various things to figure out what they have to ask for (and
> possibly get it wrong) seems, well, actually quite unhelpful.

Robert, you're working on the pipewire integration. What's your
opinion on this ?

>
> But this is indeed what we need to talk about. Let's organise a call!
>
> Thanks
> David
>
> >
> > *) The image "native" transform is reported by Rotation (this assumes
> > that any implicit rotation buried in the sensor driver, as per our
> > last discussion with Dave is not taken into account)
> >
> >    properties::Rotation = Rot270 = HFlip | Transpose
> >
> > *) Application sets CameraConfiguration::transform and call validate() to
> > check what can be done by libcamera and what should be done by next
> > layers
> >
> >    CameraConfigurat::Transform = Rot270
> >
> > *) The camera validate the transform and as it can only flip it sets
> > transform to HFlip, and the state is Adjusted
> >
> >    validate() -> The camera can only flip
> >               -> CameraConfiguration = HFlip
> >
> > *) Application gets back what the camera can do and instrument the
> > rest of the rendering pipeline to perform the additional transforms
> > the camera cannot do. To get what you have to do I presume you have to
> >
> >         additionalTransform = what_you_want & !what_you_get
> >
> > So yes, I was interpreting the usage of CameraConfiguration::transform
> > as a negotiation of what the library can do and what applications have
> > to do by themselves, which if I got you is different than what's
> > currecntly implemented ?
> >
> > > >
> > > > 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!)
> > >
> >
> > On the "it would change every time you try to validate the
> > configuration": only if the camera cannot satisfy the transform you
> > have requested. If you ask for a flip and the camera can do it,
> > transform stays un-touched and the state is Valid ?
> >
> > > >
> > > > >
> > > > > 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!
> > >
> >
> > agreed
> >
> > > >
> > > > 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"
> >
> > yes, more correct
> >
> > > because flips aren't rotations, you could in theory have a PH that
> > > could do transposes (even though we probably don't currently).
> > >
> >
> > none currently afaict
> >
> > > >
> > > > 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...
> > >
> >
> > I agree it's platform-specific, but the way transform is negotiated with
> > application should be standardized
> >
> > > >
> > > > /**
> > > >  * \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.
> > >
> >
> > That's the point that most concerns me :)
> >
> > > >
> > > > 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.
> >
> > That's acceptable as well if we prefer that, but it has to be
> > standardized among pipelines imho
> >
> > >
> > > >
> > > > 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).
> > >
> >
> > right, it depends if we decide to account for the sensor rotation
> > inside libcamera or not
> >
> > > >
> > > > 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:
> >
> > I'll try to summarize what I see differently
> >
> > >
> > > 1. The CameraConfiguration::transform field says what the application
> > > wants the final output images to have.
> >
> > 1. The CameraConfiguration::transform field says what transform the
> > library has to apply to images as they arrive from the sensor in its
> > default configuration
> >
> > > 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.
> >
> > 2. It doesn't automatically account for sensor rotation. Application
> > might decide to do that along the rendering pipeline, in example
> >
> > > 3. If the PH can do what the application requests, it leaves the
> > > transform alone and your configuration is Valid.
> >
> > Agreed
> >
> > > 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.
> >
> > Agreed
> >
> > >
> > > If folks wanted a short conference call to iron any of this out, I'd
> > > be very happy to join in!
> >
> > I've asked others to have a look here, let's see if we need a call,
> > I'm all for it !
> >
> > Thanks
> >   j
> >
> > >
> > > Thanks
> > > David
> > >
> > > >
> > > > > > Thanks everyone!
> > > > > > David
> > > > >
> > > > > Thanks!
> > > > >
> > > > > Robert
> > > > >