SG-40369: SequenceNode Stability Improvements

[markreidvfx]

Summarize your change.

This PR fixes program crashes that can occur if you modify Sequence edl properties. Most of these are due to lack of proper bounds checking on the properties in various locations.

• Add bounds checks before trying to access the edl properties
• Check source indexes are within range of nodes inputs.
• Add NULL check to ImageRenderer

We also modified the node’s behavior so that it no longer throws an exception when it enters a bad state. Instead, it now returns a newNoImage containing the error message. We discovered that when EDL properties are changed dynamically, the node can easily enter a temporary, inconsistent state. This happens because property change events may occur between the setting of individual EDL properties, and the operations that can take place in between are not trivial to manage. Displaying the error console in these situations proved to be both frustrating and confusing for users.

Describe what you have tested and on which operating system.

Linux, macOS

[pbergeron-adsk]

I'm probably quite tired, and my eyes are bugging out, but, what are you doing here? are you checking if index is out of bound? or are you checking if it's the last element, or out of bounds?

Feels to me like, if you're checking if out of bounds, >= size() is sufficient, no need for -1? I mean...

let's say:

• size() == 0. size()-1 is 0xffffffffffffffff, so index >= is false when it should be out of bounds.

• size() == 1. size()-1 is 0, which is a valid index. but you're checking if index >= [0], which would return true, and so return EvalPoint(-1,0,-1).

Was the above really the intent?
should there be another condition that says

|| size() == 0

?

[markreidvfx]

For some reason the original designed choose to pad the source, in and out arrays with a zero so they are the same length as frame.

https://aswf-openrv.readthedocs.io/en/latest/rv-manuals/rv-reference-manual/rv-reference-manual-chapter-six.html#rvsequence

edl
    {
        int frame  = [  1 11 21 31 41 ]
        int source = [  0  1  0  1  0 ]
        int in     = [  1  1 11 11  0 ]
        int out    = [ 10 10 20 20  0 ]
    }

your right about the zero case, but there is size < 1 check, earlier.

[markreidvfx]

The last edit is really just a boundary condition: it indicates how RV should handle frames past the end of the EDL. To be well formed, an RV EDL needs to include this

I don't think my code is doing this, according to the docs in the example, this should hold frame 0 of source 0 if the index is past the end. I don't think the current implementation even does this.

Also what is suppose to happen if the frame is before the first frame isn't defined either.

[markreidvfx]

if the user entered something like this:

edl
    {
        int frame  = [  1 11 21 31 41 ]
        int source = [  0  1  0  1  0 ]
        int in     = [  1  1 11 11  1 ]
        int out    = [ 10 10 20 20  10 ]
    }

I have no idea how what this is suppose to mean for a bounds condition. repeat 1-10?

ignoring the last index I think is the most sensible thing to do and avoid all these needless complications.

[pbergeron-adsk]

thanks I'm approving this.

[bernie-laberge]

LGTM
Thank you @markreidvfx !

[pbergeron-adsk]

LGTM. All of these look safe to me.

[pbergeron-adsk]

@markreidvfx as part of an effort to augment the degree of QA we do on community bug fixes, we'll be asking for repro steps and materials (media, if applicable) to reproduce the before/after states.

Though this looks good to me, do you have exact materials or repro steps that demonstrate the crashes you fixed? Not necessarily for me for the code review, but for the QA folks. (same will have to be for your other submissions, I'm afraid - we will soon update the discussion template to reflect these policy changes)

[markreidvfx]

These crashes are related to using the sequence node with autoEDL is off. Here is one of the most basic ones

With this file sequence_node_crash.rv.zip

run this command that sets a invalid sourceIndex and tries get the node geometry.

from rv import commands 
node = "defaultSequence_sequence"
commands.setIntProperty(f"{node}.edl.source",  [ 3, 1, 0 ], True)
commands.nodeImageGeometry(node, 1)

RV should just crash. nodeImageGeometry is one of the entry points that won't check the sourceIndex is valid.

With the fix the sequence should still be viewable even after setting the invalid sourceIndex. The viewer will show a error message.

It will take me a bit to track down more repos if you need more. I did these fixes almost a year ago and they are all related to crashes discovered using our timeline editor.

I don't think i have the core dump that led to the ImageRender null check, I remember that being difficult to trigger. FWIW we've been using the these fix in production for almost a year.