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

Add support for RGBA format in GpuShaderCreator for better Metal support? #1956

Open
wRosie opened this issue Mar 14, 2024 · 8 comments
Open
Labels
Feature Request New addition to OCIO functionality.
Milestone

Comments

@wRosie
Copy link

wRosie commented Mar 14, 2024

Hi everyone,

Only single channel and RGB texture formats are currently supported in GpuShaderCreator::TextureType. However, it appears that Apple does not support simple RGB format textures in Metal. Please refer to https://developer.apple.com/documentation/metal/mtlpixelformat.

Working with OCIO shaders in Metal becomes inconvenient. When I query the GpuShaderDesc for a Metal shader and its textures, I sometimes get RGB textures that are not accepted by Metal. I have to insert a placeholder alpha channel to convert them to RGBA format.

Is it an intentional decision that OCIO does not support RGBA texture? If not, is it a plan for the future?

Thanks a lot!

@doug-walker
Copy link
Collaborator

@Morteeza, since you are our Metal expert, could you please offer some help with this question?

@remia
Copy link
Collaborator

remia commented Mar 15, 2024

Note there was a similar issue created in the past #1685

@Morteeza
Copy link
Contributor

As you mentioned, we don't have RGB texture support in Metal. You may have noticed we similarly insert placeholder alpha channel in OCIO metal app.mm, too.

I believe main reason we didn't add RGBA texture type was that, it will touch OCIO interface. So, maybe in future we can add that? @hodoulp do you think we can make this change in a major release?

@wRosie
Copy link
Author

wRosie commented Mar 16, 2024

I am happy to help fix it. This will save us all from copying textures in memory.
I agree it will touch the OCIO interface. But.. When I query for a metal shader, it makes more sense to get textures that work with it, right? Please consider doing it in a major release.

@doug-walker
Copy link
Collaborator

@wRosie , thank you for your offer to help, that would be great! This is something we could include in the OCIO 2.4 release this fall. I'd suggest starting by just proposing the API change, to get feedback, before doing the full implementation.

@wRosie
Copy link
Author

wRosie commented Mar 18, 2024

Thanks @doug-walker. Here are some thoughts on interface change.

  • Add TEXTURE_RGBA_CHANNEL value to enum TextureType

  • Add a TextureType field to add3DTexture(). We could make the field optional, with a default value of TEXTURE_RGB_CHANNEL (the current behavior). This would avoid breaking existing function calls. However, I prefer adding the field to keep the signature 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'd like to update their behavior -- I’ll change 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.

I am not aware of other OCIO-supported shading languages that require RGBA format other than metal. If there is any, please point it out.

Please let me know if this makes sense to you, or if I missed anything. Thanks a lot!

@doug-walker
Copy link
Collaborator

@Morteeza, yes that's right, we couldn't modify the API at that time because of the release timing and the desire to release Metal support sooner rather than later.

@remia, thanks for linking to the other issue.

@wRosie, thank you, that sounds like an excellent proposal. I agree that it would be better to keep the signatures consistent and not use default arguments.

I think you are definitely on the right track, please proceed when you're ready!

@timeinpixels
Copy link

Thanks for refreshing my original request @wRosie - looking forward to having the change in the major release!

@carolalynn carolalynn added the Feature Request New addition to OCIO functionality. label Apr 1, 2024
@carolalynn carolalynn added this to the OCIO 2.4.0 milestone Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request New addition to OCIO functionality.
Projects
None yet
Development

No branches or pull requests

6 participants