[libcamera-devel] [RFC PATCH] qcam: format_converter: add 10 and 12 bit packed raw Bayer formats
Andrey Konovalov
andrey.konovalov at linaro.org
Mon Aug 31 13:23:22 CEST 2020
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 at 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_;
>>
>
More information about the libcamera-devel
mailing list