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

[Jacopo Mondi]

Hi Laurent, Naush

On Thu, Jun 25, 2020 at 05:37:46AM +0300, Laurent Pinchart wrote:
> Hi Naush,
>
> On Thu, Jun 25, 2020 at 05:21:26AM +0300, Laurent Pinchart wrote:
> > On Tue, Jun 23, 2020 at 08:59:49AM +0100, Naushir Patuck wrote:
> > > On Tue, 23 Jun 2020 at 00:46, Laurent Pinchart wrote:
> > >> On Mon, Jun 22, 2020 at 11:25:47AM +0100, Naushir Patuck wrote:
> > >>> On Mon, 22 Jun 2020 at 05:59, Laurent Pinchart wrote:
> > >>>> 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()),
>
> One additional comment, should this be deviceFormats.end(), to insert
> the new entries at the end of the map ? It will make a difference in

Shouldn't this be handled by using back_inserter() if that's what we
want ?