TIFF: subtle bug -- re-opening forgot about rawcolor hint. #2285
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fpsunflower
reviewed
Jul 11, 2019
| @@ -1428,7 +1428,8 @@ TIFFInput::read_native_scanline(int subimage, int miplevel, int y, int z, | |||
| ImageSpec dummyspec; | |||
| int old_subimage = current_subimage(); | |||
| int old_miplevel = current_miplevel(); | |||
| if (!close() || !open(m_filename, dummyspec) | |||
| close_tif(); | |||
There was a problem hiding this comment.
Given how tricky this was to track down -- at least leave a comment about why you call close_tif vs. close() here?
And surely this deserves a unit test? 😄
When scanlines are accessed out of order, to emulate true random access, sometimes we close the file and re-open. But in this one spot, instead of calling close_tif() we called close(), and the difference is that close() also calls init() which resets the options that might have been set by configuration hints -- such as "oiio:RawColor" and "oiio:UnassociatedAlpha". In this case, we may want to close/open the TIFF file itself, but we don't want to "forget" about any configuration hints from the original open call. The circumstances that led to this being symptomatic are obscure and amusing: - Scanline TIFF file with CMYK color space, and opened with a configuration hint "oiio:RawColor" which causes the CMYK values (4 chans) to be preserved rather than it all get auto-converted to RGB and appear to the app as if it were a 3 channel file. - The image was held in an ImageCache-backed ImageBuf. And the IC has the "autotile" feature turned on (so it will try to break it up into tiles in the cache, instead of storing the whole scanline image as a single tile). - An IBA function was running parallelized over multiple threads, and it happened that a thread needing a lower part of the image (high scanline number) made the request for its tile before another thread that needed a tile with lower scanline numbers. This caused the actual reads of the scanlines to not be in proper order, therefore necessitating a close and re-open of the file. - Upon the re-open, it had forgotten that "oiio:RawColor" was set, so even though the app had asked for the pixels to be written into a 2k x 4 chan CMYK buffer, the confused TIFF reader thought it was in the mode where it was emulating a 3 channel RGB file. So it would not only convert to RGB when it should have left it as cmyk, but since the RGB is only 3 channels, the 2048 pixels only filled in the left-most 1536 (that is, 3/4!) of the value slots in each scanline of the app's buffer. - When running valgrind on the app that was reading the image (for unrelated reasons), it was complaining about reading uninitialized memory when it would get to pixel 1536, mystifying everybody. ... And I'm so glad today I work on monster movies and cartoons, and not nuclear power plant control software or life support systems.
If you open an ImageCache-backed ImageBuf, close, then have another ImageBuf opened later with a different set of "config" hints... you may not get expected behaviors. Instead, be sure to invalidate it and add it to the cache in "replace" mode.
|
Updated with comments, unit test, and a couple more related enhancements I found while constructing the test. (It changed test output, may take me a couple rounds of CI to make sure all the output is updated properly.) |
|
LGTM! |
lgritz
added a commit
to lgritz/OpenImageIO
that referenced
this pull request
Jul 12, 2019
…ftwareFoundation#2285) When scanlines are accessed out of order, to emulate true random access, sometimes we close the file and re-open. But in this one spot, instead of calling close_tif() we called close(), and the difference is that close() also calls init() which resets the options that might have been set by configuration hints -- such as "oiio:RawColor" and "oiio:UnassociatedAlpha". In this case, we may want to close/open the TIFF file itself, but we don't want to "forget" about any configuration hints from the original open call. The circumstances that led to this being symptomatic are obscure and amusing: - Scanline TIFF file with CMYK color space, and opened with a configuration hint "oiio:RawColor" which causes the CMYK values (4 chans) to be preserved rather than it all get auto-converted to RGB and appear to the app as if it were a 3 channel file. - The image was held in an ImageCache-backed ImageBuf. And the IC has the "autotile" feature turned on (so it will try to break it up into tiles in the cache, instead of storing the whole scanline image as a single tile). - An IBA function was running parallelized over multiple threads, and it happened that a thread needing a lower part of the image (high scanline number) made the request for its tile before another thread that needed a tile with lower scanline numbers. This caused the actual reads of the scanlines to not be in proper order, therefore necessitating a close and re-open of the file. - Upon the re-open, it had forgotten that "oiio:RawColor" was set, so even though the app had asked for the pixels to be written into a 2k x 4 chan CMYK buffer, the confused TIFF reader thought it was in the mode where it was emulating a 3 channel RGB file. So it would not only convert to RGB when it should have left it as cmyk, but since the RGB is only 3 channels, the 2048 pixels only filled in the left-most 1536 (that is, 3/4!) of the value slots in each scanline of the app's buffer. - When running valgrind on the app that was reading the image (for unrelated reasons), it was complaining about reading uninitialized memory when it would get to pixel 1536, mystifying everybody. ... And I'm so glad today I work on monster movies and cartoons, and not nuclear power plant control software or life support systems. Additional fixes included: * And add a test case * Python ImageBuf: add constructor that takes a configuration hint spec * ImageBuf: when adding file to cache, careful with configuration. If you open an ImageCache-backed ImageBuf, close, then have another ImageBuf opened later with a different set of "config" hints... you may not get expected behaviors. Instead, be sure to invalidate it and add it to the cache in "replace" mode.
lgritz
added a commit
to lgritz/OpenImageIO
that referenced
this pull request
Jul 12, 2019
…ftwareFoundation#2285) When scanlines are accessed out of order, to emulate true random access, sometimes we close the file and re-open. But in this one spot, instead of calling close_tif() we called close(), and the difference is that close() also calls init() which resets the options that might have been set by configuration hints -- such as "oiio:RawColor" and "oiio:UnassociatedAlpha". In this case, we may want to close/open the TIFF file itself, but we don't want to "forget" about any configuration hints from the original open call. The circumstances that led to this being symptomatic are obscure and amusing: - Scanline TIFF file with CMYK color space, and opened with a configuration hint "oiio:RawColor" which causes the CMYK values (4 chans) to be preserved rather than it all get auto-converted to RGB and appear to the app as if it were a 3 channel file. - The image was held in an ImageCache-backed ImageBuf. And the IC has the "autotile" feature turned on (so it will try to break it up into tiles in the cache, instead of storing the whole scanline image as a single tile). - An IBA function was running parallelized over multiple threads, and it happened that a thread needing a lower part of the image (high scanline number) made the request for its tile before another thread that needed a tile with lower scanline numbers. This caused the actual reads of the scanlines to not be in proper order, therefore necessitating a close and re-open of the file. - Upon the re-open, it had forgotten that "oiio:RawColor" was set, so even though the app had asked for the pixels to be written into a 2k x 4 chan CMYK buffer, the confused TIFF reader thought it was in the mode where it was emulating a 3 channel RGB file. So it would not only convert to RGB when it should have left it as cmyk, but since the RGB is only 3 channels, the 2048 pixels only filled in the left-most 1536 (that is, 3/4!) of the value slots in each scanline of the app's buffer. - When running valgrind on the app that was reading the image (for unrelated reasons), it was complaining about reading uninitialized memory when it would get to pixel 1536, mystifying everybody. ... And I'm so glad today I work on monster movies and cartoons, and not nuclear power plant control software or life support systems. Additional fixes included: * And add a test case * Python ImageBuf: add constructor that takes a configuration hint spec * ImageBuf: when adding file to cache, careful with configuration. If you open an ImageCache-backed ImageBuf, close, then have another ImageBuf opened later with a different set of "config" hints... you may not get expected behaviors. Instead, be sure to invalidate it and add it to the cache in "replace" mode.
lgritz
added a commit
to lgritz/OpenImageIO
that referenced
this pull request
Jul 12, 2019
…ftwareFoundation#2285) When scanlines are accessed out of order, to emulate true random access, sometimes we close the file and re-open. But in this one spot, instead of calling close_tif() we called close(), and the difference is that close() also calls init() which resets the options that might have been set by configuration hints -- such as "oiio:RawColor" and "oiio:UnassociatedAlpha". In this case, we may want to close/open the TIFF file itself, but we don't want to "forget" about any configuration hints from the original open call. The circumstances that led to this being symptomatic are obscure and amusing: - Scanline TIFF file with CMYK color space, and opened with a configuration hint "oiio:RawColor" which causes the CMYK values (4 chans) to be preserved rather than it all get auto-converted to RGB and appear to the app as if it were a 3 channel file. - The image was held in an ImageCache-backed ImageBuf. And the IC has the "autotile" feature turned on (so it will try to break it up into tiles in the cache, instead of storing the whole scanline image as a single tile). - An IBA function was running parallelized over multiple threads, and it happened that a thread needing a lower part of the image (high scanline number) made the request for its tile before another thread that needed a tile with lower scanline numbers. This caused the actual reads of the scanlines to not be in proper order, therefore necessitating a close and re-open of the file. - Upon the re-open, it had forgotten that "oiio:RawColor" was set, so even though the app had asked for the pixels to be written into a 2k x 4 chan CMYK buffer, the confused TIFF reader thought it was in the mode where it was emulating a 3 channel RGB file. So it would not only convert to RGB when it should have left it as cmyk, but since the RGB is only 3 channels, the 2048 pixels only filled in the left-most 1536 (that is, 3/4!) of the value slots in each scanline of the app's buffer. - When running valgrind on the app that was reading the image (for unrelated reasons), it was complaining about reading uninitialized memory when it would get to pixel 1536, mystifying everybody. ... And I'm so glad today I work on monster movies and cartoons, and not nuclear power plant control software or life support systems. Additional fixes included: * And add a test case * Python ImageBuf: add constructor that takes a configuration hint spec * ImageBuf: when adding file to cache, careful with configuration. If you open an ImageCache-backed ImageBuf, close, then have another ImageBuf opened later with a different set of "config" hints... you may not get expected behaviors. Instead, be sure to invalidate it and add it to the cache in "replace" mode.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When scanlines are accessed out of order, to emulate true random access,
sometimes we close the file and re-open. But in this one spot, instead
of calling close_tif() we called close(), and the difference is that close()
also calls init() which resets the options that might have been set by
configuration hints -- such as "oiio:RawColor" and "oiio:UnassociatedAlpha".
In this case, we may want to close/open the TIFF file itself, but we don't
want to "forget" about any configuration hints from the original open call.
The circumstances that led to this being symptomatic are obscure and amusing:
Scanline TIFF file with CMYK color space, and opened with a
configuration hint "oiio:RawColor" which causes the CMYK values (4
chans) to be preserved rather than it all get auto-converted to RGB
and appear to the app as if it were a 3 channel file.
The image was held in an ImageCache-backed ImageBuf. And the IC has
the "autotile" feature turned on (so it will try to break it up into
tiles in the cache, instead of storing the whole scanline image as a
single tile).
An IBA function was running parallelized over multiple threads, and
it happened that a thread needing a lower part of the image (high
scanline number) made the request for its tile before another thread
that needed a tile with lower scanline numbers. This caused the actual
reads of the scanlines to not be in proper order, therefore necessitating
a close and re-open of the file.
Upon the re-open, it had forgotten that "oiio:RawColor" was set, so
even though the app had asked for the pixels to be written into a 2k
x 4 chan CMYK buffer, the confused TIFF reader thought it was in the
mode where it was emulating a 3 channel RGB file. So it would not
only convert to RGB when it should have left it as cmyk, but since the
RGB is only 3 channels, the 2048 pixels only filled in the left-most
1536 (that is, 3/4!) of the value slots in each scanline of the app's
buffer.
When running valgrind on the app that was reading the image (for
unrelated reasons), it was complaining about reading uninitialized
memory when it would get to pixel 1536, mystifying everybody.
... And I'm so glad today I work on monster movies and cartoons, and not
nuclear power plant control software or life support systems.