[{"id":4990,"web_url":"https://patchwork.libcamera.org/comment/4990/","msgid":"<20200603210907.GA27695@pendragon.ideasonboard.com>","date":"2020-06-03T21:09:07","subject":"Re: [libcamera-devel] [PATCH 4/5] v4l2: v4l2_camera_proxy: Don't\n\treturn -EINVAL for zero sizeimage in REQBUFS","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Wed, Jun 03, 2020 at 11:16:08PM +0900, Paul Elder wrote:\n> If VIDIOC_REQBUFS returns -EINVAL, it signals to the application that\n> the requested buffer or memory type is not supported. If we return\n> -EINVAL due to a zero sizeimage, then the application will think that we\n> don't support a memory type that we actually do.\n> \n> On the other hand, sizeimage will be zero for formats whose size we\n> don't know how to calculate, such as MJPEG. If we try to stream such\n> formats anyway, we will get a floating point exception and crash. Issue\n> a warning for now, and don't return -EINVAL, so that we can continue\n> operation.\n> \n> Signed-off-by: Paul Elder \n> ---\n> src/v4l2/v4l2_camera_proxy.cpp | 12 +++++++++++-\n> 1 file changed, 11 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> index d2419b96..f84982a2 100644\n> --- a/src/v4l2/v4l2_camera_proxy.cpp\n> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> @@ -352,8 +352,18 @@ int V4L2CameraProxy::vidioc_reqbufs(struct v4l2_requestbuffers *arg)\n> \t\treturn -EINVAL;\n> \n> \tsizeimage_ = calculateSizeImage(streamConfig_);\n> +\t/*\n> +\t * If we return -EINVAL here then the application will think that we\n> +\t * don't support streaming mmap. Since we don't support readwrite and\n> +\t * userptr either, the application will get confused and think that\n> +\t * we don't support anything.\n> +\t * On the other hand, if a format has a zero sizeimage (eg. MJPG),\n> +\t * we'll get a floating point exception when we try to stream it.\n> +\t */\n> \tif (sizeimage_ == 0)\n> -\t\treturn -EINVAL;\n> +\t\tLOG(V4L2Compat, Warning)\n> +\t\t\t<< \"sizeimage of at least one format is zero. \"\n> +\t\t\t<< \"Streaming this format will cause a floating point exception.\";\n> \n> \tsetFmtFromConfig(streamConfig_);\n> \n\nWhat issue does this fix, if we allow applications to move forward a\nbit, but crash very soon after ?\n\nI would also have expected V4L2CameraProxy::vidioc_s_fmt() to have\nreturned -EINVAL, and the application not to continue to\nV4L2CameraProxy::vidioc_reqbufs(). Is the application calling\nVIDIOC_REQBUFS without first calling VIDIOC_S_FMT ?\n\nWe'll have to solve this properly, which will involve reporting the\nmaximum size needed for the MJPEG frames. Have you looked into what\nwould be required for that ?\n\nBy the way, we now report a stride value in the stream configuration,\nV4L2CameraProxy::calculateSizeImage() and/or\nV4L2CameraProxy::imageSize() should be updated to use it.\n\nFinally, now that we have a PixelFormatInfo::info(), I think we should\nextend the PixelFormatInfo class from the libcamera core with the\ninformation stored in the PixelFormatInfo struct from the V4L2\nadaptation layer, and drop the latter. In a separate patch of course.","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0D3186110A\n\tfor ;\n\tWed, 3 Jun 2020 23:09:24 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7E86229B;\n\tWed, 3 Jun 2020 23:09:23 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"g5uBYAQa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1591218563;\n\tbh=86wpGXckawF2CWKdzIDZw2XNxeNyFKWYzMd1cLGzQus=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=g5uBYAQaB6zdn8Yzil0DFBxAVEOFEnm395HzQeGPPl3zthcR/6w5mnI4tmWVR34hq\n\tu55igAehcCGt2WtJTMGzBeTP/3//2vmQxwTpoD/jOIWQpc+a9wPdCIjp7eL2u6socJ\n\t34FkQyhTu9LFbLL/vD4CW+F3towUzfF8HntXz8ZU=","Date":"Thu, 4 Jun 2020 00:09:07 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200603210907.GA27695@pendragon.ideasonboard.com>","References":"<20200603141609.18584-1-paul.elder@ideasonboard.com>\n\t<20200603141609.18584-5-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200603141609.18584-5-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 4/5] v4l2: v4l2_camera_proxy: Don't\n\treturn -EINVAL for zero sizeimage in REQBUFS","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 03 Jun 2020 21:09:24 -0000"}}]