| Message ID | 20200908150739.1552-3-andrey.konovalov@linaro.org |
|---|---|
| State | Superseded |
| Delegated to: | Laurent Pinchart |
| Headers | show |
| Series |
|
| Related | show |
Hi Andrey, Thank you for the patch. On Tue, Sep 08, 2020 at 06:07:39PM +0300, 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. > > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> > --- > src/qcam/format_converter.cpp | 138 ++++++++++++++++++++++++++++++++++ > src/qcam/format_converter.h | 21 ++++++ > 2 files changed, 159 insertions(+) > > diff --git a/src/qcam/format_converter.cpp b/src/qcam/format_converter.cpp > index 3481330..f370907 100644 > --- a/src/qcam/format_converter.cpp > +++ b/src/qcam/format_converter.cpp > @@ -10,6 +10,7 @@ > #include <errno.h> > > #include <QImage> > +#include <QtDebug> > > #include <libcamera/formats.h> > > @@ -132,6 +133,62 @@ int FormatConverter::configure(const libcamera::PixelFormat &format, > params_.yuv.cb_pos = 1; > break; > > + case libcamera::formats::SRGGB10_CSI2P: > + formatFamily_ = RAW_CSI2P; > + params_.rawp.bpp_numer = 5; /* 1.25 bytes per pixel */ > + params_.rawp.bpp_denom = 4; > + params_.rawp.r_pos = 0; > + break; > + There are no blank lines between formats of the same group for other groups, I'd follow the same pattern here. > + case libcamera::formats::SGRBG10_CSI2P: > + formatFamily_ = RAW_CSI2P; > + params_.rawp.bpp_numer = 5; > + params_.rawp.bpp_denom = 4; > + params_.rawp.r_pos = 1; > + break; > + > + case libcamera::formats::SGBRG10_CSI2P: > + formatFamily_ = RAW_CSI2P; > + params_.rawp.bpp_numer = 5; > + params_.rawp.bpp_denom = 4; > + params_.rawp.r_pos = 2; > + break; > + > + case libcamera::formats::SBGGR10_CSI2P: > + formatFamily_ = RAW_CSI2P; > + params_.rawp.bpp_numer = 5; > + params_.rawp.bpp_denom = 4; > + params_.rawp.r_pos = 3; > + break; > + > + case libcamera::formats::SRGGB12_CSI2P: > + formatFamily_ = RAW_CSI2P; > + params_.rawp.bpp_numer = 3; /* 1.5 bytes per pixel */ > + params_.rawp.bpp_denom = 2; > + params_.rawp.r_pos = 0; > + break; > + > + case libcamera::formats::SGRBG12_CSI2P: > + formatFamily_ = RAW_CSI2P; > + params_.rawp.bpp_numer = 3; > + params_.rawp.bpp_denom = 2; > + params_.rawp.r_pos = 1; > + break; > + > + case libcamera::formats::SGBRG12_CSI2P: > + formatFamily_ = RAW_CSI2P; > + params_.rawp.bpp_numer = 3; > + params_.rawp.bpp_denom = 2; > + params_.rawp.r_pos = 2; > + break; > + > + case libcamera::formats::SBGGR12_CSI2P: > + formatFamily_ = RAW_CSI2P; > + params_.rawp.bpp_numer = 3; > + params_.rawp.bpp_denom = 2; > + params_.rawp.r_pos = 3; > + break; > + > case libcamera::formats::MJPEG: > formatFamily_ = MJPEG; > break; > @@ -144,6 +201,45 @@ int FormatConverter::configure(const libcamera::PixelFormat &format, > width_ = size.width(); > height_ = size.height(); > > + if (formatFamily_ == RAW_CSI2P) { > + /* > + * For RAW_CSI2P the assumption is that height_ and width_ > + * are even numbers. > + */ > + if ( width_ % 2 != 0 || height_ % 2 != 0 ) { No space after ( and before ). You can also drop the != 0. > + qWarning() << "Image width or height isn't even number"; > + return -EINVAL; > + } > + > + params_.rawp.srcLineLength = width_ * params_.rawp.bpp_numer / > + params_.rawp.bpp_denom; > + > + /* > + * Calculate [g1,g2,b]_pos based on r_pos. > + * On entrance, r_pos is the position of red pixel in the s/entrace/entry/ (Kieran can correct me if this isn't right) s/red pixel/the red pixel/ > + * 2-by-2 pixel Bayer pattern, and is from 0 to 3 range: "is in the 0 to 3 range" or "is in the [0, 3] range" > + * +---+---+ > + * | 0 | 1 | > + * +---+---+ > + * | 2 | 3 | > + * +---+---+ > + * At exit, [r,g1,g2,b]_pos are offsetts of the color s/offsetts/the offsets/ > + * values in the source buffer. > + */ > + if (params_.rawp.r_pos > 1) { > + params_.rawp.r_pos = params_.rawp.r_pos - 2 + > + params_.rawp.srcLineLength; > + params_.rawp.b_pos = 3 - params_.rawp.r_pos; > + } else { > + params_.rawp.b_pos = 1 - params_.rawp.r_pos + > + params_.rawp.srcLineLength; > + } > + params_.rawp.g1_pos = (params_.rawp.r_pos == 0 || > + params_.rawp.b_pos == 0) ? 1 : 0; > + params_.rawp.g2_pos = 1 - params_.rawp.g1_pos + > + params_.rawp.srcLineLength; > + } > + > return 0; > } > > @@ -163,9 +259,51 @@ void FormatConverter::convert(const unsigned char *src, size_t size, > case NV: > convertNV(src, dst->bits()); > break; > + case RAW_CSI2P: > + convertRawCSI2P(src, dst->bits()); > + break; > }; > } > > +void FormatConverter::convertRawCSI2P(const unsigned char *src, > + unsigned char *dst) > +{ > + unsigned int g; > + static unsigned char dst_buf[8] = { 0, 0, 0, 0xff }; > + unsigned int dstLineLength = width_ * 4; > + > + for (unsigned int y = 0; y < height_; y += 2) { > + for (unsigned x = 0; x < width_; x += params_.rawp.bpp_denom) { > + for (unsigned int i = 0; i < params_.rawp.bpp_denom ; s/ ;/;/ > + i += 2) { I wouldn't wrap this to the next line, it looks weird. > + /* > + * Process the current 2x2 group. > + * Use the average of the two green pixels as > + * the green value for all the pixels in the > + * group. > + */ > + dst_buf[0] = src[params_.rawp.b_pos]; > + g = src[params_.rawp.g1_pos]; > + g = (g + src[params_.rawp.g2_pos]) >> 1; > + dst_buf[1] = (unsigned char)g; > + dst_buf[2] = src[params_.rawp.r_pos]; > + src += 2; > + > + memcpy(dst_buf + 4, dst_buf, 4); > + memcpy(dst, dst_buf, 8); > + memcpy(dst + dstLineLength, dst_buf, 8); The memcpy() calls look really awkward. I suppose this is what you've mentioned as a performance improvement in the cover letter ? I'm concerned someone will forget about it in the future and refactor this code. We need at least a comment that explains what's going on. I wonder if you won't get better performances from splitting this function in two, and unrolling the inner loop for the 10bpp case. > + dst += 8; > + } > + src += params_.rawp.bpp_numer - params_.rawp.bpp_denom; > + } Can you add a blank line here ? > + /* odd lines are copies of the even lines they follow: */ s/odd/Odd/ s/follow:/follow/. > + memcpy(dst, dst-dstLineLength, dstLineLength); Blank line here too. > + /* move to the next even line: */ s/move/Move/ s/line:/line./ > + src += params_.rawp.srcLineLength; > + dst += dstLineLength; > + } > +} > + > 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 c53fa28..dc20e0d 100644 > --- a/src/qcam/format_converter.h > +++ b/src/qcam/format_converter.h > @@ -26,11 +26,13 @@ private: > enum FormatFamily { > MJPEG, > NV, > + RAW_CSI2P, I'd name this RawCSI2Packed. > RGB, > YUV, > }; > > void convertNV(const unsigned char *src, unsigned char *dst); > + void convertRawCSI2P(const unsigned char *src, unsigned char *dst); And s/CSI2P/CSI2Packed/ > void convertRGB(const unsigned char *src, unsigned char *dst); > void convertYUV(const unsigned char *src, unsigned char *dst); > > @@ -48,6 +50,25 @@ private: > bool nvSwap; > } nv; > > + /* RAW Bayer CSI2P parameters */ > + struct raw_csi2p_params { > + /* > + * 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; > + /* > + * The fields below are used to hold the values computed > + * in configure() based on r_pos and width_. > + */ > + unsigned int g1_pos; > + unsigned int g2_pos; > + unsigned int b_pos; > + unsigned int srcLineLength; > + } rawp; These parameters are applicable to all Bayer formats, not only the packed ones. And they're specific to Bayer formats, they're not applicable to other color filter array patterns (or other non-CFA raw formats). I would thus rename the structure from raw_csi2p_params to bayer_params and the member from rawp to bayer. > + > /* RGB parameters */ > struct rgb_params { > unsigned int bpp;
Hi Laurent, On 12/09/2020 02:03, Laurent Pinchart wrote: > Hi Andrey, > > Thank you for the patch. > > On Tue, Sep 08, 2020 at 06:07:39PM +0300, 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. >> >> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> >> --- >> src/qcam/format_converter.cpp | 138 ++++++++++++++++++++++++++++++++++ >> src/qcam/format_converter.h | 21 ++++++ >> 2 files changed, 159 insertions(+) >> >> diff --git a/src/qcam/format_converter.cpp b/src/qcam/format_converter.cpp >> index 3481330..f370907 100644 >> --- a/src/qcam/format_converter.cpp >> +++ b/src/qcam/format_converter.cpp >> @@ -10,6 +10,7 @@ >> #include <errno.h> >> >> #include <QImage> >> +#include <QtDebug> >> >> #include <libcamera/formats.h> >> >> @@ -132,6 +133,62 @@ int FormatConverter::configure(const libcamera::PixelFormat &format, >> params_.yuv.cb_pos = 1; >> break; >> >> + case libcamera::formats::SRGGB10_CSI2P: >> + formatFamily_ = RAW_CSI2P; >> + params_.rawp.bpp_numer = 5; /* 1.25 bytes per pixel */ >> + params_.rawp.bpp_denom = 4; >> + params_.rawp.r_pos = 0; >> + break; >> + > > There are no blank lines between formats of the same group for other > groups, I'd follow the same pattern here. > >> + case libcamera::formats::SGRBG10_CSI2P: >> + formatFamily_ = RAW_CSI2P; >> + params_.rawp.bpp_numer = 5; >> + params_.rawp.bpp_denom = 4; >> + params_.rawp.r_pos = 1; >> + break; >> + >> + case libcamera::formats::SGBRG10_CSI2P: >> + formatFamily_ = RAW_CSI2P; >> + params_.rawp.bpp_numer = 5; >> + params_.rawp.bpp_denom = 4; >> + params_.rawp.r_pos = 2; >> + break; >> + >> + case libcamera::formats::SBGGR10_CSI2P: >> + formatFamily_ = RAW_CSI2P; >> + params_.rawp.bpp_numer = 5; >> + params_.rawp.bpp_denom = 4; >> + params_.rawp.r_pos = 3; >> + break; >> + >> + case libcamera::formats::SRGGB12_CSI2P: >> + formatFamily_ = RAW_CSI2P; >> + params_.rawp.bpp_numer = 3; /* 1.5 bytes per pixel */ >> + params_.rawp.bpp_denom = 2; >> + params_.rawp.r_pos = 0; >> + break; >> + >> + case libcamera::formats::SGRBG12_CSI2P: >> + formatFamily_ = RAW_CSI2P; >> + params_.rawp.bpp_numer = 3; >> + params_.rawp.bpp_denom = 2; >> + params_.rawp.r_pos = 1; >> + break; >> + >> + case libcamera::formats::SGBRG12_CSI2P: >> + formatFamily_ = RAW_CSI2P; >> + params_.rawp.bpp_numer = 3; >> + params_.rawp.bpp_denom = 2; >> + params_.rawp.r_pos = 2; >> + break; >> + >> + case libcamera::formats::SBGGR12_CSI2P: >> + formatFamily_ = RAW_CSI2P; >> + params_.rawp.bpp_numer = 3; >> + params_.rawp.bpp_denom = 2; >> + params_.rawp.r_pos = 3; >> + break; >> + >> case libcamera::formats::MJPEG: >> formatFamily_ = MJPEG; >> break; >> @@ -144,6 +201,45 @@ int FormatConverter::configure(const libcamera::PixelFormat &format, >> width_ = size.width(); >> height_ = size.height(); >> >> + if (formatFamily_ == RAW_CSI2P) { >> + /* >> + * For RAW_CSI2P the assumption is that height_ and width_ >> + * are even numbers. >> + */ >> + if ( width_ % 2 != 0 || height_ % 2 != 0 ) { > > No space after ( and before ). You can also drop the != 0. > >> + qWarning() << "Image width or height isn't even number"; >> + return -EINVAL; >> + } >> + >> + params_.rawp.srcLineLength = width_ * params_.rawp.bpp_numer / >> + params_.rawp.bpp_denom; >> + >> + /* >> + * Calculate [g1,g2,b]_pos based on r_pos. >> + * On entrance, r_pos is the position of red pixel in the > > s/entrace/entry/ (Kieran can correct me if this isn't right) "Upon entry, " > s/red pixel/the red pixel/ > >> + * 2-by-2 pixel Bayer pattern, and is from 0 to 3 range: > > "is in the 0 to 3 range" or "is in the [0, 3] range" > >> + * +---+---+ >> + * | 0 | 1 | >> + * +---+---+ >> + * | 2 | 3 | >> + * +---+---+ >> + * At exit, [r,g1,g2,b]_pos are offsetts of the color > > s/offsetts/the offsets/ > >> + * values in the source buffer. >> + */ >> + if (params_.rawp.r_pos > 1) { >> + params_.rawp.r_pos = params_.rawp.r_pos - 2 + >> + params_.rawp.srcLineLength; >> + params_.rawp.b_pos = 3 - params_.rawp.r_pos; >> + } else { >> + params_.rawp.b_pos = 1 - params_.rawp.r_pos + >> + params_.rawp.srcLineLength; >> + } >> + params_.rawp.g1_pos = (params_.rawp.r_pos == 0 || >> + params_.rawp.b_pos == 0) ? 1 : 0; >> + params_.rawp.g2_pos = 1 - params_.rawp.g1_pos + >> + params_.rawp.srcLineLength; >> + } >> + >> return 0; >> } >> >> @@ -163,9 +259,51 @@ void FormatConverter::convert(const unsigned char *src, size_t size, >> case NV: >> convertNV(src, dst->bits()); >> break; >> + case RAW_CSI2P: >> + convertRawCSI2P(src, dst->bits()); >> + break; >> }; >> } >> >> +void FormatConverter::convertRawCSI2P(const unsigned char *src, >> + unsigned char *dst) >> +{ >> + unsigned int g; >> + static unsigned char dst_buf[8] = { 0, 0, 0, 0xff }; >> + unsigned int dstLineLength = width_ * 4; >> + >> + for (unsigned int y = 0; y < height_; y += 2) { >> + for (unsigned x = 0; x < width_; x += params_.rawp.bpp_denom) { >> + for (unsigned int i = 0; i < params_.rawp.bpp_denom ; > > s/ ;/;/ > >> + i += 2) { > > I wouldn't wrap this to the next line, it looks weird. > >> + /* >> + * Process the current 2x2 group. >> + * Use the average of the two green pixels as >> + * the green value for all the pixels in the >> + * group. >> + */ >> + dst_buf[0] = src[params_.rawp.b_pos]; >> + g = src[params_.rawp.g1_pos]; >> + g = (g + src[params_.rawp.g2_pos]) >> 1; >> + dst_buf[1] = (unsigned char)g; >> + dst_buf[2] = src[params_.rawp.r_pos]; >> + src += 2; >> + >> + memcpy(dst_buf + 4, dst_buf, 4); >> + memcpy(dst, dst_buf, 8); >> + memcpy(dst + dstLineLength, dst_buf, 8); > > The memcpy() calls look really awkward. I suppose this is what you've > mentioned as a performance improvement in the cover letter ? I'm > concerned someone will forget about it in the future and refactor this > code. We need at least a comment that explains what's going on. > > I wonder if you won't get better performances from splitting this > function in two, and unrolling the inner loop for the 10bpp case. > >> + dst += 8; >> + } >> + src += params_.rawp.bpp_numer - params_.rawp.bpp_denom; >> + } > > Can you add a blank line here ? > >> + /* odd lines are copies of the even lines they follow: */ > > s/odd/Odd/ > s/follow:/follow/. > >> + memcpy(dst, dst-dstLineLength, dstLineLength); > > Blank line here too. > >> + /* move to the next even line: */ > > s/move/Move/ > s/line:/line./ > >> + src += params_.rawp.srcLineLength; >> + dst += dstLineLength; >> + } >> +} >> + >> 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 c53fa28..dc20e0d 100644 >> --- a/src/qcam/format_converter.h >> +++ b/src/qcam/format_converter.h >> @@ -26,11 +26,13 @@ private: >> enum FormatFamily { >> MJPEG, >> NV, >> + RAW_CSI2P, > > I'd name this RawCSI2Packed. > >> RGB, >> YUV, >> }; >> >> void convertNV(const unsigned char *src, unsigned char *dst); >> + void convertRawCSI2P(const unsigned char *src, unsigned char *dst); > > And s/CSI2P/CSI2Packed/ > >> void convertRGB(const unsigned char *src, unsigned char *dst); >> void convertYUV(const unsigned char *src, unsigned char *dst); >> >> @@ -48,6 +50,25 @@ private: >> bool nvSwap; >> } nv; >> >> + /* RAW Bayer CSI2P parameters */ >> + struct raw_csi2p_params { >> + /* >> + * 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; >> + /* >> + * The fields below are used to hold the values computed >> + * in configure() based on r_pos and width_. >> + */ >> + unsigned int g1_pos; >> + unsigned int g2_pos; >> + unsigned int b_pos; >> + unsigned int srcLineLength; >> + } rawp; > > These parameters are applicable to all Bayer formats, not only the > packed ones. And they're specific to Bayer formats, they're not > applicable to other color filter array patterns (or other non-CFA raw > formats). I would thus rename the structure from raw_csi2p_params to > bayer_params and the member from rawp to bayer. > >> + >> /* RGB parameters */ >> struct rgb_params { >> unsigned int bpp; >
Hi Laurent, On 12.09.2020 04:03, Laurent Pinchart wrote: > Hi Andrey, > > Thank you for the patch. > > On Tue, Sep 08, 2020 at 06:07:39PM +0300, 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. >> >> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> >> --- >> src/qcam/format_converter.cpp | 138 ++++++++++++++++++++++++++++++++++ >> src/qcam/format_converter.h | 21 ++++++ >> 2 files changed, 159 insertions(+) >> >> diff --git a/src/qcam/format_converter.cpp b/src/qcam/format_converter.cpp >> index 3481330..f370907 100644 >> --- a/src/qcam/format_converter.cpp >> +++ b/src/qcam/format_converter.cpp >> @@ -10,6 +10,7 @@ >> #include <errno.h> >> >> #include <QImage> >> +#include <QtDebug> >> >> #include <libcamera/formats.h> >> >> @@ -132,6 +133,62 @@ int FormatConverter::configure(const libcamera::PixelFormat &format, >> params_.yuv.cb_pos = 1; >> break; >> >> + case libcamera::formats::SRGGB10_CSI2P: >> + formatFamily_ = RAW_CSI2P; >> + params_.rawp.bpp_numer = 5; /* 1.25 bytes per pixel */ >> + params_.rawp.bpp_denom = 4; >> + params_.rawp.r_pos = 0; >> + break; >> + > > There are no blank lines between formats of the same group for other > groups, I'd follow the same pattern here. > >> + case libcamera::formats::SGRBG10_CSI2P: >> + formatFamily_ = RAW_CSI2P; >> + params_.rawp.bpp_numer = 5; >> + params_.rawp.bpp_denom = 4; >> + params_.rawp.r_pos = 1; >> + break; >> + >> + case libcamera::formats::SGBRG10_CSI2P: >> + formatFamily_ = RAW_CSI2P; >> + params_.rawp.bpp_numer = 5; >> + params_.rawp.bpp_denom = 4; >> + params_.rawp.r_pos = 2; >> + break; >> + >> + case libcamera::formats::SBGGR10_CSI2P: >> + formatFamily_ = RAW_CSI2P; >> + params_.rawp.bpp_numer = 5; >> + params_.rawp.bpp_denom = 4; >> + params_.rawp.r_pos = 3; >> + break; >> + >> + case libcamera::formats::SRGGB12_CSI2P: >> + formatFamily_ = RAW_CSI2P; >> + params_.rawp.bpp_numer = 3; /* 1.5 bytes per pixel */ >> + params_.rawp.bpp_denom = 2; >> + params_.rawp.r_pos = 0; >> + break; >> + >> + case libcamera::formats::SGRBG12_CSI2P: >> + formatFamily_ = RAW_CSI2P; >> + params_.rawp.bpp_numer = 3; >> + params_.rawp.bpp_denom = 2; >> + params_.rawp.r_pos = 1; >> + break; >> + >> + case libcamera::formats::SGBRG12_CSI2P: >> + formatFamily_ = RAW_CSI2P; >> + params_.rawp.bpp_numer = 3; >> + params_.rawp.bpp_denom = 2; >> + params_.rawp.r_pos = 2; >> + break; >> + >> + case libcamera::formats::SBGGR12_CSI2P: >> + formatFamily_ = RAW_CSI2P; >> + params_.rawp.bpp_numer = 3; >> + params_.rawp.bpp_denom = 2; >> + params_.rawp.r_pos = 3; >> + break; >> + >> case libcamera::formats::MJPEG: >> formatFamily_ = MJPEG; >> break; >> @@ -144,6 +201,45 @@ int FormatConverter::configure(const libcamera::PixelFormat &format, >> width_ = size.width(); >> height_ = size.height(); >> >> + if (formatFamily_ == RAW_CSI2P) { >> + /* >> + * For RAW_CSI2P the assumption is that height_ and width_ >> + * are even numbers. >> + */ >> + if ( width_ % 2 != 0 || height_ % 2 != 0 ) { > > No space after ( and before ). You can also drop the != 0. > >> + qWarning() << "Image width or height isn't even number"; >> + return -EINVAL; >> + } >> + >> + params_.rawp.srcLineLength = width_ * params_.rawp.bpp_numer / >> + params_.rawp.bpp_denom; >> + >> + /* >> + * Calculate [g1,g2,b]_pos based on r_pos. >> + * On entrance, r_pos is the position of red pixel in the > > s/entrace/entry/ (Kieran can correct me if this isn't right) > s/red pixel/the red pixel/ > >> + * 2-by-2 pixel Bayer pattern, and is from 0 to 3 range: > > "is in the 0 to 3 range" or "is in the [0, 3] range" > >> + * +---+---+ >> + * | 0 | 1 | >> + * +---+---+ >> + * | 2 | 3 | >> + * +---+---+ >> + * At exit, [r,g1,g2,b]_pos are offsetts of the color > > s/offsetts/the offsets/ > >> + * values in the source buffer. >> + */ >> + if (params_.rawp.r_pos > 1) { >> + params_.rawp.r_pos = params_.rawp.r_pos - 2 + >> + params_.rawp.srcLineLength; >> + params_.rawp.b_pos = 3 - params_.rawp.r_pos; >> + } else { >> + params_.rawp.b_pos = 1 - params_.rawp.r_pos + >> + params_.rawp.srcLineLength; >> + } >> + params_.rawp.g1_pos = (params_.rawp.r_pos == 0 || >> + params_.rawp.b_pos == 0) ? 1 : 0; >> + params_.rawp.g2_pos = 1 - params_.rawp.g1_pos + >> + params_.rawp.srcLineLength; >> + } >> + >> return 0; >> } >> >> @@ -163,9 +259,51 @@ void FormatConverter::convert(const unsigned char *src, size_t size, >> case NV: >> convertNV(src, dst->bits()); >> break; >> + case RAW_CSI2P: >> + convertRawCSI2P(src, dst->bits()); >> + break; >> }; >> } >> >> +void FormatConverter::convertRawCSI2P(const unsigned char *src, >> + unsigned char *dst) >> +{ >> + unsigned int g; >> + static unsigned char dst_buf[8] = { 0, 0, 0, 0xff }; >> + unsigned int dstLineLength = width_ * 4; >> + >> + for (unsigned int y = 0; y < height_; y += 2) { >> + for (unsigned x = 0; x < width_; x += params_.rawp.bpp_denom) { >> + for (unsigned int i = 0; i < params_.rawp.bpp_denom ; > > s/ ;/;/ > >> + i += 2) { > > I wouldn't wrap this to the next line, it looks weird. > >> + /* >> + * Process the current 2x2 group. >> + * Use the average of the two green pixels as >> + * the green value for all the pixels in the >> + * group. >> + */ >> + dst_buf[0] = src[params_.rawp.b_pos]; >> + g = src[params_.rawp.g1_pos]; >> + g = (g + src[params_.rawp.g2_pos]) >> 1; >> + dst_buf[1] = (unsigned char)g; >> + dst_buf[2] = src[params_.rawp.r_pos]; >> + src += 2; >> + >> + memcpy(dst_buf + 4, dst_buf, 4); >> + memcpy(dst, dst_buf, 8); >> + memcpy(dst + dstLineLength, dst_buf, 8); > > The memcpy() calls look really awkward. I suppose this is what you've > mentioned as a performance improvement in the cover letter ? Yep, v2 looks worse here, but works a bit faster. The thing is that after setting the green value for all the pixels in the 2x2 group to the average of the two green pixels in v2, all the 4 corresponding RGB pixels in the dst_buf got the same color values. So we wouldn't lose any information if we merged these 4 pixels into one, and scaled down the dst[] to width_/2 by height_/2. This would not only save the extra memory writes to dst[], but also might speed up the scaling done by QT to match the qcam window size. Is it something that worth trying? I am also thinking about trying linear interpolation (using from 2 to 4 nearest Bayer pixels of the same color) - it will be slower, but will prevent these 2x2 clusters with the same color values (thus improving the image quality a bit). > I'm > concerned someone will forget about it in the future and refactor this > code. We need at least a comment that explains what's going on. > > I wonder if you won't get better performances from splitting this > function in two, and unrolling the inner loop for the 10bpp case. I checked this before introducing the inner loop, and the fps decreased just slightly after the inner loop was created (the difference can't be seen by just looking at the printed fps numbers - without calculating the average fps over ~20 of the fps numbers). So I decided to go with the inner loop as it allows to reuse the same code for 10bpp, and 12bpp (and 14bpp if we ever add it) - it would be easier to maintain/modify in the future. Thanks, Andrey >> + dst += 8; >> + } >> + src += params_.rawp.bpp_numer - params_.rawp.bpp_denom; >> + } > > Can you add a blank line here ? > >> + /* odd lines are copies of the even lines they follow: */ > > s/odd/Odd/ > s/follow:/follow/. > >> + memcpy(dst, dst-dstLineLength, dstLineLength); > > Blank line here too. > >> + /* move to the next even line: */ > > s/move/Move/ > s/line:/line./ > >> + src += params_.rawp.srcLineLength; >> + dst += dstLineLength; >> + } >> +} >> + >> 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 c53fa28..dc20e0d 100644 >> --- a/src/qcam/format_converter.h >> +++ b/src/qcam/format_converter.h >> @@ -26,11 +26,13 @@ private: >> enum FormatFamily { >> MJPEG, >> NV, >> + RAW_CSI2P, > > I'd name this RawCSI2Packed. > >> RGB, >> YUV, >> }; >> >> void convertNV(const unsigned char *src, unsigned char *dst); >> + void convertRawCSI2P(const unsigned char *src, unsigned char *dst); > > And s/CSI2P/CSI2Packed/ > >> void convertRGB(const unsigned char *src, unsigned char *dst); >> void convertYUV(const unsigned char *src, unsigned char *dst); >> >> @@ -48,6 +50,25 @@ private: >> bool nvSwap; >> } nv; >> >> + /* RAW Bayer CSI2P parameters */ >> + struct raw_csi2p_params { >> + /* >> + * 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; >> + /* >> + * The fields below are used to hold the values computed >> + * in configure() based on r_pos and width_. >> + */ >> + unsigned int g1_pos; >> + unsigned int g2_pos; >> + unsigned int b_pos; >> + unsigned int srcLineLength; >> + } rawp; > > These parameters are applicable to all Bayer formats, not only the > packed ones. And they're specific to Bayer formats, they're not > applicable to other color filter array patterns (or other non-CFA raw > formats). I would thus rename the structure from raw_csi2p_params to > bayer_params and the member from rawp to bayer. > >> + >> /* RGB parameters */ >> struct rgb_params { >> unsigned int bpp; >
diff --git a/src/qcam/format_converter.cpp b/src/qcam/format_converter.cpp index 3481330..f370907 100644 --- a/src/qcam/format_converter.cpp +++ b/src/qcam/format_converter.cpp @@ -10,6 +10,7 @@ #include <errno.h> #include <QImage> +#include <QtDebug> #include <libcamera/formats.h> @@ -132,6 +133,62 @@ int FormatConverter::configure(const libcamera::PixelFormat &format, params_.yuv.cb_pos = 1; break; + case libcamera::formats::SRGGB10_CSI2P: + formatFamily_ = RAW_CSI2P; + params_.rawp.bpp_numer = 5; /* 1.25 bytes per pixel */ + params_.rawp.bpp_denom = 4; + params_.rawp.r_pos = 0; + break; + + case libcamera::formats::SGRBG10_CSI2P: + formatFamily_ = RAW_CSI2P; + params_.rawp.bpp_numer = 5; + params_.rawp.bpp_denom = 4; + params_.rawp.r_pos = 1; + break; + + case libcamera::formats::SGBRG10_CSI2P: + formatFamily_ = RAW_CSI2P; + params_.rawp.bpp_numer = 5; + params_.rawp.bpp_denom = 4; + params_.rawp.r_pos = 2; + break; + + case libcamera::formats::SBGGR10_CSI2P: + formatFamily_ = RAW_CSI2P; + params_.rawp.bpp_numer = 5; + params_.rawp.bpp_denom = 4; + params_.rawp.r_pos = 3; + break; + + case libcamera::formats::SRGGB12_CSI2P: + formatFamily_ = RAW_CSI2P; + params_.rawp.bpp_numer = 3; /* 1.5 bytes per pixel */ + params_.rawp.bpp_denom = 2; + params_.rawp.r_pos = 0; + break; + + case libcamera::formats::SGRBG12_CSI2P: + formatFamily_ = RAW_CSI2P; + params_.rawp.bpp_numer = 3; + params_.rawp.bpp_denom = 2; + params_.rawp.r_pos = 1; + break; + + case libcamera::formats::SGBRG12_CSI2P: + formatFamily_ = RAW_CSI2P; + params_.rawp.bpp_numer = 3; + params_.rawp.bpp_denom = 2; + params_.rawp.r_pos = 2; + break; + + case libcamera::formats::SBGGR12_CSI2P: + formatFamily_ = RAW_CSI2P; + params_.rawp.bpp_numer = 3; + params_.rawp.bpp_denom = 2; + params_.rawp.r_pos = 3; + break; + case libcamera::formats::MJPEG: formatFamily_ = MJPEG; break; @@ -144,6 +201,45 @@ int FormatConverter::configure(const libcamera::PixelFormat &format, width_ = size.width(); height_ = size.height(); + if (formatFamily_ == RAW_CSI2P) { + /* + * For RAW_CSI2P the assumption is that height_ and width_ + * are even numbers. + */ + if ( width_ % 2 != 0 || height_ % 2 != 0 ) { + qWarning() << "Image width or height isn't even number"; + return -EINVAL; + } + + params_.rawp.srcLineLength = width_ * params_.rawp.bpp_numer / + params_.rawp.bpp_denom; + + /* + * Calculate [g1,g2,b]_pos based on r_pos. + * On entrance, r_pos is the position of red pixel in the + * 2-by-2 pixel Bayer pattern, and is from 0 to 3 range: + * +---+---+ + * | 0 | 1 | + * +---+---+ + * | 2 | 3 | + * +---+---+ + * At exit, [r,g1,g2,b]_pos are offsetts of the color + * values in the source buffer. + */ + if (params_.rawp.r_pos > 1) { + params_.rawp.r_pos = params_.rawp.r_pos - 2 + + params_.rawp.srcLineLength; + params_.rawp.b_pos = 3 - params_.rawp.r_pos; + } else { + params_.rawp.b_pos = 1 - params_.rawp.r_pos + + params_.rawp.srcLineLength; + } + params_.rawp.g1_pos = (params_.rawp.r_pos == 0 || + params_.rawp.b_pos == 0) ? 1 : 0; + params_.rawp.g2_pos = 1 - params_.rawp.g1_pos + + params_.rawp.srcLineLength; + } + return 0; } @@ -163,9 +259,51 @@ void FormatConverter::convert(const unsigned char *src, size_t size, case NV: convertNV(src, dst->bits()); break; + case RAW_CSI2P: + convertRawCSI2P(src, dst->bits()); + break; }; } +void FormatConverter::convertRawCSI2P(const unsigned char *src, + unsigned char *dst) +{ + unsigned int g; + static unsigned char dst_buf[8] = { 0, 0, 0, 0xff }; + unsigned int dstLineLength = width_ * 4; + + for (unsigned int y = 0; y < height_; y += 2) { + for (unsigned x = 0; x < width_; x += params_.rawp.bpp_denom) { + for (unsigned int i = 0; i < params_.rawp.bpp_denom ; + i += 2) { + /* + * Process the current 2x2 group. + * Use the average of the two green pixels as + * the green value for all the pixels in the + * group. + */ + dst_buf[0] = src[params_.rawp.b_pos]; + g = src[params_.rawp.g1_pos]; + g = (g + src[params_.rawp.g2_pos]) >> 1; + dst_buf[1] = (unsigned char)g; + dst_buf[2] = src[params_.rawp.r_pos]; + src += 2; + + memcpy(dst_buf + 4, dst_buf, 4); + memcpy(dst, dst_buf, 8); + memcpy(dst + dstLineLength, dst_buf, 8); + dst += 8; + } + src += params_.rawp.bpp_numer - params_.rawp.bpp_denom; + } + /* odd lines are copies of the even lines they follow: */ + memcpy(dst, dst-dstLineLength, dstLineLength); + /* move to the next even line: */ + src += params_.rawp.srcLineLength; + dst += dstLineLength; + } +} + 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 c53fa28..dc20e0d 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 convertRawCSI2P(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); @@ -48,6 +50,25 @@ private: bool nvSwap; } nv; + /* RAW Bayer CSI2P parameters */ + struct raw_csi2p_params { + /* + * 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; + /* + * The fields below are used to hold the values computed + * in configure() based on r_pos and width_. + */ + unsigned int g1_pos; + unsigned int g2_pos; + unsigned int b_pos; + unsigned int srcLineLength; + } rawp; + /* RGB parameters */ struct rgb_params { unsigned int bpp;
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> --- src/qcam/format_converter.cpp | 138 ++++++++++++++++++++++++++++++++++ src/qcam/format_converter.h | 21 ++++++ 2 files changed, 159 insertions(+)