<div dir="ltr"><div dir="ltr">Hi,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 27 Oct 2021 at 12:48, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@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">On Wed, Oct 27, 2021 at 11:48:20AM +0100, Kieran Bingham wrote:<br>
> Quoting Naushir Patuck (2021-10-27 10:27:58)<br>
> > Add BayerFormat::toPixelFormat() and BayerFormat::fromPixelFormat() helper<br>
> > functions to convert between BayerFormat and PixelFormat types.<br>
> > <br>
> > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> > ---<br>
> >  include/libcamera/internal/bayer_format.h |  4 ++++<br>
> >  src/libcamera/bayer_format.cpp            | 29 +++++++++++++++++++++++<br>
> >  2 files changed, 33 insertions(+)<br>
> > <br>
> > diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h<br>
> > index 723382d4168d..ee96b9a86707 100644<br>
> > --- a/include/libcamera/internal/bayer_format.h<br>
> > +++ b/include/libcamera/internal/bayer_format.h<br>
> > @@ -10,6 +10,8 @@<br>
> >  #include <stdint.h><br>
> >  #include <string><br>
> >  <br>
> > +#include <libcamera/pixel_format.h><br>
> > +<br>
> >  #include "libcamera/internal/v4l2_pixelformat.h"<br>
> >  <br>
> >  namespace libcamera {<br>
> > @@ -50,6 +52,8 @@ public:<br>
> >  <br>
> >         V4L2PixelFormat toV4L2PixelFormat() const;<br>
> >         static BayerFormat fromV4L2PixelFormat(V4L2PixelFormat v4l2Format);<br>
> > +       PixelFormat toPixelFormat() const;<br>
> > +       static BayerFormat fromPixelFormat(PixelFormat format);<br>
> >         BayerFormat transform(Transform t) const;<br>
> >  <br>
> >         Order order;<br>
> > diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp<br>
> > index 94e2294d7f6c..1edd238829ce 100644<br>
> > --- a/src/libcamera/bayer_format.cpp<br>
> > +++ b/src/libcamera/bayer_format.cpp<br>
> > @@ -305,6 +305,35 @@ BayerFormat BayerFormat::fromV4L2PixelFormat(V4L2PixelFormat v4l2Format)<br>
> >         return BayerFormat();<br>
> >  }<br>
> >  <br>
> > +/**<br>
> > + * \brief Convert a BayerFormat into the corresponding PixelFormat<br>
> > + * \return The PixelFormat corresponding to this BayerFormat<br>
> > + */<br>
> > +PixelFormat BayerFormat::toPixelFormat() const<br>
> > +{<br>
> > +       const auto it = bayerToFormat.find(*this);<br>
> > +       if (it != bayerToFormat.end())<br>
> > +               return it->second.pixelFormat;<br>
> > +<br>
> > +       return PixelFormat();<br>
> > +}<br>
> > +<br>
> > +/**<br>
> > + * \brief Convert a PixelFormat into the corresponding BayerFormat<br>
> > + * \return The BayerFormat corresponding to this PixelFormat<br>
> > + */<br>
> > +BayerFormat BayerFormat::fromPixelFormat(PixelFormat format)<br>
> > +{<br>
> > +       auto it = std::find_if(bayerToFormat.begin(), bayerToFormat.end(),<br>
> > +                              [format](const auto &i) {<br>
> > +                                      return i.second.pixelFormat == format;<br>
> > +                              });<br>
> > +       if (it == bayerToFormat.end())<br>
> > +               return BayerFormat{};<br>
> > +<br>
> > +       return it->first;<br>
> <br>
> I'm curious why you have used two styles in the two functions.<br>
> <br>
> BayerFormat::toPixelFormat is <br>
>   if (it != end)<br>
>      return it;<br>
>   return Empty;<br>
> <br>
> while BayerFormat::fromPixelFormat is<br>
>   if (it == end)<br>
>      return Empty;<br>
> <br>
>   return it;<br>
> <br>
> If there's no technical reason for this, I think keeping them both using<br>
> the same pattern would be more consistent, but ... they are both<br>
> functional, so I don't see this as a blocker. (Personally I'd use the<br>
> BayerFormat::toPixelFormat style on both)<br>
<br>
Funny how personal preferences differ, I would have used the latter<br>
(a.k.a. return on error) style :-)<br></blockquote><div><br></div><div>I'll use the  BayerFormat::toPixelFormat style this time round as that matches</div><div>what the other functions in this file do.</div><div><br></div><div>Regards,</div><div>Naush</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>
Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
<br>
> So either way:<br>
> <br>
> Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
> <br>
> > +}<br>
> > +<br>
> >  /**<br>
> >   * \brief Apply a transform to this BayerFormat<br>
> >   * \param[in] t The transform to apply<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>