[PATCH] libcamera: formats: Change bytesPerGroup of RGB16 from 3 to 2

[Laurent Pinchart]

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