[libcamera-devel] [RFC PATCH v2 2/2] qcam: format_converter: add 10 and 12 bit packed raw Bayer formats
Kieran Bingham
kieran.bingham at ideasonboard.com
Sat Sep 12 22:07:50 CEST 2020
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 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)
"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;
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list