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

[Laurent Pinchart]

Hi Naush,

On Wed, Oct 27, 2021 at 03:32:13PM +0100, Naushir Patuck wrote:
> On Wed, 27 Oct 2021 at 15:21, Laurent Pinchart wrote:
> > On Wed, Oct 27, 2021 at 10:28:00AM +0100, 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.
> > >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > > ---
> > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++----
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 48f561d31a31..1b78b5e74a63 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -96,14 +96,14 @@ BayerFormat mbusCodeToBayerFormat(unsigned int mbusCode)
> > >       return BayerFormat{};
> > >  }
> > >
> > > -V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format)
> > > +V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format, BayerFormat::Packing packing)
> > >  {
> > >       V4L2DeviceFormat deviceFormat;
> > >       BayerFormat bayer = mbusCodeToBayerFormat(format.mbus_code);
> > >
> > >       ASSERT(bayer.isValid());
> > >
> > > -     bayer.packing = BayerFormat::Packing::CSI2Packed;
> > > +     bayer.packing = packing;
> > >       deviceFormat.fourcc = bayer.toV4L2PixelFormat();
> >
> > Packing is handled explicitly, so going through BayerFormat here is fine
> > I think. The other calls to mbusCodeToBayerFormat() in 5/9 however
> > bother me, especially when calling .toPixelFormat() or
> > .toV4L2PixelFormat() immediately after. It would be good to wrap those
> > in helper functions that explicitly handle packing, to ensure that it
> > will be considered everywhere.
> 
> > Have you tested both packed and unpacked formats by the way ?
> 
> Yes, I see your point.  As per your comment in the previous change, I will look to
> change the mbus -> Bayer format conversion to be mbus -> V4L2PixelFormat, and
> deal with the packing separately in a helper.

To be perfecly clear, I don't object on using BayerFormat if it can
help, as long as we handle packing explicitly. If a direct conversion
ends up being simpler then let's go for that, but there's no strict need
to drop BayerFormat usage if BayerFormat results in a cleaner
implementation.

> With the changes in this version of the series Packing/unpacking works as expected,
> but you are correct in that the change in 5/9 to use .toPixelFormat() in generateConfiguration()
> will advertise an unpacked format to the application, which is not efficient  I will rework that
> along with the mbus -> V4L2PixelFormat conversion.
> 
> > >       deviceFormat.size = format.size;
> > >       return deviceFormat;
> > > @@ -413,7 +413,8 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> > >                        * the user request.
> > >                        */
> > >                       V4L2SubdeviceFormat sensorFormat = findBestFormat(data_->sensorFormats_, cfg.size);
> > > -                     V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat);
> > > +                     V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat,
> > > + BayerFormat::Packing::CSI2Packed);
> > >                       int ret = data_->unicam_[Unicam::Image].dev()->tryFormat(&unicamFormat);
> > >                       if (ret)
> > >                               return Invalid;
> > > @@ -631,6 +632,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> > >       for (auto const stream : data->streams_)
> > >               stream->reset();
> > >
> > > +     BayerFormat::Packing packing = BayerFormat::Packing::CSI2Packed;
> > Could we default to packed formats when generating the configuration too ?
> >
> > >       Size maxSize, sensorSize;
> > >       unsigned int maxIndex = 0;
> > >       bool rawStream = false;
> > > @@ -649,6 +651,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> > >                        */
> > >                       sensorSize = cfg.size;
> > >                       rawStream = true;
> > > +                     /* Check if the user has explicitly set an unpacked format. */
> > > +                     packing = BayerFormat::fromPixelFormat(cfg.pixelFormat).packing;
> > >               } else {
> > >                       if (cfg.size > maxSize) {
> > >                               maxSize = config->at(i).size;
> > > @@ -667,7 +671,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> > >        * Unicam image output format. The ISP input format gets set at start,
> > >        * just in case we have swapped bayer orders due to flips.
> > >        */
> > > -     V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat);
> > > +     V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, packing);
> > >       ret = data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat);
> > >       if (ret)
> > >               return ret;

-- 
Regards,

Laurent Pinchart