[feat]IBA:demosaic add white balancing

[antond-weta]

Description

Adds white balancing functionality to IBA:demosaic

Tests

Updated unittests

Checklist:

• I have read the contribution guidelines.
• I have updated the documentation, if applicable. (Check if there is no
  need to update the documentation, for example if this is a bug fix that
  doesn't change the API.)
• I have ensured that the change is tested somewhere in the testsuite
  (adding new test cases if necessary).
• If I added or modified a C++ API call, I have also amended the
  corresponding Python bindings (and if altering ImageBufAlgo functions, also
  exposed the new functionality as oiiotool options).
• My code follows the prevailing code style of this project. If I haven't
  already run clang-format before submitting, I definitely will look at the CI
  test that runs clang-format and fix anything that it highlights as being
  nonconforming.

[lgritz]

This LGTM. It's still marked as "draft". Is it ready to be reviewed and merged?

[antond-weta]

Hi Larry, if you're happy with the code, I just need to update the docs and add some tests.

I've also just realised we could add an "auto" or "meta" value to the Bayer layout and white balance parameters, which would pull the values from the ImageBuf attributes.

[lgritz]

I'm happy with the code. Docs+tests should take it over the finish line.

I'm not quite sure what you mean about the ImageBuf attributes? Do you mean that when the image is read in, you expect the raw reader would drop hints in the spec about the default white balancing method to use, so IBA::demosaic should honor that if it gets no other overriding instructions? Or did you mean something different?

[antond-weta]

Yep, exactly that. The raw reader currently adds the "raw:FilterPattern" attribute to the output when asked not to debayer. We could do something similar for white balancing. We can leave this for later, as adding an auto white balancing mode should not change the default behaviour, I think. If we were to add the "auto" layout pattern, it should become the default, as the current default behaviour is to assume RGGB. This is beyond the scope of this change though.

I'll update the docs, tests and the python bindings.

[antond-weta]

Done!
Larry, could you please check my python bindings, I'm not sure if there is a better way.

[lgritz]

Also, I think before this is ready to merge, we will need the docs to be updated for all of C++ (imagebufalgo.h), Python (pythonbindings.rst), and oiiotool (oiiotool.rst), to explain the new parameters.

[lgritz]

Not requesting this as part of this PR, but just noting that at some point, we should do the thing where we move these actual code snippets to the appropriate parts of testsuite/docs-examples-cpp and testsuite/docs-examples-python, and then just include them by reference in the docs, so that it is confirmed that they compile and work correct with every CI run.

[antond-weta]

I can take a look on the weekend. Marking as a draft for now.

[lgritz]

I think there is a more compact alternative, and also more correct, because it will fail if the string contains any extra non-whitespace after the 4th value.

Suggested change

	string_view str(wb);
	float white_balance[4] = { 1.0f, 1.0f, 1.0f, 1.0f };
	if (str.size() && Strutil::parse_float(str, white_balance[0])
	&& Strutil::parse_char(str, ',')
	&& Strutil::parse_float(str, white_balance[1])
	&& Strutil::parse_char(str, ',')
	&& Strutil::parse_float(str, white_balance[2])
	&& Strutil::parse_char(str, ',')
	&& Strutil::parse_float(str, white_balance[3])) {
	string_view str(wb);
	Strutil::remove_trailing_whitespace(str);
	float white_balance[4] = { 1.0f, 1.0f, 1.0f, 1.0f };
	if (Strutil::parse_values(str, "", white_balance, ",") && str.size()) {

[antond-weta]

Done! I've also decided to allow 3 values, for RGB, and extend them to RGGB automatically.

[lgritz]

This part, I'm not so sure about. Would it be better to pass white_balance as one parameter that expects a tuple or array (checked to be 4 floats), versus 4 separate float parameters?

I don't have a strong feeling one way or the other. I'm just wondering if that would be more symmetric with the C++ API, and/or more idiomatic for a Python native speaker?

[antond-weta]

I've changed that to use tuples, seems to be more inline with other python bindings

[lgritz]

Is there a use case for specifying two green values?

[antond-weta]

Yes, I have seen some images recently with the greens in red and blue rows having distinctly different levels. Sony A99, I think.

[antond-weta]

Is there a use case for specifying two green values?

RAW_PENTAX_K200D.PEF in the test suite has 2 different weights for the green sub-channels

[lgritz]

I'm confused about the white balance values.

1. When you do give 4 values, are they always in the order RGGB, or are they in the order implied by the "layout"? I think we need to be more clear about this.

2. Will anybody ever really want 4 values, with two different white balance values for the two greens? If not, a lot of things get easier if we just document the type as a Color3 instead of an array of floats that might be either 3 or 4 elements long.

[antond-weta]

Hi Larry,

I've been mostly following exiftool, which shows WB as either RGB or RGGB, regardless of the layout:

exiftool G85.RW2 | grep 'WB RG'
WB RGB Levels 1                 : 612 256 399
WB RGB Levels 2                 : 657 256 372
WB RGB Levels 3                 : 703 256 354
WB RGB Levels 4                 : 420 256 575
WB RGB Levels 5                 : 632 256 393
WB RGB Levels 6                 : 576 256 400
WB RGB Levels 7                 : 420 256 575

exiftool R5.CR3 | grep 'WB RG'
WB RGGB Levels As Shot          : 2241 1024 1024 1771
WB RGGB Levels Auto             : 2241 1024 1024 1771
WB RGGB Levels Measured         : 2241 1024 1024 1771
WB RGGB Levels Daylight         : 1960 1024 1024 1913
WB RGGB Levels Shade            : 2294 1024 1024 1636
WB RGGB Levels Cloudy           : 2118 1024 1024 1777
WB RGGB Levels Tungsten         : 1348 1024 1024 2962
WB RGGB Levels Fluorescent      : 1667 1024 1024 2819
WB RGGB Levels Kelvin           : 1960 1024 1024 1913
WB RGGB Levels Flash            : 2203 1024 1024 1708
WB RGGB Levels                  : 2241 1024 1024 1771

Libraw stores the weights as RGBG, regardless of the layout. I find the RGGB easier to understand, because the order also represents a valid Bayer layout, so it is clear where each G sits.

[lgritz]

Great, precedence from another widely used tool is a good reason for doing it a particular way.

[kmilos]

Libraw stores the weights as RGBG, regardless of the layout. I find the RGGB easier to understand, because the order also represents a valid Bayer layout, so it is clear where each G sits.

Please note that exiftool in no way chooses the order of WB coeffs, it is stored by the camera vendor in their MakerNotes in some particular order, and exiftool reports it verbatim. The majority of vendors does indeed seem to use use RGB or RGGB, but I guess this shouldn't be assumed... Canon might use GRBG for example for some older models. And this vendor WB order might not necessarily be related to the actual Bayer layout... (E.g. Fujifilm reports GRGB and most of their sensors are 6x6 X-Trans pattern.)

LibRaw, OTOH, does normalize this to RGB(G) once it parses the vendor MakerNotes.

[antond-weta]

Thanks @kmilos,
That is good info. I'm happy to change to RGB(G), if that's the preferred way.

[lgritz]

Where are we on this? Are there any intended changes still pending, or is it in its intended final form for review?

[antond-weta]

Hi @lgritz, let me change that to RGB(G) as Miloš suggested.

Also I think we should change the default ROI to be the data window, not the full size. Do you mind if I do it here instead of creating a separate PR?

[lgritz]

Also I think we should change the default ROI to be the data window, not the full size. Do you mind if I do it here instead of creating a separate PR?

Up to you. Fine to do it here.

[lgritz]

LGTM