<div dir="ltr"><div dir="ltr">Hi Jacopo and Kieran,</div><div dir="ltr"><br></div><div>Uploaded v5. Please take another look.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Sep 23, 2024 at 5:20 PM Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com">jacopo.mondi@ideasonboard.com</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 Kieran<br>
<br>
On Mon, Sep 23, 2024 at 09:48:07AM GMT, Kieran Bingham wrote:<br>
> Quoting Jacopo Mondi (2024-09-23 09:32:01)<br>
> > Hi Harvey<br>
> ><br>
> > On Thu, Sep 19, 2024 at 10:15:14PM GMT, Cheng-Hao Yang wrote:<br>
> > > Hi Jacopo,<br>
> > ><br>
> > > On Thu, Sep 19, 2024 at 7:27 PM Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@ideasonboard.com</a>><br>
> > > wrote:<br>
> > ><br>
> > > > Hi Harvey<br>
> > > ><br>
> > > > On Mon, Sep 16, 2024 at 09:24:23AM GMT, Jacopo Mondi wrote:<br>
> > > > > Hello<br>
> > > > ><br>
> > > > > On Mon, Sep 16, 2024 at 12:34:54PM GMT, Cheng-Hao Yang wrote:<br>
> > > > > > Hi Barnabás and Jacopo,<br>
> > > > > ><br>
> > > > > > On Mon, Sep 16, 2024 at 1:27 AM Barnabás Pőcze <<a href="mailto:pobrn@protonmail.com" target="_blank">pobrn@protonmail.com</a>><br>
> > > > wrote:<br>
> > > > > ><br>
> > > > > > > Hi<br>
> > > > > > ><br>
> > > > > > ><br>
> > > > > > > 2024. szeptember 5., csütörtök 10:35 keltezéssel, Jacopo Mondi <<br>
> > > > > > > <a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@ideasonboard.com</a>> írta:<br>
> > > > > > ><br>
> > > > > > > > Hi Barnabás<br>
> > > > > > > ><br>
> > > > > > > > On Wed, Sep 04, 2024 at 11:26:37AM GMT, Barnabás Pőcze wrote:<br>
> > > > > > > > > Hi<br>
> > > > > > > > ><br>
> > > > > > > > ><br>
> > > > > > > > > 2024. szeptember 3., kedd 13:39 keltezéssel, Harvey Yang <<br>
> > > > > > > <a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>> írta:<br>
> > > > > > > > ><br>
> > > > > > > > > > From: Yudhistira Erlandinata <<a href="mailto:yerlandinata@chromium.org" target="_blank">yerlandinata@chromium.org</a>><br>
> > > > > > > > > ><br>
> > > > > > > > > > Add a Rectangle constructor that accepts two points:<br>
> > > > > > > > > > topLeft and bottomRight.<br>
> > > > > > > > > ><br>
> > > > > > > > > > Signed-off-by: Yudhistira Erlandinata <<br>
> > > > <a href="mailto:yerlandinata@chromium.org" target="_blank">yerlandinata@chromium.org</a>><br>
> > > > > > > > > > Co-developed-by: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> > > > > > > > > > Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@ideasonboard.com</a>><br>
> > > > > > > > > > ---<br>
> > > > > > > > > > include/libcamera/geometry.h | 2 ++<br>
> > > > > > > > > > src/libcamera/geometry.cpp | 14 ++++++++++++++<br>
> > > > > > > > > > 2 files changed, 16 insertions(+)<br>
> > > > > > > > > ><br>
> > > > > > > > > > diff --git a/include/libcamera/geometry.h<br>
> > > > > > > b/include/libcamera/geometry.h<br>
> > > > > > > > > > index 3e6f0f5d7..dc56f180f 100644<br>
> > > > > > > > > > --- a/include/libcamera/geometry.h<br>
> > > > > > > > > > +++ b/include/libcamera/geometry.h<br>
> > > > > > > > > > @@ -262,6 +262,8 @@ public:<br>
> > > > > > > > > > {<br>
> > > > > > > > > > }<br>
> > > > > > > > > ><br>
> > > > > > > > > > + constexpr Rectangle(const Point &topLeft, const Point<br>
> > > > > > > &bottomRight);<br>
> > > > > > > > ><br>
> > > > > > > > > Don't make this `constexpr` because it is not useful since the<br>
> > > > > > > definition is not available.<br>
> > > > > > > ><br>
> > > > > > > > I found references online that constexpr constuctors are implcitly<br>
> > > > > > > > inline, is this the reason of your comment ?<br>
> > > > > > ><br>
> > > > > > > Yes.<br>
> > > > > > ><br>
> > > > > > ><br>
> > > > > > > ><br>
> > > > > > > > However, I can't find it clearly specified in cppreference. Do you<br>
> > > > > > > > have any pointer ?<br>
> > > > > > ><br>
> > > > > > > "A constexpr specifier used in a function or static data<br>
> > > > member(since<br>
> > > > > > > C++17) declaration implies inline."<br>
> > > > > > > -- <a href="https://en.cppreference.com/w/cpp/language/constexpr" rel="noreferrer" target="_blank">https://en.cppreference.com/w/cpp/language/constexpr</a><br>
> > > > > > ><br>
> > > > ><br>
> > > > ><br>
> > > > > One day I'll lear to read properly<br>
> > > > ><br>
> > > > > > > ><br>
> > > > > > > > Anyway, if inline is the reason, isn't it better to inline the<br>
> > > > > > > > definition and maintain the constexpr specifier ?<br>
> > > > > > > > [...]<br>
> > > > > > ><br>
> > > > > > > That is an option as well.<br>
> > > > > > ><br>
> > > > > ><br>
> > > > > > IIUC, you're suggesting to put the definition of the new c'tor<br>
> > > > > > back in the header file? As we use `ASSERT` in this c'tor<br>
> > > > > > definition, there will be a compile error if we do that:<br>
> > > > > > ```<br>
> > > > > ><br>
> > > > > > In file included from ../include/libcamera/base/log.h:12,<br>
> > > > > ><br>
> > > > > > from ../include/libcamera/geometry.h:16,<br>
> > > > > ><br>
> > > > > > from ../include/libcamera/controls.h:21,<br>
> > > > > ><br>
> > > > > > from ../include/libcamera/camera.h:22,<br>
> > > > > ><br>
> > > > > > from ../src/apps/common/dng_writer.h:13,<br>
> > > > > ><br>
> > > > > > from ../src/apps/common/dng_writer.cpp:8:<br>
> > > > > ><br>
> > > > > > ../include/libcamera/base/private.h:21:2: error: #error "Private<br>
> > > > headers<br>
> > > > > > must not be included in the libcamera API"<br>
> > > > > ><br>
> > > > > > 21 | #error "Private headers must not be included in the libcamera<br>
> > > > API"<br>
> > > > > > ```<br>
> > > > ><br>
> > > > > I see. log.h is private, and the fact ASSERT is not exposed in the<br>
> > > > > public API makes me think aborting execution on an invalid user input<br>
> > > > > is probably not the best idea ? That's maybe the reason why ASSERT is not<br>
> > > > > exposed to the public API ?<br>
> > > > ><br>
> > > ><br>
> > > > Assuming the top-left corner is always defined as the point with the<br>
> > > > smaller 'x' and the higher 'y' whatever the reference system the<br>
> > > > rectangle is placed in:<br>
> > > ><br>
> > > > In example:<br>
> > > ><br>
> > > > (0,0)<br>
> > > > --------------------------------><br>
> > > > |<br>
> > > > | -------------------<br>
> > > > | | |<br>
> > > > | | |<br>
> > > > | x------------------<br>
> > > > |<br>
> > > > V<br>
> > > ><br>
> > > > Or:<br>
> > > ><br>
> > > > ^<br>
> > > > |<br>
> > > > |<br>
> > > > | x------------------<br>
> > > > | | |<br>
> > > > | | |<br>
> > > > | -------------------<br>
> > > > |<br>
> > > > |<br>
> > > > --------------------------------><br>
> > > > (0,0)<br>
> > > ><br>
> > > > The assertion you have introduced in your proposed constructor<br>
> > > ><br>
> > > > constexpr Rectangle(const Point &topLeft, const Point &bottomRight)<br>
> > > > : x(topLeft.x), y(topLeft.y),<br>
> > > > width(bottomRight.x - x),<br>
> > > > height(bottomRight.y - y)<br>
> > > > {<br>
> > > > ASSERT(bottomRight.x >= x && bottomRight.y >= y);<br>
> > > > }<br>
> > > ><br>
> > > > is wrong as bottomRight.y shall always be smaller than topLeft.y<br>
> > > ><br>
> > > > Has this code been ever run ?<br>
> > > ><br>
> > ><br>
> > > Sorry for the confusion, but the `topLeft` point is actually somewhere<br>
> > > like this:<br>
> > ><br>
> > > (0,0)<br>
> > > --------------------------------><br>
> > > |<br>
> > > | x-----------------<br>
> > > | | |<br>
> > > | | |<br>
> > > | -------------------<br>
> > > |<br>
> > > V<br>
> > ><br>
> > > `top-left` is (0, 0) in Android metadata's description [1], and it seems<br>
> > > that<br>
> > > it's also in libcamera's description to indicate a point with smaller x and<br>
> > > y coordinates [2].<br>
> > ><br>
> > > [1]:<br>
> > > <a href="https://android.googlesource.com/platform/system/media/+/refs/heads/main/camera/docs/docs.html#33019" rel="noreferrer" target="_blank">https://android.googlesource.com/platform/system/media/+/refs/heads/main/camera/docs/docs.html#33019</a><br>
> > > [2]:<br>
> > > <a href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/geometry.cpp#n639" rel="noreferrer" target="_blank">https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/geometry.cpp#n639</a><br>
> ><br>
> > Is this because of<br>
> ><br>
> > /**<br>
> > * \fn Rectangle::Rectangle(const Size &size)<br>
> > * \brief Construct a Rectangle of \a size with its top left corner located<br>
> > * at (0,0)<br>
> ><br>
> > This clearly doesn't match this case<br>
> ><br>
> > ^<br>
> > |<br>
> > |<br>
> > | x------------------<br>
> > | | |<br>
> > | | |<br>
> > | -------------------<br>
> > |<br>
> > |<br>
> > --------------------------------><br>
> > (0,0)<br>
> ><br>
> > Maybe we have been a bit short-sighted and only assumed the blelow<br>
> > reference system had to be taken into account.<br>
> ><br>
> > (0,0)<br>
> > --------------------------------><br>
> > |<br>
> > | x-----------------<br>
> > | | |<br>
> > | | |<br>
> > | -------------------<br>
> > |<br>
> > V<br>
> ><br>
> > Personally I would remove this assumption in our doc and define the<br>
> > top-left corner as the point with smaller x and largest y<br>
> ><br>
> > Opinions from libcamera people ?<br>
><br>
> Aha, I would define top left as 0,0 :-)<br>
><br>
> I've just opened up GIMP which also uses top left origin.<br>
><br>
> I expect other image applications would also honour this - so worth<br>
> checking a few ...<br>
><br>
> For me - even though this is 'geometry' ... it's *image processing*<br>
> geometry.<br>
><br>
<br>
The Rectangle class documentation says already<br>
<br>
* The measure unit of the rectangle coordinates and size, as well as the<br>
* reference point from which the Rectangle::x and Rectangle::y displacements<br>
* refers to, are defined by the context were rectangle is used.<br>
<br>
Which seems to suggest that no assumptions on the reference system are<br>
made in the class, but however we also have one constructor that is<br>
documented as<br>
<br>
* \brief Construct a Rectangle of \a size with its top left corner located<br>
* at (0,0)<br>
<br>
Which, in the below example suggestes "top-left" is actually visually<br>
bottom-right.<br></blockquote><div><br></div><div>nit: bottom-left</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>
<br>
^<br>
|<br>
|<br>
| -------------------<br>
| | |<br>
| | |<br>
| X------------------<br>
|<br>
|<br>
--------------------------------><br>
(0,0)<br>
<br>
If it is instead assumed that "top-left" is is the visual top-left<br>
regardless of the coordinate system so we should clarify what<br>
directions do we increment x and y when give a constructor like<br>
Rectangle({x,y}, w, h) as in this case we grow both x and y </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
(0,0)<br>
--------------------------------><br>
|<br>
| x------------------<br>
| | |<br>
| | |<br>
| -------------------<br>
|<br>
V<br></blockquote><div><br></div><div>I think this constructor creates a Rectangle like this:<br><br></div><div> (0,0)<br> --------------------------------><br> | |<br> | |<br> | |<br> | |<br> | ------------------(Size.width, Size.height)<br> |<br> V<br></div><div> </div><div>Which is a rectangle surrounded by (0, 0) and </div><div>(Size.width, Size.height).<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
while in this we increase x and decrement y<br>
^<br>
|<br>
|<br>
| x------------------<br>
| | |<br>
| | |<br>
| -------------------<br>
|<br>
|<br>
--------------------------------><br>
(0,0)<br>
<br>
but it would be totally legit to construct the same rectangle as<br>
<br>
^ -------------------<br>
| | |<br>
| | |<br>
| x------------------<br>
|<br>
|<br>
|<br>
|<br>
|<br>
--------------------------------><br>
(0,0)<br>
<br>
To remove ambiguities I would rather redefine top-left as "origin"<br>
point defined as the point wiht the lower x and lower y of the<br>
rectangle and clarify that we always increase width and height<br>
from there when constructing Rectangle({x,y}, w, h)<br>
<br>
(0,0)<br>
--------------------------------><br>
|<br>
| x------------------<br>
| | |<br>
| | |<br>
| -------------------<br>
|<br>
V<br>
<br>
^<br>
|<br>
|<br>
| -------------------<br>
| | |<br>
| | |<br>
| x------------------<br>
|<br>
|<br>
--------------------------------><br>
(0,0)<br>
<br>
And now define the newly constructor proposed by Harvey as the<br>
two-point constructur I suggested.<br></blockquote><div><br></div><div>Updated it to be two diagonal points. I've updated the variable names</div><div>to align with the original documentation, which `topLeft` means the one</div><div>with the minimum x and y coordinates.</div><div>Let me know what's the better variable names instead though, if we </div><div>redefine `top-left` as the origin point `(0, 0)`.<br>(Or just adjust it for me before accepting the patches, if that's the last</div><div>issue in question).</div><div><br></div><div>Thanks!</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>
Hope I'm not overthinking this.<br>
<br>
> --<br>
> Kieran<br>
</blockquote></div><br clear="all"><div><br></div><span class="gmail_signature_prefix">-- </span><br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div>BR,</div>Harvey Yang</div></div></div>