| Message ID | 20231212115046.102726-5-andrey.konovalov@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi!
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
I'm not sure if we should call implementation "Linaro". Company name
seems misleading in this context. Could we call implementation "anna",
for example, after https://en.wikipedia.org/wiki/Anna_Atkins and
because it is first version, thus "A"?
1-4 Reviewed-by: Pavel Machek <pavel@ucw.cz>
Best regards,
Pavel
Quoting Pavel Machek via libcamera-devel (2023-12-12 15:51:10) > Hi! > > > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> > > I'm not sure if we should call implementation "Linaro". Company name > seems misleading in this context. Could we call implementation "anna", > for example, after https://en.wikipedia.org/wiki/Anna_Atkins and > because it is first version, thus "A"? I'd agree here ... I can't imagine libcamera want's "5 different competing softISP implementations.... all in the same code base" This is just 'SoftISP' in my opinion. -- Kieran > > 1-4 Reviewed-by: Pavel Machek <pavel@ucw.cz> > > Best regards, > Pavel > -- > People of Russia, stop Putin before his war on Ukraine escalates.
On 12.12.2023 19:04, Kieran Bingham wrote: > Quoting Pavel Machek via libcamera-devel (2023-12-12 15:51:10) >> Hi! >> >>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> >> >> I'm not sure if we should call implementation "Linaro". Company name >> seems misleading in this context. Could we call implementation "anna", >> for example, after https://en.wikipedia.org/wiki/Anna_Atkins and >> because it is first version, thus "A"? > > I'd agree here ... I can't imagine libcamera want's "5 different > competing softISP implementations.... all in the same code base" > > This is just 'SoftISP' in my opinion. Then SwIspLinaro could be merged into abstract SoftwareIsp, the result being called just SoftwareIsp. And SoftwareIspFactory* are not needed. Correct? Thanks, Andrey > -- > Kieran > > >> >> 1-4 Reviewed-by: Pavel Machek <pavel@ucw.cz> >> >> Best regards, >> Pavel >> -- >> People of Russia, stop Putin before his war on Ukraine escalates.
Quoting Andrey Konovalov (2023-12-12 16:27:49) > On 12.12.2023 19:04, Kieran Bingham wrote: > > Quoting Pavel Machek via libcamera-devel (2023-12-12 15:51:10) > >> Hi! > >> > >>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> > >> > >> I'm not sure if we should call implementation "Linaro". Company name > >> seems misleading in this context. Could we call implementation "anna", > >> for example, after https://en.wikipedia.org/wiki/Anna_Atkins and > >> because it is first version, thus "A"? > > > > I'd agree here ... I can't imagine libcamera want's "5 different > > competing softISP implementations.... all in the same code base" > > > > This is just 'SoftISP' in my opinion. > > Then SwIspLinaro could be merged into abstract SoftwareIsp, > the result being called just SoftwareIsp. > And SoftwareIspFactory* are not needed. > Correct? Has a SoftwareISPFactory requirement been discussed somewhere that I haven't been involved in ? -- Kieran > > Thanks, > Andrey > > > -- > > Kieran > > > > > >> > >> 1-4 Reviewed-by: Pavel Machek <pavel@ucw.cz> > >> > >> Best regards, > >> Pavel > >> -- > >> People of Russia, stop Putin before his war on Ukraine escalates.
Hi! > > >>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> > > >> > > >> I'm not sure if we should call implementation "Linaro". Company name > > >> seems misleading in this context. Could we call implementation "anna", > > >> for example, after https://en.wikipedia.org/wiki/Anna_Atkins and > > >> because it is first version, thus "A"? > > > > > > I'd agree here ... I can't imagine libcamera want's "5 different > > > competing softISP implementations.... all in the same code base" > > > > > > This is just 'SoftISP' in my opinion. > > > > Then SwIspLinaro could be merged into abstract SoftwareIsp, > > the result being called just SoftwareIsp. > > And SoftwareIspFactory* are not needed. > > Correct? > > Has a SoftwareISPFactory requirement been discussed somewhere that I > haven't been involved in ? One day we may have SwIspGPU or maybe SwIspIntelMMX, so that support might be worth keeping. Best regards, Pavel
Quoting Pavel Machek (2023-12-12 16:37:09) > Hi! > > > > >>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> > > > >> > > > >> I'm not sure if we should call implementation "Linaro". Company name > > > >> seems misleading in this context. Could we call implementation "anna", > > > >> for example, after https://en.wikipedia.org/wiki/Anna_Atkins and > > > >> because it is first version, thus "A"? > > > > > > > > I'd agree here ... I can't imagine libcamera want's "5 different > > > > competing softISP implementations.... all in the same code base" > > > > > > > > This is just 'SoftISP' in my opinion. > > > > > > Then SwIspLinaro could be merged into abstract SoftwareIsp, > > > the result being called just SoftwareIsp. > > > And SoftwareIspFactory* are not needed. > > > Correct? > > > > Has a SoftwareISPFactory requirement been discussed somewhere that I > > haven't been involved in ? > > One day we may have SwIspGPU or maybe SwIspIntelMMX, so that support > might be worth keeping. I envisioned that a libcamera software ISP should detect the capabilities of the system it's on and construct a pipeline accordingly. I guess there might be some scenarios where specific control over which parts can be used might want to be controlled or configured though. > > Best regards, > Pavel > -- > People of Russia, stop Putin before his war on Ukraine escalates.
Hi, On 12/12/23 17:37, Pavel Machek via libcamera-devel wrote: > Hi! > >>>>>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> >>>>> >>>>> I'm not sure if we should call implementation "Linaro". Company name >>>>> seems misleading in this context. Could we call implementation "anna", >>>>> for example, after https://en.wikipedia.org/wiki/Anna_Atkins and >>>>> because it is first version, thus "A"? >>>> >>>> I'd agree here ... I can't imagine libcamera want's "5 different >>>> competing softISP implementations.... all in the same code base" >>>> >>>> This is just 'SoftISP' in my opinion. >>> >>> Then SwIspLinaro could be merged into abstract SoftwareIsp, >>> the result being called just SoftwareIsp. >>> And SoftwareIspFactory* are not needed. >>> Correct? >> >> Has a SoftwareISPFactory requirement been discussed somewhere that I >> haven't been involved in ? > > One day we may have SwIspGPU or maybe SwIspIntelMMX, so that support > might be worth keeping. Right, so about having multiple SoftwareIsp implementations. I have been working on stasistics + debayer base classes and CPU implementations of both based on refactoring Andrey's work. I was hoping to have a clean branch with these ready but getting them in shape took longer then I hoped. Stil I would like to share what I have now before tomorrows SoftwareIsp video call / meeting. ATM the statistics and debayer classes are mostly ready, but I need to merge this with Andrey's v2 RFC and then produce a clean new series from this (assuming people like the approach). The base idea behind this is that we can have multiple statistics and debayer implementations, e.g. CPU vs GPU and that the SoftwareIsp class picks which one to instantiate. The current DebayerCpu class mostly replaces the IspWorker class making the SoftwareIsp code quite a bit simpler (note that the code is mostly moved not gone). Anyways if you have time, please take a look at the swstats base class + linaro implementation as well as at the debayer base class as well as the DebayerCpu implementation of that: https://gitlab.freedesktop.org/camera/libcamera-softisp/-/tree/SoftwareISP-v02-hans2 Which are the last few commits there. Note as I already mentioned I really need to merge this with Andrey's latest v03 branch into a new logical clean patch series suitable for upstream submission. Regards, Hans > > Best regards, > Pavel
Hi! > > One day we may have SwIspGPU or maybe SwIspIntelMMX, so that support > > might be worth keeping. > > Right, so about having multiple SoftwareIsp implementations. > > I have been working on stasistics + debayer > base classes and CPU implementations of both based on refactoring > Andrey's work. > > I was hoping to have a clean branch with these ready but > getting them in shape took longer then I hoped. > > Stil I would like to share what I have now before tomorrows > SoftwareIsp video call / meeting. Thank you, already noticed new branch in git and was browsing :-). Is there way I could join the video call? Best regards, Pavel
Hi! > ATM the statistics and debayer classes are mostly ready, > but I need to merge this with Andrey's v2 RFC and > then produce a clean new series from this > (assuming people like the approach). Ok, what about this one? I only converted half of the code, but the other half of conversion should be very similar. Compile-tested only as my sensor is bayer 8. It makes it easier to see how we are skipping the low bits in 10P bayer, and will make bayer 8 support easier, too, since the pixel_XXX helpers are designed to be shared. Best regards, Pavel diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp index 97ab6c14..a340e500 100644 --- a/src/libcamera/software_isp/debayer_cpu.cpp +++ b/src/libcamera/software_isp/debayer_cpu.cpp @@ -29,13 +29,37 @@ DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsLinaro> stats) green_[i] = i; } +struct ctxt { + /* Pointers to previous, current and next lines */ + const uint8_t *prev; + const uint8_t *curr; + const uint8_t *next; + + /* Pointers to gamma correction tables */ + const uint8_t *red; + const uint8_t *green; + const uint8_t *blue; +}; + +static inline void pixel_bggr(const struct ctxt &c, uint8_t *&dst, int x, int p, int n) +{ + *dst++ = c.blue[c.curr[x]]; + *dst++ = c.green[(c.prev[x] + c.curr[x - p] + c.curr[x + p] + c.next[x]) / 4]; + *dst++ = c.red[(c.prev[x - p] + c.prev[x + n] + c.next[x - p] + c.next[x + n]) / 4]; +} + +static inline void pixel_gbrg(const struct ctxt &c, uint8_t *&dst, int x, int p, int n) +{ + *dst++ = c.blue[(c.curr[x - p] + c.curr[x + n]) / 2]; + *dst++ = c.green[c.curr[x]]; + *dst++ = c.red[(c.prev[x] + c.next[x]) / 2]; +} + void DebayerCpu::debayerBGGR10PLine0(uint8_t *dst, const uint8_t *src) { const int width_in_bytes = window_.width * 5 / 4; - /* Pointers to previous, current and next lines */ - const uint8_t *prev = src - inputStride_; - const uint8_t *curr = src; - const uint8_t *next = src + inputStride_; + struct ctxt c = { src - inputStride_, src, src + inputStride_, + red_, green_, blue_ }; /* * For the first pixel getting a pixel from the previous column uses @@ -49,9 +73,7 @@ void DebayerCpu::debayerBGGR10PLine0(uint8_t *dst, const uint8_t *src) * RGR * Write BGR */ - *dst++ = blue_[curr[x]]; - *dst++ = green_[(prev[x] + curr[x - 2] + curr[x + 1] + next[x]) / 4]; - *dst++ = red_[(prev[x - 2] + prev[x + 1] + next[x - 2] + next[x + 1]) / 4]; + pixel_bggr(c, dst, x, 2, 1); x++; /* @@ -60,20 +82,14 @@ void DebayerCpu::debayerBGGR10PLine0(uint8_t *dst, const uint8_t *src) * GRG * Write BGR */ - *dst++ = blue_[(curr[x - 1] + curr[x + 1]) / 2]; - *dst++ = green_[curr[x]]; - *dst++ = red_[(prev[x] + next[x]) / 2]; + pixel_gbrg(c, dst, x, 1, 1); x++; /* Same thing for next 2 pixels */ - *dst++ = blue_[curr[x]]; - *dst++ = green_[(prev[x] + curr[x - 1] + curr[x + 1] + next[x]) / 4]; - *dst++ = red_[(prev[x - 1] + prev[x + 1] + next[x - 1] + next[x + 1]) / 4]; + pixel_bggr(c, dst, x, 1, 1); x++; - *dst++ = blue_[(curr[x - 1] + curr[x + 2]) / 2]; - *dst++ = green_[curr[x]]; - *dst++ = red_[(prev[x] + next[x]) / 2]; + pixel_gbrg(c, dst, x, 1, 2); } } @@ -123,10 +139,8 @@ void DebayerCpu::debayerBGGR10PLine1(uint8_t *dst, const uint8_t *src) void DebayerCpu::debayerGBRG10PLine0(uint8_t *dst, const uint8_t *src) { const int width_in_bytes = window_.width * 5 / 4; - /* Pointers to previous, current and next lines */ - const uint8_t *prev = src - inputStride_; - const uint8_t *curr = src; - const uint8_t *next = src + inputStride_; + struct ctxt c = { src - inputStride_, src, src + inputStride_, + red_, green_, blue_ }; for (int x = 0; x < width_in_bytes; x += 2) { /* @@ -135,9 +149,7 @@ void DebayerCpu::debayerGBRG10PLine0(uint8_t *dst, const uint8_t *src) * GRG * Write BGR */ - *dst++ = blue_[(curr[x - 2] + curr[x + 1]) / 2]; - *dst++ = green_[curr[x]]; - *dst++ = red_[(prev[x] + next[x]) / 2]; + pixel_gbrg(c, dst, x, 2, 1); x++; /* @@ -146,20 +158,14 @@ void DebayerCpu::debayerGBRG10PLine0(uint8_t *dst, const uint8_t *src) * RGR * Write BGR */ - *dst++ = blue_[curr[x]]; - *dst++ = green_[(prev[x] + curr[x - 1] + curr[x + 1] + next[x]) / 4]; - *dst++ = red_[(prev[x - 1] + prev[x + 1] + next[x - 1] + next[x + 1]) / 4]; + pixel_bggr(c, dst, x, 1, 1); x++; /* Same thing for next 2 pixels */ - *dst++ = blue_[(curr[x - 1] + curr[x + 1]) / 2]; - *dst++ = green_[curr[x]]; - *dst++ = red_[(prev[x] + next[x]) / 2]; + pixel_gbrg(c, dst, x, 1, 1); x++; - *dst++ = blue_[curr[x]]; - *dst++ = green_[(prev[x] + curr[x - 1] + curr[x + 2] + next[x]) / 4]; - *dst++ = red_[(prev[x - 1] + prev[x + 2] + next[x - 1] + next[x + 2]) / 4]; + pixel_bggr(c, dst, x, 1, 2); } }
On 12/12/2023 22:26, Pavel Machek wrote: > Hi! > >>> One day we may have SwIspGPU or maybe SwIspIntelMMX, so that support >>> might be worth keeping. >> >> Right, so about having multiple SoftwareIsp implementations. >> >> I have been working on stasistics + debayer >> base classes and CPU implementations of both based on refactoring >> Andrey's work. >> >> I was hoping to have a clean branch with these ready but >> getting them in shape took longer then I hoped. >> >> Stil I would like to share what I have now before tomorrows >> SoftwareIsp video call / meeting. > > Thank you, already noticed new branch in git and was browsing :-). > > Is there way I could join the video call? > > Best regards, > Pavel Added you to the invitee list. --- bod
Hi Pavel, On 12/13/23 08:39, Pavel Machek wrote: > Hi! > >> ATM the statistics and debayer classes are mostly ready, >> but I need to merge this with Andrey's v2 RFC and >> then produce a clean new series from this >> (assuming people like the approach). > > Ok, what about this one? I only converted half of the code, but the > other half of conversion should be very similar. > > Compile-tested only as my sensor is bayer 8. > > It makes it easier to see how we are skipping the low bits in 10P > bayer, and will make bayer 8 support easier, too, since the pixel_XXX > helpers are designed to be shared. I have merged a variant of this with: 1. The missing bits completed 2. Various renames to make it clear that the helpers are for 8bit bayer sources (we are treating 10bpp packed as 8 bit here) and that the output is bgr888 this should make life easier when adding support for more input / output fmts. Here is what I've merged / squashed: From 9e7bd0f37c38bf9500924187720621538f4581da Mon Sep 17 00:00:00 2001 From: Hans de Goede <hdegoede@redhat.com> Date: Wed, 13 Dec 2023 19:19:11 +0100 Subject: [PATCH] Pavel debayer changes Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- .../internal/software_isp/debayer_cpu.h | 8 +- src/libcamera/software_isp/debayer_cpu.cpp | 247 +++++++----------- 2 files changed, 97 insertions(+), 158 deletions(-) diff --git a/include/libcamera/internal/software_isp/debayer_cpu.h b/include/libcamera/internal/software_isp/debayer_cpu.h index 89824851..7fb5be77 100644 --- a/include/libcamera/internal/software_isp/debayer_cpu.h +++ b/include/libcamera/internal/software_isp/debayer_cpu.h @@ -62,10 +62,10 @@ private: void process2(const uint8_t *src, uint8_t *dst); void process4(const uint8_t *src, uint8_t *dst); /* CSI-2 packed 10-bit raw bayer format (all the 4 orders) */ - void debayerBGGR10PLine0(uint8_t *dst, const uint8_t *src); - void debayerBGGR10PLine1(uint8_t *dst, const uint8_t *src); - void debayerGBRG10PLine0(uint8_t *dst, const uint8_t *src); - void debayerGBRG10PLine1(uint8_t *dst, const uint8_t *src); + void debayer10P_BGBG_BGR888(uint8_t *dst, const uint8_t *src); + void debayer10P_GRGR_BGR888(uint8_t *dst, const uint8_t *src); + void debayer10P_GBGB_BGR888(uint8_t *dst, const uint8_t *src); + void debayer10P_RGRG_BGR888(uint8_t *dst, const uint8_t *src); typedef void (DebayerCpu::*debayerFn)(uint8_t *dst, const uint8_t *src); diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp index b6ed12de..3eacdd5d 100644 --- a/src/libcamera/software_isp/debayer_cpu.cpp +++ b/src/libcamera/software_isp/debayer_cpu.cpp @@ -29,180 +29,121 @@ DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats) green_[i] = i; } -void DebayerCpu::debayerBGGR10PLine0(uint8_t *dst, const uint8_t *src) +struct ctxt_8bit_src { + /* Pointers to previous, current and next lines */ + const uint8_t *prev; + const uint8_t *curr; + const uint8_t *next; + + /* Pointers to gamma correction tables */ + const uint8_t *red; + const uint8_t *green; + const uint8_t *blue; +}; + +static inline void bggr8_bgr888(const struct ctxt_8bit_src &c, uint8_t *&dst, int x, int p, int n) +{ + *dst++ = c.blue[c.curr[x]]; + *dst++ = c.green[(c.prev[x] + c.curr[x - p] + c.curr[x + n] + c.next[x]) / 4]; + *dst++ = c.red[(c.prev[x - p] + c.prev[x + n] + c.next[x - p] + c.next[x + n]) / 4]; +} + +static inline void grbg8_bgr888(const struct ctxt_8bit_src &c, uint8_t *&dst, int x, int p, int n) +{ + *dst++ = c.blue[(c.prev[x] + c.next[x]) / 2]; + *dst++ = c.green[c.curr[x]]; + *dst++ = c.red[(c.curr[x - p] + c.curr[x + n]) / 2]; +} + +static inline void gbrg8_bgr888(const struct ctxt_8bit_src &c, uint8_t *&dst, int x, int p, int n) +{ + *dst++ = c.blue[(c.curr[x - p] + c.curr[x + n]) / 2]; + *dst++ = c.green[c.curr[x]]; + *dst++ = c.red[(c.prev[x] + c.next[x]) / 2]; +} + +static inline void rggb8_bgr888(const struct ctxt_8bit_src &c, uint8_t *&dst, int x, int p, int n) +{ + *dst++ = c.blue[(c.prev[x - p] + c.prev[x + n] + c.next[x - p] + c.next[x + n]) / 4]; + *dst++ = c.green[(c.prev[x] + c.curr[x - p] + c.curr[x + n] + c.next[x]) / 4]; + *dst++ = c.red[c.curr[x]]; +} + +void DebayerCpu::debayer10P_BGBG_BGR888(uint8_t *dst, const uint8_t *src) { const int width_in_bytes = window_.width * 5 / 4; - /* Pointers to previous, current and next lines */ - const uint8_t *prev = src - inputConfig_.stride; - const uint8_t *curr = src; - const uint8_t *next = src + inputConfig_.stride; + struct ctxt_8bit_src c = { + src - inputConfig_.stride, src, src + inputConfig_.stride, + red_, green_, blue_ }; /* * For the first pixel getting a pixel from the previous column uses * x - 2 to skip the 5th byte with least-significant bits for 4 pixels. * Same for last pixel (uses x + 2) and looking at the next column. + * x++ in the for-loop skips the 5th byte with 4 x 2 lsb-s for 10bit packed. */ - for (int x = 0; x < width_in_bytes; x += 2) { - /* - * BGBG line even pixel: RGR - * GBG - * RGR - * Write BGR - */ - *dst++ = blue_[curr[x]]; - *dst++ = green_[(prev[x] + curr[x - 2] + curr[x + 1] + next[x]) / 4]; - *dst++ = red_[(prev[x - 2] + prev[x + 1] + next[x - 2] + next[x + 1]) / 4]; - x++; - - /* - * BGBG line odd pixel: GRG - * BGB - * GRG - * Write BGR - */ - *dst++ = blue_[(curr[x - 1] + curr[x + 1]) / 2]; - *dst++ = green_[curr[x]]; - *dst++ = red_[(prev[x] + next[x]) / 2]; - x++; - + for (int x = 0; x < width_in_bytes; x++) { + /* Even pixel */ + bggr8_bgr888(c, dst, x++, 2, 1); + /* Odd pixel BGGR -> GBRG */ + gbrg8_bgr888(c, dst, x++, 1, 1); /* Same thing for next 2 pixels */ - *dst++ = blue_[curr[x]]; - *dst++ = green_[(prev[x] + curr[x - 1] + curr[x + 1] + next[x]) / 4]; - *dst++ = red_[(prev[x - 1] + prev[x + 1] + next[x - 1] + next[x + 1]) / 4]; - x++; - - *dst++ = blue_[(curr[x - 1] + curr[x + 2]) / 2]; - *dst++ = green_[curr[x]]; - *dst++ = red_[(prev[x] + next[x]) / 2]; + bggr8_bgr888(c, dst, x++, 1, 1); + gbrg8_bgr888(c, dst, x++, 1, 2); } } -void DebayerCpu::debayerBGGR10PLine1(uint8_t *dst, const uint8_t *src) +void DebayerCpu::debayer10P_GRGR_BGR888(uint8_t *dst, const uint8_t *src) { const int width_in_bytes = window_.width * 5 / 4; - /* Pointers to previous, current and next lines */ - const uint8_t *prev = src - inputConfig_.stride; - const uint8_t *curr = src; - const uint8_t *next = src + inputConfig_.stride; - - for (int x = 0; x < width_in_bytes; x += 2) { - /* - * GRGR line even pixel: GBG - * RGR - * GBG - * Write BGR - */ - *dst++ = blue_[(prev[x] + next[x]) / 2]; - *dst++ = green_[curr[x]]; - *dst++ = red_[(curr[x - 2] + curr[x + 1]) / 2]; - x++; - - /* - * GRGR line odd pixel: BGB - * GRG - * BGB - * Write BGR - */ - *dst++ = blue_[(prev[x - 1] + prev[x + 1] + next[x - 1] + next[x + 1]) / 4]; - *dst++ = green_[(prev[x] + curr[x - 1] + curr[x + 1] + next[x]) / 4]; - *dst++ = red_[curr[x]]; - x++; + struct ctxt_8bit_src c = { + src - inputConfig_.stride, src, src + inputConfig_.stride, + red_, green_, blue_ }; + for (int x = 0; x < width_in_bytes; x++) { + /* Even pixel */ + grbg8_bgr888(c, dst, x++, 2, 1); + /* Odd pixel GRBG -> RGGB */ + rggb8_bgr888(c, dst, x++, 1, 1); /* Same thing for next 2 pixels */ - *dst++ = blue_[(prev[x] + next[x]) / 2]; - *dst++ = green_[curr[x]]; - *dst++ = red_[(curr[x - 1] + curr[x + 1]) / 2]; - x++; - - *dst++ = blue_[(prev[x - 1] + prev[x + 2] + next[x - 1] + next[x + 2]) / 4]; - *dst++ = green_[(prev[x] + curr[x - 1] + curr[x + 2] + next[x]) / 4]; - *dst++ = red_[curr[x]]; + grbg8_bgr888(c, dst, x++, 1, 1); + rggb8_bgr888(c, dst, x++, 1, 2); } } -void DebayerCpu::debayerGBRG10PLine0(uint8_t *dst, const uint8_t *src) +void DebayerCpu::debayer10P_GBGB_BGR888(uint8_t *dst, const uint8_t *src) { const int width_in_bytes = window_.width * 5 / 4; - /* Pointers to previous, current and next lines */ - const uint8_t *prev = src - inputConfig_.stride; - const uint8_t *curr = src; - const uint8_t *next = src + inputConfig_.stride; - - for (int x = 0; x < width_in_bytes; x += 2) { - /* - * GBGB line even pixel: GRG - * BGB - * GRG - * Write BGR - */ - *dst++ = blue_[(curr[x - 2] + curr[x + 1]) / 2]; - *dst++ = green_[curr[x]]; - *dst++ = red_[(prev[x] + next[x]) / 2]; - x++; - - /* - * GBGB line odd pixel: RGR - * GBG - * RGR - * Write BGR - */ - *dst++ = blue_[curr[x]]; - *dst++ = green_[(prev[x] + curr[x - 1] + curr[x + 1] + next[x]) / 4]; - *dst++ = red_[(prev[x - 1] + prev[x + 1] + next[x - 1] + next[x + 1]) / 4]; - x++; + struct ctxt_8bit_src c = { + src - inputConfig_.stride, src, src + inputConfig_.stride, + red_, green_, blue_ }; + for (int x = 0; x < width_in_bytes; x++) { + /* Even pixel */ + gbrg8_bgr888(c, dst, x++, 2, 1); + /* Odd pixel GBGR -> BGGR */ + bggr8_bgr888(c, dst, x++, 1, 1); /* Same thing for next 2 pixels */ - *dst++ = blue_[(curr[x - 1] + curr[x + 1]) / 2]; - *dst++ = green_[curr[x]]; - *dst++ = red_[(prev[x] + next[x]) / 2]; - x++; - - *dst++ = blue_[curr[x]]; - *dst++ = green_[(prev[x] + curr[x - 1] + curr[x + 2] + next[x]) / 4]; - *dst++ = red_[(prev[x - 1] + prev[x + 2] + next[x - 1] + next[x + 2]) / 4]; + gbrg8_bgr888(c, dst, x++, 1, 1); + bggr8_bgr888(c, dst, x++, 1, 2); } } -void DebayerCpu::debayerGBRG10PLine1(uint8_t *dst, const uint8_t *src) +void DebayerCpu::debayer10P_RGRG_BGR888(uint8_t *dst, const uint8_t *src) { const int width_in_bytes = window_.width * 5 / 4; - /* Pointers to previous, current and next lines */ - const uint8_t *prev = src - inputConfig_.stride; - const uint8_t *curr = src; - const uint8_t *next = src + inputConfig_.stride; - - for (int x = 0; x < width_in_bytes; x += 2) { - /* - * RGRG line even pixel: BGB - * GRG - * BGB - * Write BGR - */ - *dst++ = blue_[(prev[x - 2] + prev[x + 1] + next[x - 2] + next[x + 1]) / 4]; - *dst++ = green_[(prev[x] + curr[x - 2] + curr[x + 1] + next[x]) / 4]; - *dst++ = red_[curr[x]]; - x++; - - /* - * RGRG line odd pixel: GBG - * RGR - * GBG - * Write BGR - */ - *dst++ = blue_[(prev[x] + next[x]) / 2]; - *dst++ = green_[curr[x]]; - *dst++ = red_[(curr[x - 1] + curr[x + 1]) / 2]; - x++; + struct ctxt_8bit_src c = { + src - inputConfig_.stride, src, src + inputConfig_.stride, + red_, green_, blue_ }; + for (int x = 0; x < width_in_bytes; x++) { + /* Even pixel */ + rggb8_bgr888(c, dst, x++, 2, 1); + /* Odd pixel RGGB -> GRBG*/ + grbg8_bgr888(c, dst, x++, 1, 1); /* Same thing for next 2 pixels */ - *dst++ = blue_[(prev[x - 1] + prev[x + 1] + next[x - 1] + next[x + 1]) / 4]; - *dst++ = green_[(prev[x] + curr[x - 1] + curr[x + 1] + next[x]) / 4]; - *dst++ = red_[curr[x]]; - x++; - - *dst++ = blue_[(prev[x] + next[x]) / 2]; - *dst++ = green_[curr[x]]; - *dst++ = red_[(curr[x - 1] + curr[x + 2]) / 2]; + rggb8_bgr888(c, dst, x++, 1, 1); + grbg8_bgr888(c, dst, x++, 1, 2); } } @@ -258,22 +199,20 @@ int DebayerCpu::setDebayerFunctions(PixelFormat inputFormat, [[maybe_unused]] Pi bayerFormat.packing == BayerFormat::Packing::CSI2) { switch (bayerFormat.order) { case BayerFormat::BGGR: - debayer0_ = &DebayerCpu::debayerBGGR10PLine0; - debayer1_ = &DebayerCpu::debayerBGGR10PLine1; + debayer0_ = &DebayerCpu::debayer10P_BGBG_BGR888; + debayer1_ = &DebayerCpu::debayer10P_GRGR_BGR888; return 0; case BayerFormat::GBRG: - debayer0_ = &DebayerCpu::debayerGBRG10PLine0; - debayer1_ = &DebayerCpu::debayerGBRG10PLine1; + debayer0_ = &DebayerCpu::debayer10P_GBGB_BGR888; + debayer1_ = &DebayerCpu::debayer10P_RGRG_BGR888; return 0; case BayerFormat::GRBG: - /* GRBG is BGGR with the lines swapped */ - debayer0_ = &DebayerCpu::debayerBGGR10PLine1; - debayer1_ = &DebayerCpu::debayerBGGR10PLine0; + debayer0_ = &DebayerCpu::debayer10P_GRGR_BGR888; + debayer1_ = &DebayerCpu::debayer10P_BGBG_BGR888; return 0; case BayerFormat::RGGB: - /* RGGB is GBRG with the lines swapped */ - debayer0_ = &DebayerCpu::debayerGBRG10PLine1; - debayer1_ = &DebayerCpu::debayerGBRG10PLine0; + debayer0_ = &DebayerCpu::debayer10P_RGRG_BGR888; + debayer1_ = &DebayerCpu::debayer10P_GBGB_BGR888; return 0; default: break;
Hi! > >> ATM the statistics and debayer classes are mostly ready, > >> but I need to merge this with Andrey's v2 RFC and > >> then produce a clean new series from this > >> (assuming people like the approach). > > > > Ok, what about this one? I only converted half of the code, but the > > other half of conversion should be very similar. > > > > Compile-tested only as my sensor is bayer 8. > > > > It makes it easier to see how we are skipping the low bits in 10P > > bayer, and will make bayer 8 support easier, too, since the pixel_XXX > > helpers are designed to be shared. > > I have merged a variant of this with: > > 1. The missing bits completed > 2. Various renames to make it clear that the helpers are for 8bit bayer > sources (we are treating 10bpp packed as 8 bit here) and that > the output is bgr888 this should make life easier when adding > support for more input / output fmts. > > Here is what I've merged / squashed: Thanks a lot, looks good to me! Best regards, Pavel
Hi all, On 12/12/23 22:38, Hans de Goede wrote: <snip> > Anyways if you have time, please take a look > at the swstats base class + linaro implementation > as well as at the debayer base class as well > as the DebayerCpu implementation of that: > > https://gitlab.freedesktop.org/camera/libcamera-softisp/-/tree/SoftwareISP-v02-hans2 > > Which are the last few commits there. > > Note as I already mentioned I really need to merge > this with Andrey's latest v03 branch into a new > logical clean patch series suitable for upstream submission. And as promised during our call today I now have a new v03 based branch ready integrating my put swstats and debayer into separate classes work (with the more optimized implementations): https://gitlab.freedesktop.org/camera/libcamera-softisp/-/commits/SoftwareISP-v03-hans1/ The big change compared to Andrey's SoftwareISP-v03 / RFC v2 posting is: "libcamera: software_isp: Make swisp_linaro use the new DebayerCpu class " Andrey, if you can take a look at this and let me know if those changes are ok with you that would be great. The intend is to squash "Make swisp_linaro use the new DebayerCpu class" into the patch adding swisp_linaro before submitting a new series upstream. After the squashing the only item remaining before posting a new version upstream is renaming things away from using the linaro name for the current swisp and softipa implementations, I will do this rename tomorrow. Regards, Hans
Hi Hans, On 14.12.2023 00:54, Hans de Goede wrote: > Hi all, > > On 12/12/23 22:38, Hans de Goede wrote: > > <snip> > >> Anyways if you have time, please take a look >> at the swstats base class + linaro implementation >> as well as at the debayer base class as well >> as the DebayerCpu implementation of that: >> >> https://gitlab.freedesktop.org/camera/libcamera-softisp/-/tree/SoftwareISP-v02-hans2 >> >> Which are the last few commits there. >> >> Note as I already mentioned I really need to merge >> this with Andrey's latest v03 branch into a new >> logical clean patch series suitable for upstream submission. > > And as promised during our call today I now have > a new v03 based branch ready integrating my > put swstats and debayer into separate classes > work (with the more optimized implementations): > > https://gitlab.freedesktop.org/camera/libcamera-softisp/-/commits/SoftwareISP-v03-hans1/ > > The big change compared to Andrey's SoftwareISP-v03 / > RFC v2 posting is: > > "libcamera: software_isp: Make swisp_linaro use the new DebayerCpu class" > > Andrey, if you can take a look at this and let me know if those > changes are ok with you that would be great. Your changes look good to me. And I do see significant fps increase for the higher resolutions on my target. Now the fps reaches the frame rate from the camera sensor for all the resolutions - up to the maximal 8M@15Hz. > The intend is to squash "Make swisp_linaro use the new DebayerCpu class" > into the patch adding swisp_linaro before submitting a new series > upstream. Agreed. > After the squashing the only item remaining before posting > a new version upstream is renaming things away from using > the linaro name for the current swisp and softipa implementations, > I will do this rename tomorrow. Sounds good! Thanks, Andrey > Regards, > > Hans > > > >
diff --git a/include/libcamera/internal/software_isp/meson.build b/include/libcamera/internal/software_isp/meson.build new file mode 100644 index 00000000..a24fe7df --- /dev/null +++ b/include/libcamera/internal/software_isp/meson.build @@ -0,0 +1,5 @@ +# SPDX-License-Identifier: CC0-1.0 + +libcamera_internal_headers += files([ + 'statistics-linaro.h', +]) diff --git a/include/libcamera/internal/software_isp/statistics-linaro.h b/include/libcamera/internal/software_isp/statistics-linaro.h new file mode 100644 index 00000000..20c64e44 --- /dev/null +++ b/include/libcamera/internal/software_isp/statistics-linaro.h @@ -0,0 +1,17 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2023, Linaro Ltd + * + * statistics.h - Statistics data format used by the software ISP + */ + +#pragma once + +namespace libcamera { + +struct SwIspStats { + float bright_ratio; + float too_bright_ratio; +}; + +} /* namespace libcamera */ diff --git a/meson_options.txt b/meson_options.txt index 5fdc7be8..49939421 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -27,7 +27,7 @@ option('gstreamer', option('ipas', type : 'array', - choices : ['ipu3', 'rkisp1', 'rpi/vc4', 'vimc'], + choices : ['ipu3', 'rkisp1', 'rpi/vc4', 'simple/linaro', 'vimc'], description : 'Select which IPA modules to build') option('lc-compliance', @@ -46,6 +46,7 @@ option('pipelines', 'rkisp1', 'rpi/vc4', 'simple', + 'simple/linaro', 'uvcvideo', 'vimc' ], diff --git a/src/ipa/simple/linaro/data/meson.build b/src/ipa/simple/linaro/data/meson.build new file mode 100644 index 00000000..33548cc6 --- /dev/null +++ b/src/ipa/simple/linaro/data/meson.build @@ -0,0 +1,9 @@ +# SPDX-License-Identifier: CC0-1.0 + +conf_files = files([ + 'soft.conf', +]) + +install_data(conf_files, + install_dir : ipa_data_dir / 'soft', + install_tag : 'runtime') diff --git a/src/ipa/simple/linaro/data/soft.conf b/src/ipa/simple/linaro/data/soft.conf new file mode 100644 index 00000000..0c70e7c0 --- /dev/null +++ b/src/ipa/simple/linaro/data/soft.conf @@ -0,0 +1,3 @@ +# SPDX-License-Identifier: LGPL-2.1-or-later +# +# Dummy configuration file for the soft IPA. diff --git a/src/ipa/simple/linaro/meson.build b/src/ipa/simple/linaro/meson.build new file mode 100644 index 00000000..97bf5d6f --- /dev/null +++ b/src/ipa/simple/linaro/meson.build @@ -0,0 +1,26 @@ +# SPDX-License-Identifier: CC0-1.0 + +ipa_name = 'ipa_soft_linaro' + +mod = shared_module(ipa_name, + ['soft_linaro.cpp', libcamera_generated_ipa_headers], + name_prefix : '', + include_directories : [ipa_includes, libipa_includes, '..'], + dependencies : libcamera_private, + link_with : libipa, + link_whole : soft_ipa_common_lib, + install : true, + install_dir : ipa_install_dir) + +if ipa_sign_module + custom_target(ipa_name + '.so.sign', + input : mod, + output : ipa_name + '.so.sign', + command : [ipa_sign, ipa_priv_key, '@INPUT@', '@OUTPUT@'], + install : false, + build_by_default : true) +endif + +subdir('data') + +ipa_names += ipa_name diff --git a/src/ipa/simple/linaro/soft_linaro.cpp b/src/ipa/simple/linaro/soft_linaro.cpp new file mode 100644 index 00000000..0b2e83bf --- /dev/null +++ b/src/ipa/simple/linaro/soft_linaro.cpp @@ -0,0 +1,210 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2023, Linaro Ltd + * + * soft.cpp - Software Image Processing Algorithm module + */ + +#include <sys/mman.h> + +#include <libcamera/base/file.h> +#include <libcamera/base/log.h> + +#include <libcamera/control_ids.h> + +#include <libcamera/ipa/ipa_interface.h> +#include <libcamera/ipa/ipa_module_info.h> + +#include "libcamera/internal/camera_sensor.h" +#include "libcamera/internal/software_isp/statistics-linaro.h" + +#include "common/soft_base.h" + +namespace libcamera { + +LOG_DECLARE_CATEGORY(IPASoft) + +namespace ipa::soft { + +class IPASoftLinaro final : public IPASoftBase +{ +public: + IPASoftLinaro() + : IPASoftBase(), ignore_updates_(0) + { + } + + ~IPASoftLinaro() + { + if (stats_) + munmap(stats_, sizeof(SwIspStats)); + } + + int platformInit(const ControlInfoMap &sensorInfoMap) override; + int platformConfigure(const ControlInfoMap &sensorInfoMap) override; + int platformStart() override; + void platformStop() override; + void platformProcessStats(const ControlList &sensorControls) override; + +private: + void update_exposure(double ev_adjustment); + + SwIspStats *stats_; + int exposure_min_, exposure_max_; + int again_min_, again_max_; + int again_, exposure_; + int ignore_updates_; +}; + +int IPASoftLinaro::platformInit(const ControlInfoMap &sensorInfoMap) +{ + stats_ = static_cast<SwIspStats *>(mmap(nullptr, sizeof(SwIspStats), + PROT_READ | PROT_WRITE, MAP_SHARED, + fdStats_.get(), 0)); + if (!stats_) { + LOG(IPASoft, Error) << "Unable to map Statistics"; + return -ENODEV; + } + + if (sensorInfoMap.find(V4L2_CID_EXPOSURE) == sensorInfoMap.end()) { + LOG(IPASoft, Error) << "Don't have exposure control"; + return -EINVAL; + } + + if (sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN) == sensorInfoMap.end()) { + LOG(IPASoft, Error) << "Don't have gain control"; + return -EINVAL; + } + + return 0; +} + +int IPASoftLinaro::platformConfigure(const ControlInfoMap &sensorInfoMap) +{ + const ControlInfo &exposure_info = sensorInfoMap.find(V4L2_CID_EXPOSURE)->second; + const ControlInfo &gain_info = sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN)->second; + + exposure_min_ = exposure_info.min().get<int>(); + if (!exposure_min_) { + LOG(IPASoft, Warning) << "Minimum exposure is zero, that can't be linear"; + exposure_min_ = 1; + } + exposure_max_ = exposure_info.max().get<int>(); + again_min_ = gain_info.min().get<int>(); + if (!again_min_) { + LOG(IPASoft, Warning) << "Minimum gain is zero, that can't be linear"; + again_min_ = 100; + } + again_max_ = gain_info.max().get<int>(); + + LOG(IPASoft, Info) << "Exposure " << exposure_min_ << "-" << exposure_max_ + << ", gain " << again_min_ << "-" << again_max_; + + return 0; +} + +int IPASoftLinaro::platformStart() +{ + return 0; +} + +void IPASoftLinaro::platformStop() +{ +} + +void IPASoftLinaro::platformProcessStats(const ControlList &sensorControls) +{ + double ev_adjustment = 0.0; + ControlList ctrls(sensorControls); + + /* + * Use 2 frames delay to make sure that the exposure and the gain set + * have applied to the camera sensor + */ + if (ignore_updates_ > 0) { + LOG(IPASoft, Debug) << "Skipping exposure update: " + << ignore_updates_; + --ignore_updates_; + return; + } + + if (stats_->bright_ratio < 0.01) + ev_adjustment = 1.1; + if (stats_->too_bright_ratio > 0.04) + ev_adjustment = 0.9; + + if (ev_adjustment != 0.0) { + /* sanity check */ + if (!sensorControls.contains(V4L2_CID_EXPOSURE) || + !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) { + LOG(IPASoft, Error) << "Control(s) missing"; + return; + } + + exposure_ = ctrls.get(V4L2_CID_EXPOSURE).get<int>(); + again_ = ctrls.get(V4L2_CID_ANALOGUE_GAIN).get<int>(); + + update_exposure(ev_adjustment); + + ctrls.set(V4L2_CID_EXPOSURE, exposure_); + ctrls.set(V4L2_CID_ANALOGUE_GAIN, again_); + + ignore_updates_ = 2; + + setSensorControls.emit(ctrls); + } +} + +void IPASoftLinaro::update_exposure(double ev_adjustment) +{ + double exp = (double)exposure_; + double gain = (double)again_; + double ev = ev_adjustment * exp * gain; + + /* + * Try to use the minimal possible analogue gain. + * The exposure can be any value from exposure_min_ to exposure_max_, + * and normally this should keep the frame rate intact. + */ + + exp = ev / again_min_; + if (exp > exposure_max_) + exposure_ = exposure_max_; + else if (exp < exposure_min_) + exposure_ = exposure_min_; + else + exposure_ = (int)exp; + + gain = ev / exposure_; + if (gain > again_max_) + again_ = again_max_; + else if (gain < again_min_) + again_ = again_min_; + else + again_ = (int)gain; + + LOG(IPASoft, Debug) << "Desired EV = " << ev + << ", real EV = " << (double)again_ * exposure_; +} + +} /* namespace ipa::soft */ + +/* + * External IPA module interface + */ +extern "C" { +const struct IPAModuleInfo ipaModuleInfo = { + IPA_MODULE_API_VERSION, + 0, + "SimplePipelineHandler", + "soft/linaro", +}; + +IPAInterface *ipaCreate() +{ + return new ipa::soft::IPASoftLinaro(); +} + +} /* extern "C" */ + +} /* namespace libcamera */ diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build index 9688bbdb..14be5dc2 100644 --- a/src/ipa/simple/meson.build +++ b/src/ipa/simple/meson.build @@ -1,3 +1,12 @@ # SPDX-License-Identifier: CC0-1.0 subdir('common') + +foreach pipeline : pipelines + pipeline = pipeline.split('/') + if pipeline.length() < 2 or pipeline[0] != 'simple' + continue + endif + + subdir(pipeline[1]) +endforeach
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> --- .../internal/software_isp/meson.build | 5 + .../internal/software_isp/statistics-linaro.h | 17 ++ meson_options.txt | 3 +- src/ipa/simple/linaro/data/meson.build | 9 + src/ipa/simple/linaro/data/soft.conf | 3 + src/ipa/simple/linaro/meson.build | 26 +++ src/ipa/simple/linaro/soft_linaro.cpp | 210 ++++++++++++++++++ src/ipa/simple/meson.build | 9 + 8 files changed, 281 insertions(+), 1 deletion(-) create mode 100644 include/libcamera/internal/software_isp/meson.build create mode 100644 include/libcamera/internal/software_isp/statistics-linaro.h create mode 100644 src/ipa/simple/linaro/data/meson.build create mode 100644 src/ipa/simple/linaro/data/soft.conf create mode 100644 src/ipa/simple/linaro/meson.build create mode 100644 src/ipa/simple/linaro/soft_linaro.cpp