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

Revise OpenEXR sorting of channel names into canonical order #2595

Merged
merged 1 commit into from Jun 1, 2020

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented May 29, 2020

Brief background: OpenEXR sorts channel names alphabetically. Most
formats dont allow channel naming at all, let alone forcing a
particular order contrary to the order the user tries to write it to
the file. OIIO promises that if there is RGB, they will be first and
in that order, next will be alpha if present, then depth, then
everything else.

So our exr reader will reshuffle channels into this canonical order
before presenting to the calling app. In addition to literal R, G, and
B being first, also obvious synonyms are accepted and understood to be
first ("Red", "Green", "Blue", and also "Y" in case it's just a
1-channel intensity and then maybe alpha).

A user noticed an interesting edge case: what if it's a normal map or
positional info, so the channel names are x, y, z? You want it in that
order (coincidentally alphabetical), duh. But the current heuristic
will reorder it as Y (mistaking for intensity), Z (mistaking for
depth), then X (a non-special name, so goes last). Ick!

The solution is that we need a more sophisticaed heuristic. This patch
changes the logic to first just sort by layer, then within each layer,
look to see if there are channels "x", "y", AND "z", and if all three
are present, then we can conclude that it means something spatial and
that Y isn't really intensity and Z isn't really depth for that
particular layer, so an alternate ordering is used. If all of x,y,z
aren't found in a layer, we continue to use the old heuristic.

Signed-off-by: Larry Gritz lg@larrygritz.com

@lgritz
Copy link
Collaborator Author

lgritz commented May 29, 2020

We realized that saving 2D data ("X" and "Y" only) will reverse the order.

I just pushed an update that revises the heuristic: If "X" is found, and either "Y" or "Z", then it assumes it's some kind of spatial data and that x,y,z should be in that order, not confused with Y as luminance and Z as depth.

Copy link
Contributor

@fpsunflower fpsunflower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure the logic is sound, but feels like something that deserves unit tests spelling out all these corner cases (and the common ones).

@lgritz
Copy link
Collaborator Author

lgritz commented May 29, 2020

You're right, I'll add some tests.

Brief background: OpenEXR sorts channel names alphabetically. Most
formats dont allow channel naming at all, let alone forcing a
particular order contrary to the order the user tries to write it to
the file. OIIO promises that if there is RGB, they will be first and
in that order, next will be alpha if present, then depth, then
everything else.

So our exr reader will reshuffle channels into this canonical order
before presenting to the calling app. In addition to literal R, G, and
B being first, also obvious synonyms are accepted and understood to be
first ("Red", "Green", "Blue", and also "Y" in case it's just a
1-channel intensity and then maybe alpha).

A user noticed an interesting edge case: what if it's a normal map or
positional info, so the channel names are x, y, z? You want it in that
order (coincidentally alphabetical), duh. But the current heuristic
will reorder it as Y (mistaking for intensity), Z (mistaking for
depth), then X (a non-special name, so goes last). Ick!

The solution is that we need a more sophisticaed heuristic. This patch
changes the logic to first just sort by layer, then within each layer,
look to see if there are channels "x", and either "y" or "z", and if
so, then we can conclude that it means something spatial and that Y
isn't really intensity and Z isn't really depth for that particular
layer, so an alternate ordering is used. In all other cases, we
continue to use the old heuristic.

Signed-off-by: Larry Gritz <lg@larrygritz.com>
@lgritz
Copy link
Collaborator Author

lgritz commented May 30, 2020

Turns out the openexr-images collection had something with exactly the case we are addressing here. Added a test of that to our testsuite.

@lgritz lgritz merged commit 30ce101 into AcademySoftwareFoundation:master Jun 1, 2020
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Jun 1, 2020
…SoftwareFoundation#2595)

Brief background: OpenEXR sorts channel names alphabetically. Most
formats dont allow channel naming at all, let alone forcing a
particular order contrary to the order the user tries to write it to
the file. OIIO promises that if there is RGB, they will be first and
in that order, next will be alpha if present, then depth, then
everything else.

So our exr reader will reshuffle channels into this canonical order
before presenting to the calling app. In addition to literal R, G, and
B being first, also obvious synonyms are accepted and understood to be
first ("Red", "Green", "Blue", and also "Y" in case it's just a
1-channel intensity and then maybe alpha).

A user noticed an interesting edge case: what if it's a normal map or
positional info, so the channel names are x, y, z? You want it in that
order (coincidentally alphabetical), duh. But the current heuristic
will reorder it as Y (mistaking for intensity), Z (mistaking for
depth), then X (a non-special name, so goes last). Ick!

The solution is that we need a more sophisticaed heuristic. This patch
changes the logic to first just sort by layer, then within each layer,
look to see if there are channels "x", and either "y" or "z", and if
so, then we can conclude that it means something spatial and that Y
isn't really intensity and Z isn't really depth for that particular
layer, so an alternate ordering is used. In all other cases, we
continue to use the old heuristic.

Signed-off-by: Larry Gritz <lg@larrygritz.com>
@lgritz lgritz deleted the lg-exrchanorder branch June 1, 2020 16:08
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.

None yet

2 participants