Add color management to texture lookups #3761

lgritz · 2023-01-31T09:28:50Z

This isn't 100% final -- I'm sure there are some corner cases I missed, maybe things that will eventually need some performance work, but enough works and passes tests that I'm confident in the API and so I want to get this merged and we can iterate on internals later.

The use case is as follows: User or shader knows that a particular texture is in something other than the default working color space, this is not necessarily apparent from the file itself and so cannot be fully automatic, but it is presumed that it can be communicated in the texture call. We aim to be efficient, fully converting each tile upon input. Note that we are sweeping under the rug the fact that a non-default color space lookup is not mathematically correct with respect to the MIP-map downsizing math if that was not considered at texture creation time. But by converting texels as they are read, rather than the shader itself trying to do a color transformation on the final texture result, at least we are doing the right math for the filtering within and among MIP levels.

The new interface consists of communicating the presumed working color space, finding the colortransform ID of a given from/to (with the "to" defaulting to the working space, if not specified), and then on each texture call, supplying that colortransform ID as one of the settings in TextureOpt (or TextureOptBatch).

The outline of changes are as follows:

BREAKING CHANGE: TextureOpt and TextureOptBatch now contain an additional int colortransformid. The default, 0, is the old (and, frankly, preferred) behavior. Nonzero values indicate that we want the texels to go through a color transformation as the tiles are read into the cache.
BREAKING CHANGE: New public methods have been added to TextureSystem get_colortransform_id(fromspace, tospace) that return a transform ID given the from and to color spaces (ustring or ustringhash). This is the value that goes in the TextureOpt to request a color transformation on the pixels as they are read.
BREAKING CHANGE: IC::get_image_handle / TS::get_texture_handle now take an additional optinal paramter that is a TextureOpt*. This is currently unused, but is reserved for possible future use and/or for alternate TextureSystem implementations that choose a different implementation strategy wherein an entirely different TextureHandle may correspond to each color transformation. That's not how I've done it here, but it's a valid approach (and maybe one we'll switch to in the future). We presume that it's ill-defined to ask for a handle with one explicit color transformation, then use that handle with a TextureOpt that implies a different color transformation. Using those consistently ensure that either TS strategy will work equally well.
New TextureSystem attributes "colorspace" gives the working color space name (defaulting to "scene_linear"), and "colorconfig" can optionally point to a color config file (though this is not yet functional -- it just uses the default color config).
Internally, the tile cache hash has been adjusted so that the same tile in the disk file will appear as a separate tile in the cache for each color space pair that is applied to it. Thus, there is no conflict if some different texture calls ask for different color space interpretations of the same texture file. (That said, it's not recommended to be throwing color spaces around willy nilly -- it still takes multiple cache entrie, plus extra color conversion math as the tiles are read in.)
A new preprocessor symbol OIIO_TEXTURESYSTEM_SUPPORTS_COLORSPACE in texture.h allows client software can easily tell if this is a version new enough to support color management and has the additional fields in the texture option structures.

Because there are breaking changes here to ABI, this is destined only for master (future 2.5) and will not be backported to OIIO 2.4.

lgritz · 2023-01-31T09:32:29Z

@MasterZap this is the OIIO side. It's getting pretty late, but in the morning I'll post a (smaller, simpler) PR for some changes you'll also need on the OSL side that interfaces with this.

etheory · 2023-01-31T12:26:45Z

src/include/OpenImageIO/texture.h

@@ -264,6 +266,8 @@ class OIIO_API TextureOpt {
    float rblur;   ///< Blur amount in the r direction
    float rwidth;  ///< Multiplier for derivatives in r direction

+    int colortransformid;       ///< Color space id of the texture


If colortransformid can never be negative, why not make it an unsigned int?

Just general animus toward unsigned types? All the other fields in this struct were int even though they technical have no meaningful negative values.

etheory · 2023-01-31T12:28:34Z

src/libtexture/imagecache.cpp

-                          int miplevel, int x, int y, int z, int chbegin,
-                          int chend, TypeDesc format, void* data)
+ImageCacheFile::read_tile(ImageCachePerThreadInfo* thread_info,
+                          const TileID& id, void* data)


Wow, I MUCH prefer the parsing of the structure here, that's WAY nicer as an API.

etheory · 2023-01-31T12:29:53Z

src/libtexture/imagecache.cpp

@@ -852,6 +860,19 @@ ImageCacheFile::read_tile(ImageCachePerThreadInfo* thread_info, int subimage,
        thread_info->m_stats.bytes_read += b;
        m_bytesread += b;
        ++m_tilesread;
+        if (id.colortransformid() > 0) {


This is nice, but it feels a little scary that when a tile gets evicted, we have to do this all over again.
Sure, that's not a reason not to, but I'm just pointlessly sighing about that, even though it seems unavoidable.

Not sure what the alternative could be. The I/O is probably as expensive than the computation. Also, remember the context: people who really care about performance should be ensuring that their textures are in the working space when they are created, not in the middle of a shader.

etheory · 2023-01-31T12:31:02Z

src/libtexture/imagecache.cpp

+    int miplevel = id.miplevel();
+    int x        = id.x();
+    int y        = id.y();
+    // int z        = id.z();


excuse my ignorance, but are 3d textures now deprecated?

No, I'm just commenting out a variable that wasn't used. But 3d textures were never correctly supported by this special function that populates downsized MIP level tiles for unmipped files. MIP mapping is not used for volume textures currently, thus no mip level > 0 is read from for a texture3d lookup, so this function never is called for the 3d volume case.

etheory

LGTM so far, I need to digest this a little bit more, however, but it's a really great PR. Thanks.

This isn't 100% final -- I'm sure there are some corner cases I missed, maybe things that will eventually need some performance work, but enough works and passes tests that I'm confident in the API and so I want to get this merged and we can iterate on internals later. The use case is as follows: User or shader knows that a particular texture is in something other than the default working color space, this is not necessarily apparent from the file itself and so cannot be fully automatic, but it is presumed that it can be communicated in the texture call. We aim to be efficient, fully converting each tile upon input. Note that we are sweeping under the rug the fact that a non-default color space lookup is not mathematically correct with respect to the MIP-map downsizing math if that was not considered at texture creation time. But by converting texels as they are read, rather than the shader itself trying to do a color transformation on the final texture result, at least we are doing the right math for the filtering within and among MIP levels. The new interface consists of communicating the presumed working color space, finding the colortransform ID of a given from/to (with the "to" defaulting to the working space, if not specified), and then on each texture call, supplying that colortransform ID as one of the settings in TextureOpt (or TextureOptBatch). The outline of changes are as follows: * BREAKING CHANGE: TextureOpt and TextureOptBatch now contain an additional int `colortransformid`. The default, 0, is the old (and, frankly, preferred) behavior. Nonzero values indicate that we want the texels to go through a color transformation as the tiles are read into the cache. * BREAKING CHANGE: New public methods have been added to TextureSystem `get_colortransform_id(fromspace, tospace)` that return a transform ID given the from and to color spaces (ustring or ustringhash). This is the value that goes in the TextureOpt to request a color transformation on the pixels as they are read. * BREAKING CHANGE: IC::get_image_handle / TS::get_texture_handle now take an additional optinal parameter that is a TextureOpt*. This is currently unused, but is reserved for possible future use and/or for alternate TextureSystem implementations that choose a different implementation strategy wherein an entirely different TextureHandle may correspond to each color transformation. That's not how I've done it here, but it's a valid approach (and maybe one we'll switch to in the future). We presume that it's ill-defined to ask for a handle with one explicit color transformation, then use that handle with a TextureOpt that implies a different color transformation. Using those consistently ensure that either TS strategy will work equally well. * New TextureSystem attribute "colorspace" gives the working color space name (defaulting to "scene_linear"), and "colorconfig" can optionally point to a color config file (though this is not yet functional -- it just uses the default color config). * Internally, the tile cache hash has been adjusted so that the same tile in the disk file will appear as a separate tile in the cache for each color transform that is applied to it. Thus, there is no conflict if some different texture calls ask for different color space interpretations of the same texture file. (That said, it's not recommended to be throwing color spaces around willy nilly -- it still takes multiple cache entries, plus extra color conversion math as the tiles are read in.) * A new preprocessor symbol OIIO_TEXTURESYSTEM_SUPPORTS_COLORSPACE in texture.h allows client software can easily tell if this is a version new enough to support color management and has the additional fields in the texture option structures. Because there are breaking changes here to ABI, this is destined only for master (future 2.5) and will not be backported to OIIO 2.4.

This makes some minor changes to the RS APIs to add an additional optional parameter to get_texture_handle so it takes a `TextureOpt*` to accommodate a similar change on the OIIO TextureSystem side from AcademySoftwareFoundation/OpenImageIO#3761 The purpose here is to add color management options to texture calls. However, please note that this PR only reserves a public API change we know we'll want, but it does not fully implement the color management. Further work will be needed, as follows: * Current calls to RS::get_texture_handle should be updated to pass the TextureOpt*. (This is a formality for future-proofing in case future TS or alternate TS implementations want to bake the color space choice into the texture handle itself. But the OIIO change doesn't currently rely on that at the moment.) * When the texture.h symbol `OIIO_TEXTURESYSTEM_SUPPORTS_COLORSPACE` exists, we are building against an OIIO that has the new int field `colortransformid` in the TextureOpt and TextureOptBatch structures. We should fill this in with a nonzero value when we want the texture call to be doing a color transformation as it reads texture tiles. * Use the new TextureSystem:get_colortransform_id() to get that color transform ID given two named from/to color spaces. We agreed that it's a reasonable restriction that the color space names should be known by the end of runtime optimization, so that should be constant foldable. * Recognize new optional argument(s) to the texture() call family that lets the caller specify the presumed color space of the texture, and use that value (with constant folded lookup of the ID, above) to set the value in the TextureOpt. Signed-off-by: Larry Gritz <[email protected]>

lgritz · 2023-01-31T19:37:55Z

The corresponding change on the OSL side is AcademySoftwareFoundation/OpenShadingLanguage#1641

lgritz · 2023-02-02T23:23:16Z

I will merge this at end of day tomorrow unless somebody has concerns.

…ce (#1641) This makes some minor changes to the RS APIs to add an additional optional parameter to get_texture_handle so it takes a `TextureOpt*` to accommodate a similar change on the OIIO TextureSystem side from AcademySoftwareFoundation/OpenImageIO#3761 The purpose here is to add color management options to texture calls. However, please note that this PR only reserves a public API change we know we'll want, but it does not fully implement the color management. Further work will be needed, as follows: * Current calls to RS::get_texture_handle should be updated to pass the TextureOpt*. (This is a formality for future-proofing in case future TS or alternate TS implementations want to bake the color space choice into the texture handle itself. But the OIIO change doesn't currently rely on that at the moment.) * When the texture.h symbol `OIIO_TEXTURESYSTEM_SUPPORTS_COLORSPACE` exists, we are building against an OIIO that has the new int field `colortransformid` in the TextureOpt and TextureOptBatch structures. We should fill this in with a nonzero value when we want the texture call to be doing a color transformation as it reads texture tiles. * Use the new TextureSystem:get_colortransform_id() to get that color transform ID given two named from/to color spaces. We agreed that it's a reasonable restriction that the color space names should be known by the end of runtime optimization, so that should be constant foldable. * Recognize new optional argument(s) to the texture() call family that lets the caller specify the presumed color space of the texture, and use that value (with constant folded lookup of the ID, above) to set the value in the TextureOpt. Signed-off-by: Larry Gritz <[email protected]>

etheory reviewed Jan 31, 2023

View reviewed changes

lgritz force-pushed the lg-colortex branch from 1e301a5 to 754802f Compare January 31, 2023 17:56

lgritz force-pushed the lg-colortex branch from 754802f to 1ab6444 Compare January 31, 2023 18:03

lgritz mentioned this pull request Jan 31, 2023

RendererServices API for letting get_texture_handle consider colorspace AcademySoftwareFoundation/OpenShadingLanguage#1641

Merged

lgritz merged commit 271f911 into AcademySoftwareFoundation:master Feb 4, 2023

lgritz deleted the lg-colortex branch February 13, 2023 21:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add color management to texture lookups #3761

Add color management to texture lookups #3761

Uh oh!

lgritz commented Jan 31, 2023

Uh oh!

lgritz commented Jan 31, 2023

Uh oh!

etheory Jan 31, 2023

Uh oh!

lgritz Jan 31, 2023

Uh oh!

etheory Jan 31, 2023

Uh oh!

etheory Jan 31, 2023

Uh oh!

lgritz Jan 31, 2023

Uh oh!

etheory Jan 31, 2023

Uh oh!

lgritz Jan 31, 2023

Uh oh!

etheory left a comment

Uh oh!

lgritz commented Jan 31, 2023

Uh oh!

lgritz commented Feb 2, 2023

Uh oh!

Uh oh!

Add color management to texture lookups #3761

Add color management to texture lookups #3761

Uh oh!

Conversation

lgritz commented Jan 31, 2023

Uh oh!

lgritz commented Jan 31, 2023

Uh oh!

etheory Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

lgritz Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

etheory Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

etheory Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

lgritz Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

etheory Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

lgritz Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

etheory left a comment

Choose a reason for hiding this comment

Uh oh!

lgritz commented Jan 31, 2023

Uh oh!

lgritz commented Feb 2, 2023

Uh oh!

Uh oh!