Page MenuHome

OpenColorIO: update processors and transforms for version 2.0
ClosedPublic

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

Details

Summary

CPU processors now need to be created to do CPU processing. These are cached
internally, but the cache lookup is not fast enough to execute per pixel or
texture sample, so for performance these are now also exposed in the C API.

The C API for transform will no longer be needed afer all changes, so remove
it to simplify the API and fallback implementation.

Event Timeline

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

Note this is part of a set of patches. To get all the patches for testing, use the tmp-ocio-v2 branch or apply the last patch in the stack with arc.

The individual patches do not compile, but I've split them up for easier review.

What is the relation between OCIO_ConstProcessorRcPtr and OCIO_ConstCPUProcessorRcPtr ? Is the latter one a subclass of the former?
The rest of feedback kind of depends on this =\

intern/opencolorio/fallback_impl.cc
106–107

Please follow canonical C++ initialization:

FallbackProcessor(const FallbackProcessor &other) : transform(new FallbackTransform(*other.transform))
{
}

Or, since the change is so big anyway:

FallbackProcessor(const FallbackProcessor &other) : transform(make_unique<FallbackTransform>(*other.transform))
{
}
...
unique_ptr<FallbackTransform> transform;
source/blender/imbuf/intern/colormanagement.c
987–994

Not related to the review, but it reads very weird "FooPtr *bar".
Did you figure out why is it so, and whether we can remove one level of indirection? You also mention C-API removal, so maybe there things are more clear?

990

Almost everywhere else in the file explicit comparison with NULL is used.

What is the relation between OCIO_ConstProcessorRcPtr and OCIO_ConstCPUProcessorRcPtr ? Is the latter one a subclass of the former?

They are not subclasses, you call processor->getDefaultCPUProcessor to create a CPU processor object from a processor object. (Or retrieve it from an internal cache, but that's hidden from the API).

source/blender/imbuf/intern/colormanagement.c
987–994

I'm not sure what you mean?

All OpenColorIO pointers are std::shared_ptr. If it's the RcPtr part of the name that is strange, we could drop that from the C API since it's not really adding information there. But I would prefer to do that as a cleanup after these changes.

Regarding C-API removal, what I meant is the removal of API functions related to OpenColorIO Transform objects to slim it down a bit. The C API is still there.

Sergey Sharybin (sergey) requested changes to this revision.Feb 3 2021, 9:37 AM

They are not subclasses, you call processor->getDefaultCPUProcessor to create a CPU processor object from a processor object. (Or retrieve it from an internal cache, but that's hidden from the API).

Then please stick to a clear variable naming. Currently we have p, processor, device_processor, and in the function naming we use *cpu_processor*. While the cpu_processor and device_processor are fine, we should not use p at all, and should not use processor of type OCIO_ConstCPUProcessorRcPtr.

source/blender/imbuf/intern/colormanagement.c
987–994

Maybe we should move discussion to the chat or the office.

But check this out: in C++ you do OCIO::ConstProcessorRcPtr processor which makes sense: is a pointer. In C we do ConstProcessorRcPtr *processor which purely based on name seems redundant (the Ptr *, is like pointer to pointer).

But I would prefer to do that as a cleanup after these changes.

Sure. As I've said in the comment is not related to the review process. Just something what I always found confusing in the naming and usage, and wanted to get your thoughts on it.

This revision now requires changes to proceed.Feb 3 2021, 9:37 AM
Brecht Van Lommel (brecht) marked 2 inline comments as done.

Improve variable naming.

Simplify memory allocation in fallback transform.

From reading seems fine.
As talked in person, think is easier and more efficient to rely on your tests since there are no expected user-level differences.

This revision is now accepted and ready to land.Feb 3 2021, 3:34 PM