<html>
<head>
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Hi!</p>
<p>I'd love if we could clarify the following part to ensure the
Pipewire implementation is correct:<br>
</p>
<p>
<blockquote type="cite">
<pre>> +
> + /*
> + * The camera sensor cannot do Transpose. Adjust any combined result
> + * that includes a transpose by flipping the transpose bit to notify
> + * applications they either get the transform they requested, or have
> + * to do a simple transpose themselves (they don't have to worry about
> + * the other possible cases).
> + */
> + if (!!(combined & Transform::Transpose)) {
> + /*
> + * Flipping the transpose bit in "transform" flips it in the
> + * combined result too (as it's the last thing that happens),
> + * which is of course clearing it.
> + */
> + *transform ^= Transform::Transpose;
> + combined &= ~Transform::Transpose;
> + }
> +
> + /*
> + * If the sensor can do no transforms, then combined must be changed to
> + * the identity and the sensor rotation must be cleared from the
> + * requested "transform".
Don't really follow this comment. Maybe
* If the sensor cannot do transforms, then combined must be changed to
* the identity and the only transform the user can have is the sensor
* rotation itself.
(Which rather suggests the code following has got the transform the
wrong way round...)
> + */
> + if (!supportFlips_ && !!combined) {
> + *transform = -rotationTransform;
This might really be "rotationTransform" (without the inverse).
Depending on which we round we view the sensor rotation (from in front
of or behind the camera).
Also, as we remarked elsewhere, we should possibly consider the
existence of H and V flips separately.
But apart from my existential doubts about inverse rotations, it all
seems reasonable to me, though I sense we'll be coming back to this
again anyway:</pre>
</blockquote>
</p>
<p>As example I'd like to take the back-camera of the Pinephone Pro
(imx258). Pipewire will ask for an identity transform and the
kernel will most likely report `V4L2_CID_CAMERA_SENSOR_ROTATION ==
270` (once driver patches and device tree updates land), so the
initial values we'll get are `properties::Rotation == 270` ->
`combined == Transform::Rot270`.</p>
<p>Right now neither transpose nor flips are supported, so the steps
in the current revision are:</p>
<ol>
<li>`combined == rot270` / `transform == identity`</li>
<li>`combined == hflip` / `transform == transpose`</li>
<li>`combined == `identity` / `transform == rot90`</li>
</ol>
<p>The value from `transform` will then be used for
`CameraConfiguration::transform`, signalling to the client that it
will have to compensate accordingly. Following the comment to not
use the inverse it would be `rot270` instead.</p>
<p>From an API-point of view, I'd say `rot90` is the more intuitive
value for clients, because they can then just follow the
documentation in camera_sensor.cpp and apply a transformation
accordingly. Would you agree that this should be the desired
outcome? I.e. should we stick to the proposed value?<br>
</p>
<p>Best regards,</p>
<p>Robert<br>
</p>
</body>
</html>