<div dir="ltr"><div dir="ltr">Hi Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Sep 19, 2024 at 7:27 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 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>> 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 <<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 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 headers<br>
> > must not be included in the libcamera API"<br>
> ><br>
> >    21 | #error "Private headers must not be included in the libcamera 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></blockquote><div><br></div><div>Sorry for the confusion, but the `topLeft` point is actually somewhere</div><div>like this:</div><div><br></div><div>(0,0)<br>  --------------------------------><br>  |<br>  |      x-----------------<br>  |      |                 |<br>  |      |                 |<br>  |      -------------------<br>  |<br>  V<br></div><div><br></div><div>`top-left` is (0, 0) in Android metadata's description [1], and it seems that</div><div>it's also in libcamera's description to indicate a point with smaller x and</div><div>y coordinates [2].</div><div><br></div><div>[1]: <a href="https://android.googlesource.com/platform/system/media/+/refs/heads/main/camera/docs/docs.html#33019">https://android.googlesource.com/platform/system/media/+/refs/heads/main/camera/docs/docs.html#33019</a></div><div>[2]: <a href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/geometry.cpp#n639">https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/geometry.cpp#n639</a></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>
> I'll ask others what they think about this<br>
<br>
As they're smarter than me, they made me notice that you should rather<br>
define a constructor with Point1 and Point2 and construct a Rectangle<br>
that spans between these two points defined using the existing<br>
Rectangle constructor as:<br>
<br>
        constexpr Rectangle(const Point &point1, const Point &point2)<br>
                : Rectangle(std::min(point1.x, point2.x), std::max(point1.y, point2.y),<br>
                            std::max(point1.x, point2.x) - std::min(point1.x, point2.x),<br>
                            std::max(point1.y, point2.y) - std::min(point1.y, point2.y))<br>
        {<br>
        }<br></blockquote><div><br></div><div>We originally were trying to avoid adding this powerful c'tor that allows every</div><div>combination, as libcamera's Rectangle API seems to prefer indicating the</div><div>`topLeft` point in c'tors [3].</div><div><br></div><div>Are you sure that the span is what we prefer?</div><div><br></div><div>[3]: <a href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/geometry.cpp#n609">https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/geometry.cpp#n609</a></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>
Please make sure to add proper documentation for this new constructor.<br>
<br>
You can also add the following snippet to test/geometry.cpp<br>
<br>
                Point topLeft(3, 30);<br>
                Point bottomRight(30, 3);<br>
                Point topRight(30, 30);<br>
                Point bottomLeft(3, 3);<br>
                Rectangle rect1(topLeft, bottomRight);<br>
                Rectangle rect2(topRight, bottomLeft);<br>
                Rectangle rect3(bottomRight, topLeft);<br>
                Rectangle rect4(bottomLeft, topRight);<br>
<br>
                if (rect1 != rect2 || rect1 != rect3 || rect1 != rect4) {<br>
                        cout << "Point-from-point construction failed" << endl;<br>
                        return TestFail;<br>
                } </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Thanks<br>
  j<br>
<br>
> ><br>
> > BR,<br>
> > Harvey<br>
> ><br>
> ><br>
> > ><br>
> > > Regards,<br>
> > > Barnabás Pőcze<br>
> > ><br>
> > ><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>