| Message ID | 20200522071145.GA17375@kaaira-HP-Pavilion-Notebook (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: vimc: Apply right blue and red channels to BGR | expand |
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) >
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) > >
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)
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(-)