| Message ID | 20200908150739.1552-2-andrey.konovalov@linaro.org |
|---|---|
| State | Superseded |
| Delegated to: | Laurent Pinchart |
| Headers | show |
| Series |
|
| Related | show |
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@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__ */
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@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__ */ >
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; + } params_; + }; #endif /* __QCAM_FORMAT_CONVERTER_H__ */
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@linaro.org> --- src/qcam/format_converter.cpp | 133 ++++++++++++++++++---------------- src/qcam/format_converter.h | 37 ++++++---- 2 files changed, 92 insertions(+), 78 deletions(-)