[libcamera-devel] [RFC PATCH v2 1/2] qcam: format_converter: put parameters of different formats into union
Andrey Konovalov
andrey.konovalov at linaro.org
Tue Sep 22 22:03:48 CEST 2020
Hi Laurent,
Thanks for the review.
On 12.09.2020 03:44, Laurent Pinchart wrote:
> Hi Andrey,
>
> Thank you for the patch.
>
> On Tue, Sep 08, 2020 at 06:07:38PM +0300, Andrey Konovalov wrote:
>> As the number of supported formats in the converter grows, having the
>> format-specific parameters in a union makes the code cleaner, and save
>> some amount of memory.
>>
>> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
>> ---
>> src/qcam/format_converter.cpp | 133 ++++++++++++++++++----------------
>> src/qcam/format_converter.h | 37 ++++++----
>> 2 files changed, 92 insertions(+), 78 deletions(-)
>>
>> diff --git a/src/qcam/format_converter.cpp b/src/qcam/format_converter.cpp
>> index 4b9722d..3481330 100644
>> --- a/src/qcam/format_converter.cpp
>> +++ b/src/qcam/format_converter.cpp
>> @@ -33,103 +33,103 @@ int FormatConverter::configure(const libcamera::PixelFormat &format,
>> switch (format) {
>> case libcamera::formats::NV12:
>> formatFamily_ = NV;
>> - horzSubSample_ = 2;
>> - vertSubSample_ = 2;
>> - nvSwap_ = false;
>> + params_.nv.horzSubSample = 2;
>> + params_.nv.vertSubSample = 2;
>> + params_.nv.nvSwap = false;
>> break;
>> case libcamera::formats::NV21:
>> formatFamily_ = NV;
>> - horzSubSample_ = 2;
>> - vertSubSample_ = 2;
>> - nvSwap_ = true;
>> + params_.nv.horzSubSample = 2;
>> + params_.nv.vertSubSample = 2;
>> + params_.nv.nvSwap = true;
>> break;
>> case libcamera::formats::NV16:
>> formatFamily_ = NV;
>> - horzSubSample_ = 2;
>> - vertSubSample_ = 1;
>> - nvSwap_ = false;
>> + params_.nv.horzSubSample = 2;
>> + params_.nv.vertSubSample = 1;
>> + params_.nv.nvSwap = false;
>> break;
>> case libcamera::formats::NV61:
>> formatFamily_ = NV;
>> - horzSubSample_ = 2;
>> - vertSubSample_ = 1;
>> - nvSwap_ = true;
>> + params_.nv.horzSubSample = 2;
>> + params_.nv.vertSubSample = 1;
>> + params_.nv.nvSwap = true;
>> break;
>> case libcamera::formats::NV24:
>> formatFamily_ = NV;
>> - horzSubSample_ = 1;
>> - vertSubSample_ = 1;
>> - nvSwap_ = false;
>> + params_.nv.horzSubSample = 1;
>> + params_.nv.vertSubSample = 1;
>> + params_.nv.nvSwap = false;
>> break;
>> case libcamera::formats::NV42:
>> formatFamily_ = NV;
>> - horzSubSample_ = 1;
>> - vertSubSample_ = 1;
>> - nvSwap_ = true;
>> + params_.nv.horzSubSample = 1;
>> + params_.nv.vertSubSample = 1;
>> + params_.nv.nvSwap = true;
>> break;
>>
>> case libcamera::formats::RGB888:
>> formatFamily_ = RGB;
>> - r_pos_ = 2;
>> - g_pos_ = 1;
>> - b_pos_ = 0;
>> - bpp_ = 3;
>> + params_.rgb.r_pos = 2;
>> + params_.rgb.g_pos = 1;
>> + params_.rgb.b_pos = 0;
>> + params_.rgb.bpp = 3;
>> break;
>> case libcamera::formats::BGR888:
>> formatFamily_ = RGB;
>> - r_pos_ = 0;
>> - g_pos_ = 1;
>> - b_pos_ = 2;
>> - bpp_ = 3;
>> + params_.rgb.r_pos = 0;
>> + params_.rgb.g_pos = 1;
>> + params_.rgb.b_pos = 2;
>> + params_.rgb.bpp = 3;
>> break;
>> case libcamera::formats::ARGB8888:
>> formatFamily_ = RGB;
>> - r_pos_ = 2;
>> - g_pos_ = 1;
>> - b_pos_ = 0;
>> - bpp_ = 4;
>> + params_.rgb.r_pos = 2;
>> + params_.rgb.g_pos = 1;
>> + params_.rgb.b_pos = 0;
>> + params_.rgb.bpp = 4;
>> break;
>> case libcamera::formats::RGBA8888:
>> formatFamily_ = RGB;
>> - r_pos_ = 3;
>> - g_pos_ = 2;
>> - b_pos_ = 1;
>> - bpp_ = 4;
>> + params_.rgb.r_pos = 3;
>> + params_.rgb.g_pos = 2;
>> + params_.rgb.b_pos = 1;
>> + params_.rgb.bpp = 4;
>> break;
>> case libcamera::formats::ABGR8888:
>> formatFamily_ = RGB;
>> - r_pos_ = 0;
>> - g_pos_ = 1;
>> - b_pos_ = 2;
>> - bpp_ = 4;
>> + params_.rgb.r_pos = 0;
>> + params_.rgb.g_pos = 1;
>> + params_.rgb.b_pos = 2;
>> + params_.rgb.bpp = 4;
>> break;
>> case libcamera::formats::BGRA8888:
>> formatFamily_ = RGB;
>> - r_pos_ = 1;
>> - g_pos_ = 2;
>> - b_pos_ = 3;
>> - bpp_ = 4;
>> + params_.rgb.r_pos = 1;
>> + params_.rgb.g_pos = 2;
>> + params_.rgb.b_pos = 3;
>> + params_.rgb.bpp = 4;
>> break;
>>
>> case libcamera::formats::VYUY:
>> formatFamily_ = YUV;
>> - y_pos_ = 1;
>> - cb_pos_ = 2;
>> + params_.yuv.y_pos = 1;
>> + params_.yuv.cb_pos = 2;
>> break;
>> case libcamera::formats::YVYU:
>> formatFamily_ = YUV;
>> - y_pos_ = 0;
>> - cb_pos_ = 3;
>> + params_.yuv.y_pos = 0;
>> + params_.yuv.cb_pos = 3;
>> break;
>> case libcamera::formats::UYVY:
>> formatFamily_ = YUV;
>> - y_pos_ = 1;
>> - cb_pos_ = 0;
>> + params_.yuv.y_pos = 1;
>> + params_.yuv.cb_pos = 0;
>> break;
>> case libcamera::formats::YUYV:
>> formatFamily_ = YUV;
>> - y_pos_ = 0;
>> - cb_pos_ = 1;
>> + params_.yuv.y_pos = 0;
>> + params_.yuv.cb_pos = 1;
>> break;
>>
>> case libcamera::formats::MJPEG:
>> @@ -178,18 +178,20 @@ static void yuv_to_rgb(int y, int u, int v, int *r, int *g, int *b)
>>
>> void FormatConverter::convertNV(const unsigned char *src, unsigned char *dst)
>> {
>> - unsigned int c_stride = width_ * (2 / horzSubSample_);
>> - unsigned int c_inc = horzSubSample_ == 1 ? 2 : 0;
>> - unsigned int cb_pos = nvSwap_ ? 1 : 0;
>> - unsigned int cr_pos = nvSwap_ ? 0 : 1;
>> + unsigned int c_stride = width_ * (2 / params_.nv.horzSubSample);
>> + unsigned int c_inc = params_.nv.horzSubSample == 1 ? 2 : 0;
>> + unsigned int cb_pos = params_.nv.nvSwap ? 1 : 0;
>> + unsigned int cr_pos = params_.nv.nvSwap ? 0 : 1;
>> const unsigned char *src_c = src + width_ * height_;
>> int r, g, b;
>>
>> for (unsigned int y = 0; y < height_; y++) {
>> const unsigned char *src_y = src + y * width_;
>> - const unsigned char *src_cb = src_c + (y / vertSubSample_) *
>> + const unsigned char *src_cb = src_c +
>> + (y / params_.nv.vertSubSample) *
>> c_stride + cb_pos;
>> - const unsigned char *src_cr = src_c + (y / vertSubSample_) *
>> + const unsigned char *src_cr = src_c +
>> + (y / params_.nv.vertSubSample) *
>> c_stride + cr_pos;
>>
>> for (unsigned int x = 0; x < width_; x += 2) {
>> @@ -223,9 +225,9 @@ void FormatConverter::convertRGB(const unsigned char *src, unsigned char *dst)
>>
>> for (y = 0; y < height_; y++) {
>> for (x = 0; x < width_; x++) {
>> - r = src[bpp_ * x + r_pos_];
>> - g = src[bpp_ * x + g_pos_];
>> - b = src[bpp_ * x + b_pos_];
>> + r = src[params_.rgb.bpp * x + params_.rgb.r_pos];
>> + g = src[params_.rgb.bpp * x + params_.rgb.g_pos];
>> + b = src[params_.rgb.bpp * x + params_.rgb.b_pos];
>>
>> dst[4 * x + 0] = b;
>> dst[4 * x + 1] = g;
>> @@ -233,7 +235,7 @@ void FormatConverter::convertRGB(const unsigned char *src, unsigned char *dst)
>> dst[4 * x + 3] = 0xff;
>> }
>>
>> - src += width_ * bpp_;
>> + src += width_ * params_.rgb.bpp;
>> dst += width_ * 4;
>> }
>> }
>> @@ -246,16 +248,18 @@ void FormatConverter::convertYUV(const unsigned char *src, unsigned char *dst)
>> unsigned int cr_pos;
>> int r, g, b, y, cr, cb;
>>
>> - cr_pos = (cb_pos_ + 2) % 4;
>> + cr_pos = (params_.yuv.cb_pos + 2) % 4;
>> src_stride = width_ * 2;
>> dst_stride = width_ * 4;
>>
>> for (src_y = 0, dst_y = 0; dst_y < height_; src_y++, dst_y++) {
>> for (src_x = 0, dst_x = 0; dst_x < width_; ) {
>> - cb = src[src_y * src_stride + src_x * 4 + cb_pos_];
>> + cb = src[src_y * src_stride + src_x * 4 +
>> + params_.yuv.cb_pos];
>> cr = src[src_y * src_stride + src_x * 4 + cr_pos];
>>
>> - y = src[src_y * src_stride + src_x * 4 + y_pos_];
>> + y = src[src_y * src_stride + src_x * 4 +
>> + params_.yuv.y_pos];
>> yuv_to_rgb(y, cb, cr, &r, &g, &b);
>> dst[dst_y * dst_stride + 4 * dst_x + 0] = b;
>> dst[dst_y * dst_stride + 4 * dst_x + 1] = g;
>> @@ -263,7 +267,8 @@ void FormatConverter::convertYUV(const unsigned char *src, unsigned char *dst)
>> dst[dst_y * dst_stride + 4 * dst_x + 3] = 0xff;
>> dst_x++;
>>
>> - y = src[src_y * src_stride + src_x * 4 + y_pos_ + 2];
>> + y = src[src_y * src_stride + src_x * 4 +
>> + params_.yuv.y_pos + 2];
>> yuv_to_rgb(y, cb, cr, &r, &g, &b);
>> dst[dst_y * dst_stride + 4 * dst_x + 0] = b;
>> dst[dst_y * dst_stride + 4 * dst_x + 1] = g;
>> diff --git a/src/qcam/format_converter.h b/src/qcam/format_converter.h
>> index e389b24..c53fa28 100644
>> --- a/src/qcam/format_converter.h
>> +++ b/src/qcam/format_converter.h
>> @@ -40,20 +40,29 @@ private:
>>
>> enum FormatFamily formatFamily_;
>>
>> - /* NV parameters */
>> - unsigned int horzSubSample_;
>> - unsigned int vertSubSample_;
>> - bool nvSwap_;
>> -
>> - /* RGB parameters */
>> - unsigned int bpp_;
>> - unsigned int r_pos_;
>> - unsigned int g_pos_;
>> - unsigned int b_pos_;
>> -
>> - /* YUV parameters */
>> - unsigned int y_pos_;
>> - unsigned int cb_pos_;
>> + union {
>> + /* NV parameters */
>> + struct nv_params {
>> + unsigned int horzSubSample;
>> + unsigned int vertSubSample;
>> + bool nvSwap;
>> + } nv;
>> +
>> + /* RGB parameters */
>> + struct rgb_params {
>> + unsigned int bpp;
>> + unsigned int r_pos;
>> + unsigned int g_pos;
>> + unsigned int b_pos;
>> + } rgb;
>> +
>> + /* YUV parameters */
>> + struct yuv_params {
>> + unsigned int y_pos;
>> + unsigned int cb_pos;
>> + } yuv;
>
> The NV* formats are YUV too, we should try to find a better name. It's a
> pre-existing issue though.
>
>> + } params_;
>
> I wonder if it wouldn't make more sense to move all of these to a
> separate structure, stored in a map indexed by the pixel format. That
> would greatly simplify FormatConverter::configure(). We actually have
> such a structure already, PixelFormatInfo, but it's an internal API.
> Exposing it as a public API runs into the issue that not all the fields
> it contains should be exposed. The V4L2 4CC, for instance, should stay
> internal. There's probably a way to handle that, but I'm also no really
> keen at this point to committing to a stable API for PixelFormatInfo. I
> thus think it would be better to duplicate the data we need here.
Yes, this makes sense, and will probably take some more patch review
iterations.
> To avoid blocking Bayer support in qcam on this, how about moving
> forward with patch 2/2 for now without 1/2 ? The small space increase is
> negligible.
That's fine for me. Let's go this way.
In this case should I still use:
/* RAW Bayer parameters */
struct bayer_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;
} bayer_;
- as the member of FormatConverter (all Bayer parameters grouped together into one member),
or should I follow the current pattern and make each of the above parameters a separate member of
FormatConverter (and invent some other name for r_pos_ and b_pos_ not to clash with the existing
RGB parameters)?
Thanks,
Andrey
>> +
>
> Extra blank line.
>
>> };
>>
>> #endif /* __QCAM_FORMAT_CONVERTER_H__ */
>
More information about the libcamera-devel
mailing list