Skip to content

RGB to HSY fixed functions using CPU#17

Merged
larochj merged 4 commits into
flame_ocio_featuresfrom
fixed-function-cpu
Jul 26, 2024
Merged

RGB to HSY fixed functions using CPU#17
larochj merged 4 commits into
flame_ocio_featuresfrom
fixed-function-cpu

Conversation

@larochj
Copy link
Copy Markdown
Collaborator

@larochj larochj commented Jul 18, 2024

No description provided.

@larochj larochj requested a review from doug-walker July 18, 2024 22:39

void Renderer_HSY_LOG_TO_RGB::apply(const void * inImg, void * outImg, long numPixels) const
{
applyRGBToHSY(inImg, outImg, numPixels, MIN_0);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need an applyHSYToRGB, with a corresponding implementation above.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh true, I only made one part of the convertion, thanks!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, but I still get a failure for the test style_RGB_TO_HSY_LIN_inv.
Any Idea why?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the code but did not notice a problem. The tolerance you're using is tighter than SynColor. It might help if you paste the results into the thread here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it is:

[ 49/230] [FixedFunction / style_RGB_TO_HSY_LIN_inv          ] - FAILED - 
Maximum error: 45990.25 at pixel: 63694 on component 2 larger than epsilon.
scr = {1.915679932, 1.915691376, 1.91570282, 1.915714264}
cpu = {-1317085.375, 736788.9375, -3420181.75, 1.915714264}
gpu = {-1334796.875, 746696.5625, -3466172, 1.915714264}
absolute tolerance=9.999999747e-05

Comment thread tests/cpu/ops/fixedfunction/FixedFunctionOpCPU_tests.cpp Outdated
@larochj larochj force-pushed the fixed-function-cpu branch from 72f008e to 5aef6df Compare July 19, 2024 16:03
@larochj larochj requested a review from doug-walker July 19, 2024 16:11
Copy link
Copy Markdown

@doug-walker doug-walker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please let me know which tests pass and paste the results for any that don't.

= std::make_shared<OCIO::FixedFunctionOpData>(OCIO::FixedFunctionOpData::RGB_TO_HSY_LIN);

std::vector<float> img = rgbFrame;
ApplyFixedFunction(&img[0], &hsyFrame[0], 4, dataFwdLin, 1e-6f, __LINE__);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tolerance in SynColor was 1e-4, is it passing at 1e-6?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes they all pass exept style_RGB_TO_HSY_LIN_inv.
And the error margin is bigger than 1e-4:

[ 49/230] [FixedFunction / style_RGB_TO_HSY_LIN_inv          ] - FAILED - 
Maximum error: 45990.25 at pixel: 63694 on component 2 larger than epsilon.
scr = {1.915679932, 1.915691376, 1.91570282, 1.915714264}
cpu = {-1317085.375, 736788.9375, -3420181.75, 1.915714264}
gpu = {-1334796.875, 746696.5625, -3466172, 1.915714264}
absolute tolerance=9.999999747e-05

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Jonathan. Looks like you pasted the result of the GPU test. Let's start with the CPU tests. Would you please paste the four input and output values?

Copy link
Copy Markdown
Collaborator Author

@larochj larochj Jul 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All CPU test passes:

[ 667/1091] [FixedFunctionOpCPU / RGB_TO_HSY_LIN                         ] - PASSED
[ 668/1091] [FixedFunctionOpCPU / RGB_TO_HSY_LOG                         ] - PASSED
[ 669/1091] [FixedFunctionOpCPU / RGB_TO_HSY_VID                         ] - PASSED
[ 688/1091] [FixedFunctionOps / RGB_TO_HSY_LIN                           ] - PASSED
[ 689/1091] [FixedFunctionOps / RGB_TO_HSY_LOG                           ] - PASSED
[ 690/1091] [FixedFunctionOps / RGB_TO_HSY_VID                           ] - PASSED

For RGB_TO_HSY_LIN here are the value used in the CPU test in file
tests/cpu/ops/fixedfunction/FixedFunctionOpCPU_tests.cpp

    const std::vector<float> hsyFrame {
         4.70554752e-01f, 9.12594033f, 3.26650218e-02f, 0.f,
         0.75f,           0.22196741f, 0.38596f,        1.f,
         0.08333333f,     0.12976444f, 0.034974f,       0.f,
         0.96296296f,     9.7034f,     -0.1862f,        1.f };

    const std::vector<float> rgbFrame {
        -0.075290f,  0.078996f, -0.108397f, 0.f,
         0.3f,       0.4f,       0.5f,      1.f,
         0.05f,      0.03f,      0.04f,     0.f,
         0.3f,      -0.4f,       0.5f,      1.f };

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, that's confusing. Above you wrote that style_RGB_TO_HSY_LIN_inv was not passing, but now it is?

If it is, that means we should be able to rely on the CPU values for comparison against the GPU tests.

The GPU result you pasted in a different thread had a very extreme output value. If the GPU matches the CPU through the normal range of values, that is the main criteria. If they diverge only at extreme values, that would not be too surprising given that it's a separate implementation. We don't need to fix that. You could limit the GPU test input values or perhaps adjust the parameter values so that it passes.

Copy link
Copy Markdown
Collaborator Author

@larochj larochj Jul 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I changed it to use custom values for this specific test, thanks!

Comment thread tests/cpu/ops/fixedfunction/FixedFunctionOpCPU_tests.cpp Outdated

void Renderer_HSY_LOG_TO_RGB::apply(const void * inImg, void * outImg, long numPixels) const
{
applyRGBToHSY(inImg, outImg, numPixels, MIN_0);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the code but did not notice a problem. The tolerance you're using is tighter than SynColor. It might help if you paste the results into the thread here.

@larochj larochj requested a review from doug-walker July 24, 2024 15:21
@larochj larochj merged commit a1dced6 into flame_ocio_features Jul 26, 2024
@larochj larochj deleted the fixed-function-cpu branch July 26, 2024 13:02
doug-walker pushed a commit that referenced this pull request Oct 12, 2024
doug-walker pushed a commit that referenced this pull request Dec 17, 2024
doug-walker pushed a commit that referenced this pull request Mar 10, 2025
doug-walker pushed a commit that referenced this pull request Jul 28, 2025
(cherry picked from commit a25c1c7)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>
doug-walker added a commit that referenced this pull request Oct 1, 2025
* Hue Curve Transform GPU

(cherry picked from commit 393c0f6)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Review Fixes P1

(cherry picked from commit 5b9ff85)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Review Fixes P2

(cherry picked from commit d1a5d7a)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Review Fixes P3

(cherry picked from commit 83340c5)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Review Fixes P4

(cherry picked from commit b243d1d)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* GradingHueCurveOpData_tests

(cherry picked from commit 2a96167)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Review Fix, ChannelCrosstalk

(cherry picked from commit 0bac401)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Review Fix, missing fixed function enums in switch cases

(cherry picked from commit 051bd7d)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Review Fix, Missing comment on unused test parameters

(cherry picked from commit 07aca3c)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* CI Fix, removing multi-line comment

(cherry picked from commit bf78019)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Fixes for Test 2: test_cpu

(cherry picked from commit 4a5001e)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Fix Windows CI

Towards clean

(cherry picked from commit d298681)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Fix for missing slopes on periodic curves

(cherry picked from commit 3f393f8)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Remove leftover #include from debuging

(cherry picked from commit e7fd3f2)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Enable to draw the curve eval only for a specific hue curve. (#11)

(cherry picked from commit d658e70)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* RGB to HSY fixed functions using CPU (#17)

(cherry picked from commit a25c1c7)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Fix identity hue curve draw only (#20)

(cherry picked from commit 4f276e5)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* <<Operator upgrade for list of floats (#23)

(cherry picked from commit 7e6378f)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Fix unit tests

Signed-off-by: Doug Walker <doug.walker@autodesk.com>
(cherry picked from commit a9e51c8)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Fix hue curve unit tests

Signed-off-by: Doug Walker <doug.walker@autodesk.com>
(cherry picked from commit 6c233c8)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Comment out OSL tests

Signed-off-by: Doug Walker <doug.walker@autodesk.com>
(cherry picked from commit cf7744e)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Finish the implementation

Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Improve interface, add tests

Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Minor clean-up

Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Op<< should contain slopes

Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Fix python doc line

Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Improve validation of control points

Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Update API calls based on Vulkan PR

Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Saturation robustness

Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* HSY transform setter adjustment

Signed-off-by: Doug Walker <doug.walker@autodesk.com>

---------

Signed-off-by: Doug Walker <doug.walker@autodesk.com>
Co-authored-by: Jonathan Laroche <jonathan.laroche@autodesk.com>
Co-authored-by: larochj <143038478+larochj@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants