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

Message ID 20200605090106.15424-6-paul.elder@ideasonboard.com
State Accepted
Headers show
Series
  • Support qv4l2 with v4l2-compat
Related show

Commit Message

Paul Elder June 5, 2020, 9:01 a.m. UTC
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. We cannot error on a
zero sizeimage, because reqbufs could be called merely to probe what IO
methods we support; qv4l2, for example, called reqbufs once with userptr
and once more with mmap, both times with count=1.

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@ideasonboard.com>

---
Changes in v2: expand changelog
---
 src/v4l2/v4l2_camera_proxy.cpp | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Comments

Laurent Pinchart June 5, 2020, 10:16 a.m. UTC | #1
Hi Paul,

Thank you for the patch.

On Fri, Jun 05, 2020 at 06:01:04PM +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. We cannot error on a
> zero sizeimage, because reqbufs could be called merely to probe what IO
> methods we support; qv4l2, for example, called reqbufs once with userptr
> and once more with mmap, both times with count=1.
> 
> 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@ideasonboard.com>

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

> ---
> Changes in v2: expand changelog
> ---
>  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 a0c6deea..cbe9e026 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. MJPEG),
> +	 * 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_);
>

Patch

diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
index a0c6deea..cbe9e026 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. MJPEG),
+	 * 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_);