[libcamera-devel] [PATCH v2 3/6] pipeline: raspberrypi: Set packing formats for the Unicam image node

[Naushir Patuck]

Hi Laurent,

Thank you for your feedback.

On Mon, 25 Oct 2021 at 17:59, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> On Fri, Oct 22, 2021 at 04:30:45PM +0100, Naushir Patuck wrote:
> > On Fri, 22 Oct 2021 at 16:22, David Plowman wrote:
> > > On Fri, 22 Oct 2021 at 15:40, Naushir Patuck wrote:
> > > >
> > > > Default to using CSI2 packed formats when setting up the Unicam
> image format,
> > > > but use an unpacked format if the user requests one through
> StreamConfiguration.
> > >
> > > Yes, I think this is the right way to do it!
> > >
> > > >
> > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > ---
> > > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++++--
> > > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > index a31b0f81eba7..45725527d66e 100644
> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > @@ -61,11 +61,14 @@ SensorFormats
> populateSensorFormats(std::unique_ptr<CameraSensor> &sensor)
> > > >         return formats;
> > > >  }
> > > >
> > > > -inline V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat
> &mode)
> > > > +inline V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat
> &mode, bool unpacked = false)
>
> By the way, I'd drop the inline keyword and let the compiler decide.
>
> > > I'm not totally sure I like the default value here - I kind of feel
> > > people should be forced to think about it. But that's a teeny thing so
> > > I wouldn't worry.
> >
> > I can remove the default value in the next version, and switch the
> argument
> > to be "packed".
>
> How about also replacing the bool argument with a BayerFormat::Packing ?
>

That seems reasonable, and would really force the caller to make a decision.


>
> > > And also ignoring the very minor brain contortion involved in passing
> > > *un*-packed which gets flipped to set the *packed* field!
> > >
> > > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > >
> > > >  {
> > > >         V4L2DeviceFormat deviceFormat;
> > > >         BayerFormat bayer =
> BayerFormat::fromMbusCode(mode.mbus_code);
> > > >
> > > > +       bayer.packing = unpacked ? BayerFormat::Packing::None
> > > > +                                : BayerFormat::Packing::CSI2Packed;
> > > > +
> > > >         deviceFormat.fourcc = bayer.toV4L2PixelFormat();
> > > >         deviceFormat.size = mode.size;
> > > >         return deviceFormat;
> > > > @@ -598,6 +601,7 @@ int PipelineHandlerRPi::configure(Camera
> *camera, CameraConfiguration *config)
> > > >         Size maxSize, sensorSize;
> > > >         unsigned int maxIndex = 0;
> > > >         bool rawStream = false;
> > > > +       bool unpacked = false;
> > > >
> > > >         /*
> > > >          * Look for the RAW stream (if given) size as well as the
> largest
> > > > @@ -613,6 +617,10 @@ int PipelineHandlerRPi::configure(Camera
> *camera, CameraConfiguration *config)
> > > >                          */
> > > >                         sensorSize = cfg.size;
> > > >                         rawStream = true;
> > > > +                       /* Check if the user has explicitly set an
> unpacked format. */
> > > > +                       V4L2PixelFormat fourcc =
> V4L2PixelFormat::fromPixelFormat(cfg.pixelFormat);
> > > > +                       BayerFormat bayer =
> BayerFormat::fromV4L2PixelFormat(fourcc);
>
> This should go to a BayerFormat::fromPixelFormat() function, given that
> you'd added BayerFormat::toPixelFormat() in 1/6.
>

Will do!

Thanks,
Naush


>
> > > > +                       unpacked = bayer.packing ==
> BayerFormat::Packing::None;
> > > >                 } else {
> > > >                         if (cfg.size > maxSize) {
> > > >                                 maxSize = config->at(i).size;
> > > > @@ -623,7 +631,7 @@ int PipelineHandlerRPi::configure(Camera
> *camera, CameraConfiguration *config)
> > > >
> > > >         /* First calculate the best sensor mode we can use based on
> the user request. */
> > > >         V4L2SubdeviceFormat sensorFormat =
> findBestMode(data->sensorFormats_, rawStream ? sensorSize : maxSize);
> > > > -       V4L2DeviceFormat unicamFormat =
> toV4L2DeviceFormat(sensorFormat);
> > > > +       V4L2DeviceFormat unicamFormat =
> toV4L2DeviceFormat(sensorFormat, unpacked);
> > > >
> > > >         ret = data->sensor_->setFormat(&sensorFormat);
> > > >         if (ret)
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20211026/b774ca49/attachment-0001.htm>