[libcamera-devel] [PATCH] v4l2: v4l2_camera_proxy: Support MJPEG

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Sat, Jun 06, 2020 at 05:07:55PM +0900, Paul Elder wrote:
> Add an entry for MJPEG in V4L2CameraProxy's PixelFormatInfo list to
> allow proper calculation of sizeimage for MJPEG, such that the
> parameters to mmap can align properly instead of failing. This allows
> MJPEG to be used in the V4L2 compatibility layer.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>

This works, but it's quite a bit of a hack. It will result in an
overestimate of the required buffer size, while devices that can provide
MJPEG natively (that's just UVC really) have the ability to provide a
better estimate. The uvcvideo driver reports the estimate through the
sizeimage field in the V4L2 format ioctls.

You're not the only one to take the short route though, there are UVC
devices that report the same value as this patch :-)

We have a field in StreamConfiguration to report the stride, but no
field to report the full image size. Would it make sense to add one ? Or
are there better options ?

I think we can merge this patch for now, but I wouldn't stop here. Could
you add a todo comment to capture this ?

> ---
>  src/v4l2/v4l2_camera_proxy.cpp | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index 059f3cbe..e4511207 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -358,8 +358,8 @@ int V4L2CameraProxy::vidioc_reqbufs(struct v4l2_requestbuffers *arg)
>  	 * 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.
> +	 * On the other hand, if a format has a zero sizeimage we'll get a
> +	 * floating point exception when we try to stream it.
>  	 */
>  	if (sizeimage_ == 0)
>  		LOG(V4L2Compat, Warning)

Should this still be a warning, or should it be turned back into an
error ? I think the comment should then be updated accordingly.

> @@ -556,7 +556,7 @@ struct PixelFormatInfo {
>  
>  namespace {
>  
> -static const std::array<PixelFormatInfo, 13> pixelFormatInfo = {{
> +static const std::array<PixelFormatInfo, 14> pixelFormatInfo = {{
>  	/* RGB formats. */
>  	{ PixelFormat(DRM_FORMAT_RGB888),	V4L2_PIX_FMT_BGR24,	1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
>  	{ PixelFormat(DRM_FORMAT_BGR888),	V4L2_PIX_FMT_RGB24,	1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
> @@ -573,6 +573,8 @@ static const std::array<PixelFormatInfo, 13> pixelFormatInfo = {{
>  	{ PixelFormat(DRM_FORMAT_NV61),		V4L2_PIX_FMT_NV61,	2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },
>  	{ PixelFormat(DRM_FORMAT_NV24),		V4L2_PIX_FMT_NV24,	2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },
>  	{ PixelFormat(DRM_FORMAT_NV42),		V4L2_PIX_FMT_NV42,	2, {{ {  8, 1, 1 }, { 16, 1, 1 }, {  0, 0, 0 } }} },
> +	/* Compressed formats. */
> +	{ PixelFormat(DRM_FORMAT_MJPEG),	V4L2_PIX_FMT_MJPEG,	1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
>  }};
>  
>  } /* namespace */

-- 
Regards,

Laurent Pinchart