[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 17:15:25 CEST 2020
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 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());
>
> 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_;
>
More information about the libcamera-devel
mailing list