[libcamera-devel] [RFC PATCH v2 1/2] qcam: format_converter: put parameters of different formats into union
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Sep 12 02:44:19 CEST 2020
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.
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.
> +
Extra blank line.
> };
>
> #endif /* __QCAM_FORMAT_CONVERTER_H__ */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list