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

[David Plowman]

Hi Jacopo

Thanks for your reply!

On Tue, 17 Jan 2023 at 12:09, Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> Hi David,
>         cc Sakari for context about the imx258 sensor patches
>
> On Thu, Jan 12, 2023 at 11:18:51AM +0000, David Plowman wrote:
> > Hi everyone
> >
> > I wanted to give this discussion a little nudge because I don't think
> > it was ever quite concluded, perhaps Christmas got in the way!
> >
> > To summarise a little bit, at least from my point of view, my
> > impression was that the current approach is correct, and the transform
> > field is for the user to request the transform that they want, and may
> > get adjusted to be the transform that they will get. Details of any
> > rotation in the way the sensor is mounted is hidden from the user -
> > they can just say "I want the image the right way up" and it
> > "magically" happens. (This is exactly how the Raspberry Pi pipeline
> > handler works today.)
>
> I have been able to discuss a bit with others, and I'm here trying to
> summarize the outcome
>
> * Recap of the assumptions:
>
> From the kernel interface we have:
> - Physical mounting rotation (V4L2_CID_CAMERA_SENSOR_ROTATION)
> - Sensor flips capabilities (which depends on the V4L2_CID_[H|F]LIP
>   presence and if they are RW)
>
> It is fair to assume that:
> - Sensors cannot do transpose (flip along the diagonal axis)
> - They can only HVFLIP (ie flip, mirror or 180 degrees rotate)
>
> The ISP can also do transforms like transpose or even rotate, but
> compared to acting flips on the sensor this transformation does not
> change the row/col reading order in the image (the first pixels
> visible in the will end up being the ones read last) which might have
> an impact for rolling shutter sensors.
>
> * Proposed implementation
>
> libcamera retrieves the mounting rotation, and if it can fully
> compensate it by setting sensor's flips (or if flips are already hardcoded to
> 1 as it happens in some drivers) it will adjust properties::Rotation
> to report that the image will be rotated upright and application
> shouldn't care. This is what happens today with your implementation of
> the Transform handling part.
>
> This also implies that sensor's can correct only 180 degrees rotation
> and everything else will be deferred to
> 1) other processing layers
> 2) additional transforms in the ISP if the pipeline supports those
>
> If you're ok with such plan I propose:
>
> - let's land my series of 5 patches that centralize flip handling in
>   CameraSensor but does not introduce functional changes
> - let's on top make the CameraSensor class register property::Rotation
>   compensated by the sensor (basically only for 180deg correction)
> - let's adjust CameraSensor::setFormat() to apply flips correctly and
>   remove transpose handling from there but only handle 180 degrees
>   transformations.
>
> For RPi it means 180deg rotation will be automatically compensated as
> they're today by sensor's flips. The only difference is that your
> consumers will see a properties::Rotation = 0 instead of 180, as for
> them the image is already 'correct'.
>
> For other devices with different rotations which require transpose
> (as 90/270 deg, typical for smartphones) the whole rotation adjustment
> will be performed by the application's upper layers.
>
> Does this sound viable to you ?

Mostly that's sounding good to me. The only thing I have a couple of
questions about is what we do if we can't compensate for the camera
rotation.

* It feels a bit strange to clear the advertised camera mounting
rotation if we can handle it, but not otherwise. I think we should
always advertise what it really is (even if that's in a separate
property). Perhaps there's another way to indicate what transforms a
PH can handle (a Transform is just a bit field, after all...)?

* I think the code currently tries to compensate for everything except
transpose, so that would be the only case you have to implement
outside libcamera. But obviously there's a choice there. Might an
application prefer you to do "nothing" instead of "everything bar the
transpose"? Obviously you could work out what you need to ask for to
get the "left over" transform that you want...

Finally, on the subject of rotation orientations, I think it would be
nice if you could request transformFromRotation(sensor_rotation) in
your configuration, and that would guarantee that no transforms would
be applied by libcamera anywhere. That feels "natural" to me.

But otherwise, yes, let's push this forward.

Thanks!
David

>
> >
> > There was a small detail about whether one of the transforms in the
> > code wanted reversing, that's mostly a matter of conventions about
> > using clockwise or anticlockwise rotations (or the direction of the
> > rotation axis).
> >
> > Does anyone have anything else to add here?
> >
> > Thanks!
> > David
> >
> > On Wed, 14 Dec 2022 at 15:55, Robert Mader via libcamera-devel
> > <libcamera-devel at lists.libcamera.org> wrote:
> > >
> > > Hi,
> > >
> > > just wanted to drop that
> > >
> > > I'm personally not very opinionated about the API design
> > > the new Pipewire implementation matches the current Raspberry Pi code
> > > I think/agree this patch series is the ideal place to change/stabilize this API
> > > I'll happily adopt the Pipewire implementation as soon as a consensus is found / the implementation lands :)
> > >
> > > Best regards and thanks to everyone joining the discussion!
> > >
> > > Robert
> > >
> > > On 13.12.22 15:59, Naushir Patuck via libcamera-devel wrote:
> > >
> > > Hi all,
> > >
> > >
> > > On Tue, 13 Dec 2022 at 14:30, David Plowman via libcamera-devel <libcamera-devel at lists.libcamera.org> 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.
> > >>
> > >> On Tue, 13 Dec 2022 at 11:22, Jacopo Mondi <jacopo at jmondi.org> wrote:
> > >> >
> > >> > Hi David
> > >> >
> > >> > On Mon, Dec 12, 2022 at 11:46:31AM +0000, David Plowman 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.
> > >> > >
> > >> >
> > >> > Yes!
> > >> >
> > >> > > > >
> > >> > > > > 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
> > >> >
> > >> > "Applied by the library" == happens inside libcamera, ISP or sensor
> > >> > flips..
> > >> >
> > >> > > 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.
> > >> > >
> > >> >
> > >> > Here you go, this is where we disconnect.
> > >> >
> > >> > As I've interpreted the API so far, and the way we designed
> > >> > properties::Rotation was to signal to the application what is the
> > >> > status of the image as it comes from the sensor. Application could then
> > >> > correct that by setting CameraConfiguration::transform, but in most
> > >> > cases the image will be corrected at rendering time by the application
> > >> > itself.
> > >> >
> > >> > IWO I was not expecting auto-correction and Transform::Identity to me
> > >> > means "give me the images as they come from 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.)
> > >> > >
> > >> >
> > >> > 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.
> > >
> > >
> > > FWIW, I do agree that applications should not have to trouble themselves
> > > with munging sensor rotation andrequested rotation to achieve their
> > > desired outcome.  This definitely wants to be hidden in the libcamera core
> > > or pipeline handler layers.
> > >
> > > Naush
> > >
> > >
> > >>
> > >>
> > >> 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
> > >> > > > >