<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 27 Oct 2021 at 15: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>
On Wed, Oct 27, 2021 at 03:32:13PM +0100, Naushir Patuck wrote:<br>
> On Wed, 27 Oct 2021 at 15:21, Laurent Pinchart wrote:<br>
> > On Wed, Oct 27, 2021 at 10:28:00AM +0100, Naushir Patuck wrote:<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>
> > > 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>
> > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++----<br>
> > > 1 file changed, 8 insertions(+), 4 deletions(-)<br>
> > ><br>
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > index 48f561d31a31..1b78b5e74a63 100644<br>
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > @@ -96,14 +96,14 @@ BayerFormat mbusCodeToBayerFormat(unsigned int mbusCode)<br>
> > > return BayerFormat{};<br>
> > > }<br>
> > ><br>
> > > -V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format)<br>
> > > +V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format, BayerFormat::Packing packing)<br>
> > > {<br>
> > > V4L2DeviceFormat deviceFormat;<br>
> > > BayerFormat bayer = mbusCodeToBayerFormat(format.mbus_code);<br>
> > ><br>
> > > ASSERT(bayer.isValid());<br>
> > ><br>
> > > - bayer.packing = BayerFormat::Packing::CSI2Packed;<br>
> > > + bayer.packing = packing;<br>
> > > deviceFormat.fourcc = bayer.toV4L2PixelFormat();<br>
> ><br>
> > Packing is handled explicitly, so going through BayerFormat here is fine<br>
> > I think. The other calls to mbusCodeToBayerFormat() in 5/9 however<br>
> > bother me, especially when calling .toPixelFormat() or<br>
> > .toV4L2PixelFormat() immediately after. It would be good to wrap those<br>
> > in helper functions that explicitly handle packing, to ensure that it<br>
> > will be considered everywhere.<br>
> <br>
> > Have you tested both packed and unpacked formats by the way ?<br>
> <br>
> Yes, I see your point. As per your comment in the previous change, I will look to<br>
> change the mbus -> Bayer format conversion to be mbus -> V4L2PixelFormat, and<br>
> deal with the packing separately in a helper.<br>
<br>
To be perfecly clear, I don't object on using BayerFormat if it can<br>
help, as long as we handle packing explicitly. If a direct conversion<br>
ends up being simpler then let's go for that, but there's no strict need<br>
to drop BayerFormat usage if BayerFormat results in a cleaner<br>
implementation.<br></blockquote><div><br></div><div>After having prototyped a change with using V4L2DeviceFormat for the mbus conversion,</div><div>I think it would be better to use BayerFormat in this case. BayerFormat makes things a bit</div><div>easier to deal with packing/unpacking formats as it is not a separate enum, but just a property</div><div>of the format. Given that we have no intention of supporting YUV sensor (this will come back to bite me :)</div><div>in our pipeline handler, using BayerFormat should be ok for now.</div><div><br></div><div>I will however, remove the table in the pipeline handler, and use the existing table in BayerFormat.</div><div>In fact, this is what I had for v1 and v2. I'll also fix things so the existing mbusCodeToBayerFormat()</div><div>will handle switching between packing/unpacking though a helper.</div><div><br></div><div>Thanks,</div><div>Naush</div><div><br></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>
> With the changes in this version of the series Packing/unpacking works as expected,<br>
> but you are correct in that the change in 5/9 to use .toPixelFormat() in generateConfiguration()<br>
> will advertise an unpacked format to the application, which is not efficient I will rework that<br>
> along with the mbus -> V4L2PixelFormat conversion.<br>
> <br>
> > > deviceFormat.size = format.size;<br>
> > > return deviceFormat;<br>
> > > @@ -413,7 +413,8 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()<br>
> > > * the user request.<br>
> > > */<br>
> > > V4L2SubdeviceFormat sensorFormat = findBestFormat(data_->sensorFormats_, cfg.size);<br>
> > > - V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat);<br>
> > > + V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat,<br>
> > > + BayerFormat::Packing::CSI2Packed);<br>
> > > int ret = data_->unicam_[Unicam::Image].dev()->tryFormat(&unicamFormat);<br>
> > > if (ret)<br>
> > > return Invalid;<br>
> > > @@ -631,6 +632,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<br>
> > > for (auto const stream : data->streams_)<br>
> > > stream->reset();<br>
> > ><br>
> > > + BayerFormat::Packing packing = BayerFormat::Packing::CSI2Packed;<br>
> > Could we default to packed formats when generating the configuration too ?<br>
> ><br>
> > > Size maxSize, sensorSize;<br>
> > > unsigned int maxIndex = 0;<br>
> > > bool rawStream = false;<br>
> > > @@ -649,6 +651,8 @@ 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>
> > > + packing = BayerFormat::fromPixelFormat(cfg.pixelFormat).packing;<br>
> > > } else {<br>
> > > if (cfg.size > maxSize) {<br>
> > > maxSize = config->at(i).size;<br>
> > > @@ -667,7 +671,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<br>
> > > * Unicam image output format. The ISP input format gets set at start,<br>
> > > * just in case we have swapped bayer orders due to flips.<br>
> > > */<br>
> > > - V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat);<br>
> > > + V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, packing);<br>
> > > ret = data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat);<br>
> > > if (ret)<br>
> > > return ret;<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>