[libcamera-devel,v2] libcamera: pipeline: simple: enable mplane devices using contiguous memory

10026 diff mbox series

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.

The following code in SimpleCameraData::init() filters out the pixel formats
libcamera doesn't support:

	PixelFormat pixelFormat = videoFormat.first.toPixelFormat();
	if (!pixelFormat)
		continue;

So the check for the number of memory planes this patch adds would not
trigger until non-contiguous planar formats becomes supported in libcamera,
and video devices using these formats are enabled in the simple pipeline
handler. Then this check will remind one to review the simple pipeline
handler code.

Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 Changes in v2:
 - The commit message extended to explain how the simple pipeline handler
   filters out pixel formats not supported in libcamera. No changes to
   the code vs v1.

 src/libcamera/pipeline/simple/simple.cpp | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Hi Andrey,

Thank you for the patch.

On Fri, Oct 09, 2020 at 02:17:39PM +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.
> 
> The following code in SimpleCameraData::init() filters out the pixel formats
> libcamera doesn't support:
> 
> 	PixelFormat pixelFormat = videoFormat.first.toPixelFormat();
> 	if (!pixelFormat)
> 		continue;
> 
> So the check for the number of memory planes this patch adds would not
> trigger until non-contiguous planar formats becomes supported in libcamera,
> and video devices using these formats are enabled in the simple pipeline
> handler. Then this check will remind one to review the simple pipeline
> handler code.

Perfect :-)

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  Changes in v2:
>  - The commit message extended to explain how the simple pipeline handler
>    filters out pixel formats not supported in libcamera. No changes to
>    the code vs v1.
> 
>  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));

On Sat, Oct 10, 2020 at 01:40:28AM +0300, Laurent Pinchart wrote:
> Hi Andrey,
> 
> Thank you for the patch.
> 
> On Fri, Oct 09, 2020 at 02:17:39PM +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.
> > 
> > The following code in SimpleCameraData::init() filters out the pixel formats
> > libcamera doesn't support:
> > 
> > 	PixelFormat pixelFormat = videoFormat.first.toPixelFormat();
> > 	if (!pixelFormat)
> > 		continue;
> > 
> > So the check for the number of memory planes this patch adds would not
> > trigger until non-contiguous planar formats becomes supported in libcamera,
> > and video devices using these formats are enabled in the simple pipeline
> > handler. Then this check will remind one to review the simple pipeline
> > handler code.
> 
> Perfect :-)
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  Changes in v2:
> >  - The commit message extended to explain how the simple pipeline handler
> >    filters out pixel formats not supported in libcamera. No changes to
> >    the code vs v1.
> > 
> >  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";

Actually, s/  / /

I'll fix when applying. May I recommend using the checkstyle.py script
as a post-commit or pre-commit git hook ? The hooks can be found in
utils/hook/, you can just copy the one you prefer to .git/hooks/

> > +		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));