[libcamera-devel] [PATCH v3 6/9] pipeline: raspberrypi: Set packing formats for the Unicam image node
Naushir Patuck
naush at raspberrypi.com
Wed Oct 27 17:04:57 CEST 2021
Hi Laurent,
On Wed, 27 Oct 2021 at 15:59, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> 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.
>
After having prototyped a change with using V4L2DeviceFormat for the mbus
conversion,
I think it would be better to use BayerFormat in this case. BayerFormat
makes things a bit
easier to deal with packing/unpacking formats as it is not a separate enum,
but just a property
of the format. Given that we have no intention of supporting YUV sensor
(this will come back to bite me :)
in our pipeline handler, using BayerFormat should be ok for now.
I will however, remove the table in the pipeline handler, and use the
existing table in BayerFormat.
In fact, this is what I had for v1 and v2. I'll also fix things so the
existing mbusCodeToBayerFormat()
will handle switching between packing/unpacking though a helper.
Thanks,
Naush
>
> > 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20211027/d88b4db3/attachment.htm>
More information about the libcamera-devel
mailing list