<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 25 Oct 2021 at 17:59, 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">Hi Naush,<br>
<br>
Thank you for the patch.<br>
<br>
On Fri, Oct 22, 2021 at 04:30:45PM +0100, Naushir Patuck wrote:<br>
> On Fri, 22 Oct 2021 at 16:22, David Plowman wrote:<br>
> > On Fri, 22 Oct 2021 at 15:40, Naushir Patuck wrote:<br>
> > ><br>
> > > Default to using CSI2 packed formats when setting up the Unicam image format,<br>
> > > but use an unpacked format if the user requests one through StreamConfiguration.<br>
> ><br>
> > Yes, I think this is the right way to do it!<br>
> ><br>
> > ><br>
> > > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > > ---<br>
> > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++++--<br>
> > >  1 file changed, 10 insertions(+), 2 deletions(-)<br>
> > ><br>
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > index a31b0f81eba7..45725527d66e 100644<br>
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > @@ -61,11 +61,14 @@ SensorFormats populateSensorFormats(std::unique_ptr<CameraSensor> &sensor)<br>
> > >         return formats;<br>
> > >  }<br>
> > ><br>
> > > -inline V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &mode)<br>
> > > +inline V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &mode, bool unpacked = false)<br>
<br>
By the way, I'd drop the inline keyword and let the compiler decide.<br>
<br>
> > I'm not totally sure I like the default value here - I kind of feel<br>
> > people should be forced to think about it. But that's a teeny thing so<br>
> > I wouldn't worry.<br>
> <br>
> I can remove the default value in the next version, and switch the argument<br>
> to be "packed".<br>
<br>
How about also replacing the bool argument with a BayerFormat::Packing ?<br></blockquote><div><br></div><div>That seems reasonable, and would really force the caller to make a decision.</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>
> > And also ignoring the very minor brain contortion involved in passing<br>
> > *un*-packed which gets flipped to set the *packed* field!<br>
> ><br>
> > Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> ><br>
> > >  {<br>
> > >         V4L2DeviceFormat deviceFormat;<br>
> > >         BayerFormat bayer = BayerFormat::fromMbusCode(mode.mbus_code);<br>
> > ><br>
> > > +       bayer.packing = unpacked ? BayerFormat::Packing::None<br>
> > > +                                : BayerFormat::Packing::CSI2Packed;<br>
> > > +<br>
> > >         deviceFormat.fourcc = bayer.toV4L2PixelFormat();<br>
> > >         deviceFormat.size = mode.size;<br>
> > >         return deviceFormat;<br>
> > > @@ -598,6 +601,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<br>
> > >         Size maxSize, sensorSize;<br>
> > >         unsigned int maxIndex = 0;<br>
> > >         bool rawStream = false;<br>
> > > +       bool unpacked = false;<br>
> > ><br>
> > >         /*<br>
> > >          * Look for the RAW stream (if given) size as well as the largest<br>
> > > @@ -613,6 +617,10 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<br>
> > >                          */<br>
> > >                         sensorSize = cfg.size;<br>
> > >                         rawStream = true;<br>
> > > +                       /* Check if the user has explicitly set an unpacked format. */<br>
> > > +                       V4L2PixelFormat fourcc = V4L2PixelFormat::fromPixelFormat(cfg.pixelFormat);<br>
> > > +                       BayerFormat bayer = BayerFormat::fromV4L2PixelFormat(fourcc);<br>
<br>
This should go to a BayerFormat::fromPixelFormat() function, given that<br>
you'd added BayerFormat::toPixelFormat() in 1/6.<br></blockquote><div><br></div><div>Will do!</div><div><br></div><div>Thanks,</div><div>Naush</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>
> > > +                       unpacked = bayer.packing == BayerFormat::Packing::None;<br>
> > >                 } else {<br>
> > >                         if (cfg.size > maxSize) {<br>
> > >                                 maxSize = config->at(i).size;<br>
> > > @@ -623,7 +631,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<br>
> > ><br>
> > >         /* First calculate the best sensor mode we can use based on the user request. */<br>
> > >         V4L2SubdeviceFormat sensorFormat = findBestMode(data->sensorFormats_, rawStream ? sensorSize : maxSize);<br>
> > > -       V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat);<br>
> > > +       V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, unpacked);<br>
> > ><br>
> > >         ret = data->sensor_->setFormat(&sensorFormat);<br>
> > >         if (ret)<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>