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

sRGB-related crash in 1.4 beta #5695

Closed
BrianHanke opened this issue Feb 23, 2024 · 11 comments
Closed

sRGB-related crash in 1.4 beta #5695

BrianHanke opened this issue Feb 23, 2024 · 11 comments

Comments

@BrianHanke
Copy link
Contributor

Version: Gaffer 1.4.0.0b1-windows
Third-party tools: Arnold
Third-party modules: None

Description

Congrats on the 1.4 beta release! I tried running it on Windows 10 but it errors out with a few dozen lines in the console about not finding sRGB. I use my own custom OCIO config that is closely based on the official "CG" one. It works fine in Gaffer 1.3.

Here's a portion of the error output:

ERROR : IECore.loadConfig : Error executing file "C:\Users\Brian\Desktop\gaffer-1.4.0.0b1-windows\startup\gui\editor.py" - "ColorAlgo::transformImage : Color space 'sRGB' could not be found.".
ERROR :  Traceback (most recent call last):
ERROR :   File "C:\Users\Brian\Desktop\gaffer-1.4.0.0b1-windows\python\IECore\ConfigLoader.py", line 76, in loadConfig
ERROR :     exec(
ERROR :   File "C:\Users\Brian\Desktop\gaffer-1.4.0.0b1-windows\startup\gui\editor.py", line 39, in <module>
ERROR :     import GafferSceneUI
ERROR :   File "C:\Users\Brian\Desktop\gaffer-1.4.0.0b1-windows\python\shiboken2\files.dir\shibokensupport\feature.py", line 142, in _import
ERROR :     return original_import(name, *args, **kwargs)
ERROR :   File "C:\Users\Brian\Desktop\gaffer-1.4.0.0b1-windows\python\GafferSceneUI\__init__.py", line 38, in <module>
ERROR :     __import__( "GafferImageUI" )
ERROR :   File "C:\Users\Brian\Desktop\gaffer-1.4.0.0b1-windows\python\shiboken2\files.dir\shibokensupport\feature.py", line 142, in _import
ERROR :     return original_import(name, *args, **kwargs)
ERROR :   File "C:\Users\Brian\Desktop\gaffer-1.4.0.0b1-windows\python\GafferImageUI\__init__.py", line 100, in <module>
ERROR :     from . import CatalogueUI
ERROR :   File "C:\Users\Brian\Desktop\gaffer-1.4.0.0b1-windows\python\shiboken2\files.dir\shibokensupport\feature.py", line 142, in _import
ERROR :     return original_import(name, *args, **kwargs)
ERROR :   File "C:\Users\Brian\Desktop\gaffer-1.4.0.0b1-windows\python\GafferImageUI\CatalogueUI.py", line 1180, in <module>
ERROR :     GafferUI.Pointer.registerPointer( "plus", GafferUI.Pointer( "plus.png" ) )
ERROR : IECore.Exception: ColorAlgo::transformImage : Color space 'sRGB' could not be found.
@johnhaddon
Copy link
Member

Thanks for testing Brian! We've updated OpenImageIO in 1.4, and I think that's where the error originates. I don't know all the details, but I think there are various heuristics in OCIO and/or OIIO to try to find an sRGB transform from an OCIO config. Is there any way you could share the config in question?

@BrianHanke
Copy link
Contributor Author

@johnhaddon Sure, here you go: https://github.com/BrianHanke/bhgc_v2.ocio/blob/main/bhgc_v2.ocio There's a lot of sRGB stuff in there, but maybe not the right kind!

@murraystevenson
Copy link
Contributor

Thanks @BrianHanke ! We can repro this on Linux too with your example OCIO config.

@BrianHanke
Copy link
Contributor Author

I think I found the source of the problem: Gaffer 1.4 seems to specifically look for an sRGB - Texture colorspace. I get an error when spelling it sRGB texture or sRGB_texture in my config, for example.

@johnhaddon
Copy link
Member

I'm pretty sure it's OpenImageIO that is failing to find the sRGB config rather than Gaffer itself, because we defer the color conversion to OIIO, and you get the exact same error using oiitool directly :

OCIO=bhgc_v2.ocio gaffer env oiiotool test.png --colorconvert sRGB linear -o test.exr
oiiotool ERROR: colorconvert : Color space 'sRGB' could not be found.

I think the real question is if we should file a bug against OIIO, or if there's a good reason the sRGB transform from bhgc_v2.ocio isn't being detected any more. This bit of the OIIO 2.5 release notes seems relevant :

OIIO tries to find and honor the common color space aliases
"scene_linear", "srgb", "lin_srgb", and "ACEScg". When building against
OCIO 2.2+, it will know which of any config's color spaces are
equivalent to these, even if they are named something totally different,
thanks to the magic of OCIO 2.2 built-in configs. For older OCIO (2.1 or
older), it is less robust and may have to make best guesses based on the
name of the color spaces it finds.

I wonder if the old "best guesses" found the sRGB transform, but the new "magic of OCIO 2.2" doesn't. I think the magic actually analyses the color space (rather than the name) to see if it truly is sRGB, so perhaps it isn't? Or it is, but not described in some canonical form that OCIO can recognise?

@BrianHanke
Copy link
Contributor Author

Aha, adding those aliases fixes it. For sRGB texture for example, this line

aliases: [srgb_tx, Utility - sRGB texture, srgb_texture, Input - Generic - sRGB texture]

becomes this

aliases: [srgb, srgb_tx, Utility - sRGB texture, srgb_texture, Input - Generic - sRGB texture]

and now Gaffer launches.

Seems like a pretty useless change to OIIO seeing as how it worked fine for years before this. The "magic" needs some fine tuning!

@johnhaddon
Copy link
Member

I'm now thinking it might be reasonable to file a bug against OIIO. I think this is the code in question :

https://github.com/AcademySoftwareFoundation/OpenImageIO/blob/master/src/libOpenImageIO/color_ocio.cpp#L845-L857

It looks very much like it does some basic classification based on names (which is why it works when you change the name), then uses some heuristics if OCIO < 2.2.0 (which I think is how it worked in Gaffer 1.3 before we updated OCIO), then uses OCIO's heuristics if OCIO is on version 2.3.0 or above (which is isn't in Gaffer). We're using OCIO 2.2.1, which falls through the gap between the two set of heuristics.

@BrianHanke
Copy link
Contributor Author

Interesting. I agree, would be good to bring this up with OIIO. I scanned through and noticed this:

https://github.com/AcademySoftwareFoundation/OpenImageIO/blob/424f9137f798faee7b40342503d06e11bd18d1a1/src/libOpenImageIO/color_ocio.cpp#L1124

It seems to look specifically for srgb and sRGB - Texture? The official ASWF CG config does not have the srgb alias, which is why it was missing in mine. And you should be able to name colorspaces whatever you want, having it hardcoded to sRGB - Texture isn't right.

@tomc-cinesite
Copy link
Contributor

There is some rationale in the commit message here.

@johnhaddon
Copy link
Member

There is some rationale in the commit message here.

I suspect things might have worked in that commit, and that they might have been broken by this one. Need to do some more digging.

In the meantime, it looks like another workaround might be to set the OIIO_DISABLE_OCIO environment variable.

@johnhaddon
Copy link
Member

it looks like another workaround might be to set the OIIO_DISABLE_OCIO environment variable.

Scratch that. In fact, that breaks things even worse.

johnhaddon added a commit to johnhaddon/gaffer that referenced this issue Mar 8, 2024
The TextureLoader came with several problems :

- It contained a hack to apply a simple sRGB->linear transform to all `.png` images, but that stopped working with OIIO 2.5 and certain custom OCIO configs. See GafferHQ#5695.
- It returned non-const textures, even though they were potentially shared between multiple clients who had loaded the same thing.

We now just do our own loading using an LRUCache and direct calls to OIIO.
johnhaddon added a commit to johnhaddon/gaffer that referenced this issue Mar 8, 2024
This fixes GafferHQ#5695, by avoiding the colour conversions done by `IECoreImage::ImageReader` which were failing due to AcademySoftwareFoundation/OpenImageIO#4165. We want to phase `IECoreImage` out completely anyway, so this represents good progress in that direction.

Although `_qtPixmapFromImagePrimitive()` and the `Image( ImagePrimitive )` constructor are now completely unused in Gaffer itself, we can't remove them just yet because they are used in extension code at IE.

There's a reasonable case for using OpenImageIO here instead of Qt, to get access to a broader range of image formats and to match exactly the capabilities of ImageGadget. I tried implementing that but OpenImageIO isn't really usable from Python without `numpy`, and we don't ship with that module at present. But practically speaking we only really care about PNGs here, and Qt does fine with those, so that's what we're using for now.
johnhaddon added a commit to johnhaddon/gaffer that referenced this issue Mar 8, 2024
This fixes GafferHQ#5695, by avoiding the colour conversions done by `IECoreImage::ImageReader` which were failing due to AcademySoftwareFoundation/OpenImageIO#4165. We want to phase `IECoreImage` out completely anyway, so this represents good progress in that direction.

Although `_qtPixmapFromImagePrimitive()` and the `Image( ImagePrimitive )` constructor are now completely unused in Gaffer itself, we can't remove them just yet because they are used in extension code at IE.

There's a reasonable case for using OpenImageIO here instead of Qt, to get access to a broader range of image formats and to match exactly the capabilities of ImageGadget. I tried implementing that but OpenImageIO isn't really usable from Python without `numpy`, and we don't ship with that module at present. It would be possible to implement an ImageBuf->QPixmap conversion in C++ and then bind that to Python if we consider this important. But practically speaking I think we only really care about common icon formats here, and Qt does fine with those, so I've gone with the simple option for now.
johnhaddon added a commit to johnhaddon/gaffer that referenced this issue Mar 8, 2024
This fixes GafferHQ#5695, by avoiding the colour conversions done by `IECoreImage::ImageReader` which were failing due to AcademySoftwareFoundation/OpenImageIO#4165. We want to phase `IECoreImage` out completely anyway, so this represents good progress in that direction.

Although `_qtPixmapFromImagePrimitive()` and the `Image( ImagePrimitive )` constructor are now completely unused in Gaffer itself, we can't remove them just yet because they are used in extension code at IE.

There's a reasonable case for using OpenImageIO here instead of Qt, to get access to a broader range of image formats and to match exactly the capabilities of ImageGadget. I tried implementing that but OpenImageIO isn't really usable from Python without `numpy`, and we don't ship with that module at present. It would be possible to implement an ImageBuf->QPixmap conversion in C++ and then bind that to Python if we consider this important. But practically speaking I think we only really care about common icon formats here, and Qt does fine with those, so I've gone with the simple option for now.
johnhaddon added a commit to johnhaddon/gaffer that referenced this issue Mar 12, 2024
The TextureLoader came with several problems :

- It contained a hack to apply a simple sRGB->linear transform to all `.png` images, but that stopped working with OIIO 2.5 and certain custom OCIO configs. See GafferHQ#5695.
- It returned non-const textures, even though they were potentially shared between multiple clients who had loaded the same thing.

We now just do our own loading using an LRUCache and direct calls to OIIO.
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

No branches or pull requests

4 participants