Skip to content

ImageReader, TextureLoader : Remove special case for png colorspaces - #1553

Merged
ivanimanishi merged 2 commits into
ImageEngine:RB-10.7from
ivanimanishi:pngLoadingAndOIIO3
Aug 28, 2026
Merged

ImageReader, TextureLoader : Remove special case for png colorspaces#1553
ivanimanishi merged 2 commits into
ImageEngine:RB-10.7from
ivanimanishi:pngLoadingAndOIIO3

Conversation

@ivanimanishi

Copy link
Copy Markdown
Member

OIIO 3 doesn't have "linear" built-in anymore, and it now needs to come from the config.

This is causing errors when trying to open png files, as often there isn't a colorspace specifically called "linear".

The original reasons for hard-coding that colorspace don't seem to apply anymore, neither ImageEngine nor gaffer are loading UI icons that way, so it seems reasonable to remove it, and treat png files just like any other image.

Generally describe what this PR will do, and why it is needed.

Checklist

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • I have tested my change(s) in the test suite, and added new test cases where necessary.
  • My code follows the Cortex project's prevailing coding style and conventions.

@ivanimanishi

ivanimanishi commented Aug 20, 2026

Copy link
Copy Markdown
Member Author

@johnhaddon , see if you have any issues with this fix. We are trying to switch to OIIO 3 / Gaffer 1.7, but are getting some unittests failing because they can't read pngs anymore.

As stated in the PR description and commit message, I can't see any obvious other issue with the pngs in UIs, which was the reason why Andrew added the special case there in the first place.

@danieldresser-ie thought this fix is a reasonable one, but there is a potential discussion about the way all the other image formats were being read, but I'd prefer to limit this PR to the immediate fix, so we can move forward here, particularly since I don't have clear alternative. But if you think there is a better solution, just let me know.

At IE, we are trying to avoid the use of IECoreImage on new code, but it will still take some time to remove all of the existing ones.

@johnhaddon

johnhaddon commented Aug 24, 2026

Copy link
Copy Markdown
Member

Looking through the Gaffer source, the only thing I can find that would be affected by this is the RenderMan CookierVisualiser, which passes a map filename as a texture parameter to IECoreGL. It seems unlikely that PNG files would be used with that since .tex files are a requirement of the cookie filter.

But this will also affect anything that does texture lookups from a file in a GLSL shader in Gaffer's viewport, so we can't be confident that it won't affect some user-defined setup we're not in control of. We ship an OpenGL Texture shader with Gaffer, and it's not implausible that someone will have thrown a PNG into it. That makes me uncomfortable.

The code we're removing was always pretty hacky. But I think it's a bit of a stretch to say that it's a bug fix when the only visible effects in Gaffer will be unwanted. Looking at the OCIO source I see things like ColorConfig::Impl::identify_builtin_equivalents() that make it sound like it's trying to find spaces like the ones we want. Perhaps they are available under some other name now?

@johnhaddon

Copy link
Copy Markdown
Member

I did some practical testing with this and Gaffer's standard ACES config, and the results are closer to intended than before. LGTM.

OIIO 3 doesn't have "linear" built-in anymore, and it now needs to come
from the config.

This is causing errors when trying to open png files, as often there
isn't a colorspace specifically called "linear".

The original reasons for hard-coding that colorspace don't seem to apply
anymore, neither ImageEngine nor gaffer are loading UI icons that way,
so it seems reasonable to remove it, and treat png files just like any
other image.
@danieldresser-ie

Copy link
Copy Markdown
Contributor

When discussing this with Ivan, I did think of one possible scenario where the current code might be an improvement: if the OCIO config were to set the "color_picking" role to something other than sRGB, then it would be desirable to force PNGs to load as sRGB. I don't fully understand OCIO, but naively, it could certainly make sense in some cases to use a color space other than sRGB for a color picking UI.

What doesn't make sense to me is using the color_picking space to determine how files are loaded, which is done here: https://github.com/ImageEngine/cortex/blob/main/src/IECoreImage/OpenImageIOAlgo.cpp#L224

... but if color_picking is set to something weird, then a lot of file types will be messed up when loaded through this path, not just PNGs, so even then, it's still kind of weird to hack up just PNGs.

I guess the end conclusion is this PR is probably fine, but it's a good thing that we're now in progress on getting rid of any use of this code.

@ivanimanishi
ivanimanishi merged commit 1f6d7e0 into ImageEngine:RB-10.7 Aug 28, 2026
6 checks passed
@ivanimanishi
ivanimanishi deleted the pngLoadingAndOIIO3 branch August 28, 2026 16:48
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.

3 participants