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

fix(exr): suppress exr attributes that interfere with our hints #4008

Merged
merged 1 commit into from
Oct 10, 2023

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Oct 9, 2023

Our exr writer uses some special hints starting with "openexr:" to control aspects of the writing, and the reader sets these to communicate some things about the file it's reading.

But what happens if... the actual exr file has metadata of tha same name. Oops, those get carried along and mess with things. We had previously accounted for this by suppressing arbitrary metaname named "openexr:lineOrder" if we find it in the input (the actual lineorder comes from a header field, not named metadata, but OIIO uses the name "openexr:lineOrder" to communicate it back as metadata.

Turns out that there are two more that we need to proactively suppress: "openexr:levelmode" and "openexr:roundingmode". There's no good reason for those to ever be in an exr file (like I said, those names are NOT how openexr itself sets those in a file). And yet, we found at least one example in the wile where it was.

Copy link
Collaborator

@ThiagoIze ThiagoIze left a comment

Choose a reason for hiding this comment

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

CI appears to be failing.
Also, if this is OIIO specific data, should we start namespacing them so that conflicts are less likely?

Our exr writer uses some special hints starting with "openexr:" to
control aspects of the writing, and the reader sets these to
communicate some things about the file it's reading.

But we identified a case where the actual exr file had metadata named
"openexr:levelmode" -- which is of no particular meaning to openexr
itself, but it is the name we use to communicate hints to our writer
by make_texture. We just need to make sure that if we don't set it
proactivesly, we make sure to clear it if there is a stray bit of
metadata with that same name coincidentally.

Signed-off-by: Larry Gritz <[email protected]>
@lgritz
Copy link
Collaborator Author

lgritz commented Oct 10, 2023

I just replaced this with an entirely different solution that is simpler and that I like better, the fix is now specific to make_texture.

Copy link
Collaborator

@ThiagoIze ThiagoIze left a comment

Choose a reason for hiding this comment

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

So either we overwrite the levelmode with our good info or we erase the bad preexisting levelmode. Either way this gets cleaned up. This looks good to me.

@lgritz
Copy link
Collaborator Author

lgritz commented Oct 10, 2023

You pointed out a good issue, which is that currently there is a corner case where the user wanted something called "openexr:levelmode" as metadata but not for it to trigger any special behavior on our end. I just want to acknowledge that possibility and kick the can down the road, as this ticket is the wrong place to tackle such a thorny (but also obscure?) problem.

But I'll review our presumptions here:

  • Metadata that starts with the prefix "oiio:" is only intended for OIIO signaling -- information that OIIO sets to be informative to readers, or hints to writers, or other internally carried state.
  • Metadata starts with the name of a file format itself, like "openexr:" is meant to give hints from or to that format reader/writer, and should definitely be ignored if encountered by any other format writers.
  • Some other prefixes are semi-special, for example ones starting with "Exif:" are exif fields that will be honored by any file format that supports Exif in some fashion.

It is also worth noting that I think OpenEXR is the only format we regularly deal with that truly allows "arbitrarily named" metadata. Most formats have a fixed list by their specification and/or the structure fields of their headers. So really, any conflict between "openexr:foo" and intentional user data is an odd case for sure. It still gnaws at me a little, though, so maybe there is a possible future revision about the rules for how we handle hints vs oiio-specific metadata vs "user" metadata to eliminate any rare edge cases.

@lgritz lgritz merged commit 2e1e5f9 into AcademySoftwareFoundation:master Oct 10, 2023
23 of 25 checks passed
@lgritz lgritz deleted the lg-exr branch October 10, 2023 19:40
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Oct 10, 2023
…hints (AcademySoftwareFoundation#4008)

Our exr writer uses some special hints starting with "openexr:" to
control aspects of the writing, and the reader sets these to
communicate some things about the file it's reading.

But we identified a case where the actual exr file had metadata named
"openexr:levelmode" -- which is of no particular meaning to openexr
itself, but it is the name we use to communicate hints to our writer
by make_texture. We just need to make sure that if we don't set it
proactivesly, we make sure to clear it if there is a stray bit of
metadata with that same name coincidentally.

Signed-off-by: Larry Gritz <[email protected]>
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Oct 10, 2023
…hints (AcademySoftwareFoundation#4008)

Our exr writer uses some special hints starting with "openexr:" to
control aspects of the writing, and the reader sets these to
communicate some things about the file it's reading.

But we identified a case where the actual exr file had metadata named
"openexr:levelmode" -- which is of no particular meaning to openexr
itself, but it is the name we use to communicate hints to our writer
by make_texture. We just need to make sure that if we don't set it
proactivesly, we make sure to clear it if there is a stray bit of
metadata with that same name coincidentally.

Signed-off-by: Larry Gritz <[email protected]>
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.

2 participants