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

[Paul Elder]

Hi Laurent,

Thank you for the review.

On Sat, Jun 06, 2020 at 02:03:24PM +0300, Laurent Pinchart wrote:
> 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 could add one. Or, does stride * height work?

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

Okay.

> > ---
> >  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.

I think warning should be fine. It's a warning, that we can continue,
but there might be a problem if you choose a format with zero sizeimage.
On the other hand, this warning will only print if that format is set at
the time of reqbufs, so it might not display.

> > @@ -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 */


Thanks,

Paul