| Message ID | 20200831084645.20683-1-andrey.konovalov@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Andrey, On 31/08/2020 09:46, Andrey Konovalov wrote: > No interpolation is used to get more speed by the price of lower image > quality. In qcam format_converter is used for viewfinder only, and in > this case lower lag is more important than the image quality. > Great, I think this will be useful for early bring up of a new platform / sensor when we just want to 'see' the image as quickly as possible. We should really get more 'information' presented in Qcam regarding what processing is going on - so that it's clear to a viewer this is doing software debayering or such - but that's way out of scope for this patch. > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> > --- > Only SRGGB10P and SRGGB12P formats were tested (the ones I can get from > the camera sensor I am currently using) > > src/qcam/format_converter.cpp | 118 ++++++++++++++++++++++++++++++++++ > src/qcam/format_converter.h | 14 ++++ > 2 files changed, 132 insertions(+) > > diff --git a/src/qcam/format_converter.cpp b/src/qcam/format_converter.cpp > index 4b9722d..c9f94d3 100644 > --- a/src/qcam/format_converter.cpp > +++ b/src/qcam/format_converter.cpp > @@ -136,6 +136,62 @@ int FormatConverter::configure(const libcamera::PixelFormat &format, > formatFamily_ = MJPEG; > break; > > + case libcamera::formats::SRGGB10_CSI2P: > + formatFamily_ = RAW_CSI2P; > + r_pos_ = 0; > + bpp_numer_ = 4; > + bpp_denom_ = 5; > + break; > + > + case libcamera::formats::SGRBG10_CSI2P: > + formatFamily_ = RAW_CSI2P; > + r_pos_ = 1; > + bpp_numer_ = 4; > + bpp_denom_ = 5; > + break; > + > + case libcamera::formats::SGBRG10_CSI2P: > + formatFamily_ = RAW_CSI2P; > + r_pos_ = 2; > + bpp_numer_ = 4; > + bpp_denom_ = 5; > + break; > + > + case libcamera::formats::SBGGR10_CSI2P: > + formatFamily_ = RAW_CSI2P; > + r_pos_ = 3; > + bpp_numer_ = 4; > + bpp_denom_ = 5; > + break; > + > + case libcamera::formats::SRGGB12_CSI2P: > + formatFamily_ = RAW_CSI2P; > + r_pos_ = 0; > + bpp_numer_ = 2; > + bpp_denom_ = 3; > + break; > + > + case libcamera::formats::SGRBG12_CSI2P: > + formatFamily_ = RAW_CSI2P; > + r_pos_ = 1; > + bpp_numer_ = 2; > + bpp_denom_ = 3; > + break; > + > + case libcamera::formats::SGBRG12_CSI2P: > + formatFamily_ = RAW_CSI2P; > + r_pos_ = 2; > + bpp_numer_ = 2; > + bpp_denom_ = 3; > + break; > + > + case libcamera::formats::SBGGR12_CSI2P: > + formatFamily_ = RAW_CSI2P; > + r_pos_ = 3; > + bpp_numer_ = 2; > + bpp_denom_ = 3; > + break; > + > default: > return -EINVAL; > }; > @@ -163,9 +219,71 @@ void FormatConverter::convert(const unsigned char *src, size_t size, > case NV: > convertNV(src, dst->bits()); > break; > + case RAW_CSI2P: > + convertRAW_CSI2P(src, dst->bits()); > + break; > }; > } > > +/* > + * The pixels are processed in groups of 4 (2 by 2 squares), and the > + * assumption is that height_ and width_ are even numbers. Do we need an assertion, or round-down in here to make sure we don't overflow if the assumption is even numbers? (I can't imagine having an odd number on a bayer pattern, but with cropping or such perhaps it could happen - I don't know) Also - I wonder how cache-efficient it is processing like that. I'm not sure if there's a way to make it more efficient, but even if there was, that could be handled later and out of scope of this patch as this just gets new functionality all the same. > + */ > +void FormatConverter::convertRAW_CSI2P(const unsigned char *src, > + unsigned char *dst) > +{ > + unsigned int r_pos, b_pos, g1_pos, g2_pos; > + unsigned char r, g1, g2, b; > + unsigned int s_linelen = width_ * bpp_denom_ / bpp_numer_; > + unsigned int d_linelen = width_ * 4; > + > + /* > + * Calculate the offsets of the color values in the src buffer. > + * g1 is green value from the even (upper) line, g2 is the green > + * value from the odd (lower) line. > + */ > + if ( r_pos_ > 1) { > + r_pos = r_pos_ - 2 + s_linelen; > + b_pos = 3 - r_pos_; > + } else { > + r_pos = r_pos_; > + b_pos = 1 - r_pos_ + s_linelen; > + } > + g1_pos = (r_pos == 0 || b_pos == 0) ? 1 : 0; > + g2_pos = 1 - g1_pos + s_linelen; Could those calculations be done at configure time rather than per-frame? > + > + for (unsigned int y = 0; y < height_; y += 2) { > + for (unsigned x = 0; x < width_; x += bpp_numer_) { > + for (unsigned int i = 0; i < bpp_numer_ ; i += 2) { > + /* read the colors for the current 2x2 group: */ > + r = src[r_pos]; > + g1 = src[g1_pos]; > + g2 = src[g2_pos]; > + b = src[b_pos]; > + src += 2; > + /* two left pixels of the four: */ > + dst[0] = dst[0 + d_linelen] = b; > + dst[1] = g1; > + dst[1 + d_linelen] = g2; > + dst[2] = dst[2 + d_linelen] = r; > + dst[3] = dst[3 + d_linelen] = 0xff; > + dst += 4; > + /* two right pixels of the four: */ > + dst[0] = dst[0 + d_linelen] = b; > + dst[1] = g1; > + dst[1 + d_linelen] = g2; > + dst[2] = dst[2 + d_linelen] = r; > + dst[3] = dst[3 + d_linelen] = 0xff; > + dst += 4; > + } > + src += bpp_denom_ - bpp_numer_; > + } > + /* move to the next even line: */ > + src += s_linelen; > + dst += d_linelen; > + } > +} > + > static void yuv_to_rgb(int y, int u, int v, int *r, int *g, int *b) > { > int c = y - 16; > diff --git a/src/qcam/format_converter.h b/src/qcam/format_converter.h > index e389b24..5d4f31f 100644 > --- a/src/qcam/format_converter.h > +++ b/src/qcam/format_converter.h > @@ -26,11 +26,13 @@ private: > enum FormatFamily { > MJPEG, > NV, > + RAW_CSI2P, > RGB, > YUV, > }; > > void convertNV(const unsigned char *src, unsigned char *dst); > + void convertRAW_CSI2P(const unsigned char *src, unsigned char *dst); > void convertRGB(const unsigned char *src, unsigned char *dst); > void convertYUV(const unsigned char *src, unsigned char *dst); > > @@ -45,6 +47,18 @@ private: > unsigned int vertSubSample_; > bool nvSwap_; > > + /* RAW Bayer CSI2P parameters */ > + /* > + * Bytes per pixel is a fractional number, and is represented by > + * integer numerator and denominator. > + */ > + unsigned int bpp_numer_; > + unsigned int bpp_denom_; > + /* > + * Unsigned int r_pos_ from RGB parameters is reused; blue > + * and green positions are deduced from the red one. > + */ Hrm ... I suspect it might be nicer to put the different configurations into a union, rather than using a parameters of another configuration type. That would also make it neater to pre-compute and store the Bayer specific offsets too... -- Kieran > + > /* RGB parameters */ > unsigned int bpp_; > unsigned int r_pos_; >
Hi Kieran, Thank you for the review! On 31.08.2020 12:33, Kieran Bingham wrote: > Hi Andrey, > > On 31/08/2020 09:46, Andrey Konovalov wrote: >> No interpolation is used to get more speed by the price of lower image >> quality. In qcam format_converter is used for viewfinder only, and in >> this case lower lag is more important than the image quality. >> > > Great, I think this will be useful for early bring up of a new platform > / sensor when we just want to 'see' the image as quickly as possible. > > We should really get more 'information' presented in Qcam regarding what > processing is going on - so that it's clear to a viewer this is doing > software debayering or such - but that's way out of scope for this patch. > > > >> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> >> --- >> Only SRGGB10P and SRGGB12P formats were tested (the ones I can get from >> the camera sensor I am currently using) >> >> src/qcam/format_converter.cpp | 118 ++++++++++++++++++++++++++++++++++ >> src/qcam/format_converter.h | 14 ++++ >> 2 files changed, 132 insertions(+) >> >> diff --git a/src/qcam/format_converter.cpp b/src/qcam/format_converter.cpp >> index 4b9722d..c9f94d3 100644 >> --- a/src/qcam/format_converter.cpp >> +++ b/src/qcam/format_converter.cpp >> @@ -136,6 +136,62 @@ int FormatConverter::configure(const libcamera::PixelFormat &format, >> formatFamily_ = MJPEG; >> break; >> >> + case libcamera::formats::SRGGB10_CSI2P: >> + formatFamily_ = RAW_CSI2P; >> + r_pos_ = 0; >> + bpp_numer_ = 4; >> + bpp_denom_ = 5; >> + break; >> + >> + case libcamera::formats::SGRBG10_CSI2P: >> + formatFamily_ = RAW_CSI2P; >> + r_pos_ = 1; >> + bpp_numer_ = 4; >> + bpp_denom_ = 5; >> + break; >> + >> + case libcamera::formats::SGBRG10_CSI2P: >> + formatFamily_ = RAW_CSI2P; >> + r_pos_ = 2; >> + bpp_numer_ = 4; >> + bpp_denom_ = 5; >> + break; >> + >> + case libcamera::formats::SBGGR10_CSI2P: >> + formatFamily_ = RAW_CSI2P; >> + r_pos_ = 3; >> + bpp_numer_ = 4; >> + bpp_denom_ = 5; >> + break; >> + >> + case libcamera::formats::SRGGB12_CSI2P: >> + formatFamily_ = RAW_CSI2P; >> + r_pos_ = 0; >> + bpp_numer_ = 2; >> + bpp_denom_ = 3; >> + break; >> + >> + case libcamera::formats::SGRBG12_CSI2P: >> + formatFamily_ = RAW_CSI2P; >> + r_pos_ = 1; >> + bpp_numer_ = 2; >> + bpp_denom_ = 3; >> + break; >> + >> + case libcamera::formats::SGBRG12_CSI2P: >> + formatFamily_ = RAW_CSI2P; >> + r_pos_ = 2; >> + bpp_numer_ = 2; >> + bpp_denom_ = 3; >> + break; >> + >> + case libcamera::formats::SBGGR12_CSI2P: >> + formatFamily_ = RAW_CSI2P; >> + r_pos_ = 3; >> + bpp_numer_ = 2; >> + bpp_denom_ = 3; >> + break; >> + >> default: >> return -EINVAL; >> }; >> @@ -163,9 +219,71 @@ void FormatConverter::convert(const unsigned char *src, size_t size, >> case NV: >> convertNV(src, dst->bits()); >> break; >> + case RAW_CSI2P: >> + convertRAW_CSI2P(src, dst->bits()); >> + break; >> }; >> } >> >> +/* >> + * The pixels are processed in groups of 4 (2 by 2 squares), and the >> + * assumption is that height_ and width_ are even numbers. > > Do we need an assertion, or round-down in here to make sure we don't > overflow if the assumption is even numbers? Indeed, I need to handle this (if e.g. height_ is odd, the [height_] line would be accessed while (y < height_) in the "for" loop is still true). I would go with round-down and qWarning() (so that qcam would continue to work, with 1-pixel wide lines of random data at the leftmost and the lowest positions in the viewfinder window in the worst case). Will fix that in the next version of the patch. > (I can't imagine having an odd number on a bayer pattern, but with > cropping or such perhaps it could happen - I don't know) > > > Also - I wonder how cache-efficient it is processing like that. I'm not > sure if there's a way to make it more efficient, but even if there was, > that could be handled later and out of scope of this patch as this just > gets new functionality all the same. OK. I'll see if I could do something with that (not as part of this patch). >> + */ >> +void FormatConverter::convertRAW_CSI2P(const unsigned char *src, >> + unsigned char *dst) >> +{ >> + unsigned int r_pos, b_pos, g1_pos, g2_pos; >> + unsigned char r, g1, g2, b; >> + unsigned int s_linelen = width_ * bpp_denom_ / bpp_numer_; >> + unsigned int d_linelen = width_ * 4; >> + >> + /* >> + * Calculate the offsets of the color values in the src buffer. >> + * g1 is green value from the even (upper) line, g2 is the green >> + * value from the odd (lower) line. >> + */ >> + if ( r_pos_ > 1) { >> + r_pos = r_pos_ - 2 + s_linelen; >> + b_pos = 3 - r_pos_; >> + } else { >> + r_pos = r_pos_; >> + b_pos = 1 - r_pos_ + s_linelen; >> + } >> + g1_pos = (r_pos == 0 || b_pos == 0) ? 1 : 0; >> + g2_pos = 1 - g1_pos + s_linelen; > > Could those calculations be done at configure time rather than per-frame? Right, they could. My only concern is how fast using the class member vs (hopefully) a register as an array index is (src[r_pos_] vs src[r_pos]). I'll check, and if there is no impact on performance will fix that in the next version of the patch. >> + >> + for (unsigned int y = 0; y < height_; y += 2) { >> + for (unsigned x = 0; x < width_; x += bpp_numer_) { >> + for (unsigned int i = 0; i < bpp_numer_ ; i += 2) { >> + /* read the colors for the current 2x2 group: */ >> + r = src[r_pos]; >> + g1 = src[g1_pos]; >> + g2 = src[g2_pos]; >> + b = src[b_pos]; >> + src += 2; >> + /* two left pixels of the four: */ >> + dst[0] = dst[0 + d_linelen] = b; >> + dst[1] = g1; >> + dst[1 + d_linelen] = g2; >> + dst[2] = dst[2 + d_linelen] = r; >> + dst[3] = dst[3 + d_linelen] = 0xff; >> + dst += 4; >> + /* two right pixels of the four: */ >> + dst[0] = dst[0 + d_linelen] = b; >> + dst[1] = g1; >> + dst[1 + d_linelen] = g2; >> + dst[2] = dst[2 + d_linelen] = r; >> + dst[3] = dst[3 + d_linelen] = 0xff; >> + dst += 4; >> + } >> + src += bpp_denom_ - bpp_numer_; >> + } >> + /* move to the next even line: */ >> + src += s_linelen; >> + dst += d_linelen; >> + } >> +} >> + >> static void yuv_to_rgb(int y, int u, int v, int *r, int *g, int *b) >> { >> int c = y - 16; >> diff --git a/src/qcam/format_converter.h b/src/qcam/format_converter.h >> index e389b24..5d4f31f 100644 >> --- a/src/qcam/format_converter.h >> +++ b/src/qcam/format_converter.h >> @@ -26,11 +26,13 @@ private: >> enum FormatFamily { >> MJPEG, >> NV, >> + RAW_CSI2P, >> RGB, >> YUV, >> }; >> >> void convertNV(const unsigned char *src, unsigned char *dst); >> + void convertRAW_CSI2P(const unsigned char *src, unsigned char *dst); >> void convertRGB(const unsigned char *src, unsigned char *dst); >> void convertYUV(const unsigned char *src, unsigned char *dst); >> >> @@ -45,6 +47,18 @@ private: >> unsigned int vertSubSample_; >> bool nvSwap_; >> >> + /* RAW Bayer CSI2P parameters */ >> + /* >> + * Bytes per pixel is a fractional number, and is represented by >> + * integer numerator and denominator. >> + */ >> + unsigned int bpp_numer_; >> + unsigned int bpp_denom_; >> + /* >> + * Unsigned int r_pos_ from RGB parameters is reused; blue >> + * and green positions are deduced from the red one. >> + */ > > Hrm ... I suspect it might be nicer to put the different configurations > into a union, rather than using a parameters of another configuration type. I'd add a patch to put NV parameters, RGB parameters, and YUV parameters into the union first, and this "RAW Bayer CSI2P" patch would be the next in the series. Does it make sense? > That would also make it neater to pre-compute and store the Bayer > specific offsets too... Sounds good. Thanks, Andrey > -- > Kieran > > >> + >> /* RGB parameters */ >> unsigned int bpp_; >> unsigned int r_pos_; >> >
Hi Andrey, On Mon, Aug 31, 2020 at 02:23:22PM +0300, Andrey Konovalov wrote: > On 31.08.2020 12:33, Kieran Bingham wrote: > > On 31/08/2020 09:46, Andrey Konovalov wrote: > >> No interpolation is used to get more speed by the price of lower image > >> quality. In qcam format_converter is used for viewfinder only, and in > >> this case lower lag is more important than the image quality. > > > > Great, I think this will be useful for early bring up of a new platform > > / sensor when we just want to 'see' the image as quickly as possible. > > > > We should really get more 'information' presented in Qcam regarding what > > processing is going on - so that it's clear to a viewer this is doing > > software debayering or such - but that's way out of scope for this patch. > > > >> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> > >> --- > >> Only SRGGB10P and SRGGB12P formats were tested (the ones I can get from > >> the camera sensor I am currently using) > >> > >> src/qcam/format_converter.cpp | 118 ++++++++++++++++++++++++++++++++++ > >> src/qcam/format_converter.h | 14 ++++ > >> 2 files changed, 132 insertions(+) > >> > >> diff --git a/src/qcam/format_converter.cpp b/src/qcam/format_converter.cpp > >> index 4b9722d..c9f94d3 100644 > >> --- a/src/qcam/format_converter.cpp > >> +++ b/src/qcam/format_converter.cpp > >> @@ -136,6 +136,62 @@ int FormatConverter::configure(const libcamera::PixelFormat &format, > >> formatFamily_ = MJPEG; > >> break; > >> > >> + case libcamera::formats::SRGGB10_CSI2P: > >> + formatFamily_ = RAW_CSI2P; > >> + r_pos_ = 0; > >> + bpp_numer_ = 4; > >> + bpp_denom_ = 5; > >> + break; > >> + > >> + case libcamera::formats::SGRBG10_CSI2P: > >> + formatFamily_ = RAW_CSI2P; > >> + r_pos_ = 1; > >> + bpp_numer_ = 4; > >> + bpp_denom_ = 5; > >> + break; > >> + > >> + case libcamera::formats::SGBRG10_CSI2P: > >> + formatFamily_ = RAW_CSI2P; > >> + r_pos_ = 2; > >> + bpp_numer_ = 4; > >> + bpp_denom_ = 5; > >> + break; > >> + > >> + case libcamera::formats::SBGGR10_CSI2P: > >> + formatFamily_ = RAW_CSI2P; > >> + r_pos_ = 3; > >> + bpp_numer_ = 4; > >> + bpp_denom_ = 5; > >> + break; > >> + > >> + case libcamera::formats::SRGGB12_CSI2P: > >> + formatFamily_ = RAW_CSI2P; > >> + r_pos_ = 0; > >> + bpp_numer_ = 2; > >> + bpp_denom_ = 3; > >> + break; > >> + > >> + case libcamera::formats::SGRBG12_CSI2P: > >> + formatFamily_ = RAW_CSI2P; > >> + r_pos_ = 1; > >> + bpp_numer_ = 2; > >> + bpp_denom_ = 3; > >> + break; > >> + > >> + case libcamera::formats::SGBRG12_CSI2P: > >> + formatFamily_ = RAW_CSI2P; > >> + r_pos_ = 2; > >> + bpp_numer_ = 2; > >> + bpp_denom_ = 3; > >> + break; > >> + > >> + case libcamera::formats::SBGGR12_CSI2P: > >> + formatFamily_ = RAW_CSI2P; > >> + r_pos_ = 3; > >> + bpp_numer_ = 2; > >> + bpp_denom_ = 3; > >> + break; > >> + > >> default: > >> return -EINVAL; > >> }; > >> @@ -163,9 +219,71 @@ void FormatConverter::convert(const unsigned char *src, size_t size, > >> case NV: > >> convertNV(src, dst->bits()); > >> break; > >> + case RAW_CSI2P: > >> + convertRAW_CSI2P(src, dst->bits()); convertRawCSI2P ? > >> + break; > >> }; > >> } > >> > >> +/* > >> + * The pixels are processed in groups of 4 (2 by 2 squares), and the > >> + * assumption is that height_ and width_ are even numbers. > > > > Do we need an assertion, or round-down in here to make sure we don't > > overflow if the assumption is even numbers? > > Indeed, I need to handle this (if e.g. height_ is odd, the [height_] line > would be accessed while (y < height_) in the "for" loop is still true). > > I would go with round-down and qWarning() (so that qcam would continue to > work, with 1-pixel wide lines of random data at the leftmost and the lowest > positions in the viewfinder window in the worst case). > > Will fix that in the next version of the patch. It could be good if the warning was printed at configure time, to avoid repeating it for every frame. > > (I can't imagine having an odd number on a bayer pattern, but with > > cropping or such perhaps it could happen - I don't know) > > > > Also - I wonder how cache-efficient it is processing like that. I'm not > > sure if there's a way to make it more efficient, but even if there was, > > that could be handled later and out of scope of this patch as this just > > gets new functionality all the same. > > OK. I'll see if I could do something with that (not as part of this patch). A more cache-friendly implementation could indeed improv performances. > >> + */ > >> +void FormatConverter::convertRAW_CSI2P(const unsigned char *src, > >> + unsigned char *dst) > >> +{ > >> + unsigned int r_pos, b_pos, g1_pos, g2_pos; > >> + unsigned char r, g1, g2, b; > >> + unsigned int s_linelen = width_ * bpp_denom_ / bpp_numer_; src_linelen, or better, srcLineLength. > >> + unsigned int d_linelen = width_ * 4; And dst there. I think we need to take the stride into account, not just the width. That's true for all other formats too, so it doesn't need to be addressed in this patch. > >> + > >> + /* > >> + * Calculate the offsets of the color values in the src buffer. > >> + * g1 is green value from the even (upper) line, g2 is the green > >> + * value from the odd (lower) line. > >> + */ > >> + if ( r_pos_ > 1) { Extra space after ( > >> + r_pos = r_pos_ - 2 + s_linelen; > >> + b_pos = 3 - r_pos_; > >> + } else { > >> + r_pos = r_pos_; > >> + b_pos = 1 - r_pos_ + s_linelen; > >> + } > >> + g1_pos = (r_pos == 0 || b_pos == 0) ? 1 : 0; > >> + g2_pos = 1 - g1_pos + s_linelen; > > > > Could those calculations be done at configure time rather than per-frame? > > Right, they could. > My only concern is how fast using the class member vs (hopefully) a register > as an array index is (src[r_pos_] vs src[r_pos]). > I'll check, and if there is no impact on performance will fix that in the next > version of the patch. > > >> + > >> + for (unsigned int y = 0; y < height_; y += 2) { > >> + for (unsigned x = 0; x < width_; x += bpp_numer_) { > >> + for (unsigned int i = 0; i < bpp_numer_ ; i += 2) { > >> + /* read the colors for the current 2x2 group: */ > >> + r = src[r_pos]; > >> + g1 = src[g1_pos]; > >> + g2 = src[g2_pos]; I wonder if we shouldn't average the two green components. > >> + b = src[b_pos]; > >> + src += 2; > >> + /* two left pixels of the four: */ > >> + dst[0] = dst[0 + d_linelen] = b; > >> + dst[1] = g1; > >> + dst[1 + d_linelen] = g2; > >> + dst[2] = dst[2 + d_linelen] = r; > >> + dst[3] = dst[3 + d_linelen] = 0xff; > >> + dst += 4; > >> + /* two right pixels of the four: */ > >> + dst[0] = dst[0 + d_linelen] = b; > >> + dst[1] = g1; > >> + dst[1 + d_linelen] = g2; > >> + dst[2] = dst[2 + d_linelen] = r; > >> + dst[3] = dst[3 + d_linelen] = 0xff; > >> + dst += 4; > >> + } > >> + src += bpp_denom_ - bpp_numer_; > >> + } > >> + /* move to the next even line: */ > >> + src += s_linelen; > >> + dst += d_linelen; > >> + } > >> +} > >> + > >> static void yuv_to_rgb(int y, int u, int v, int *r, int *g, int *b) > >> { > >> int c = y - 16; > >> diff --git a/src/qcam/format_converter.h b/src/qcam/format_converter.h > >> index e389b24..5d4f31f 100644 > >> --- a/src/qcam/format_converter.h > >> +++ b/src/qcam/format_converter.h > >> @@ -26,11 +26,13 @@ private: > >> enum FormatFamily { > >> MJPEG, > >> NV, > >> + RAW_CSI2P, > >> RGB, > >> YUV, > >> }; > >> > >> void convertNV(const unsigned char *src, unsigned char *dst); > >> + void convertRAW_CSI2P(const unsigned char *src, unsigned char *dst); > >> void convertRGB(const unsigned char *src, unsigned char *dst); > >> void convertYUV(const unsigned char *src, unsigned char *dst); > >> > >> @@ -45,6 +47,18 @@ private: > >> unsigned int vertSubSample_; > >> bool nvSwap_; > >> > >> + /* RAW Bayer CSI2P parameters */ > >> + /* > >> + * Bytes per pixel is a fractional number, and is represented by > >> + * integer numerator and denominator. > >> + */ > >> + unsigned int bpp_numer_; > >> + unsigned int bpp_denom_; > >> + /* > >> + * Unsigned int r_pos_ from RGB parameters is reused; blue > >> + * and green positions are deduced from the red one. > >> + */ > > > > Hrm ... I suspect it might be nicer to put the different configurations > > into a union, rather than using a parameters of another configuration type. > > I'd add a patch to put NV parameters, RGB parameters, and YUV parameters > into the union first, and this "RAW Bayer CSI2P" patch would be the next > in the series. > Does it make sense? > > > That would also make it neater to pre-compute and store the Bayer > > specific offsets too... > > Sounds good. > > >> + > >> /* RGB parameters */ > >> unsigned int bpp_; > >> unsigned int r_pos_;
Hi Laurent, Thank you for the review! On 31.08.2020 17:52, Laurent Pinchart wrote: > Hi Andrey, > > On Mon, Aug 31, 2020 at 02:23:22PM +0300, Andrey Konovalov wrote: >> On 31.08.2020 12:33, Kieran Bingham wrote: >>> On 31/08/2020 09:46, Andrey Konovalov wrote: >>>> No interpolation is used to get more speed by the price of lower image >>>> quality. In qcam format_converter is used for viewfinder only, and in >>>> this case lower lag is more important than the image quality. >>> >>> Great, I think this will be useful for early bring up of a new platform >>> / sensor when we just want to 'see' the image as quickly as possible. >>> >>> We should really get more 'information' presented in Qcam regarding what >>> processing is going on - so that it's clear to a viewer this is doing >>> software debayering or such - but that's way out of scope for this patch. >>> >>>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> >>>> --- >>>> Only SRGGB10P and SRGGB12P formats were tested (the ones I can get from >>>> the camera sensor I am currently using) >>>> >>>> src/qcam/format_converter.cpp | 118 ++++++++++++++++++++++++++++++++++ >>>> src/qcam/format_converter.h | 14 ++++ >>>> 2 files changed, 132 insertions(+) >>>> >>>> diff --git a/src/qcam/format_converter.cpp b/src/qcam/format_converter.cpp >>>> index 4b9722d..c9f94d3 100644 >>>> --- a/src/qcam/format_converter.cpp >>>> +++ b/src/qcam/format_converter.cpp >>>> @@ -136,6 +136,62 @@ int FormatConverter::configure(const libcamera::PixelFormat &format, >>>> formatFamily_ = MJPEG; >>>> break; >>>> >>>> + case libcamera::formats::SRGGB10_CSI2P: >>>> + formatFamily_ = RAW_CSI2P; >>>> + r_pos_ = 0; >>>> + bpp_numer_ = 4; >>>> + bpp_denom_ = 5; >>>> + break; >>>> + >>>> + case libcamera::formats::SGRBG10_CSI2P: >>>> + formatFamily_ = RAW_CSI2P; >>>> + r_pos_ = 1; >>>> + bpp_numer_ = 4; >>>> + bpp_denom_ = 5; >>>> + break; >>>> + >>>> + case libcamera::formats::SGBRG10_CSI2P: >>>> + formatFamily_ = RAW_CSI2P; >>>> + r_pos_ = 2; >>>> + bpp_numer_ = 4; >>>> + bpp_denom_ = 5; >>>> + break; >>>> + >>>> + case libcamera::formats::SBGGR10_CSI2P: >>>> + formatFamily_ = RAW_CSI2P; >>>> + r_pos_ = 3; >>>> + bpp_numer_ = 4; >>>> + bpp_denom_ = 5; >>>> + break; >>>> + >>>> + case libcamera::formats::SRGGB12_CSI2P: >>>> + formatFamily_ = RAW_CSI2P; >>>> + r_pos_ = 0; >>>> + bpp_numer_ = 2; >>>> + bpp_denom_ = 3; >>>> + break; >>>> + >>>> + case libcamera::formats::SGRBG12_CSI2P: >>>> + formatFamily_ = RAW_CSI2P; >>>> + r_pos_ = 1; >>>> + bpp_numer_ = 2; >>>> + bpp_denom_ = 3; >>>> + break; >>>> + >>>> + case libcamera::formats::SGBRG12_CSI2P: >>>> + formatFamily_ = RAW_CSI2P; >>>> + r_pos_ = 2; >>>> + bpp_numer_ = 2; >>>> + bpp_denom_ = 3; >>>> + break; >>>> + >>>> + case libcamera::formats::SBGGR12_CSI2P: >>>> + formatFamily_ = RAW_CSI2P; >>>> + r_pos_ = 3; >>>> + bpp_numer_ = 2; >>>> + bpp_denom_ = 3; >>>> + break; >>>> + >>>> default: >>>> return -EINVAL; >>>> }; >>>> @@ -163,9 +219,71 @@ void FormatConverter::convert(const unsigned char *src, size_t size, >>>> case NV: >>>> convertNV(src, dst->bits()); >>>> break; >>>> + case RAW_CSI2P: >>>> + convertRAW_CSI2P(src, dst->bits()); > > convertRawCSI2P ? OK, will fix in v2 of the patch. >>>> + break; >>>> }; >>>> } >>>> >>>> +/* >>>> + * The pixels are processed in groups of 4 (2 by 2 squares), and the >>>> + * assumption is that height_ and width_ are even numbers. >>> >>> Do we need an assertion, or round-down in here to make sure we don't >>> overflow if the assumption is even numbers? >> >> Indeed, I need to handle this (if e.g. height_ is odd, the [height_] line >> would be accessed while (y < height_) in the "for" loop is still true). >> >> I would go with round-down and qWarning() (so that qcam would continue to >> work, with 1-pixel wide lines of random data at the leftmost and the lowest >> positions in the viewfinder window in the worst case). >> >> Will fix that in the next version of the patch. > > It could be good if the warning was printed at configure time, to avoid > repeating it for every frame. That's good point. Thanks! >>> (I can't imagine having an odd number on a bayer pattern, but with >>> cropping or such perhaps it could happen - I don't know) >>> >>> Also - I wonder how cache-efficient it is processing like that. I'm not >>> sure if there's a way to make it more efficient, but even if there was, >>> that could be handled later and out of scope of this patch as this just >>> gets new functionality all the same. >> >> OK. I'll see if I could do something with that (not as part of this patch). > > A more cache-friendly implementation could indeed improv performances. OK. Something for me to investigate >>>> + */ >>>> +void FormatConverter::convertRAW_CSI2P(const unsigned char *src, >>>> + unsigned char *dst) >>>> +{ >>>> + unsigned int r_pos, b_pos, g1_pos, g2_pos; >>>> + unsigned char r, g1, g2, b; >>>> + unsigned int s_linelen = width_ * bpp_denom_ / bpp_numer_; > > src_linelen, or better, srcLineLength. OK >>>> + unsigned int d_linelen = width_ * 4; > > And dst there. OK > I think we need to take the stride into account, not just the width. > That's true for all other formats too, so it doesn't need to be > addressed in this patch. OK, fine for me. >>>> + >>>> + /* >>>> + * Calculate the offsets of the color values in the src buffer. >>>> + * g1 is green value from the even (upper) line, g2 is the green >>>> + * value from the odd (lower) line. >>>> + */ >>>> + if ( r_pos_ > 1) { > > Extra space after ( Oops.. >>>> + r_pos = r_pos_ - 2 + s_linelen; >>>> + b_pos = 3 - r_pos_; >>>> + } else { >>>> + r_pos = r_pos_; >>>> + b_pos = 1 - r_pos_ + s_linelen; >>>> + } >>>> + g1_pos = (r_pos == 0 || b_pos == 0) ? 1 : 0; >>>> + g2_pos = 1 - g1_pos + s_linelen; >>> >>> Could those calculations be done at configure time rather than per-frame? >> >> Right, they could. >> My only concern is how fast using the class member vs (hopefully) a register >> as an array index is (src[r_pos_] vs src[r_pos]). >> I'll check, and if there is no impact on performance will fix that in the next >> version of the patch. >> >>>> + >>>> + for (unsigned int y = 0; y < height_; y += 2) { >>>> + for (unsigned x = 0; x < width_; x += bpp_numer_) { >>>> + for (unsigned int i = 0; i < bpp_numer_ ; i += 2) { >>>> + /* read the colors for the current 2x2 group: */ >>>> + r = src[r_pos]; >>>> + g1 = src[g1_pos]; >>>> + g2 = src[g2_pos]; > > I wonder if we shouldn't average the two green components. Probably. Just I don't have proper means other than naked eyes to check that at the moment. (E.g. if I had a more precise demosaic implementation - this is in plans too - I could just check which of the options gives the result closest to the output of the advanced algorithm. And I would need a good set of test images to make a valid choice.) Thanks, Andrey >>>> + b = src[b_pos]; >>>> + src += 2; >>>> + /* two left pixels of the four: */ >>>> + dst[0] = dst[0 + d_linelen] = b; >>>> + dst[1] = g1; >>>> + dst[1 + d_linelen] = g2; >>>> + dst[2] = dst[2 + d_linelen] = r; >>>> + dst[3] = dst[3 + d_linelen] = 0xff; >>>> + dst += 4; >>>> + /* two right pixels of the four: */ >>>> + dst[0] = dst[0 + d_linelen] = b; >>>> + dst[1] = g1; >>>> + dst[1 + d_linelen] = g2; >>>> + dst[2] = dst[2 + d_linelen] = r; >>>> + dst[3] = dst[3 + d_linelen] = 0xff; >>>> + dst += 4; >>>> + } >>>> + src += bpp_denom_ - bpp_numer_; >>>> + } >>>> + /* move to the next even line: */ >>>> + src += s_linelen; >>>> + dst += d_linelen; >>>> + } >>>> +} >>>> + >>>> static void yuv_to_rgb(int y, int u, int v, int *r, int *g, int *b) >>>> { >>>> int c = y - 16; >>>> diff --git a/src/qcam/format_converter.h b/src/qcam/format_converter.h >>>> index e389b24..5d4f31f 100644 >>>> --- a/src/qcam/format_converter.h >>>> +++ b/src/qcam/format_converter.h >>>> @@ -26,11 +26,13 @@ private: >>>> enum FormatFamily { >>>> MJPEG, >>>> NV, >>>> + RAW_CSI2P, >>>> RGB, >>>> YUV, >>>> }; >>>> >>>> void convertNV(const unsigned char *src, unsigned char *dst); >>>> + void convertRAW_CSI2P(const unsigned char *src, unsigned char *dst); >>>> void convertRGB(const unsigned char *src, unsigned char *dst); >>>> void convertYUV(const unsigned char *src, unsigned char *dst); >>>> >>>> @@ -45,6 +47,18 @@ private: >>>> unsigned int vertSubSample_; >>>> bool nvSwap_; >>>> >>>> + /* RAW Bayer CSI2P parameters */ >>>> + /* >>>> + * Bytes per pixel is a fractional number, and is represented by >>>> + * integer numerator and denominator. >>>> + */ >>>> + unsigned int bpp_numer_; >>>> + unsigned int bpp_denom_; >>>> + /* >>>> + * Unsigned int r_pos_ from RGB parameters is reused; blue >>>> + * and green positions are deduced from the red one. >>>> + */ >>> >>> Hrm ... I suspect it might be nicer to put the different configurations >>> into a union, rather than using a parameters of another configuration type. >> >> I'd add a patch to put NV parameters, RGB parameters, and YUV parameters >> into the union first, and this "RAW Bayer CSI2P" patch would be the next >> in the series. >> Does it make sense? >> >>> That would also make it neater to pre-compute and store the Bayer >>> specific offsets too... >> >> Sounds good. >> >>>> + >>>> /* RGB parameters */ >>>> unsigned int bpp_; >>>> unsigned int r_pos_; >
diff --git a/src/qcam/format_converter.cpp b/src/qcam/format_converter.cpp index 4b9722d..c9f94d3 100644 --- a/src/qcam/format_converter.cpp +++ b/src/qcam/format_converter.cpp @@ -136,6 +136,62 @@ int FormatConverter::configure(const libcamera::PixelFormat &format, formatFamily_ = MJPEG; break; + case libcamera::formats::SRGGB10_CSI2P: + formatFamily_ = RAW_CSI2P; + r_pos_ = 0; + bpp_numer_ = 4; + bpp_denom_ = 5; + break; + + case libcamera::formats::SGRBG10_CSI2P: + formatFamily_ = RAW_CSI2P; + r_pos_ = 1; + bpp_numer_ = 4; + bpp_denom_ = 5; + break; + + case libcamera::formats::SGBRG10_CSI2P: + formatFamily_ = RAW_CSI2P; + r_pos_ = 2; + bpp_numer_ = 4; + bpp_denom_ = 5; + break; + + case libcamera::formats::SBGGR10_CSI2P: + formatFamily_ = RAW_CSI2P; + r_pos_ = 3; + bpp_numer_ = 4; + bpp_denom_ = 5; + break; + + case libcamera::formats::SRGGB12_CSI2P: + formatFamily_ = RAW_CSI2P; + r_pos_ = 0; + bpp_numer_ = 2; + bpp_denom_ = 3; + break; + + case libcamera::formats::SGRBG12_CSI2P: + formatFamily_ = RAW_CSI2P; + r_pos_ = 1; + bpp_numer_ = 2; + bpp_denom_ = 3; + break; + + case libcamera::formats::SGBRG12_CSI2P: + formatFamily_ = RAW_CSI2P; + r_pos_ = 2; + bpp_numer_ = 2; + bpp_denom_ = 3; + break; + + case libcamera::formats::SBGGR12_CSI2P: + formatFamily_ = RAW_CSI2P; + r_pos_ = 3; + bpp_numer_ = 2; + bpp_denom_ = 3; + break; + default: return -EINVAL; }; @@ -163,9 +219,71 @@ void FormatConverter::convert(const unsigned char *src, size_t size, case NV: convertNV(src, dst->bits()); break; + case RAW_CSI2P: + convertRAW_CSI2P(src, dst->bits()); + break; }; } +/* + * The pixels are processed in groups of 4 (2 by 2 squares), and the + * assumption is that height_ and width_ are even numbers. + */ +void FormatConverter::convertRAW_CSI2P(const unsigned char *src, + unsigned char *dst) +{ + unsigned int r_pos, b_pos, g1_pos, g2_pos; + unsigned char r, g1, g2, b; + unsigned int s_linelen = width_ * bpp_denom_ / bpp_numer_; + unsigned int d_linelen = width_ * 4; + + /* + * Calculate the offsets of the color values in the src buffer. + * g1 is green value from the even (upper) line, g2 is the green + * value from the odd (lower) line. + */ + if ( r_pos_ > 1) { + r_pos = r_pos_ - 2 + s_linelen; + b_pos = 3 - r_pos_; + } else { + r_pos = r_pos_; + b_pos = 1 - r_pos_ + s_linelen; + } + g1_pos = (r_pos == 0 || b_pos == 0) ? 1 : 0; + g2_pos = 1 - g1_pos + s_linelen; + + for (unsigned int y = 0; y < height_; y += 2) { + for (unsigned x = 0; x < width_; x += bpp_numer_) { + for (unsigned int i = 0; i < bpp_numer_ ; i += 2) { + /* read the colors for the current 2x2 group: */ + r = src[r_pos]; + g1 = src[g1_pos]; + g2 = src[g2_pos]; + b = src[b_pos]; + src += 2; + /* two left pixels of the four: */ + dst[0] = dst[0 + d_linelen] = b; + dst[1] = g1; + dst[1 + d_linelen] = g2; + dst[2] = dst[2 + d_linelen] = r; + dst[3] = dst[3 + d_linelen] = 0xff; + dst += 4; + /* two right pixels of the four: */ + dst[0] = dst[0 + d_linelen] = b; + dst[1] = g1; + dst[1 + d_linelen] = g2; + dst[2] = dst[2 + d_linelen] = r; + dst[3] = dst[3 + d_linelen] = 0xff; + dst += 4; + } + src += bpp_denom_ - bpp_numer_; + } + /* move to the next even line: */ + src += s_linelen; + dst += d_linelen; + } +} + static void yuv_to_rgb(int y, int u, int v, int *r, int *g, int *b) { int c = y - 16; diff --git a/src/qcam/format_converter.h b/src/qcam/format_converter.h index e389b24..5d4f31f 100644 --- a/src/qcam/format_converter.h +++ b/src/qcam/format_converter.h @@ -26,11 +26,13 @@ private: enum FormatFamily { MJPEG, NV, + RAW_CSI2P, RGB, YUV, }; void convertNV(const unsigned char *src, unsigned char *dst); + void convertRAW_CSI2P(const unsigned char *src, unsigned char *dst); void convertRGB(const unsigned char *src, unsigned char *dst); void convertYUV(const unsigned char *src, unsigned char *dst); @@ -45,6 +47,18 @@ private: unsigned int vertSubSample_; bool nvSwap_; + /* RAW Bayer CSI2P parameters */ + /* + * Bytes per pixel is a fractional number, and is represented by + * integer numerator and denominator. + */ + unsigned int bpp_numer_; + unsigned int bpp_denom_; + /* + * Unsigned int r_pos_ from RGB parameters is reused; blue + * and green positions are deduced from the red one. + */ + /* RGB parameters */ unsigned int bpp_; unsigned int r_pos_;
No interpolation is used to get more speed by the price of lower image quality. In qcam format_converter is used for viewfinder only, and in this case lower lag is more important than the image quality. Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> --- Only SRGGB10P and SRGGB12P formats were tested (the ones I can get from the camera sensor I am currently using) src/qcam/format_converter.cpp | 118 ++++++++++++++++++++++++++++++++++ src/qcam/format_converter.h | 14 ++++ 2 files changed, 132 insertions(+)