Add data-dir argument and auto matrix method #189

mikaelsundell · 2025-09-08T16:11:02Z

Added --data-dir argument (Fixes add a command line parameter for the database search path #180)
Added auto matrix method (Fixes Add "auto" matrix method and make it default. #181)
Add size checks for lambda array assignments

antond-weta · 2025-09-08T20:56:07Z

src/rawtoaces_util/image_converter.cpp

-            for ( int i = 0; i < 4; i++ )
-                settings.WB_box[i] = 0;
-        } );
+    if ( WB_box.size() == 4 )


This check is not needed as it is performed inside of check_param, I assume you were fixing the crash we introduced recently, we do have a PR with a fix #186

Yes, I was too fast — now removed.

src/rawtoaces_util/image_converter.cpp

antond-weta · 2025-09-08T21:17:13Z

src/rawtoaces_util/image_converter.cpp

+    std::string data_dir = arg_parser["data-dir"].get();
+    if ( data_dir.size() )
+    {
+        OIIO::Strutil::split( data_dir, settings.database_directories, ":" );


Will this work on windows? See the logic in database_paths(). Perhaps it would be better to pass this string as a parameter to database_paths() so all pass resolving logic is in one place.

Fixed in latest commit.

codecov-commenter · 2025-09-09T07:28:41Z

Codecov Report

❌ Patch coverage is 27.89116% with 106 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.33%. Comparing base (c2a61d2) to head (88a5f9b).

Files with missing lines	Patch %	Lines
src/rawtoaces_util/image_converter.cpp	27.39%	106 Missing ⚠️

❌ Your patch status has failed because the patch coverage (27.89%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #189      +/-   ##
==========================================
- Coverage   74.40%   71.33%   -3.07%     
==========================================
  Files          10       10              
  Lines        2227     2348     +121     
  Branches      292      311      +19     
==========================================
+ Hits         1657     1675      +18     
- Misses        570      673     +103

Files with missing lines	Coverage Δ
include/rawtoaces/image_converter.h	`100.00% <100.00%> (ø)`
src/rawtoaces_core/rawtoaces_core.cpp	`86.15% <ø> (ø)`
src/rawtoaces_util/image_converter.cpp	`52.12% <27.39%> (-4.58%)`	⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2a61d2...88a5f9b. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mikaelsundell · 2025-09-09T07:54:26Z

The new changes, along with a draft for verbose output, are ready for review. Once they are approved, I’ll add tests to improve coverage and address the Codecov report.

antond-weta · 2025-09-09T08:59:41Z

include/rawtoaces/image_converter.h

+    ///     Output flag that becomes true if no files were processed.
+    /// @result
+    ///     true if all files were processed successfully, false otherwise.
+    bool process_batch(


I like all other changes, but I find this one questionable.

Is this important enough to warrant an API change? It is just a loop, why can't it live on the client side?

the name is misleading, it should be called process_batches

the output empty flag looks kludgy for a public API

I prefer this logic being on the client side, because the user can decide how to treat failures - is it important that all files succeed? shall we continue if a file failed? does a failed file means the whole sequence is broken, or a single batch? All these decisions should be on the client side.

Thanks, this is a very quick draft/ idea for discussion, in short, I moved it into the image converter to make it a part of the library (using settings.verbosity) etc. The empty flag is redundant as-is. Guess we would need to provide some sort of status per file after a run to make it work, let's drop the idea for now and let the client do all batch processing?

Yes, let's drop it for now, we can return to this at a later stage. I'm anticipating the image processing pipeline being much longer in the future, with many more steps added. We may need to reconsider the error reporting and failure detection logic then. Please finish up the rest of the PR and we'll get it merged in.

Done, you could argue for having a bit more output even when not calling -v, like at least giving info about the current file being processed. Right now it's silent except for OIIO output for debug builds.

soswow · 2025-09-10T23:16:09Z

The new changes, along with a draft for verbose output, are ready for review. Once they are approved, I’ll add tests to improve coverage and address the Codecov report.

See my other PR curretly open. I was adding those tests before I saw your PR. Just be mindfull of it.

mikaelsundell · 2025-09-11T08:17:33Z

To get better test coverage are we using util_usage.cpp for matrix mode Auto and new data-dir arguments? The rest of the PR should be about verbose output.

antond-weta · 2025-09-11T08:50:12Z

To get better test coverage are we using util_usage.cpp for matrix mode Auto and new data-dir arguments? The rest of the PR should be about verbose output.

No, the util_usage.cpp is an example of using the new API, as it is very different from the old one. I linked it in the release notes. We just added it to the test suite to be sure it works. Any additional testing should be in proper unit-tests.

antond-weta · 2025-09-11T08:51:27Z

we may want to rename it to util_usage_example.cpp perhaps

antond-weta · 2025-09-11T22:11:25Z

@mikaelsundell could you please rebase from main and resolve conflicts

antond-weta · 2025-09-12T21:04:16Z

src/rawtoaces/main.cpp

+
+    for ( auto const &batch: batches )
+        total_files += static_cast<int>( batch.size() );
+


why not use size_t and avoid casting? this use case is exactly what size_t is for

antond-weta · 2025-09-12T21:06:20Z

src/rawtoaces_util/image_converter.cpp

    if ( settings.verbosity > 0 )
    {
-        std::cerr << "Input transform matrix:" << std::endl;
+        std::cerr << "Input Device Transform (IDT) matrix:" << std::endl;


The ACES group has stopped using the term IDT some time ago. I've been trying to minimise its usage here.

antond-weta · 2025-09-12T21:15:31Z

Looks good. Please don't forget to address your changes in README.

soswow · 2025-09-13T02:31:41Z

database_paths is now fully unit tests in tests/test_image_converter.cpp. Now you can add/modify tests to cover new logic.

soswow · 2025-09-13T02:35:35Z

src/rawtoaces_util/image_converter.cpp

+    else
+    {
+        settings.database_directories = database_paths();
+    }


there are unit tests now for list-cameras and list-illuminants. They are the only but maybe the easest way you can test functionality above. Tests currently setup test-dit location via env. You could have one more test for cameras with same exact logic, but setup path with --data-dir and it would test new branch above.

soswow · 2025-09-13T02:39:14Z

src/rawtoaces_util/image_converter.cpp

+                  << ( settings.create_dirs ? "yes" : "no" ) << std::endl;
+        std::cerr << "  Verbosity: " << settings.verbosity << std::endl;
+    }
+


I feel line all this above can live in own private function. Takes settings in and prints it out. print_out_settings() or something.

soswow · 2025-09-13T02:39:54Z

src/rawtoaces_util/image_converter.cpp

+        {
+            std::cerr << "  Crop box: [" << settings.crop_box[0] << ", "
+                      << settings.crop_box[1] << ", " << settings.crop_box[2]
+                      << ", " << settings.crop_box[3] << "]" << std::endl;


check out OIIO::Strutil::join()

soswow · 2025-09-13T02:47:06Z

src/rawtoaces_util/image_converter.cpp

+        if ( settings.matrix_method == Settings::MatrixMethod::Custom )
+        {
+            std::cerr << "  Custom matrix:" << std::endl;
+            for ( int i = 0; i < 3; i++ )


/nit there is FORI(3) template available if you want

Please don't. Let them die already.

oh! I didn't know that's the direction we have. Sry, I don't have good feel for what's better, I thought it's short and neat. Let me kill the rest of them them, so it's not confusinb

soswow · 2025-09-13T02:48:51Z

src/rawtoaces_util/image_converter.cpp

        settings.WB_method == Settings::WBMethod::Illuminant;
-    bool spectral_matrix =
-        settings.matrix_method == Settings::MatrixMethod::Spectral;
+    bool spectral_matrix = matrix_method == Settings::MatrixMethod::Spectral;


/nit is_spectral_matrix? otherwise one might think it's spectral matrix itself
Though, I can see spectral_white_balance above already spoils my ask ha ha

soswow · 2025-09-13T02:57:54Z

include/rawtoaces/image_converter.h

+            ///   use `Spectral`.
+            /// - Otherwise, fall back to `Metadata`.
+            /// The "auto" mode solves this by first attempting "spectral", and if no
+            /// spectral data is available for the camera, it falls back to "metadata".


/nit last two lines are repeating 2 lines before that.

mikaelsundell · 2025-09-13T10:23:27Z

The ACES group is moving away from using IDTs. Should we make rawtoaces consistent with this naming convention?
rawtoaces_util_priv.h is now introduced. Should we include it in image_converter.cpp? If not, would it be better to move it to the tests? At the moment, we’re duplicating the function definition with a default argument for override_path.
Running rawtoaces without -v produces no output. Should we at least print the file being processed or any failures as a default output in main.cpp?

antond-weta · 2025-09-13T22:58:57Z

The ACES group is moving away from using IDTs. Should we make rawtoaces consistent with this naming convention?

I would leave them in the code, where it makes sense. get_IT_matrix is not any better than get_IDT_matrix IMO. I would just refrain from introducing IDT in the documentation and comments if we can.

rawtoaces_util_priv.h is now introduced. Should we include it in image_converter.cpp? If not, would it be better to move it to the tests? At the moment, we’re duplicating the function definition with a default argument for override_path.

Yes, that is purely for unit-testing. No objection to moving to test. We should avoid duplicating things.

Running rawtoaces without -v produces no output. Should we at least print the file being processed or any failures as a default output in main.cpp?

Yes, we should be reporting the errors regardless of the verbosity level. I have no strong opinion on reporting progress. If we could suppress the OIIO debug messages, that would be great.

soswow · 2025-09-14T10:53:03Z

/nit

Reordered non–class member functions in rawtoaces_core.cpp so that dependencies are defined before use

Was this required in this PR? I just worry it get's very big and I am not sure if there were any changes in those methods that got moved. Seems could have been own separate PR.

mikaelsundell · 2025-09-14T11:09:10Z

"Yes, that is purely for unit-testing. No objection to moving to test. We should avoid duplicating things."

The reordering was required to remove the private header (which belongs in tests). It’s not actually needed in core, it was just there to satisfy the tests. Removing it exposed an ordering issue: functions must be declared before they are used.

... and Yes, it could of course be its own PR, it just depends on where we want to spend our time :-) The project is still manageable, but I’d like to move on as quickly as possible to focus on the GUI and the actual RAW processing. Every PR/ change/ rebase is extra work to make sure it stays up to date with the changes in main.

@antond-weta Please double-check the updated comments for calculate_daylight_SPD and calculate_blackbody_SPD. I first wrote my own version and then refined it with copilot.

antond-weta · 2025-09-14T20:13:45Z

Was this required in this PR? I just worry it get's very big and I am not sure if there were any changes in those methods that got moved. Seems could have been own separate PR.

I agree with this. We should strive to have smaller PRs, which are easier to write, to review, and, probably the most important, to maintain the code afterwards. When you have a PR implementing 10 different things, the git history is essentially useless after squashing the commits.

The reordering was required to remove the private header (which belongs in tests). It’s not actually needed in core, it was just there to satisfy the tests. Removing it exposed an ordering issue: functions must be declared before they are used.

Doesn't reordering also mean that we loose git history, or making using git blame much more difficult? I'd rather had duplicated declarations than all history lost.

Every PR/ change/ rebase is extra work to make sure it stays up to date with the changes in main.

This is exactly why you want several smaller PRs. The smaller the PR, the less the chance it will require any rebasing. Sure, it is a bit more work to create them, but then the amount of time saved for everyone involved is huge.

This PR is actually a very good example - the amount of back and forth this has seen is ridiculous comparing to the amount of change it introduces. 3 separate PRs for each of the features would have been merged in long time ago, and had clean git history, and be easier to maintain for those who come after us.

antond-weta · 2025-09-14T20:19:24Z

Please double-check the updated comments for calculate_daylight_SPD and calculate_blackbody_SPD.

These already had comments on the public header, what am I missing?

mikaelsundell · 2025-09-15T13:31:27Z

Ok, you win - 2 against 1! :-)
I promise not to submit any more mega-PRs that complicate the history or make reviews harder.

src/rawtoaces_util/image_converter.cpp

antond-weta · 2025-09-17T07:14:26Z

@mikaelsundell it seems I can't merge this in. Could you please squash your commits on top of up-to-date main and force push again please.

- Added --data-dir argument (Fixes AcademySoftwareFoundation#180) - Added auto matrix method (Fixes AcademySoftwareFoundation#181) Signed-off-by: Mikael Sundell <[email protected]>

antond-weta reviewed Sep 8, 2025

View reviewed changes

antond-weta reviewed Sep 9, 2025

View reviewed changes

mikaelsundell force-pushed the ms-250908 branch from 58263ff to 5e8f8ff Compare September 11, 2025 08:12

mikaelsundell force-pushed the ms-250908 branch from 5e8f8ff to 0471ab5 Compare September 12, 2025 20:28

antond-weta reviewed Sep 12, 2025

View reviewed changes

soswow reviewed Sep 13, 2025

View reviewed changes

mikaelsundell force-pushed the ms-250908 branch from c432cb7 to b8795cf Compare September 14, 2025 08:53

antond-weta reviewed Sep 15, 2025

View reviewed changes

src/rawtoaces_util/image_converter.cpp Show resolved Hide resolved

antond-weta force-pushed the ms-250908 branch from ab3ba4f to cbf17d2 Compare September 17, 2025 04:37

Add data-dir argument and auto matrix method

88a5f9b

- Added --data-dir argument (Fixes AcademySoftwareFoundation#180) - Added auto matrix method (Fixes AcademySoftwareFoundation#181) Signed-off-by: Mikael Sundell <[email protected]>

mikaelsundell force-pushed the ms-250908 branch from cbf17d2 to 88a5f9b Compare September 19, 2025 18:23

antond-weta approved these changes Sep 22, 2025

View reviewed changes

antond-weta merged commit 1d95f00 into AcademySoftwareFoundation:main Sep 22, 2025
14 checks passed


		for ( auto const &batch: batches )
		total_files += static_cast<int>( batch.size() );

Add data-dir argument and auto matrix method #189

Add data-dir argument and auto matrix method #189

Uh oh!

Conversation

mikaelsundell commented Sep 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mikaelsundell commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

soswow commented Sep 10, 2025

Uh oh!

mikaelsundell commented Sep 11, 2025

Uh oh!

antond-weta commented Sep 11, 2025

Uh oh!

antond-weta commented Sep 11, 2025

Uh oh!

antond-weta commented Sep 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antond-weta commented Sep 12, 2025

Uh oh!

soswow commented Sep 13, 2025

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!

soswow Sep 13, 2025 • 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!

mikaelsundell commented Sep 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antond-weta commented Sep 13, 2025

Uh oh!

soswow commented Sep 14, 2025

Uh oh!

mikaelsundell commented Sep 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antond-weta commented Sep 14, 2025

Uh oh!

antond-weta commented Sep 14, 2025

Uh oh!

codecov-commenter commented Sep 9, 2025 •

edited

Loading

mikaelsundell commented Sep 9, 2025 •

edited

Loading

soswow Sep 13, 2025 •

edited

Loading

mikaelsundell commented Sep 13, 2025 •

edited

Loading

mikaelsundell commented Sep 14, 2025 •

edited

Loading