OpenColorIO: update display transforms for version 2.0
ClosedPublic
Actions

Authored by Brecht Van Lommel (brecht) on Feb 1 2021, 3:56 PM.

Details

Reviewers

Sergey Sharybin (sergey)

Maniphest Tasks

T84819: OpenColorIO 2.0 Upgrade

Summary

Needs a bit more manual work constructing the transform. LegacyViewingPipeline
could also have been used, but isn't really any simpler and since it's legacy
we better not rely on it.

This moves more logic into the opencolorio module, to simplify the API. There is
no need to wrap a dozen functions just to be able to do this in C rather than C++.
It's also tightly coupled to the GPU shader logic, and so should be in the same
module.

Depends on D10271

Diff Detail

Repository

rB Blender

Build Status

Buildable 12576
Build 12576: arc lint + arc unit

Event Timeline

Brecht Van Lommel (brecht) requested review of this revision.Feb 1 2021, 3:56 PM

Brecht Van Lommel (brecht) created this revision.

Harbormaster completed remote builds in B12512: Diff 33386.Feb 1 2021, 3:56 PM

Brecht Van Lommel (brecht) added a child revision: D10273: OpenColorIO: update GPU display shader for version 2.0.Feb 1 2021, 3:57 PM

Brecht Van Lommel (brecht) added a task: T84819: OpenColorIO 2.0 Upgrade.Feb 1 2021, 4:00 PM

There is no need to wrap a dozen functions just to be able to do this in C rather than C++.

YAY!

intern/opencolorio/fallback_impl.cc
465–469	It this how clang-format formatted it? Would expect it to be `OCIO_ConstConfigRcPtr * /config/,`
intern/opencolorio/ocio_impl.cc
413–415	What are the C-style casts here for? From reading the code below doesn't seem you was using strict compiler flag to avoid mixing single and double precision. I would either use C++ style cast, or not bother with explicit cast at all.
764	`if (scale != 1.0f) {` Or, use `double` in API.
783	That's a good an sneaky one ;)
784–785	What is the `GetLooksResultColorSpace()` when using for a non-existing look?
807	`if (exponent != 1.0f) {` Or, `double` in API.

Brecht Van Lommel (brecht) marked 6 inline comments as done.Feb 2 2021, 7:38 PM

Brecht Van Lommel (brecht) added inline comments.

intern/opencolorio/ocio_impl.cc
784–785	It will be `""`. In general the mechanism is that you create the transforms, and then when creating the processor it will validate the entire transform group and throw an error (which we catch below).

marked 6 inline comments as done

Did you forget to update the diff? :)

In general the mechanism is that you create the transforms, and then when creating the processor it will validate the entire transform group and throw an error (which we catch below).

This is good to know. Thanks for clarification.

Address comments.

Harbormaster completed remote builds in B12576: Diff 33484.Feb 3 2021, 3:08 PM

@Sergey Sharybin (sergey), can you accept this one as well?

Guessing it was an oversight as you accepted the other two patches that I updated at the same time.

@Brecht Van Lommel (brecht), It is generally all fine.

I'm on a split with the strlen but i see your point of being more semantic clear. Ideally would switch to string_view, but that's totally outside of the scope of this patch.

This revision is now accepted and ready to land.Feb 8 2021, 2:14 PM

Brecht Van Lommel (brecht) closed this revision.Feb 12 2021, 7:39 PM