[libcamera-devel,v2] v4l2: v4l2_camera_proxy: Support MJPEG
diff mbox series

Message ID 20200608080529.7727-1-paul.elder@ideasonboard.com
State Accepted
Headers show
Series
  • [libcamera-devel,v2] v4l2: v4l2_camera_proxy: Support MJPEG
Related show

Commit Message

Paul Elder June 8, 2020, 8:05 a.m. UTC 
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@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

---
Changes in v2:
- add todo for better MJPEG sizeimage estimate
- update comment that the warning will only be printed if the set format
  has zero sizeimage at the time of reqbufs
---
 src/v4l2/v4l2_camera_proxy.cpp | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Laurent Pinchart June 8, 2020, 2:12 p.m. UTC | #1 
Hi Paul,

Thank you for the patch.

On Mon, Jun 08, 2020 at 05:05:29PM +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@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> 
> ---
> Changes in v2:
> - add todo for better MJPEG sizeimage estimate
> - update comment that the warning will only be printed if the set format
>   has zero sizeimage at the time of reqbufs
> ---
>  src/v4l2/v4l2_camera_proxy.cpp | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index 059f3cbe..07e746b4 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -358,8 +358,9 @@ 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 the set format at the time of reqbufs has a
> +	 * zero sizeimage we'll get a floating point exception when we try to
> +	 * stream it.
>  	 */
>  	if (sizeimage_ == 0)
>  		LOG(V4L2Compat, Warning)
> @@ -556,7 +557,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 +574,9 @@ 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. */
> +	/* \todo Get better estimate from UVC device, via StreamConfiguration. */

I'd write "Get a better image size estimate for MJPEG, via
StreamConfiguration, instead of using the worst-case width * height *
bpp of uncompressed data" or something along those lines. At least
mention image size, otherwise one may wonder what should be estimated.

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

> +	{ PixelFormat(DRM_FORMAT_MJPEG),	V4L2_PIX_FMT_MJPEG,	1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
>  }};
>  
>  } /* namespace */
Kieran Bingham June 8, 2020, 2:17 p.m. UTC | #2 
Hi Paul / Laurent,

On 08/06/2020 15:12, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Mon, Jun 08, 2020 at 05:05:29PM +0900, Paul Elder wrote:

<snip>

>>  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 +574,9 @@ 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 } }} },

Not related to this patch, but I only commenting here because I can see it:

Above I see NV24 and NV42 which I expect are nvSwap versions of each other.

The second plane for NV24 shows h/v subsampling of 2,1 where NV42 shows 1,1.

That looks suspicious to me.

Looking at the rest of the table, perhaps NV24 should be 16,1,1 too ?

--
Kieran
Laurent Pinchart June 8, 2020, 2:24 p.m. UTC | #3 
Hi Kieran,

On Mon, Jun 08, 2020 at 03:17:06PM +0100, Kieran Bingham wrote:
> Hi Paul / Laurent,
> 
> On 08/06/2020 15:12, Laurent Pinchart wrote:
> > Hi Paul,
> > 
> > Thank you for the patch.
> > 
> > On Mon, Jun 08, 2020 at 05:05:29PM +0900, Paul Elder wrote:
> 
> <snip>
> 
> >>  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 +574,9 @@ 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 } }} },
> 
> Not related to this patch, but I only commenting here because I can see it:
> 
> Above I see NV24 and NV42 which I expect are nvSwap versions of each other.
> 
> The second plane for NV24 shows h/v subsampling of 2,1 where NV42 shows 1,1.
> 
> That looks suspicious to me.
> 
> Looking at the rest of the table, perhaps NV24 should be 16,1,1 too ?

I think you're right. Wanna send a patch ?

Patch
diff mbox series 

diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
index 059f3cbe..07e746b4 100644
--- a/src/v4l2/v4l2_camera_proxy.cpp
+++ b/src/v4l2/v4l2_camera_proxy.cpp
@@ -358,8 +358,9 @@  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 the set format at the time of reqbufs has a
+	 * zero sizeimage we'll get a floating point exception when we try to
+	 * stream it.
 	 */
 	if (sizeimage_ == 0)
 		LOG(V4L2Compat, Warning)
@@ -556,7 +557,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 +574,9 @@  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. */
+	/* \todo Get better estimate from UVC device, via StreamConfiguration. */
+	{ PixelFormat(DRM_FORMAT_MJPEG),	V4L2_PIX_FMT_MJPEG,	1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
 }};
 
 } /* namespace */