[libcamera-devel] [PATCH] libcamera: pipeline: simple: enable mplane devices using contiguous memory
Niklas Söderlund
niklas.soderlund at ragnatech.se
Fri Oct 9 02:17:37 CEST 2020
Hi Andrey,
Thanks for researching this and proposing a way forward.
On 2020-10-09 03:05:41 +0300, Laurent Pinchart wrote:
> Hi Andrey,
>
> On Fri, Oct 09, 2020 at 12:48:14AM +0300, Andrey Konovalov wrote:
> > Hi,
> >
> > As suggested by Laurent during the discussion on #libcamera irc channel,
> > I considered filtering out pixel formats not supported in libcamera in
> > SimpleCameraData::init().
> >
> > The thing is that in SimpleCameraData::init() we only have the information
> > from V4L2VideoDevice::formats() (enum_fmt ioctl) which doesn't include
> > the number of memory planes for a given format.
> > While in SimplePipelineHandler::configure() we have the data returned by
> > s_fmt ioctl, so the number of memory planes for the format set is known,
> > and I could easily use it in v1 of the patch.
> >
> > As libcamera currently supports only single memory plane (contiguous
> > memory) formats, this is enough to filter out unsupported formats
> > from what V4L2VideoDevice::formats() returns (easy to implement in
> > SimpleCameraData::init()). And checking the number of memory planes
> > for a given format isn't necessary.
> >
> > But after looking at SimpleCameraData::init(), it turned out that unsupported
> > formats are filtered out by the already existing code:
> >
> > -----8<-----
> > /*
> > * Store the configuration in the formats_ map, mapping the
> > * PixelFormat to the corresponding configuration. Any
> > * previously stored value is overwritten, as the pipeline
> > * handler currently doesn't care about how a particular
> > * PixelFormat is achieved.
> > */
> > for (const auto &videoFormat : videoFormats) {
> > PixelFormat pixelFormat = videoFormat.first.toPixelFormat();
> > if (!pixelFormat)
> > continue;
> > -----8<-----
> >
> > V4L2PixelFormat::toPixelFormat() method checks if the V4L2 pixel format is present
> > in the vpf2pf map, and if it is not there returns the invalid PixelFormat (with zero fourcc_).
> >
> > And indeed with the "if (video->caps().isMultiplanar())" check removed, and the NV61 entry
> > commented out from the vpf2pf map (to simulate unsupported format) I've got:
> >
> > -----8<-----
> > [2:29:09.524492795] [1396] DEBUG SimplePipeline simple.cpp:292 Adding configuration for 2592x1944 in pixel formats [ NV61, NV16]
> > [2:29:09.524833004] [1396] WARN V4L2 v4l2_pixelformat.cpp:180 Unsupported V4L2 pixel format NV61
> > [2:29:09.527873323] [1396] DEBUG Camera camera_manager.cpp:152 Pipeline handler "SimplePipelineHandler" matched
> > Using camera /base/soc/cci at 1b0c000/i2c-bus at 0/camera_rear at 3c
> > [2:29:09.529992233] [1395] DEBUG Camera camera.cpp:754 streams configuration: (0) 2592x1944-NV16
> > [2:29:09.530364682] [1395] INFO Camera camera.cpp:811 configuring streams: (0) 2592x1944-NV16
> > -----8<-----
> >
> > So the "unsupported" NV61 format was filtered out by the existing code, and NV16 was used for the capture instead.
> >
> > This leaves us with the two options for v2 of the patch:
> > * v2a
> > Just drop the "if (video->caps().isMultiplanar())" guard.
> > Don't add any new checks. As unsupported formats are filtered out in SimpleCameraData::init(),
> > and all the currently supported formats will just work with the simple pipeline handler.
> > * v2b
> > Only change the commit message in the v1 patch.
> > To explain that the added check is no op in the current libcamera, but
> > this "if (captureFormat.planesCount != 1)" will trigger and serve as the reminder to review
> > the simple pipeline handler code when 1) libcamera gets support for non-contiguous formats,
> > and 2) the hardware which uses such formats is enabled in the simple pipeline handler.
> >
> > What would be the best option?
>
> I'd go for the second option, a reminder is useful, but I don't have a
> very strong preference.
I also have no strong preference and reminders are good I'm also OK with
the second option.
>
> > On 07.10.2020 21:22, Kieran Bingham wrote:
> > > On 07/10/2020 18:16, Andrey Konovalov wrote:
> > >> On 07.10.2020 16:04, Kieran Bingham wrote:
> > >>> On 07/10/2020 14:01, Niklas Söderlund wrote:
> > >>>> On 2020-10-07 10:54:57 +0300, Andrey Konovalov wrote:
> > >>>>> The current simple pipeline handler refuses to work with capture
> > >>>>> devices
> > >>>>> which have V4L2_CAP_VIDEO_CAPTURE_MPLANE flag set in the device
> > >>>>> capabilities
> > >>>>> field. This is too restrictive, as devices supporting the
> > >>>>> multi-planar API
> > >>>>> can be using contiguous memory for semi-planar and planar formats,
> > >>>>> and this
> > >>>>> would just work without any changes to libcamera.
> > >>>>>
> > >>>>> Drop the guard against MPLANE devices, and replace it with the check of
> > >>>>> the number of planes in the format the simple pipeline handler is
> > >>>>> going to
> > >>>>> use for capture. This will let MPLANE devices which don't use
> > >>>>> non-contiguous
> > >>>>> memory for frame buffers to work with the simple pipeline handler.
> > >>>>
> > >>>> I wonder if the check should not be moved to SimpleCameraData::init()
> > >>>> where the formats_ array is built. The array contains all supported
> > >>>> formats of the camera and excluding mplaner formats from it will make it
> > >>>> not show up at all for applications. Also validate() would Adjust if any
> > >>>> format is asked for that is not in the formats_ array.
> > >>
> > >> Yes, this is a better option, thanks! I'll send the v2 shortly.
> > >>
> > >>> That sounds pretty good too. If we go that route, I think it will need a
> > >>> highlighting '\todo: support mplane formats' so that it's clear/easy to
> > >>> find the code which is mysteriously removing supported formats from a
> > >>> device which is capable of using them ;-) (after we really support
> > >>> Multiplanar).
> > >>
> > >> OK, will add the \todo.
> > >> BTW, the "mplane format" in this context means the one using non-contiguous
> > >> memory for frame buffers. E.g. 'NV16' I've tested is allowed, while 'NM16'
> > >> (aka V4L2_PIX_FMT_NV16M) would be excluded.
> > >
> > > Yes indeed - Agreed ;-)
> > >
> > >>>>> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
> > >>>>> ---
> > >>>>> src/libcamera/pipeline/simple/simple.cpp | 12 ++++++------
> > >>>>> 1 file changed, 6 insertions(+), 6 deletions(-)
> > >>>>>
> > >>>>> diff --git a/src/libcamera/pipeline/simple/simple.cpp
> > >>>>> b/src/libcamera/pipeline/simple/simple.cpp
> > >>>>> index 10223a9b..8dc23623 100644
> > >>>>> --- a/src/libcamera/pipeline/simple/simple.cpp
> > >>>>> +++ b/src/libcamera/pipeline/simple/simple.cpp
> > >>>>> @@ -592,6 +592,12 @@ int SimplePipelineHandler::configure(Camera
> > >>>>> *camera, CameraConfiguration *c)
> > >>>>> if (ret)
> > >>>>> return ret;
> > >>>>> + if (captureFormat.planesCount != 1) {
> > >>>>> + LOG(SimplePipeline, Error)
> > >>>>> + << "Planar formats using non-contiguous memory not supported";
> > >>>>> + return -EINVAL;
> > >>>>> + }
> > >>>>> +
> > >>>>> if (captureFormat.fourcc != videoFormat ||
> > >>>>> captureFormat.size != pipeConfig.captureSize) {
> > >>>>> LOG(SimplePipeline, Error)
> > >>>>> @@ -845,12 +851,6 @@ V4L2VideoDevice
> > >>>>> *SimplePipelineHandler::video(const MediaEntity *entity)
> > >>>>> if (video->open() < 0)
> > >>>>> return nullptr;
> > >>>>> - if (video->caps().isMultiplanar()) {
> > >>>>> - LOG(SimplePipeline, Error)
> > >>>>> - << "V4L2 multiplanar devices are not supported";
> > >>>>> - return nullptr;
> > >>>>> - }
> > >>>>> -
> > >>>>> video->bufferReady.connect(this, &SimplePipelineHandler::bufferReady);
> > >>>>> auto element = videos_.emplace(entity, std::move(video));
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list