[libcamera-devel] [RFC PATCH v2 2/2] qcam: format_converter: add 10 and 12 bit packed raw Bayer formats
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Sep 12 03:03:31 CEST 2020
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 at 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;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list