Add RGBA format in GpuShaderCreator for better Metal support (Issue #1956) #1984
Conversation
|
Thank you for the PR @wRosie ! Have you started the process of getting the corporate CLA agreement signed? Please don't hesitate to reach out on Slack if you have any questions about that. |
Yes I am working on that. |
wRosie
force-pushed
the
feature/add_rgba_for_metal
branch
from
1b2664e to
c1a48db
Compare
There was a problem hiding this comment.
Thanks for the contribution @wRosie and sorry for the delay in reviewing.
I triggered the CI workflows, please ignore the macOS stalled jobs, we use a macOS version no longer available in Github and will need to update the workflows shortly. Unfortunately, I don't think we have the Metal tests running in Github currently, you can try running them on your side and using ociodisplay in metal mode as well (use -metal flag).
I believe you will need to update the msl.mm file and remove the RGB_to_RGBA conversions after this change to have it work?
I think we are missing a Python binding updates, in PyGPUShaderCreator.cpp, you need to add the new textureType in enumTextureType.
I don't think we have GPU unit tests dedicated to specific APIs for now, might be a good addition, making sure Metal always returns Red or RGBA LUTs only.
There was a problem hiding this comment.
Thank you for the review, and I apologize for the delayed response.
Please let me know if you have additional comments. Thanks!
@doug-walker I am aware of the CLA approval issue. Thank you for the patience here, I am actively tracking it down -- it is just a lengthy process :(
wRosie
force-pushed
the
feature/add_rgba_for_metal
branch
2 times, most recently
from
d35ff53 to
7b611ef
Compare
Signed-off-by: Rosie Wang <[email protected]>
Signed-off-by: Rosie Wang <[email protected]>
wRosie
force-pushed
the
feature/add_rgba_for_metal
branch
from
646cf51 to
1f0fdb2
Compare
| Interpolation interpolation = INTERP_LINEAR; | ||
| m_shaderDesc->get3DTexture(idx, textureName, samplerName, edgelen, interpolation); | ||
| m_shaderDesc->get3DTexture(idx, textureName, samplerName, edgelen, channel, interpolation); | ||
|
|
There was a problem hiding this comment.
Following up on Remi's comment above, GitHub won't let me leave a comment at that line, but on line 233, AllocateTexture3D seems to assume it is getting an RGB array. It calls RGB_to_RGBA on it. Isn't the idea that this should not be necessary anymore?
Are the Metal GPU unit tests passing on your local build?
When I run "ctest -v" on your branch, I get a seg-fault in the Metal tests in the RGB_to_RGBA function.
|
@wRosie , just wanted to check in with you about this PR. Given that this would be an API-breaking change, it would need to be included in OCIO 2.4.0 which will be released on Sept. 30. Do you think you will be able to make the requested changes in time? Otherwise it will likely have to wait for OCIO 2.5.0 next fall. |
wRosie
force-pushed
the
feature/add_rgba_for_metal
branch
from
ad10217 to
69c7780
Compare
There was a problem hiding this comment.
Thanks for the follow up @wRosie!
There are still a couple of things to address (see the code comments as well):
- Can you please add the new Texture type to the Python bindings?
- The unit tests for Metal currently Segfault, having a deeper look, this is due to the internal copy of the LUT array within the GPUShader not handling RGBA yet.
I have another issue on Metal test locally but it is only happening for the first test being run and is due to an OpenGL setup error where a non existing texture is attached to a framebuffer, simply initialising m_imageTexID to 0 and checking before calling glFramebufferTexture2D seems to fix it. But we can address this in a later PR and review if the Metal / OpenGL interactions are correctly setup.
| std::vector<float> float4AdaptedLutValues; | ||
| RGB_to_RGBA(lutValues, 3*edgelen*edgelen*edgelen, float4AdaptedLutValues); | ||
| memcpy(float4AdaptedLutValues.data(), lutValues, 4 * edgelen*edgelen*edgelen * sizeof(float)); |
There was a problem hiding this comment.
This results in a Segfault because the float4AdaptedLutValues vector is empty. My suggestion would be to remove this copy as it is no longer required and we can use the lutValues pointer directly below. Similar update can also applies to AllocateTexture2D where you can remove the two memcpy.
| // (Using CacheID here to potentially allow reuse of existing textures.) | ||
| shaderCreator->add3DTexture(name.c_str(), | ||
|
|
||
| if(shaderCreator->getLanguage() == GPU_LANGUAGE_MSL_2_0){ |
There was a problem hiding this comment.
[Minor] Brace opening should be on the next line.
Hi OCIO team,
As per the discussion in #1956, I added RGBA texture format in GpuShaderCreator and made GpuShaderCreator store LUTs in RGBA when the shader language is in metal.
Here is the interface and behavior change involved in this PR:
AddTEXTURE_RGBA_CHANNEL value to enum TextureType
Add a TextureType parameter to add3DTexture(). The signature is consistent with addTexture().
Add a TextureType& field to get3DTexture().
The addTexture()/getTexture() functions already take TextureType as a parameter, so I will not change their function signature. However, I updated their behavior -- I changed GPUShaderCreator to use RGBA formats when GPULanguage is Metal. That involves updating functions such as GetLut1DGPUShaderProgram() and GetLut3DGPUShaderProgram(). Also, getTexture() and get3DTexture() will return textures in RGBA type in Metal mode.
Additionally, I updated the tests and function calls to work with the new interface.
I am aware that the behavior is not tested on a per-platform basis, and I would love suggestions on that. If possible, I'd like to add tests to ensure that RGBA is used only in metal mode.
Thanks!