[libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add StreamFormats to StreamConfiguration

[Naushir Patuck]

Hi,

On Thu, 18 Jun 2020 at 10:03, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Hi Naush, Jacopo,
>
> On 18/06/2020 09:41, Jacopo Mondi wrote:
> > Hi Naush,
> >    sorry for the delay, this fell through the cracks
> >
> > 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);
> >>                      /* 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
> >> +                                    };
> >> +                           });
> >
> > 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.
>
> 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>
> >
> > Thanks
> >   j
> >
> >>              config->addConfiguration(cfg);
> >>      }
> >>
> >> --
> >> 2.25.1
> >>
> >> _______________________________________________
> >> libcamera-devel mailing list
> >> libcamera-devel at lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
> >
>
> --
> Regards
> --
> Kieran

Regards,
Naush