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

[Jacopo Mondi]

Hi Nasuh,

On Thu, Jun 25, 2020 at 08:47:36AM +0100, Naushir Patuck wrote:
> Hi all,
>
> On Thu, 25 Jun 2020 at 08:35, Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > 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 ?
>
> From my understanding, yes, this is probably cleaner.  However, they
> should be both equivalent?

Ah wait, possibly not, I overlooked the type of deviceFormats.
back_inserter() creates an std::back_insert_iterator which calls the
container's push_back() operation which we don't have for maps. I
think std::inserter() with deviceFormats.end() is the way to go.

Sorry for the noise.

>
> Regards,
> Naush