<div dir="ltr"><div dir="ltr">Hi all,<div><br></div><div>Thank you for your reviews!</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 6 Dec 2021 at 17:49, 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">Hello,<br>
<br>
On Mon, Dec 06, 2021 at 05:19:56PM +0000, Kieran Bingham wrote:<br>
> Quoting Naushir Patuck (2021-12-03 11:32:05)<br>
> > Return the available sensor PixelFormats and sizes from generateConfiguration()<br>
> > if the StreamRole is set to StreamRole::Raw. The existing code returns the<br>
> > PixelFormats and sizes for all other StreamRole types.<br>
> > <br>
> > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > ---<br>
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 21 ++++++++++++++-----<br>
> >  1 file changed, 16 insertions(+), 5 deletions(-)<br>
> > <br>
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > index 5b76916e9e98..cbfb58562626 100644<br>
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > @@ -595,12 +595,23 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,<br>
> >                         return nullptr;<br>
> >                 }<br>
> >  <br>
> > -               /* Translate the V4L2PixelFormat to PixelFormat. */<br>
> >                 std::map<PixelFormat, std::vector<SizeRange>> deviceFormats;<br>
> > -               for (const auto &format : fmts) {<br>
> > -                       PixelFormat pf = format.first.toPixelFormat();<br>
> > -                       if (pf.isValid())<br>
> > -                               deviceFormats[pf] = format.second;<br>
> > +               if (role == StreamRole::Raw) {<br>
> > +                       /* Translate the MBUS codes to a PixelFormat. */<br>
> > +                       for (const auto &format : data->sensorFormats_) {<br>
> > +                               PixelFormat pf = mbusCodeToPixelFormat(format.first,<br>
> > +                                                                      BayerFormat::Packing::CSI2);<br>
> > +                               if (pf.isValid())<br>
> > +                                       deviceFormats.emplace(std::piecewise_construct, std::forward_as_tuple(pf),<br>
> > +                                               std::forward_as_tuple(format.second.begin(), format.second.end()));<br>
> <br>
> I can't tell right now if this could be simpler, but I think it's fine<br>
> as it is if it works for you and produces the right result.<br>
<br>
That's likely the shortest piece of code possible, even if not the most<br>
explicit one (declaring an explicit intermediate std::vector<SizeRange><br>
would be more readable I think).<br></blockquote><div><br></div><div>This is the only way I could think of constructing deviceFormat without an</div><div>intermediate temporary std::vector.  If others feel this construct is somewhat</div><div>obfuscated, I am happy to add a temporary for readability.  If not, then feel</div><div>free to merge this :-)</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>
> I think this functionality could or rather /should/ be done in the<br>
> CameraSensor class to return these formats to simplify this for each<br>
> pipeline handler, as they should all do this in the same way if they use<br>
> a CameraSensor.<br>
<br>
Except that how a media bus code is translated to a pixel format is<br>
specific to the video device, it's not an intrinsic property of the<br>
camera sensor.<br>
<br>
> However, I think that move to CameraSensor and adapting of the other<br>
> handlers can be done on top.<br>
> <br>
> So,<br>
> <br>
> Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
<br>
Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
<br>
> > +                       }<br>
> > +               } else {<br>
> > +                       /* Translate the V4L2PixelFormat to PixelFormat. */<br>
> > +                       for (const auto &format : fmts) {<br>
> > +                               PixelFormat pf = format.first.toPixelFormat();<br>
> > +                               if (pf.isValid())<br>
> > +                                       deviceFormats[pf] = format.second;<br>
> > +                       }<br>
> >                 }<br>
> >  <br>
> >                 /* Add the stream format based on the device node used for the use case. */<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>