ImageReader: Provide configurable fallback/failure mode

[mikepkes]

On ImageRead; we should provide a configurable failure mode when the image cannot be read (missing/corrupt).

I imagine the following options; though there may be other useful options: Error Empty (uses default format) Hold (before/after/closest) Input Plug (This would allow covering all sorts of other image-generating defaults)

[johnhaddon]

I've been wondering a bit about doing this in a separate node rather than in the ImageReader itself. Such an ErrorHandler node would in theory be able to work for any type of data like the Switch and Loop nodes we already have. It would have two inputs, the first the one with the potential error, and the second the one to use in the case of error - it would simply pass through the first input and revert to the second in the case of errors. We could either use that internally to the ImageReader, or people could put it downstream themselves.

From a development perspective I definitely like the thought of separating out the functionality like this. I suspect people might feel that from a user perspective it should just be a setting on the ImageReader rather than a manually placed downstream node, so let me point out a couple of benefits of the downstream node approach, to see if they appeal at all :

• It forces users to jump through a hoop (albeit a very small one) to mask errors. I know this feature has uses, but I also think that any kind of error masking is a pretty dangerous thing in a pipeline, so adding one more step doesn't seem necessarily to be a bad thing to me.
• It doesn't totally mask errors. The ErrorHandler itself would be outputting a valid image, but the upstream ImageReader would have the error indicator set in the UI. This gives users a chance to spot problems that might otherwise be masked by the "Hold" mode.
• It doesn't require a weird image input on an ImageReader node, which you would logically expect to just be a source.
• It means we can reuse the functionality elsewhere.

Another thing to bear in mind is that at some point the ImageReader might well become a compound node, using an OIIOReader (the current reader renamed) and a ColorSpace node internally to automatically convert to linear. If we're doing that then it would be very natural to use an ErrorHandler internally even if we provided the setting direct on the node.

This (and internal conversion to linear) seem to be an interesting question of where you draw the line in the "nodes should do one thing (and only one thing) well" principle. I'd be interested to hear what other folks think - @HughMacdonald, @andrewkaufman, @danieldresser, @lucienfostier?

[lucienfostier]

Hey John,

what about using a switch node with an expression that query the error state of one of the input?

that would be still be a node by itself to handle error and you dont have to create a whole new node.

I'm not sure about the benefit of displaying the error flag on the write node if we include the error handler in the the reader,

when I'm comping I know what image sequence could be a problem ( not finished rendering ) but the sup wants to see a version so I just need something that hold or render black( at framestore we also had an optical flow interpolation option which could render ok sometimes, and that would always come with a burn in to know there is missing/broken frame).

The only annoyance of this type in nuke is when you render, you have "continue on error" mode which can mislead you into thinking it rendered ok...but that is another story.

to sum, I would prefer to use a switch node with an expression ( more flexible with arbitrary expression that return the plug index to use ) and have an error handler included in the reader.

cheers

Lucien

Sent from my iPhone

On Aug 27, 2015, at 02:24, John Haddon notifications@github.com wrote:

I've been wondering a bit about doing this in a separate node rather than in the ImageReader itself. Such an ErrorHandler node would in theory be able to work for any type of data like the Switch and Loop nodes we already have. It would have two inputs, the first the one with the potential error, and the second the one to use in the case of error - it would simply pass through the first input and revert to the second in the case of errors. We could either use that internally to the ImageReader, or people could put it downstream themselves.

From a development perspective I definitely like the thought of separating out the functionality like this. I suspect people might feel that from a user perspective it should just be a setting on the ImageReader rather than a manually placed downstream node, so let me point out a couple of benefits of the downstream node approach, to see if they appeal at all :

It doesn't totally mask errors. The ErrorHandler itself would be outputting a valid image, but the upstream ImageReader would have the error indicator set. This gives users a chance to spot problems that might otherwise be masked by the "Hold" mode. It doesn't require a weird image input on an ImageReader node, which you would logically expect to just be a source. It means we can reuse the functionality elsewhere. Another thing to bear in mind is that at some point the ImageReader might well become a compound node, using an OIIOReader (the current reader renamed) and a ColorSpace node internally to automatically convert to linear. If we're doing that then it would be very natural to use an ErrorHandler internally even if we provided the setting direct on the node.

This, and the conversion to linear seem to be an interesting question of where you draw the line in the "nodes should do one thing (and only one thing) well" principle. I'd be interested to hear what other folks think - @HughMacdonald, @andrewkaufman, @danieldresser, @lucienfostier?

— Reply to this email directly or view it on GitHub.

[andrewkaufman]

I definitely think we should go for the secondary node if its possible. We have an internal IE ticket to do the same error masking for our VDBReader as well. I do think in both cases, and embedded error handler with the user facing plug on the Reader node is more intuitive. And I was planning that embedded ColorSpace->linear node for my current OCIO PR by the way, so its not a distant future type of thing, unless you strongly advise against it.

[johnhaddon]

Yeah, there's no such thing as an error status that can be queried by an expression, so we'd need a secondary node if it's going to be something that can be shared. Let's get the basics of your OCIO PR merged as one unit of work before we talk about the ImageReader side of things. I'm not strongly against it by any means, but I'd rather do things one at a time.

[HughMacdonald]

Hey all, sorry I didn't get back to this yesterday - I was out for the day...

I like the idea of having a specific node (maybe a wrapper around Switch) called something like ErrorSwitch, or SwitchOnError, etc. It could be useful in a multitude of places.

Also, adding this kind of functionality into ImageReader is potentially good, but it'd be nice to have an ImageRawReader (the same as the current ImageReader), plus an ImageReader that wraps ImageRawReader, ColourSpace and ErrorSwitch.

Having both available allows users to decide how much they want to mask the errors. If they just want to bring images in, they can use the ImageReader, which'll do everything internally, or they can be more explicit with separate nodes.

On a similar note, it'd be good to add first/last frame functionality to the ImageReader node, with black/hold options.

[johnhaddon]

Cool. I like the ErrorSwitch name.

Making the individual nodes available but also having an ImageReader meganode sounds like a reasonable compromise. To your mind would the first/last black/hold be on the ImageReader or on the ImageRawReader? I suspect it'd be easier to implement at the ImageReader level if that sounds OK.

[HughMacdonald]

Yeah, that sounds okay to put on the ImageReader node. Probably good to create a new node that does that kind of functionality too - FrameRange, maybe?

Also, I'd need to check, but can the file browser return the frame range of a selected image sequence as well as the sequence name?

[johnhaddon]

Yeah, a FrameRange node sounds like it could be handy. The file browser returns a FileSystemPath object from which the frame range can be extracted via a property() call - Andrew added that just the other day. We'd need to poke around a bit in the UI code, but we could find a way of squeezing that into the right plugs somehow.

[HughMacdonald]

Awesome - sounds good to me.

[andrewkaufman]

I'm a bit confused by the separate FrameRange node. I thought that black/hold stuff was all coming from the ErrorSwitch in some what-to-do-on-error mode.

[HughMacdonald]

The FrameRange node would have to be a little different from the ErrorSwitch, in that it would need to know what the first and last frames of the sequence are, and to clamp to those if we're outside. I would see this as not necessarily erroring behaviour. A missing frame inside the defined sequence would be an error, but wanting to only show a sequence and either holding the first/last frame or showing black would be expected behaviour.

The other thing with the FrameRange node would be the ability to set the frame range to shorter than the actual sequence on disk. So a sequence of frames 1-100 may have a FrameRange of 25-50, so anything before 25 would show frame 25 (or black) and same for after 50. If this was done with an ErrorSwitch, it wouldn't error until before 1 or after 100.

[johnhaddon]

We have a similar use case to the FrameRange node when dispatching tasks from ExecutableNodes - there's a master framerange the dispatcher uses to decide what frames to evaluate, but we might want to mask those frames so that a particular node is only dispatched for a subset. I'm going to put some thought into whether or not we can share the code for this with the FrameRange code, so hold off on diving into implementing that just now...

[HughMacdonald]

I wasn't planning on touching this one yet - just mentioning how I would imagine it working.

I could see this being done as a MixIn, in the same way as TimeWarp - basically it would be identical, other than the internals of TimeWarp::processContext, which would just clamp the frame number.

[johnhaddon]

Closing this, since it has been done already.