Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not merge - Proposal for the New GPU API #529

Closed
wants to merge 14 commits into from
Closed

Do not merge - Proposal for the New GPU API #529

wants to merge 14 commits into from

Conversation

hodoulp
Copy link
Member

@hodoulp hodoulp commented Apr 5, 2018

To ease the code review you could start with the main.cpp of the ociodisplay app and the OpenColorIO.h files.

Please do not forget that the branch is a prototype to demonstrate the new API, and it will never be merged.

@Dithermaster
Copy link

Dithermaster commented Apr 5, 2018

Interesting! If I'm reading it correctly, it allows the OpenGL path to create accurate Processors with multiple GPU 1D,2D,3D texture resources instead of baking all LUTs into a single 3D LUT.

If so, that is very similar to the OpenCL additions I made. To do that, I added a new GpuLanguage (called GPU_LANGUAGE_OCL_1_1), and added Processor methods to get individual GPU resources (getNumGpuParameters, getGpuParameterData, and getGpuParameterDataCacheID). The resources can be OpenCL buffers, 1D, 2D, and 3D images. Each Op can supply a resource via a new getGpuParameterData method, and the (commented!) generated OpenCL kernel takes them as parameters. Like the legacy OpenGL path, the host is responsible for uploading these resources, and removing them when done. I kind of like what you did making the Processor responsible for this, but it also means the library would need to have bindings to all the different GPU subsystems (OpenGL, OpenCL, future etc.).

@hodoulp
Copy link
Member Author

hodoulp commented Apr 6, 2018

Interesting! If I'm reading it correctly, it allows the OpenGL path to create accurate Processors with multiple GPU 1D,2D,3D texture resources instead of baking all LUTs into a single 3D LUT.

Yes. This branch only implements the 'bake' color transformations as an example to demonstrate the enhanced GpuShaderDesc class. But the class now allows to support any 'not baked' color transformations (i.e. any number of textures & texture types).

If so, that is very similar to the OpenCL additions I made. To do that, I added a new GpuLanguage (called GPU_LANGUAGE_OCL_1_1), and added Processor methods to get individual GPU resources (getNumGpuParameters, getGpuParameterData, and getGpuParameterDataCacheID). The resources can be OpenCL buffers, 1D, 2D, and 3D images. Each Op can supply a resource via a new getGpuParameterData method, and the (commented!) generated OpenCL kernel takes them as parameters.

The enhanced GpuShaderDesc is now in charge of collecting all the needed resources for a specific color transformation (i.e. processor instance). The processor responsibility is only to loop through all the ops, and to delegate the GPU 'building' (using the GpuShaderDesc) to the op. For example, the Lut3DOp now creates its texture request (instead of having the processor & main having to deal with it). That approach should fill your needs around OpenCL support. As you mention, only a new enumeration value (i.e. GPU_LANGUAGE_OCL_1_1) should be needed.

Like the legacy OpenGL path, the host is responsible for uploading these resources, and removing them when done. I kind of like what you did making the Processor responsible for this, but it also means the library would need to have bindings to all the different GPU subsystems (OpenGL, OpenCL, future etc.).

Yes except that the OpenGLBuilder class greatly simplifies the work (please refer to main.cpp from ociodisplay app). But the OpenGLBuilder is a separate library so that the core could compile even if OpenGL libraries are not present. The main idea is that anyone could now add a builder (for example: OpenCLBuilder for OpenCL, Dx11Build for Dx11 support and so on) but only compile the one needed. The core is independent from the builders.

@scoopxyz scoopxyz added the v2.0 label Apr 6, 2018
Copy link
Collaborator

@michdolan michdolan left a comment

Choose a reason for hiding this comment

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

This looks great! The interface is clear, accommodating, and well documented. The GpuShaderDesc implementation is logical and extensible. Great work @hodoulp !

I'll be curious to see in-practice how an arbitrary number of discrete 1D/3D LUTs on the GPU affects texture memory. Something else for config authors to consider, since the "legacy" shader had the normalizing edge length parameter. "generic" could prove to be more efficient though, depending on colorspace design.

The other question will be the use of allocation vars. It will remain the responsibility of the config author to set these appropriately to maintain matching interpolation of 3D LUTs between CPU and GPU. Though with 1D LUT support, a non-allocation shaper will be a useful alternative.

@scoopxyz
Copy link
Contributor

scoopxyz commented May 3, 2018

I have a general question, and I'm not familiar enough with the individual libraries to know how it translates, but how well does this proposed API translate to non-OpenGL graphics frameworks (e.g. Vulcan, Metal)?

@hodoulp
Copy link
Member Author

hodoulp commented May 3, 2018

I do not know enough of Vulkan or Metal to have a correct answer; however, the new GPU API only provides an access to the resources needed to fully implement the color transformation (i.e. textures & GPU shader program).
The resources have to be managed by the developer (even if a OpenGL/GLSL helper class will be part of OCIO to help in most frequent use cases). And the GPU shader program is generated for a dedicated language (i.e. CG or GLSL for now). But it widely opens to door to other GPU languages.
So the new API should be generic enough to accommodate new needs.

@hodoulp
Copy link
Member Author

hodoulp commented Jun 12, 2018

I close the review. Please refer to #539

@hodoulp hodoulp closed this Jun 12, 2018
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.

4 participants