iv: tolerate partially-written EXR files - #5463
linsen458-spec wants to merge 2 commits into
Conversation
|
|
8b322dd to
fa31cab
Compare
| auto pixel_at = [&](int x, int y, int z) -> void* { | ||
| return (char*)buf.localpixels() + (z - spec.z) * buf.z_stride() | ||
| + (y - spec.y) * buf.scanline_stride() | ||
| + (x - spec.x) * buf.pixel_stride(); | ||
| }; |
There was a problem hiding this comment.
There is already ImageBuf::pixeladdr()
|
I approve of this generally, but I can't help but wonder if having all the logic live in iv is kind of the wrong place for it? At the very least, it means that anybody wanting the same thing in an app other than iv needs to start from scratch. Maybe we can restructure this so it's more reusable? A few thoughts follow. These are not mutually exclusive, and not definitive -- just throwing some things out for discussion. Some are just questions. Do we want to support the "missing data is ok" for scanlines, or only tiles? Is OpenEXR the only format that can do this or that we care about doing it -- i.e. it's sort of tolerated to have an openexr file with missing tiles and it's understood to be a "checkpointed" file, perhaps with the missing tiles representing "work that wasn't done, and maybe still needs to be done and can be resumed." Are there any other file formats where we would want to tolerate missing data, or where that's even possible? Especially if OpenEXR is the only file format we're really talking about, maybe it should be a "open-with-config hint" to openexr to tolerate missing tiles or scanlines and just fill them with black and not issue errors for that case? Or SHOULD it issue errors, but we make sure the errors generated by missing scanlines/tiles have a particular keyword so the caller can screen for that and suppress the error and keep reading? Or a global Or should the logic for all this be at the ImageBuf level, where IB would gain
In short, I'd like to push the bulk of this logic to one of ImageBuf or OpenEXRInput, and have the iv part just be the couple lines it takes to turn on the signal to that lower level to operate in the "tolerate missing parts" mode. |
|
Thanks for the quick review — and for talking this through rather than just waving it through. Honestly I think you're right. I put the logic in iv mostly because that's where I hit the problem, but there's nothing really iv-specific about it. What I'd suggest:
On your questions: I'd keep scanlines in scope along with tiles — checkpointed renders stop mid-scanline all the time, and the tolerant read I wrote handles both the same way. I also think OpenEXR is the only format where this really makes sense today, so I'd rather make it an OpenEXR-specific hint than pretend it's more general than it is. My gut says no to the global If the input-level hint sounds right to you, I'll restructure the patch that way. |
|
If openexr is the only format where it's reasonable for a failed scanline or tile to be interpret as a "valid file" but that has missing data, then I think the right place is the "openexr:missing_data_ok" hint to that reader. (The other readers will just ignore it.) You can still make it work at the ImageBuf level just by passing the config to the IB constructor, so it works there as well, and that's what you'll want to do on the iv side. I'm honestly not sure how to communicate back to the caller WHICH tiles were missing. If you don't need the inventory for now, let's just skip it for now and we'll figure it out later when somebody has more of a need for it. |
|
That all sounds right to me — simpler, too. I'll move the tolerant-read logic into OpenEXRInput behind the |
50978ee to
10bf70a
Compare
|
The reworked patch is up — single commit, rebased on current main. The tolerance logic now lives in OpenEXRInput behind While testing with a truncated file I also ran into a stack overflow that turned out to be pre-existing: with Thanks again for the design discussion — this ended up much cleaner than what I originally sent. |
|
I just want to flag that the
So is it effectively redundant to think about adding a new dedicated attribute for this? |
Oh, yeah, I'd forgotten all about that. It certainly looks like we'd thought of this at some point before, and it is more or less already implemented, everything we need on the OpenEXR side, right? The only thing really missing is iv setting the config option to get this behavior? |
|
Fair point — and it matches what we found while digging: the missingcolor machinery is all there, which is why the current version of this PR already routes through it (the new attribute was just a thin alias that turns it on with a black fill). But you're both right that the alias adds nothing — I'll simplify this to iv simply setting One piece I'd still keep: testing the truncated-file case exposed an infinite recursion in the chunk cache path — with |
When iv opens an image file that was only partially written -- for example, an EXR that a renderer crashed on or was killed before it finished writing the pixel data -- the read fails and iv refuses to display anything at all. Now, if the straightforward read fails, iv reopens the file with the existing "oiio:missingcolor" config attribute set to black, which asks the OpenEXR reader to fill unreadable scanlines or tiles with that color instead of failing the read. Readers without that support ignore the config, so the re-read simply fails the same way for them. In the partial case, the status bar displays a note explaining that the file is only partially readable. For images that were read through the ImageCache (where pixel data isn't touched until display time), a cheap probe of the last scanline or tile checks that the pixel data is intact, and falls back to the re-read if it isn't. One prerequisite fix along the way: a failed multi-scanline chunk decode with oiio:missingcolor set re-entered the per-scanline retry, which re-entered the chunk cache until the stack blew up -- the per-scanline retry now bypasses the chunk cache, fixing that for missingcolor users too and making the iv fallback actually work on compressed multi- scanline files. Fixes AcademySoftwareFoundation#4713 Assisted-by: Claude Code / glm-5.3-flash Signed-off-by: linsen <251731047+linsen458-spec@users.noreply.github.com>
10bf70a to
2be42e1
Compare
| /// True if the last read only partially succeeded, i.e. the image | ||
| /// specification was readable but some of the pixel data could not be | ||
| /// read (for example, a file that was only partially written before a | ||
| /// renderer crashed). |
There was a problem hiding this comment.
I'm not asking you to change this, because you have merely matched the style of the surrounding code, so this is more about me just talking as a note to myself: These /// doxygen-style comments should never have been in this file, since it's not a public header and we don't automatically extract any documentation from this class.
lgritz
left a comment
lgritz
left a comment
There was a problem hiding this comment.
I'm not sure I understand
| // Internal version with a flag letting the per-scanline retry bypass the | ||
| // chunk cache (see read_native_scanlines_individually). | ||
| bool read_native_scanlines(int subimage, int miplevel, int ybegin, int yend, | ||
| int z, int chbegin, int chend, void* data, | ||
| bool bypass_chunk_cache); |
There was a problem hiding this comment.
I find this a little confusing, with this custom function mixed in among the overrides of the parent class. Maybe you can move the declaration of this function into the private section toward the end of the class declaration? And maybe it should have a different name so it's not easily confused with the regular read_native_scanlines?
| return read_native_scanlines(subimage, miplevel, ybegin, yend, z, chbegin, | ||
| chend, data, /*bypass_chunk_cache=*/false); | ||
| } |
There was a problem hiding this comment.
Let's add a comment to clarify. Something like:
This is the externally-called read_native_scanlines, which is a wrapper around the internal version that takes an additional parameter saying whether to use the chunk cache or not. When called externally, we always do.
This also raises the question of whether it should be bypass_chunk_cache (defaulting to false), or use_chunk_cache (defaulting to true). Of course, logically they are equivalently useful, but you should think about which one is less awkward to use when explaining how it works and when you'd use it.
| /// partially written). | ||
| inline bool | ||
| image_data_readable(string_view filename, const ImageSpec* config, int subimage, | ||
| int miplevel) | ||
| { |
There was a problem hiding this comment.
Is this still needed?
I understand the strategy of "read normally, and only if there's an error, try again using the missingcolor option."
But why not make the "try again" step go through ImageBuf like the first one, instead of needing all this code that drops down to the ImageInput level API?
Also, we should consider the merits of three approaches for iv:
- Try, and if you fail, try again with missing pixels tolerated. (The current strategy you've implemented.)
- An option -- perhaps an iv persistent preference, with a command-line override in either direction -- that specifies whether missing pixels should be tolerated and that affects even the "first try."
- Always succeed. That is, just make iv always tolerate missing pixels and call it a day.
Sorry, that fragment was just a stray half sentence that I meant to abandon in favor of commenting inline. |
|
@linsen458-spec I'm sorry if I'm guessing wrong, but the prose in your replies uses a lot of phrasing that makes me think it's LLM-written. We don't mind if you use coding assistants to help with the implementation, but strongly prefer that your interaction with the community (like, responding to review comments) be authentic. If you're using it to aid in language translation or the like, then please at least briefly disclose it. |
|
You're right — yes, I did use an LLM. My native language is not English, and I've been away from English both at work and in daily life for many years. To avoid grammar mistakes, I had my writing translated and polished by an LLM. But the code and the PR content were all reviewed and revised by me. If this caused you any concern, I'm sorry — I'll be more careful about this kind of thing when using AI going forward. Hope you don't mind. |
|
No, it's all fine, and I totally support LLMs to handle fixing grammar in your non-native language. Just state it in the message: "This is not my native language, I'm using an LLM to restate and fix grammar in my replies." What we don't want to be doing in having conversations with bots when we think we're talking to people. Some open source projects are really getting clobbered with LLM slop (we're not so far, thankfully), so it's important for us to know that there's a real person there that we should pay attention to. |
|
There are still some CI failures that seem related to this PR. Maybe the newer compilers are giving warnings about things that the older compilers missed? |
|
This is not my native language, I'm using an LLM to restate and fix grammar in my replies and to double-check my work for oversights. Everything you see here has been viewed, reviewed, and approved by me personally before submitting. Confirmed — gcc 15/16 with Fixed with a |
Newer GCC (15/16 with -Woverloaded-virtual) flags that OpenEXRInput's pointer-based read_native_scanlines overloads hide the span-based overloads from the ImageInput base class. Add a using declaration to unhide them, per the base-class guidance that inputs may implement either form.
|
There were several comments/questions I made when I reviewed it a couple days ago that I'd like to hear your opinions about. Please review the earlier discussion above. |
3 participants
Fixes #4713
If iv opens an EXR that a renderer crashed on mid-write, the read fails and iv refuses to show anything, even though most of the pixels are actually fine.
As discussed above, everything needed on the OpenEXR side already exists (
oiio:missingcolor), so this version keeps iv's part to a minimum:oiio:missingcolorset to black, and the OpenEXR reader fills the unreadable scanlines/tiles instead of failing. In the partial case the status bar notes that the file is only partially readable. For ImageCache-backed reads, a cheap probe of the last scanline/tile still catches the partially-written case before display time.oiio:missingcolorset used to recurse forever (per-scanline retry -> chunk cache -> retry -> ...), because the retry re-entered the chunk cache it had just failed to fill. The retry now bypasses the chunk cache. This fixes that pre-existing bug formissingcolorusers too, and it's what makes the iv fallback work at all on compressed multi-scanline files.Verified locally with a checker EXR truncated at 55%: the straightforward read fails with the expected error, the
missingcolorread succeeds with the readable portion intact and the truncated tail black, and full files behave identically with and without the attribute.Thanks for steering this into a much simpler shape — using the existing attribute is obviously the right call.