libcamera: formats: Change bytesPerGroup of RGB16 from 3 to 2
diff mbox series

Message ID 20241025033125.2275054-1-qi.hou@nxp.com
State Accepted
Commit ff069d87e263cd41f9d5d74774609c8b0ecbcb2a
Headers show
Series
  • libcamera: formats: Change bytesPerGroup of RGB16 from 3 to 2
Related show

Commit Message

Qi Hou Oct. 25, 2024, 3:31 a.m. UTC 
Change the bytesPerGroup in plane[0] of RGB16 format from 3 to 2,
otherwise calculated stride using below formula will be incorrect.

/* ceil(width / pixelsPerGroup) * bytesPerGroup */
unsigned int stride = (width + pixelsPerGroup - 1) / pixelsPerGroup
	    * planes[plane].bytesPerGroup;
---
 src/libcamera/formats.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Jacopo Mondi Oct. 25, 2024, 8:40 a.m. UTC | #1 
Hi Hou Qi,
  thanks for the patch

in subject: s/RGB16/RGB565/

On Fri, Oct 25, 2024 at 12:31:25PM +0900, Hou Qi wrote:
> Change the bytesPerGroup in plane[0] of RGB16 format from 3 to 2,

Same here, let's mention RGB565 and RGB565_BE formats explicitly

> otherwise calculated stride using below formula will be incorrect.
>
> /* ceil(width / pixelsPerGroup) * bytesPerGroup */
> unsigned int stride = (width + pixelsPerGroup - 1) / pixelsPerGroup
> 	    * planes[plane].bytesPerGroup;
> ---
>  src/libcamera/formats.cpp | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> index dbefb094..bfcdfc08 100644
> --- a/src/libcamera/formats.cpp
> +++ b/src/libcamera/formats.cpp
> @@ -157,7 +157,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.colourEncoding = PixelFormatInfo::ColourEncodingRGB,
>  		.packed = false,
>  		.pixelsPerGroup = 1,
> -		.planes = {{ { 3, 1 }, { 0, 0 }, { 0, 0 } }},
> +		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},

RGB565 is indeed 2 bytes per pixel group, unless I'm missing something
really obvious, as it seems weird this went unnoticed. Maybe RGB565 is
not that popular ?

Anyway, to me this looks correct
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

>  	} },
>  	{ formats::RGB565_BE, {
>  		.name = "RGB565_BE",
> @@ -167,7 +167,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.colourEncoding = PixelFormatInfo::ColourEncodingRGB,
>  		.packed = false,
>  		.pixelsPerGroup = 1,
> -		.planes = {{ { 3, 1 }, { 0, 0 }, { 0, 0 } }},
> +		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
>  	} },
>  	{ formats::BGR888, {
>  		.name = "BGR888",
> --
> 2.34.1
>
Laurent Pinchart Oct. 27, 2024, 6:02 p.m. UTC | #2 
On Fri, Oct 25, 2024 at 10:40:45AM +0200, Jacopo Mondi wrote:
> Hi Hou Qi,
>   thanks for the patch
> 
> in subject: s/RGB16/RGB565/
> 
> On Fri, Oct 25, 2024 at 12:31:25PM +0900, Hou Qi wrote:
> > Change the bytesPerGroup in plane[0] of RGB16 format from 3 to 2,
> 
> Same here, let's mention RGB565 and RGB565_BE formats explicitly

You could write

The RGB565 and RGB565_BE formats incorrectly specify a wrong value of 3
bytes per group of pixels, when they actually use 2. Fix them.

or something like that.

> > otherwise calculated stride using below formula will be incorrect.
> >
> > /* ceil(width / pixelsPerGroup) * bytesPerGroup */
> > unsigned int stride = (width + pixelsPerGroup - 1) / pixelsPerGroup
> > 	    * planes[plane].bytesPerGroup;

Your Signed-off-by line is missing. See
https://libcamera.org/contributing.html#submitting-patches

> > ---
> >  src/libcamera/formats.cpp | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > index dbefb094..bfcdfc08 100644
> > --- a/src/libcamera/formats.cpp
> > +++ b/src/libcamera/formats.cpp
> > @@ -157,7 +157,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRGB,
> >  		.packed = false,
> >  		.pixelsPerGroup = 1,
> > -		.planes = {{ { 3, 1 }, { 0, 0 }, { 0, 0 } }},
> > +		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> 
> RGB565 is indeed 2 bytes per pixel group, unless I'm missing something
> really obvious, as it seems weird this went unnoticed. Maybe RGB565 is
> not that popular ?
> 
> Anyway, to me this looks correct
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

The change looks fine. I could update the subject and commit message
myself, but I can't add your Signed-off-by line. Could you send a v2 ?

> >  	} },
> >  	{ formats::RGB565_BE, {
> >  		.name = "RGB565_BE",
> > @@ -167,7 +167,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRGB,
> >  		.packed = false,
> >  		.pixelsPerGroup = 1,
> > -		.planes = {{ { 3, 1 }, { 0, 0 }, { 0, 0 } }},
> > +		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> >  	} },
> >  	{ formats::BGR888, {
> >  		.name = "BGR888",
Qi Hou Oct. 28, 2024, 3:06 a.m. UTC | #3 
Hi Laurent Pinchart and Jacopo Mondi

Thank you for your review, I have sent out the v2.

Regards,
Qi Hou

-----Original Message-----
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Sent: 2024年10月28日 2:03
To: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Cc: Qi Hou <qi.hou@nxp.com>; libcamera-devel@lists.libcamera.org; Jared Hu <jared.hu@nxp.com>; Julien Vuillaumier <julien.vuillaumier@nxp.com>
Subject: [EXT] Re: [PATCH] libcamera: formats: Change bytesPerGroup of RGB16 from 3 to 2

Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button


On Fri, Oct 25, 2024 at 10:40:45AM +0200, Jacopo Mondi wrote:
> Hi Hou Qi,
>   thanks for the patch
>
> in subject: s/RGB16/RGB565/
>
> On Fri, Oct 25, 2024 at 12:31:25PM +0900, Hou Qi wrote:
> > Change the bytesPerGroup in plane[0] of RGB16 format from 3 to 2,
>
> Same here, let's mention RGB565 and RGB565_BE formats explicitly

You could write

The RGB565 and RGB565_BE formats incorrectly specify a wrong value of 3 bytes per group of pixels, when they actually use 2. Fix them.

or something like that.

> > otherwise calculated stride using below formula will be incorrect.
> >
> > /* ceil(width / pixelsPerGroup) * bytesPerGroup */ unsigned int
> > stride = (width + pixelsPerGroup - 1) / pixelsPerGroup
> >         * planes[plane].bytesPerGroup;

Your Signed-off-by line is missing. See
https://libcamera.org/contributing.html#submitting-patches

> > ---
> >  src/libcamera/formats.cpp | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > index dbefb094..bfcdfc08 100644
> > --- a/src/libcamera/formats.cpp
> > +++ b/src/libcamera/formats.cpp
> > @@ -157,7 +157,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >             .colourEncoding = PixelFormatInfo::ColourEncodingRGB,
> >             .packed = false,
> >             .pixelsPerGroup = 1,
> > -           .planes = {{ { 3, 1 }, { 0, 0 }, { 0, 0 } }},
> > +           .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
>
> RGB565 is indeed 2 bytes per pixel group, unless I'm missing something
> really obvious, as it seems weird this went unnoticed. Maybe RGB565 is
> not that popular ?
>
> Anyway, to me this looks correct
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

The change looks fine. I could update the subject and commit message myself, but I can't add your Signed-off-by line. Could you send a v2 ?

> >     } },
> >     { formats::RGB565_BE, {
> >             .name = "RGB565_BE",
> > @@ -167,7 +167,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >             .colourEncoding = PixelFormatInfo::ColourEncodingRGB,
> >             .packed = false,
> >             .pixelsPerGroup = 1,
> > -           .planes = {{ { 3, 1 }, { 0, 0 }, { 0, 0 } }},
> > +           .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> >     } },
> >     { formats::BGR888, {
> >             .name = "BGR888",

--
Regards,

Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
index dbefb094..bfcdfc08 100644
--- a/src/libcamera/formats.cpp
+++ b/src/libcamera/formats.cpp
@@ -157,7 +157,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.colourEncoding = PixelFormatInfo::ColourEncodingRGB,
 		.packed = false,
 		.pixelsPerGroup = 1,
-		.planes = {{ { 3, 1 }, { 0, 0 }, { 0, 0 } }},
+		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
 	} },
 	{ formats::RGB565_BE, {
 		.name = "RGB565_BE",
@@ -167,7 +167,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.colourEncoding = PixelFormatInfo::ColourEncodingRGB,
 		.packed = false,
 		.pixelsPerGroup = 1,
-		.planes = {{ { 3, 1 }, { 0, 0 }, { 0, 0 } }},
+		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
 	} },
 	{ formats::BGR888, {
 		.name = "BGR888",