[libcamera-devel] [RFC PATCH] qcam: format_converter: add 10 and 12 bit packed raw Bayer formats
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Aug 31 16:52:55 CEST 2020
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 ?
> >> + 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.
> > (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.
> >> + */
> >> +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.
> >> + unsigned int d_linelen = width_ * 4;
And dst there.
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.
> >> +
> >> + /*
> >> + * 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 (
> >> + 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.
> >> + 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_;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list