[libcamera-devel] [PATCH 4/5] v4l2: v4l2_camera_proxy: Don't return -EINVAL for zero sizeimage in REQBUFS

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Wed, Jun 03, 2020 at 11:16:08PM +0900, Paul Elder wrote:
> If VIDIOC_REQBUFS returns -EINVAL, it signals to the application that
> the requested buffer or memory type is not supported. If we return
> -EINVAL due to a zero sizeimage, then the application will think that we
> don't support a memory type that we actually do.
> 
> On the other hand, sizeimage will be zero for formats whose size we
> don't know how to calculate, such as MJPEG. If we try to stream such
> formats anyway, we will get a floating point exception and crash. Issue
> a warning for now, and don't return -EINVAL, so that we can continue
> operation.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
>  src/v4l2/v4l2_camera_proxy.cpp | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index d2419b96..f84982a2 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -352,8 +352,18 @@ int V4L2CameraProxy::vidioc_reqbufs(struct v4l2_requestbuffers *arg)
>  		return -EINVAL;
>  
>  	sizeimage_ = calculateSizeImage(streamConfig_);
> +	/*
> +	 * If we return -EINVAL here then the application will think that we
> +	 * don't support streaming mmap. Since we don't support readwrite and
> +	 * userptr either, the application will get confused and think that
> +	 * we don't support anything.
> +	 * On the other hand, if a format has a zero sizeimage (eg. MJPG),
> +	 * we'll get a floating point exception when we try to stream it.
> +	 */
>  	if (sizeimage_ == 0)
> -		return -EINVAL;
> +		LOG(V4L2Compat, Warning)
> +			<< "sizeimage of at least one format is zero. "
> +			<< "Streaming this format will cause a floating point exception.";
>  
>  	setFmtFromConfig(streamConfig_);
>  

What issue does this fix, if we allow applications to move forward a
bit, but crash very soon after ?

I would also have expected V4L2CameraProxy::vidioc_s_fmt() to have
returned -EINVAL, and the application not to continue to
V4L2CameraProxy::vidioc_reqbufs(). Is the application calling
VIDIOC_REQBUFS without first calling VIDIOC_S_FMT ?

We'll have to solve this properly, which will involve reporting the
maximum size needed for the MJPEG frames. Have you looked into what
would be required for that ?

By the way, we now report a stride value in the stream configuration,
V4L2CameraProxy::calculateSizeImage() and/or
V4L2CameraProxy::imageSize() should be updated to use it.

Finally, now that we have a PixelFormatInfo::info(), I think we should
extend the PixelFormatInfo class from the libcamera core with the
information stored in the PixelFormatInfo struct from the V4L2
adaptation layer, and drop the latter. In a separate patch of course.

-- 
Regards,

Laurent Pinchart