[libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add StreamFormats to StreamConfiguration
Naushir Patuck
naush at raspberrypi.com
Tue Jun 23 09:59:49 CEST 2020
Hi Laurent,
On Tue, 23 Jun 2020 at 00:46, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Naush,
>
> 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()),
> > >>>>> + [&](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?
>
> StreamConfigure::size is the recommended size for the stream.
> StreamFormats is a map containing all the supported pixel formats, and
> for each of them, the supported sizes. The sizes are expressed as a
> vector of SizeRange, which must contain at least one entry.
>
> You populate StreamConfigure::size correctly as far as I can tell,
> there's no reason to change. The StreamFormats, however, should be
> populated with what the camera can produce, and that should be
> conditioned by the sizes supported by the sensor and the ISP
> capabilities. You could, for instance, take all the sizes supported by
> the sensor, and for each of them, create a SizeRange with the minimum
> set to the sensor size downscaled as much as possible by the ISP, and
> the maximum set to the sensor size.
>
Apologies if this is a silly question, but since the ISP hardware
genuinely does allow frame sizes in the range [64,16384] in both
dimensions irrespective of the input size, is what I currently have
for populating SizeRange not correct?
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