[libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add StreamFormats to StreamConfiguration
Naushir Patuck
naush at raspberrypi.com
Mon Jun 22 12:25:47 CEST 2020
Hi Laurent,
Thank you for the review.
On Mon, 22 Jun 2020 at 05:59, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Naush and everybody,
>
> Thank you for the patch.
>
> On Thu, Jun 18, 2020 at 10:18:38AM +0100, Naushir Patuck wrote:
> > On Thu, 18 Jun 2020 at 10:03, Kieran Bingham wrote:
> > > On 18/06/2020 09:41, Jacopo Mondi wrote:
> > >> On Wed, Jun 10, 2020 at 03:26:40PM +0100, Naushir Patuck wrote:
> > >>> In generateConfiguration(), add the device node specific formats to the
> > >>> StreamConfiguration for each StreamRole requested.
> > >>>
> > >>> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > >>> ---
> > >>> .../pipeline/raspberrypi/raspberrypi.cpp | 52 +++++++++++++------
> > >>> 1 file changed, 36 insertions(+), 16 deletions(-)
> > >>>
> > >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > >>> index e16a9c7f..03a1e641 100644
> > >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > >>> @@ -518,41 +518,45 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,
> > >>> RPiCameraData *data = cameraData(camera);
> > >>> CameraConfiguration *config = new RPiCameraConfiguration(data);
> > >>> V4L2DeviceFormat sensorFormat;
> > >>> + unsigned int bufferCount;
> > >>> + PixelFormat pixelFormat;
> > >>> V4L2PixFmtMap fmts;
> > >>> + Size size;
> > >>>
> > >>> if (roles.empty())
> > >>> return config;
> > >>>
> > >>> for (const StreamRole role : roles) {
> > >>> - StreamConfiguration cfg{};
> > >>> -
> > >>> switch (role) {
> > >>> case StreamRole::StillCaptureRaw:
> > >>> - cfg.size = data->sensor_->resolution();
> > >>> + size = data->sensor_->resolution();
> > >>> fmts = data->unicam_[Unicam::Image].dev()->formats();
> > >>> - sensorFormat = findBestMode(fmts, cfg.size);
> > >>> - cfg.pixelFormat = sensorFormat.fourcc.toPixelFormat();
> > >>> - ASSERT(cfg.pixelFormat.isValid());
> > >>> - cfg.bufferCount = 1;
> > >>> + sensorFormat = findBestMode(fmts, size);
> > >>> + pixelFormat = sensorFormat.fourcc.toPixelFormat();
> > >>> + ASSERT(pixelFormat.isValid());
> > >>> + bufferCount = 1;
> > >>> break;
> > >>>
> > >>> case StreamRole::StillCapture:
> > >>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);
> > >>> + fmts = data->isp_[Isp::Output0].dev()->formats();
> > >>> + pixelFormat = PixelFormat(DRM_FORMAT_NV12);
>
> This conflicts with recent rework of format handling, this line should
> now be
>
> + pixelFormat = formats::NV12;
>
> Same for the other formats below.
Will update in the next patch.
>
> > >>> /* Return the largest sensor resolution. */
> > >>> - cfg.size = data->sensor_->resolution();
> > >>> - cfg.bufferCount = 1;
> > >>> + size = data->sensor_->resolution();
> > >>> + bufferCount = 1;
> > >>> break;
> > >>>
> > >>> case StreamRole::VideoRecording:
> > >>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);
> > >>> - cfg.size = { 1920, 1080 };
> > >>> - cfg.bufferCount = 4;
> > >>> + fmts = data->isp_[Isp::Output0].dev()->formats();
> > >>> + pixelFormat = PixelFormat(DRM_FORMAT_NV12);
> > >>> + size = { 1920, 1080 };
> > >>> + bufferCount = 4;
> > >>> break;
> > >>>
> > >>> case StreamRole::Viewfinder:
> > >>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);
> > >>> - cfg.size = { 800, 600 };
> > >>> - cfg.bufferCount = 4;
> > >>> + fmts = data->isp_[Isp::Output0].dev()->formats();
> > >>> + pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);
> > >>> + size = { 800, 600 };
> > >>> + bufferCount = 4;
> > >>> break;
> > >>>
> > >>> default:
> > >>> @@ -561,6 +565,22 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,
> > >>> break;
> > >>> }
> > >>>
> > >>> + /* Translate the V4L2PixelFormat to PixelFormat. */
> > >>> + std::map<PixelFormat, std::vector<SizeRange>> deviceFormats;
> > >>> + std::transform(fmts.begin(), fmts.end(), std::inserter(deviceFormats, deviceFormats.begin()),
> > >>> + [&](const decltype(fmts)::value_type &format) {
> > >>> + return decltype(deviceFormats)::value_type{
> > >>> + format.first.toPixelFormat(),
> > >>> + format.second
> > >>> + };
> > >>> + });
>
> For the unicam device this looks fine to me, as the driver will
> give us formats and sizes that match the capabilities of the sensor. For
> the ISP, the list of supported pixel formats is queried from the
> firmware, filtered against the list of pixel formats supported by the
> driver, and all should be fine. However, for the frame sizes, the ISP
> driver reports hardcoded minimum and maximum values of 64 and 16384
> respectively. Shouldn't we instead, in the pipeline handler, take the
> capabilities of both the sensor and the ISP into account to create a
> range of sizes that would be closer to what can actually be achieved ?
Just checking, so I should populate
StreamFormat::std::vector<SizeRange> with the device ranges? What
about the StreamConfiguration::size field that I currently populate
with a single value, should I leave that blank/empty?
Regards,
Naush
>
> For UVC the situation is different, as the kernel directly reports the
> list of formats and sizes supported by the device.
>
> > >> Really took me a while to parse this, as I was not expecting to see
> > >> std::inserter() but just deviceFormats.begin() there, but then I
> > >> learned about inserter, and indeed if I remove std::inserter() I get
> > >> an error due to the fact the copy operator of std::pair is deleted.
> > >
> > > I think that's the same as the conversion routine in the UVC pipeline
> > > handler (which I think came from Laurent).
> >
> > Yes, I picked this from the uvc pipeline handler. Took me some goes
> > to understand it as well :)
> >
> > > Not for this patch (I think this can go in as is) but we should really
> > > make a helper for that conversion as it's likely to be used in multiple
> > > places, and it is quite hard to parse on it's own ;-(.
> >
> > That would make sense. All pipeline handlers will require this
> > conversion, I think!
> >
> > > However, I think that overlaps with changes that you (Jacopo) were
> > > working on with ImageFormats anyway.
>
> Yes, we know this API needs to be reworked, so I wouldn't spend time on
> creating a helper right now, but would rather land the ImageFormats
> series first, and then discuss how we should rework stream configuration
> handling. The fact that this code should consider both the sensor and
> the ISP, as explained above, also makes it more difficult to create a
> single helper function.
>
> > > Eitherway, this patch will help support more formats and enumeration on
> > > the RPi pipeline handler:
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > >
> > >>> +
> > >>> + /* Add the stream format based on the device node used for the use case. */
> > >>> + StreamFormats formats(deviceFormats);
> > >>> + StreamConfiguration cfg(formats);
> > >>> + cfg.size = size;
> > >>> + cfg.pixelFormat = pixelFormat;
> > >>> + cfg.bufferCount = bufferCount;
> > >>
> > >> Patch looks good to me
> > >>
> > >> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > >>
> > >>> config->addConfiguration(cfg);
> > >>> }
> > >>>
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list