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

[Laurent Pinchart]

Hi Paul,

On Mon, Jun 08, 2020 at 04:57:57PM +0900, Paul Elder wrote:
> On Sat, Jun 06, 2020 at 02:03:24PM +0300, Laurent Pinchart wrote:
> > 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?

stride is ill-defined for compressed formats. I think that regardless of
what we decide to do, we need to think about how all the fields should
be used with the different types of formats (packed, planar, compressed,
...). Could you please make sure to take that into consideration, and
document it in any patch you may submit ?

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

But is it possible to do so anymore, given that all formats supported by
libcamera should now be listed in pixelFormatInfo ?

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