<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Hi,</p>
<p>just wanted to drop that</p>
<ol>
<li>I'm personally not very opinionated about the API design<br>
</li>
<li>the new Pipewire implementation matches the current Raspberry
Pi code</li>
<li>I think/agree this patch series is the ideal place to
change/stabilize this API</li>
<li>I'll happily adopt the Pipewire implementation as soon as a
consensus is found / the implementation lands :)</li>
</ol>
<p>Best regards and thanks to everyone joining the discussion!</p>
<p>Robert<br>
</p>
<div class="moz-cite-prefix">On 13.12.22 15:59, Naushir Patuck via
libcamera-devel wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAEmqJPopLuzBu-zsYtZE_kF0T2O4G6mi-bU7m=qauucKdjqF8A@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div dir="ltr">
<div dir="ltr">Hi all,
<div><br>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Tue, 13 Dec 2022 at
14:30, David Plowman via libcamera-devel <<a
href="mailto:libcamera-devel@lists.libcamera.org"
moz-do-not-send="true" class="moz-txt-link-freetext">libcamera-devel@lists.libcamera.org</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">Hi Jacopo<br>
<br>
Thanks for the reply. I won't write a great deal this time
round, I<br>
agree that possibly a call would be a good idea.<br>
<br>
On Tue, 13 Dec 2022 at 11:22, Jacopo Mondi <<a
href="mailto:jacopo@jmondi.org" target="_blank"
moz-do-not-send="true" class="moz-txt-link-freetext">jacopo@jmondi.org</a>>
wrote:<br>
><br>
> Hi David<br>
><br>
> On Mon, Dec 12, 2022 at 11:46:31AM +0000, David Plowman
wrote:<br>
> > Hi Jacopo<br>
> ><br>
> > Thanks for wading into the sea on this question!
(Very appropriate now<br>
> > that the stick person example has been turned into
a shark...!)<br>
> ><br>
> > On Fri, 9 Dec 2022 at 12:47, Jacopo Mondi via
libcamera-devel<br>
> > <<a
href="mailto:libcamera-devel@lists.libcamera.org"
target="_blank" moz-do-not-send="true"
class="moz-txt-link-freetext">libcamera-devel@lists.libcamera.org</a>>
wrote:<br>
> > ><br>
> > > Hi again,<br>
> > ><br>
> > > On Wed, Dec 07, 2022 at 03:48:40PM +0000,
Robert Mader via libcamera-devel wrote:<br>
> > > > On 07.12.22 15:28, David Plowman via
libcamera-devel wrote:<br>
> > > > > Hi Robert<br>
> > > > ><br>
> > > > > Thanks for prodding us all on this
question!<br>
> > > ><br>
> > > > Hi, thanks for the quick reply! And a
pleasure :P<br>
> > > ><br>
> > > > > I looked back over this again, and
found Jacopo's description of the<br>
> > > > > V4L2_CID_CAMERA_SENSOR_ROTATION
control<br>
> > > > > (<a
href="https://patchwork.kernel.org/project/linux-media/patch/20191204091056.4842-5-jacopo@jmondi.org/"
rel="noreferrer" target="_blank" moz-do-not-send="true"
class="moz-txt-link-freetext">https://patchwork.kernel.org/project/linux-media/patch/20191204091056.4842-5-jacopo@jmondi.org/</a><br>
> > > > > is what I stumbled into)<br>
> > > ><br>
> > > > The current kernel docu can be found at<br>
> > > ><br>
> > > > <a
href="https://github.com/torvalds/linux/blob/8ed710da2873c2aeb3bb805864a699affaf1d03b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst"
rel="noreferrer" target="_blank" moz-do-not-send="true"
class="moz-txt-link-freetext">https://github.com/torvalds/linux/blob/8ed710da2873c2aeb3bb805864a699affaf1d03b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst</a><br>
> ><br>
> > Thanks for the updated reference. The stick person
may have turned<br>
> > into a shark, but apart from that I think the
sense of the<br>
> > documentation is unchanged. So if
V4L2_CID_CAMERA_SENSOR_ROTATION says<br>
> > 90 degrees, then your image in memory looks like
it has had a 90<br>
> > degree *clockwise* rotation applied.<br>
> ><br>
><br>
> Yes!<br>
><br>
> > > ><br>
> > > > the libcamera one at<br>
> > > ><br>
> > > > <a
href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/transform.cpp#n54"
rel="noreferrer" target="_blank" moz-do-not-send="true"
class="moz-txt-link-freetext">https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/transform.cpp#n54</a><br>
> > > ><br>
> > > > > From that, and the nice example
with the stick person, I deduce that<br>
> > > > > if your camera's
V4L2_CID_CAMERA_SENSOR_ROTATION property says 90<br>
> > > > > degrees, then the image you capture
(if nothing else happens to it)<br>
> > > > > will look like it has had a 90
degree clockwise rotation performed.<br>
> > > > > Everyone agree with that?<br>
> > > > Yep, agreed. That would mean
V4L2_CID_CAMERA_SENSOR_ROTATION==90 would<br>
> > > > translate to Transform::Rot90.<br>
> > ><br>
> > > Let's here put some definitions in place<br>
> > ><br>
> > > V4L2_CID_CAMERA_SENSOR_ROTATION<br>
> > > This read-only control describes the rotation
correction in degrees in<br>
> > > the counter-clockwise direction to be applied
to the captured images<br>
> > > once captured to memory to compensate for the
camera sensor mounting<br>
> > > rotation.<br>
> > ><br>
> > > \var Transform::Rot270<br>
> > > Rotation by 270 degrees clockwise (90
degrees anticlockwise).<br>
> > ><br>
> > > Let's start by saying we did a sub-optimal
(to be nice) job in<br>
> > > having Transform and V4L2_CID_ROTATION
operate in two different<br>
> > > directions. Before the usage of Transform in
libcamera gets widely<br>
> > > adopted sould we align the two ?<br>
> ><br>
> > I don't mind at all... we just have to decide what
we want!<br>
> ><br>
> > ><br>
> > > Hence if I'm not mistaken, if your camera
has Rotation=90 and an<br>
> > > application supplies Transform::Identity in<br>
> > > CameraConfiguration::transform what you
should get back is Rot270<br>
> > ><br>
> > > Correct ?<br>
> ><br>
> > The way things are currently, if the camera says
Rotation=90, then<br>
> > your image in memory looks like it is rotated 90
degrees clockwise, so<br>
> > the Transform (if your pipeline and sensor can't
apply any<br>
> > flips/rotations at all) will come back as Rot90.<br>
> ><br>
> > That is, the only transform you can specify that
won't be adjusted, is<br>
> > a 90 degree clockwise rotation, so transform=Rot90
as things stand<br>
> > currently.<br>
> ><br>
> > If we choose to change the sense of the
libcamera.Transform rotation,<br>
> > then we'd get transform=Rot270 instead.<br>
> ><br>
> > (If your pipeline handler can do flips and
transposes, it may be able<br>
> > to do what you ask and so the transform would come
back unchanged and<br>
> > would be "Valid".)<br>
> ><br>
> > ><br>
> > > > > So in turn, if you have a
camera/ISP that can apply no transforms at<br>
> > > > > all, then the only value for the
user transform that will not come<br>
> > > > > back as adjusted is "rot90",
because it looks like it's had a 90<br>
> > > > > degree clockwise rotation performed
(and libcamera transforms count<br>
> > > > > rotations as being clockwise). Does
that also make sense?<br>
> > ><br>
> > > It might be the case today, but I wonder if
that's desirable.<br>
> > ><br>
> > > If an application supplies
CameraConfiguration::transform = Identity<br>
> > > it means it doesn't want any rotation applied
by the library. It will<br>
> > > get back 270 to indicate that that's how the
image is rotated and will<br>
> > > get an Adjusted configuration.<br>
> ><br>
> > I'm not quite sure I get that, perhaps it depends
how we interpret<br>
> > "applied by the library". I guess I agree if we
mean "applied by the<br>
> > library together with any rotation of the sensor"?
Maybe a real<br>
><br>
> "Applied by the library" == happens inside libcamera,
ISP or sensor<br>
> flips..<br>
><br>
> > example is helpful:<br>
> ><br>
> > So for a Raspberry Pi, for example, our sensors
are mostly mounted<br>
> > upside down (V4L2_CID_CAMERA_SENSOR_ROTATION says
"180"). Our users<br>
> > say the transform they want is transform=Identity
i.e. images come<br>
> > back "the right way up". Internally it means that
we do apply an extra<br>
> > h+v flip to undo the 180 degree rotation of the
sensor.<br>
> ><br>
><br>
> Here you go, this is where we disconnect.<br>
><br>
> As I've interpreted the API so far, and the way we
designed<br>
> properties::Rotation was to signal to the application
what is the<br>
> status of the image as it comes from the sensor.
Application could then<br>
> correct that by setting CameraConfiguration::transform,
but in most<br>
> cases the image will be corrected at rendering time by
the application<br>
> itself.<br>
><br>
> IWO I was not expecting auto-correction and
Transform::Identity to me<br>
> means "give me the images as they come from the sensor"<br>
><br>
> > ><br>
> > > If instead an application supplies
CameraConfiguration::transform = Rot270<br>
> > > and your camera -cannot- do any rotation, it
will still receive back<br>
> > > Rot270 and the state is Valid.<br>
> ><br>
> > I think that's true if we change the sense of the
libcamera.Transform<br>
> > rotation. Currently, as I said earlier, I think we
get Rot90?<br>
> ><br>
> > ><br>
> > > If it asks for CameraConfiguration::transform
= Rot270 and the camera<br>
> > > -can- do that, it will clear
CameraConfiguration::transform to<br>
> > > Identity and return Adjusted ?<br>
> ><br>
> > I'm not sure there... The
CameraConfiguration::transform is the final<br>
> > transform you want. If that can be done, it's left
alone and you get<br>
> > back Valid. It's not telling you what else you
would need to do<br>
> > afterwards.<br>
> ><br>
> > (To find out what you need to do afterwards you'd
work out<br>
> > "inverse(what-you-will-get) * what-you-wanted", or
something like<br>
> > that.)<br>
> ><br>
><br>
> What I would like is to make as easier as possible for
applications<br>
> to know what they have to do to have the image rotated
in the way they<br>
> expect. There are several "transform" steps on the
image, the first<br>
> one happens in libcamera, all the other ones are left
to the<br>
> appplication.<br>
<br>
I think this is exactly what I want, namely "to make it as
easy as<br>
possible for applications".<br>
<br>
On Raspberry Pi - and this has been the case for a couple of
years<br>
now! - if a user wants images the "normal" way up then they
simply<br>
pass Transform::Identity. If they want upside down images
they pass<br>
Transform::Rot180. It's as easy as that, there is simply
nothing else<br>
to do.<br>
<br>
Requiring all applications to look at the sensor rotation
and then<br>
combine various things to figure out what they have to ask
for (and<br>
possibly get it wrong) seems, well, actually quite
unhelpful.<br>
</blockquote>
<div><br>
</div>
<div>FWIW, I do agree that applications should not have to
trouble themselves</div>
<div>with munging sensor rotation andrequested rotation to
achieve their</div>
<div>desired outcome. This definitely wants to be hidden in
the libcamera core</div>
<div>or pipeline handler layers.</div>
<div><br>
</div>
<div>Naush</div>
<div><br>
</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<br>
But this is indeed what we need to talk about. Let's
organise a call!<br>
<br>
Thanks<br>
David<br>
<br>
><br>
> *) The image "native" transform is reported by Rotation
(this assumes<br>
> that any implicit rotation buried in the sensor driver,
as per our<br>
> last discussion with Dave is not taken into account)<br>
><br>
> properties::Rotation = Rot270 = HFlip | Transpose<br>
><br>
> *) Application sets CameraConfiguration::transform and
call validate() to<br>
> check what can be done by libcamera and what should be
done by next<br>
> layers<br>
><br>
> CameraConfigurat::Transform = Rot270<br>
><br>
> *) The camera validate the transform and as it can only
flip it sets<br>
> transform to HFlip, and the state is Adjusted<br>
><br>
> validate() -> The camera can only flip<br>
> -> CameraConfiguration = HFlip<br>
><br>
> *) Application gets back what the camera can do and
instrument the<br>
> rest of the rendering pipeline to perform the
additional transforms<br>
> the camera cannot do. To get what you have to do I
presume you have to<br>
><br>
> additionalTransform = what_you_want &
!what_you_get<br>
><br>
> So yes, I was interpreting the usage of
CameraConfiguration::transform<br>
> as a negotiation of what the library can do and what
applications have<br>
> to do by themselves, which if I got you is different
than what's<br>
> currecntly implemented ?<br>
><br>
> > ><br>
> > > Is this what happens (I'm looking at the
above code with David's<br>
> > > suggestion to use the inverse of
roationTransform).<br>
> > ><br>
> > ><br>
> > > > ><br>
> > > > > So by a slightly arbitrary mixture
of conventions, it looks to me as<br>
> > > > > though the use of the inverse that
I queried is indeed wrong, because<br>
> > > > > we "need camera 90 degree rotation"
=> "user transform rot90" in this<br>
> > > > > case.<br>
> > > > ><br>
> > > > > Does that sound plausible... what
do folks think?<br>
> > > ><br>
> > > > Ah, I think the question is whether the
adjusted value means: "this is what<br>
> > > > you'll get" vs "this is what you have to
do yourself".<br>
> > > ><br>
> > > > Above I argued in favor of the later,
but I guess the first one is what the<br>
> > > > API is supposed to mean. That would
imply that the client has compute the<br>
> > > > difference between requested and
adjusted value itself.<br>
> > ><br>
> > > I always presumed the latter was meant to
happen..<br>
> ><br>
> > Yes, I think this is the crux of the matter. I
believe it's "what you<br>
> > will get". It's not "what you need to do after".
(If it was "what you<br>
> > need to do after", it would change every time you
try to validate the<br>
> > configuration!)<br>
> ><br>
><br>
> On the "it would change every time you try to validate
the<br>
> configuration": only if the camera cannot satisfy the
transform you<br>
> have requested. If you ask for a flip and the camera
can do it,<br>
> transform stays un-touched and the state is Valid ?<br>
><br>
> > ><br>
> > > ><br>
> > > > In my case that would mean:<br>
> > > ><br>
> > > > requested
CameraConfiguration::transform: Transform::Identity,<br>
> > > > V4L2_CID_CAMERA_SENSOR_ROTATION: 270,
adjusted<br>
> > > > CameraConfiguration::transform:
Transform::Rot270<br>
> > > ><br>
> > ><br>
> > > I guess it's Rot90 because of the
clockwise/counter-clockwise issue ?<br>
> ><br>
> > Yes, looks right to me!<br>
> ><br>
> > ><br>
> > > > To compute what the client has to do, it
would need to do:<br>
> > > > Transform::Identity - Transform::Rot270
= Transform::Rot90. And then apply<br>
> > > > that, a 90 degr. rotation clockwise.<br>
> ><br>
> > Yes, I guess I wrote it above, it's
"inverse(what-you-got) *<br>
> > what-you-wanted", in this case inverse(rot270) *
identity which is<br>
> > rot90 (in the current clockwise sense).<br>
> ><br>
> > > ><br>
> > > > Does that sound reasonable? In that case
the patch here should indeed be<br>
> > > > `*transform = rotationTransform;`,
without the inverse.<br>
> > > ><br>
> ><br>
> > Yes, agree!<br>
> ><br>
> > ><br>
> > > /0\<br>
> > ><br>
> > > before we plumb this into and make
assumptions that will be then<br>
> > > set into stone in our adaption layers, can we
take a step back and<br>
> > > define what kind of API we would like to have
?<br>
> > ><br>
> > > Let's start by the definition of
CameraConfiguration::transform.<br>
> > ><br>
> > > It's a member of CameraConfiguration, and as
by definition it is<br>
> > > filled-in by the application and eventually
adjusted by the Camera to<br>
> > > a value that's acceptable/doable for the
current configuration.<br>
> > ><br>
> > > 1) Application to Camera =
(counter?)clockwise degrees the camera<br>
> > > system should rotate the image before
presenting it to the user<br>
> ><br>
> > This may depend on what exactly we mean by "the
camera system". I'd<br>
> > explain it as<br>
> > "the final resulting transform that the
application wants to be<br>
> > applied to all the output images". After all, this
is actually the<br>
> > only thing the application knows!<br>
> ><br>
><br>
> agreed<br>
><br>
> > ><br>
> > > 2) Camera to application = the rotation the
camera can actually do<br>
> > > (ie. only sensor flips == no Transpose)<br>
> ><br>
> > Agree, mostly, though I would say "transform"
rather than "rotation"<br>
><br>
> yes, more correct<br>
><br>
> > because flips aren't rotations, you could in
theory have a PH that<br>
> > could do transposes (even though we probably don't
currently).<br>
> ><br>
><br>
> none currently afaict<br>
><br>
> > ><br>
> > > The current documentation doesn't explain how
it gets adjusted<br>
> ><br>
> > This is true, though I also think it's platform
dependent. If you had<br>
> > a PH that can't do transposes but can do flips,
you have a choice how<br>
> > you handle (for example) rot90. You could say "oh
I can't do that, so<br>
> > in this case I won't do anything at all" and leave
the application to<br>
> > sort everything out. The Pi tries to say "well,
I'll do everything<br>
> > except for the transpose, so an application only
has to worry about<br>
> > that one case". But there is a choice...<br>
> ><br>
><br>
> I agree it's platform-specific, but the way transform
is negotiated with<br>
> application should be standardized<br>
><br>
> > ><br>
> > > /**<br>
> > > * \var CameraConfiguration::transform<br>
> > > * \brief User-specified transform to be
applied to the image<br>
> > > *<br>
> > > * The transform is a user-specified 2D plane
transform that will be applied<br>
> > > * to the camera images by the processing
pipeline before being handed to<br>
> > > * the application. This is subsequent to any
transform that is already<br>
> > > * required to fix up any platform-defined
rotation.<br>
> > > *<br>
> > > * The usual 2D plane transforms are allowed
here (horizontal/vertical<br>
> > > * flips, multiple of 90-degree rotations
etc.), but the validate() function<br>
> > > * may adjust this field at its discretion if
the selection is not supported.<br>
> > > */<br>
> > ><br>
> > > The last part in particular, "at its
discrection" it's really too<br>
> > > generic.<br>
> ><br>
> > I guess it's hinting at the fact that it's up to
the PH, and we don't<br>
> > mandate the exact way in which it can fail to do
what you asked for!<br>
> > But it would be fine to have a discussion on that.<br>
> ><br>
><br>
> That's the point that most concerns me :)<br>
><br>
> > ><br>
> > > Let's start from the beginning: what
application should use transform<br>
> > > for ? I presume they should:<br>
> > ><br>
> > > 1) Inspect propertis::Rotation<br>
> > > 2) Correct the image for the
properties::Rotation amount (actually,<br>
> > > the inverse because of the
clockwise/counter-clockwise thing)<br>
> > > 3) Add any additional rotation they would
like on top of this<br>
> ><br>
> > So I think the point of the transform is that you
*don't* have to<br>
> > worry about the properties::Rotation. It takes
care of that for you.<br>
> > You say "I want upside down images" and you get
them, whatever the<br>
> > sensor's rotation.<br>
><br>
> That's acceptable as well if we prefer that, but it has
to be<br>
> standardized among pipelines imho<br>
><br>
> ><br>
> > ><br>
> > > How should transform be adjusted ?<br>
> > ><br>
> > > Identity:<br>
> > > If application->identity the camera does
not have to apply any<br>
> > > transform. Should the transform be adjutes
like it happens today to<br>
> > > compensate the Rotation (at least that's what
I presume happens) ? I<br>
> > > presume no, applications can infer rotation
from properties and if they<br>
> > > chose Identity either other layers above will
do the transform or<br>
> > > they're fine with images as they are.<br>
> ><br>
> > If the application says it wants the "identity"
transform, then it<br>
> > should get output images with no transformation
applied. But this does<br>
> > mean it will undo the sensor rotation (this being
the normal case on<br>
> > the Pi).<br>
> ><br>
><br>
> right, it depends if we decide to account for the
sensor rotation<br>
> inside libcamera or not<br>
><br>
> > ><br>
> > > A supported/non-supported transform:<br>
> > > An application asks for a VFLIP, the camera
can do it, transform is<br>
> > > not changed. If the camera cannot flip, the
transform is set back to<br>
> > > Identity and the state is adjusted.<br>
> ><br>
> > Yes, mostly, except that the sensor may be mounted
with a rotation,<br>
> > but if it isn't, then I agree with this.<br>
> ><br>
> > ><br>
> > > A partially non-supported transform:<br>
> > > An application asks for Rot270 (Transpose |
HFLIP) and the camera can<br>
> > > only do HFLIP. transform sould be then set to
HFLIP and applications<br>
> > > know that the remaining transformation should
be handled elsewhere.<br>
> ><br>
> > As I suggested above, I'm not sure whether we want
to mandate this. I<br>
> > agree it seems useful behaviour, which is why the
Pi does it, but I'm<br>
> > open minded about requiring it or not.<br>
> ><br>
> > ><br>
> > > Are we missing anything with such behaviour ?
I presume it's rather<br>
> > > similar to what happens today if not for the
fact that the validate()<br>
> > > implementation adjusts transform to rotation
in case it cannot flip<br>
> > ><br>
> > > Transform combined = transform *
data_->rotationTransform_;<br>
> > > if (!data_->supportsFlips_
&& !!combined)<br>
> > > transform =
-data_->rotationTransform_;<br>
> > ><br>
> > > Not really sure I got why.<br>
> > ><br>
> > > All of this, but an even more fundamental
question: is it too late/is<br>
> > > it worh it to adjust the Transform definition
to adopt the same<br>
> > > direction as the kernel defined rotation ?<br>
> > ><br>
> > > Sorry for the long email :/<br>
> ><br>
> > No problem! I like nothing more than discussing
transforms, except<br>
> > possibly colour spaces!<br>
> ><br>
><br>
> :)<br>
><br>
> > More seriously, here are my key points:<br>
><br>
> I'll try to summarize what I see differently<br>
><br>
> ><br>
> > 1. The CameraConfiguration::transform field says
what the application<br>
> > wants the final output images to have.<br>
><br>
> 1. The CameraConfiguration::transform field says what
transform the<br>
> library has to apply to images as they arrive from the
sensor in its<br>
> default configuration<br>
><br>
> > 2. It takes account of the sensor rotation. The
most obvious example<br>
> > is the Pi: if the sensor is mounted upside down,
and the applications<br>
> > ask for images "the right way up" (i.e. transform
= identity), then<br>
> > the PH will apply h and v flips to compensate for
the sensor mounting.<br>
><br>
> 2. It doesn't automatically account for sensor
rotation. Application<br>
> might decide to do that along the rendering pipeline,
in example<br>
><br>
> > 3. If the PH can do what the application requests,
it leaves the<br>
> > transform alone and your configuration is Valid.<br>
><br>
> Agreed<br>
><br>
> > 4. If the PH can't do what the application wants,
it will set the<br>
> > value to something that it can do, and the
configuration will be<br>
> > Adjusted.<br>
><br>
> Agreed<br>
><br>
> ><br>
> > If folks wanted a short conference call to iron
any of this out, I'd<br>
> > be very happy to join in!<br>
><br>
> I've asked others to have a look here, let's see if we
need a call,<br>
> I'm all for it !<br>
><br>
> Thanks<br>
> j<br>
><br>
> ><br>
> > Thanks<br>
> > David<br>
> ><br>
> > ><br>
> > > > > Thanks everyone!<br>
> > > > > David<br>
> > > ><br>
> > > > Thanks!<br>
> > > ><br>
> > > > Robert<br>
> > > ><br>
</blockquote>
</div>
</div>
</blockquote>
</body>
</html>