Paths

Table of Contentst

Differential D7201

Fix T74958: Infinite loop on using strip as modifier mask
ClosedPublic
Actions

Authored by Richard Antalik (ISS) on Mar 20 2020, 8:29 PM.

Tags

Subscribers

Sergey Sharybin (sergey)

Details

Reviewers

Campbell Barton (campbellbarton)

Maniphest Tasks

T74958: Infinite loop on using strip as modifier mask

Summary

Add recursion check before assigning strip as a mask for modifier.

Same check is used for recursion check when reassigning effect input, so it should not be possible to create recursion at all.

I have tried to add this check to effect input RNA accessor functions, but changing input from python is broken currently - see D6868

Diff Detail

Repository

Branch

arcpatch-D7201 (branched from master)

Build Status

Buildable 8347
Build 8347: arc lint + arc unit

Event Timeline

Richard Antalik (ISS) created this revision.Mar 20 2020, 8:29 PM

Harbormaster completed remote builds in B7247: Diff 22983.Mar 20 2020, 8:29 PM

Check for recursion in effec strips - changed my mind...

Harbormaster completed remote builds in B7249: Diff 22985.Mar 21 2020, 12:30 AM

Richard Antalik (ISS) edited the summary of this revision. (Show Details)Mar 21 2020, 1:00 AM

Richard Antalik (ISS) added a project: Video Sequencer.

Generally seems fine, notes inline.

source/blender/blenkernel/intern/sequencer.c
5982	This removes the `seq_is_parent` compared to `seq_is_predecessor`, as far as I can tell for Python there is nothing stopping the user from assigning a parent/child strip.
source/blender/editors/space_sequencer/sequencer_edit.c
595–596	These changes seem unrelated.

This revision now requires changes to proceed.Jun 2 2020, 6:08 AM

Address inlines.

source/blender/blenkernel/intern/sequencer.c
5982	It's more or less same function. `seq_is_parent`, seq_is_predecessor, and BKE_sequencer_check_recursivity all boil down to hierarchy traversing and comparing pointers of used and user. Will add check for assigning from python.
source/blender/editors/space_sequencer/sequencer_edit.c
595–596	This change was to report message and cancel operator when reassigning inputs for strips with 0 inputs. With operator cancelled I don't have to make another check if effect has inputs before I do `BKE_sequencer_check_recursivity()` checks. Though this change affects adding strips, which may be addressed in D6868. I guess I will revert this for now. I forgot what all things I have done there and why exactly.

Harbormaster completed remote builds in B8347: Diff 25432.Jun 3 2020, 4:15 AM

@Campbell Barton (campbellbarton), is there still any concerns from your side?

source/blender/blenkernel/intern/sequencer.c
6040	Think it's more clear to operate in terms `Check if "user" (indirectly) uses strip "used"`.

Campbell Barton (campbellbarton) accepted this revision.Jul 21 2020, 1:04 PM

Campbell Barton (campbellbarton) added inline comments.

source/blender/blenkernel/intern/sequencer.c
6041	Names user/used while correct read a bit odd, not sure of better names though. Also, I'd expect `seq_` prefix, unless this is a convention we want to move away from.
source/blender/editors/space_sequencer/sequencer_edit.c
597	picky no need to change.

This revision is now accepted and ready to land.Jul 21 2020, 1:04 PM

Campbell Barton (campbellbarton) added inline comments.Jul 21 2020, 1:13 PM

source/blender/blenkernel/intern/sequencer.c
6041	Suggestion: I was looking at other parts of the API and I find this reads better . `BKE_object_parent_loop_check(parent, ob)`. Even though parent/child terminology doesn't quite apply, we could use same naming here. eg: `BKE_sequencer_parent_loop_check(seq_parent, seq)`.

Forgot to link diff.
Closed by rB7ceb6ffe57e1

source/blender/blenkernel/intern/sequencer.c
6041	Personally I would avoid using term parent for this. I think `BKE_sequencer_render_loop_check(seq_main, seq)` is good compromise.

Revision Contents
Changeset List

Path

Size

source/

blender/

blenkernel/

BKE_sequencer.h

1 line

intern/

23 lines

editors/

space_sequencer/

sequencer_edit.c

36 lines

makesrna/

intern/

rna_sequencer.c

64 lines

Diff 25432

source/blender/blenkernel/BKE_sequencer.h

Loading...

source/blender/blenkernel/intern/sequencer.c

Loading...

source/blender/editors/space_sequencer/sequencer_edit.c

Loading...

source/blender/makesrna/intern/rna_sequencer.c

Loading...