Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] media: vimc: Apply right blue and red channels to BGR
@ 2020-05-22  7:11 Kaaira Gupta
  2020-05-22 14:15 ` Helen Koike
  0 siblings, 1 reply; 3+ messages in thread
From: Kaaira Gupta @ 2020-05-22  7:11 UTC (permalink / raw)
  To: linux-media, Kieran Bingham

rgb[] is already calculated in the right order, there is no need to swap
the blue and red channels in it for BGR images. Hence give right rgb
channels to the src_frame.

Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
---
 drivers/media/test-drivers/vimc/vimc-debayer.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/media/test-drivers/vimc/vimc-debayer.c b/drivers/media/test-drivers/vimc/vimc-debayer.c
index c3f6fef34f68..d41d9f6180df 100644
--- a/drivers/media/test-drivers/vimc/vimc-debayer.c
+++ b/drivers/media/test-drivers/vimc/vimc-debayer.c
@@ -318,21 +318,11 @@ static void vimc_deb_process_rgb_frame(struct vimc_deb_device *vdeb,
 				       unsigned int col,
 				       unsigned int rgb[3])
 {
-	const struct vimc_pix_map *vpix;
 	unsigned int i, index;
 
-	vpix = vimc_pix_map_by_code(vdeb->src_code);
 	index = VIMC_FRAME_INDEX(lin, col, vdeb->sink_fmt.width, 3);
-	for (i = 0; i < 3; i++) {
-		switch (vpix->pixelformat) {
-		case V4L2_PIX_FMT_RGB24:
-			vdeb->src_frame[index + i] = rgb[i];
-			break;
-		case V4L2_PIX_FMT_BGR24:
-			vdeb->src_frame[index + i] = rgb[2 - i];
-			break;
-		}
-	}
+	for (i = 0; i < 3; i++)
+		vdeb->src_frame[index + i] = rgb[i];
 }
 
 static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable)
-- 
2.17.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] media: vimc: Apply right blue and red channels to BGR
  2020-05-22  7:11 [PATCH] media: vimc: Apply right blue and red channels to BGR Kaaira Gupta
@ 2020-05-22 14:15 ` Helen Koike
  2020-05-22 15:27   ` Kaaira Gupta
  0 siblings, 1 reply; 3+ messages in thread
From: Helen Koike @ 2020-05-22 14:15 UTC (permalink / raw)
  To: Kaaira Gupta, linux-media, Kieran Bingham

Hi Kaaira,

Thanks for the patch.

On 5/22/20 4:11 AM, Kaaira Gupta wrote:
> rgb[] is already calculated in the right order, there is no need to swap
> the blue and red channels in it for BGR images. Hence give right rgb
> channels to the src_frame.

It would be nice if you explain the issue you are facing, and what this fixes.

> 
> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> ---
>  drivers/media/test-drivers/vimc/vimc-debayer.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/test-drivers/vimc/vimc-debayer.c b/drivers/media/test-drivers/vimc/vimc-debayer.c
> index c3f6fef34f68..d41d9f6180df 100644
> --- a/drivers/media/test-drivers/vimc/vimc-debayer.c
> +++ b/drivers/media/test-drivers/vimc/vimc-debayer.c
> @@ -318,21 +318,11 @@ static void vimc_deb_process_rgb_frame(struct vimc_deb_device *vdeb,
>  				       unsigned int col,
>  				       unsigned int rgb[3])
>  {
> -	const struct vimc_pix_map *vpix;
>  	unsigned int i, index;
>  
> -	vpix = vimc_pix_map_by_code(vdeb->src_code);
>  	index = VIMC_FRAME_INDEX(lin, col, vdeb->sink_fmt.width, 3);
> -	for (i = 0; i < 3; i++) {
> -		switch (vpix->pixelformat) {
> -		case V4L2_PIX_FMT_RGB24:
> -			vdeb->src_frame[index + i] = rgb[i];
> -			break;
> -		case V4L2_PIX_FMT_BGR24:
> -			vdeb->src_frame[index + i] = rgb[2 - i];
> -			break;
> -		}
> -	}

This code looks correct to me.

The rgb[] arrays is an intermediary representation of the pixel, in the order of Red Green Blue.

If you look at vimc_deb_calc_rgb_sink(), you will see that rgb[] is indexed by this enum:

enum vimc_deb_rgb_colors {
	VIMC_DEB_RED = 0,
	VIMC_DEB_GREEN = 1,
	VIMC_DEB_BLUE = 2,
};

So rgb[0] is the red component, rgb[1] green and rgb[2] blue.

The, in this part of the code that you removed, we use rgb[] array to calculate the final frame.

So, if there is an error, then I don't think it is here.
Maybe the rgb[] array is wrong, and the error would be somewhere in vimc_deb_calc_rgb_sink().

Regards,
Helen


> +	for (i = 0; i < 3; i++)
> +		vdeb->src_frame[index + i] = rgb[i];
>  }
>  
>  static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable)
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread

* [PATCH] media: vimc: Apply right blue and red channels to BGR
  2020-05-22 14:15 ` Helen Koike
@ 2020-05-22 15:27   ` Kaaira Gupta
  0 siblings, 0 replies; 3+ messages in thread
From: Kaaira Gupta @ 2020-05-22 15:27 UTC (permalink / raw)
  To: Helen Koike; +Cc: Kaaira Gupta, linux-media, Kieran Bingham

On Fri, May 22, 2020 at 11:15:21AM -0300, Helen Koike wrote:
> Hi Kaaira,

Hii!

> 
> Thanks for the patch.
> 
> On 5/22/20 4:11 AM, Kaaira Gupta wrote:
> > rgb[] is already calculated in the right order, there is no need to swap
> > the blue and red channels in it for BGR images. Hence give right rgb
> > channels to the src_frame.
> 
> It would be nice if you explain the issue you are facing, and what this fixes.

Sure. So libcamera's qcam configures the Capture to BGR for certain qt versions
(<5.14). So when we test qcam using vimc (by configuring the subdevs
from qcam side to BGR as well), we see that the red and blue channels on
the test image get inter-changed.This is a fix for that.

Should I describe this in the commit message as well?

> 
> > 
> > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> > ---
> >  drivers/media/test-drivers/vimc/vimc-debayer.c | 14 ++------------
> >  1 file changed, 2 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/media/test-drivers/vimc/vimc-debayer.c b/drivers/media/test-drivers/vimc/vimc-debayer.c
> > index c3f6fef34f68..d41d9f6180df 100644
> > --- a/drivers/media/test-drivers/vimc/vimc-debayer.c
> > +++ b/drivers/media/test-drivers/vimc/vimc-debayer.c
> > @@ -318,21 +318,11 @@ static void vimc_deb_process_rgb_frame(struct vimc_deb_device *vdeb,
> >  				       unsigned int col,
> >  				       unsigned int rgb[3])
> >  {
> > -	const struct vimc_pix_map *vpix;
> >  	unsigned int i, index;
> >  
> > -	vpix = vimc_pix_map_by_code(vdeb->src_code);
> >  	index = VIMC_FRAME_INDEX(lin, col, vdeb->sink_fmt.width, 3);
> > -	for (i = 0; i < 3; i++) {
> > -		switch (vpix->pixelformat) {
> > -		case V4L2_PIX_FMT_RGB24:
> > -			vdeb->src_frame[index + i] = rgb[i];
> > -			break;
> > -		case V4L2_PIX_FMT_BGR24:
> > -			vdeb->src_frame[index + i] = rgb[2 - i];
> > -			break;
> > -		}
> > -	}
> 
> This code looks correct to me.
> 
> The rgb[] arrays is an intermediary representation of the pixel, in the order of Red Green Blue.
> 
> If you look at vimc_deb_calc_rgb_sink(), you will see that rgb[] is indexed by this enum:
> 
> enum vimc_deb_rgb_colors {
> 	VIMC_DEB_RED = 0,
> 	VIMC_DEB_GREEN = 1,
> 	VIMC_DEB_BLUE = 2,
> };
> 
> So rgb[0] is the red component, rgb[1] green and rgb[2] blue.
> 
> The, in this part of the code that you removed, we use rgb[] array to calculate the final frame.
> 
> So, if there is an error, then I don't think it is here.
> Maybe the rgb[] array is wrong, and the error would be somewhere in vimc_deb_calc_rgb_sink().

I printed out the rbg[] array, and it was correctly calculating the r,g,
and b components. I think the channels should not be swapped while
assigning to src_frame, because for all the cases (BGR or RGB) it takes
input in the order of R,then G and finally B. Hence when we swap the
assignment to the frame, we are interchanging the channels.

> 
> Regards,
> Helen
> 
> 
> > +	for (i = 0; i < 3; i++)
> > +		vdeb->src_frame[index + i] = rgb[i];
> >  }
> >  
> >  static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable)
> > 

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-22  7:11 [PATCH] media: vimc: Apply right blue and red channels to BGR Kaaira Gupta
2020-05-22 14:15 ` Helen Koike
2020-05-22 15:27   ` Kaaira Gupta

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git