[feat]IBA:demosaic add white balancing #4499

antond-weta · 2024-10-22T02:11:01Z

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 · 2024-10-25T21:00:27Z

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 · 2024-10-25T22:21:39Z

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 · 2024-10-26T01:43:18Z

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 · 2024-10-27T01:18:27Z

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 · 2024-10-27T22:09:59Z

src/doc/imagebufalgo.rst

+          float wb[4] = {2.0, 0.8, 1.2, 1.5};
+          ParamValue options[] = {
+                { "layout", "GRBG" },
+                ParamValue("white-balance", TypeFloat, 4, wb)
+          };


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.

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

lgritz · 2024-10-27T22:26:48Z

src/oiiotool/oiiotool.cpp

+    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])) {


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()) {

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

lgritz · 2024-10-27T22:34:10Z

src/python/py_imagebufalgo.cpp

+                    "wb_r"_a = 1.0f, "wb_g1"_a = 1.0f, "wb_g2"_a = 1.0f,
+                    "wb_b"_a = 1.0f, "roi"_a = ROI::All(), "nthreads"_a = 0)


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?

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

testsuite/oiiotool-demosaic/run.py

src/doc/pythonbindings.rst

lgritz · 2024-10-29T16:52:45Z

src/include/OpenImageIO/imagebufalgo.h

+///   - "white-balance" : float[3] or float[4] (default: {1.0, 1.0, 1.0, 1.0})
+///
+///     Optional white-balancing weights. Can contain either three (RGB), or four (RGGB) values.


Is there a use case for specifying two green values?

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

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

src/doc/oiiotool.rst

lgritz · 2024-10-29T17:00:07Z

I'm confused about the white balance values.

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.
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 marked this pull request as draft October 22, 2024 02:11

lgritz reviewed Oct 25, 2024

View reviewed changes

antond-weta marked this pull request as ready for review October 27, 2024 01:19

lgritz requested changes Oct 27, 2024

View reviewed changes

lgritz requested changes Oct 29, 2024

View reviewed changes

		"wb_r"_a = 1.0f, "wb_g1"_a = 1.0f, "wb_g2"_a = 1.0f,
		"wb_b"_a = 1.0f, "roi"_a = ROI::All(), "nthreads"_a = 0)

[feat]IBA:demosaic add white balancing #4499

[feat]IBA:demosaic add white balancing #4499

Uh oh!

Conversation

antond-weta commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Tests

Checklist:

Uh oh!

lgritz left a comment

Choose a reason for hiding this comment

Uh oh!

antond-weta commented Oct 25, 2024

Uh oh!

lgritz commented Oct 25, 2024

Uh oh!

antond-weta commented Oct 26, 2024

Uh oh!

antond-weta commented Oct 27, 2024

Uh oh!

lgritz left a comment

Choose a reason for hiding this comment

Uh oh!

lgritz Oct 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lgritz commented Oct 29, 2024

antond-weta commented Oct 22, 2024 •

edited

Loading

lgritz Oct 27, 2024 •

edited

Loading