[libcamera-devel] [PATCH 5/9] libcamera: camera_sensor: Validate Transform
Robert Mader
robert.mader at posteo.de
Wed Dec 14 16:55:46 CET 2022
Hi,
just wanted to drop that
1. I'm personally not very opinionated about the API design
2. the new Pipewire implementation matches the current Raspberry Pi code
3. I think/agree this patch series is the ideal place to
change/stabilize this API
4. 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
> > > > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221214/f4e8ab6f/attachment.htm>
More information about the libcamera-devel
mailing list