| Message ID | 20201007075457.4455-1-andrey.konovalov@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Andrey, On 07/10/2020 08:54, 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. Agreed, and I'm sure in-kernel, the use of the mplane api is preferred over the splane, so this likely isn't an uncommon case. It's just that we haven't hit it ourselves yet. > 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. It sounds to me like this is a more accurate check of the restrictions we are currently have imposed. > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > 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)); >
Hello Andrey, Thanks for your patch. 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. > > Signed-off-by: Andrey Konovalov <andrey.konovalov@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)); > -- > 2.17.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, Andrey, On 07/10/2020 14:01, Niklas Söderlund wrote: > Hello Andrey, > > Thanks for your patch. > > 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. 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). -- Kieran > >> >> Signed-off-by: Andrey Konovalov <andrey.konovalov@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)); >> -- >> 2.17.1 >> >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Niklas, Kieran, On 07.10.2020 16:04, Kieran Bingham wrote: > Hi Niklas, Andrey, > > On 07/10/2020 14:01, Niklas Söderlund wrote: >> Hello Andrey, >> >> Thanks for your patch. >> >> 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. Thanks, Andrey > -- > Kieran > > > >> >>> >>> Signed-off-by: Andrey Konovalov <andrey.konovalov@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)); >>> -- >>> 2.17.1 >>> >>> _______________________________________________ >>> libcamera-devel mailing list >>> libcamera-devel@lists.libcamera.org >>> https://lists.libcamera.org/listinfo/libcamera-devel >> >
Hi Andrey, On 07/10/2020 18:16, Andrey Konovalov wrote: > Hi Niklas, Kieran, > > On 07.10.2020 16:04, Kieran Bingham wrote: >> Hi Niklas, Andrey, >> >> On 07/10/2020 14:01, Niklas Söderlund wrote: >>> Hello Andrey, >>> >>> Thanks for your patch. >>> >>> 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 ;-) -- Kieran > > Thanks, > Andrey > >> -- >> Kieran >> >> >> >>> >>>> >>>> Signed-off-by: Andrey Konovalov <andrey.konovalov@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)); >>>> -- >>>> 2.17.1 >>>> >>>> _______________________________________________ >>>> libcamera-devel mailing list >>>> libcamera-devel@lists.libcamera.org >>>> https://lists.libcamera.org/listinfo/libcamera-devel >>> >>
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@1b0c000/i2c-bus@0/camera_rear@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?
Thanks,
Andrey
On 07.10.2020 21:22, Kieran Bingham wrote:
> Hi Andrey,
>
> On 07/10/2020 18:16, Andrey Konovalov wrote:
>> Hi Niklas, Kieran,
>>
>> On 07.10.2020 16:04, Kieran Bingham wrote:
>>> Hi Niklas, Andrey,
>>>
>>> On 07/10/2020 14:01, Niklas Söderlund wrote:
>>>> Hello Andrey,
>>>>
>>>> Thanks for your patch.
>>>>
>>>> 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 ;-)
> --
> Kieran
>
>
>>
>> Thanks,
>> Andrey
>>
>>> --
>>> Kieran
>>>
>>>
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Andrey Konovalov <andrey.konovalov@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));
>>>>> --
>>>>> 2.17.1
>>>>>
>>>>> _______________________________________________
>>>>> libcamera-devel mailing list
>>>>> libcamera-devel@lists.libcamera.org
>>>>> https://lists.libcamera.org/listinfo/libcamera-devel
>>>>
>>>
>
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@1b0c000/i2c-bus@0/camera_rear@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. > 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@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));
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@1b0c000/i2c-bus@0/camera_rear@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@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
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));
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. Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> --- src/libcamera/pipeline/simple/simple.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)