⚙ D11939 VSE: Cleanup speed effect math

Richard Antalik (ISS) requested review of this revision.Jul 15 2021, 7:26 PM

Harbormaster completed remote builds in B15861: Diff 39616.Jul 15 2021, 7:26 PM

Richard Antalik (ISS) edited the summary of this revision. (Show Details)Jul 15 2021, 7:35 PM

Richard Antalik (ISS) added a task: T89120: VSE: Speed Strip bugs.

Cleanup unused arguments

Harbormaster completed remote builds in B15863: Diff 39618.Jul 15 2021, 8:45 PM

In Multiply, an unreported bug in the previous versions, if the multiply value is dropping, it would freeze the image until it is raising again. In your version it'll reverse playing when the multiply value is dropping(even though the value is still positive):

I would expect Multiply to multiply the normal speed of the strip so a value at 1 would make it play at normal speed, 2 at double speed, 0.5 half speed, -0.5 half reverse speed and -1 normal speed, but reverse.

Frame Number seems to be missing bezier interpolation and aligned handles.

Length seems to be working ok.

If you're reworking the math here, consider the idea of using the movie strip's transformed duration as input duration and a manipulatable duration of the speed strip as the output duration. As it is, is Stretch always stretching the full untransformed in/out points of the strip and users do not know what the resulting duration is.

In the other "modes" the duration of the speed strip could also be used actively and could show the "final" output duration.

Additional findings and thoughts while testing this patch:

When transforming a handle position and an effect strip is applied(ex. speed) it'll snap to the original handle/(effect strip?) position, making it impossible to add a few frames by dragging the handle.
Snapping Playhead to keyframes as an option would be a very useful feature, when working with keyframes in strips like the speed strip keyframes.

In D11939#307665, @Peter Fog (tintwotin) wrote:

In Multiply, an unreported bug in the previous versions, if the multiply value is dropping, it would freeze the image until it is raising again. In your version it'll reverse playing when the multiply value is dropping(even though the value is still positive):

Thanks for checking, I realized later, that there could be differences, and I should have a look at original code a bit more, since the values could be integrated instead of discrete math per each frame...
That would complicate things a bit, but only for multiply mode I think.

In D11939#307665, @Peter Fog (tintwotin) wrote:

Frame Number seems to be missing bezier interpolation and aligned handles.

This is because property is integer, this change is from D11856

If you're reworking the math here, consider the idea of using the movie strip's transformed duration as input duration and a manipulatable duration of the speed strip as the output duration. As it is, is Stretch always stretching the full untransformed in/out points of the strip and users do not know what the resulting duration is.

I don't understand this.
In this patch I don't want to change anything but code.

When transforming a handle position and an effect strip is applied(ex. speed) it'll snap to the original handle/(effect strip?) position, making it impossible to add a few frames by dragging the handle.

Snapping Playhead to keyframes as an option would be a very useful feature, when working with keyframes in strips like the speed strip keyframes.

These are snapping issues. I would probably consider first to be feature. This is way off topic, devtalk thread is probably best place for such suggestion. https://devtalk.blender.org/t/vse-snapping-feedback/19570

In D11939#307693, @Richard Antalik (ISS) wrote:

In D11939#307665, @Peter Fog (tintwotin) wrote:

Frame Number seems to be missing bezier interpolation and aligned handles.

This is because property is integer, this change is from D11856

Does the value type determine keyframe interpolation and handle type? Do values need to be float? Sounds strange?

If you're reworking the math here, consider the idea of using the movie strip's transformed duration as input duration and a manipulatable duration of the speed strip as the output duration. As it is, is Stretch always stretching the full untransformed in/out points of the strip and users do not know what the resulting duration is.

I don't understand this.
In this patch I don't want to change anything but code.

https://developer.blender.org/D6110#304895

When transforming a handle position and an effect strip is applied(ex. speed) it'll snap to the original handle/(effect strip?) position, making it impossible to add a few frames by dragging the handle.

Snapping Playhead to keyframes as an option would be a very useful feature, when working with keyframes in strips like the speed strip keyframes.

These are snapping issues. I would probably consider first to be feature. This is way off topic, devtalk thread is probably best place for such suggestion. https://devtalk.blender.org/t/vse-snapping-feedback/19570

As mentioned these things I noticed while testing this patch, but now I've added them to devtalk. The first one seems to me as a bug, since it is basically snapping to it's previous point, and it shouldn't.

In D11939#307696, @Peter Fog (tintwotin) wrote:

In D11939#307693, @Richard Antalik (ISS) wrote:

In D11939#307665, @Peter Fog (tintwotin) wrote:

Frame Number seems to be missing bezier interpolation and aligned handles.

This is because property is integer, this change is from D11856

Does the value type determine keyframe interpolation and handle type? Do values need to be float? Sounds strange?

Yes, int type uses linear interpolation. Not sure if there is benefits of using float property. I guess you may want to use interpolation in this mode too.

If you're reworking the math here, consider the idea of using the movie strip's transformed duration as input duration and a manipulatable duration of the speed strip as the output duration. As it is, is Stretch always stretching the full untransformed in/out points of the strip and users do not know what the resulting duration is.

I don't understand this.
In this patch I don't want to change anything but code.

https://developer.blender.org/D6110#304895

That's not how that mode works though, it takes input and stretches it to length of speed strip. The fact that you can't edit this strip is by design, which is a bit sad.
Only solution is to break this mode on edit like split.

The way you propose this it would have to move both handles at the same time, which goes against (unwritten) design that elements shouldn't change size or position automatically.
if strip was anchored to speed effect edge I guess this could be possible, but then what happens if you split such strip?

Richard Antalik (ISS) planned changes to this revision.Jul 16 2021, 12:45 AM

Richard Antalik (ISS) edited the summary of this revision. (Show Details)

The way you propose this it would have to move both handles at the same time, which goes against (unwritten) design that elements shouldn't change size or position automatically.

Moving elements together is just like parenting in the 3d view, which also is an important feature in the vse, for keeping sync. It's implemented in snu's add-on for that reason.

When in stretch I think moving both handles should be allowed and also independently of each other, and then mute the movie file, so it doesn't show up outside the speed strip.

if strip was anchored to speed effect edge I guess this could be possible, but then what happens if you split such strip?

When in Stretch, there is no animation/keyframes, so the speed is constant, so when splitting the input strip, you detect where the split is in % and then you split the speed strip at the % relative to the current length of the speed strip. Then you do the same with the other part.

Here is an example(using color strips):
We want all of the (trimmed) frames from the movie strip played in the short space in between the two pink strips.
So first we find the in and out frame of the movie strip, then we adjust the green strip in between to two pink strips,
and since we're in stretch the first and last frame of the blue/movie strip, will become the first and last frame of the green strip,
though it is squeezed in a much shorter space.

Playing double speed(speed strip has half the duration of movie strip):

Playing half speed(speed strip has double the duration of the movie strip):

rebase on master

Harbormaster completed remote builds in B16032: Diff 39960.Jul 26 2021, 3:48 PM

bring back integrator

Harbormaster completed remote builds in B16040: Diff 39972.Jul 26 2021, 6:37 PM

Fix unintended changes
Minor cleanup.

Harbormaster completed remote builds in B16041: Diff 39973.Jul 26 2021, 6:50 PM

Bit more cleanup

Harbormaster completed remote builds in B16042: Diff 39974.Jul 26 2021, 6:52 PM

Fix incorrect definey

Harbormaster completed remote builds in B16043: Diff 39975.Jul 26 2021, 6:58 PM

Richard Antalik (ISS) edited the summary of this revision. (Show Details)Jul 26 2021, 7:18 PM

Nice cleanup.
It would be nice if we could remove the frameMap, but actually calculating the integral/area of an f-curve can be a heavy operation. (Creating a utility that does this might even be feasible if f-curve couldn't have modifiers).

I just added a few remarks:

source/blender/makesdna/DNA_sequence_types.h
503	At first glance, it's weird to use `_NOT` in a flag. Perhaps a more conventional name would be `SEQ_SPEED_FRAME_MAP_DIRTY`. (It would be nice to have a third opinion). But is this flag really necessary? We could free and set `frameMap` to `NULL` when it is modified.
source/blender/sequencer/intern/effects.c
3181	Could this flag be handled inside the function? (I haven't analyzed the other calls to this function in detail) if ((s->flags & SEQ_SPEED_FRAME_MAP_NOT_BUILT) == 0) { return; } s->flags &= ~SEQ_SPEED_FRAME_MAP_NOT_BUILT; (...) If that's okay, the function could be renamed to `seq_effect_speed_framemap_ensure`.

address inlines
Fix array length calculated incorrectly
Cleanup

Harbormaster completed remote builds in B16059: Diff 40002.Jul 27 2021, 2:15 PM

In D11939#310731, @Germano Cavalcante (mano-wii) wrote:

Nice cleanup.
It would be nice if we could remove the frameMap, but actually calculating the integral/area of an f-curve can be a heavy operation. (Creating a utility that does this might even be feasible if f-curve couldn't have modifiers).

It's probably fine to have frameMap precalculated for this one instance.

I will think a bit more about usage of seq_effect_speed_rebuild_map in seq_speed_effect_target_frame_get, because the function does 3 things:

Check if frameMap is to be used
Check if it is valid
Rebuild map

I think it could be split in a way that would allow to write ensure function and rebuild function

source/blender/makesdna/DNA_sequence_types.h
503	`SEQ_SPEED_FRAME_MAP_DIRTY` sounds OK. I tried to follow convention in file - see `SEQ_FONT_NOT_LOADED`, `SEQ_EFFECT_NOT_LOADED`. But is this flag really necessary? You can't rely on `frameMap` value, as it is `NULL` when not animated. Only way would be to rely on F-curve. My concern was `id_data_find_fcurve` being too slow, but checking again, it takes only 7 microseconds in debug build. So it's probably not necessary at all. Not sure if this was some recent improvement or I misremember this being an issue. This flag also allowed me to remove bool `force` argument from `seq_effect_speed_rebuild_map`, but having `force` argument is probably better than introducing new flag?
source/blender/sequencer/intern/effects.c
3181	Not sure if `seq_effect_speed_framemap_ensure` is best name, because it does not have to initialize `frameMap` Also since I have removed flag, it now returns, whether `frameMap` is used too.

In D11939#310940, @Richard Antalik (ISS) wrote:

I will think a bit more about usage of seq_effect_speed_rebuild_map in seq_speed_effect_target_frame_get, because the function does 3 things:

Check if frameMap is to be used

Check if it is valid

Rebuild map

I think it could be split in a way that would allow to write ensure function and rebuild function

With that maybe we could avoid checking if there is an f-curve with id_data_find_fcurve. But a 7ms overhead doesn't seem to be a problem.

In the patch I noticed something that could be a problem:

source/blender/sequencer/intern/effects.c
3152	`v->frameMap` can be different from `NULL` and `force` can be `true`. This seems to cause a memory leak.

This revision now requires changes to proceed.Jul 27 2021, 2:50 PM

When not using interpolation I'm getting flicker and render errors with this patch:

(Using Frame Number going from a key with the full length to zero)

Fix memleak

Harbormaster completed remote builds in B16106: Diff 40100.Jul 29 2021, 8:03 AM

In D11939#310961, @Germano Cavalcante (mano-wii) wrote:

With that maybe we could avoid checking if there is an f-curve with id_data_find_fcurve. But a 7ms overhead doesn't seem to be a problem.

Microseconds, not milliseconds. 7ms would be quite unacceptable.

I don't think we can avoid using id_data_find_fcurve. But I still don't want to overuse it. that's why seq_effect_speed_rebuild_map currently returns whether frameMap should be used or not.

This should be documented better though

So it could be like this

P2297 (An Untitled Masterwork)

1	diff --git a/source/blender/sequencer/intern/effects.c b/source/blender/sequencer/intern/effects.c
2	index d1d91097cd1..7589e254af4 100644
3	--- a/source/blender/sequencer/intern/effects.c
4	+++ b/source/blender/sequencer/intern/effects.c
5	@@ -3133,54 +3133,62 @@ static int early_out_speed(Sequence *UNUSED(seq), float UNUSED(facf0), float UNU
6	*/
7	static int seq_effect_speed_get_strip_content_length(const Sequence *seq)
8	{
9	if ((seq->type & SEQ_TYPE_EFFECT) != 0 && SEQ_effect_get_num_inputs(seq->type) == 0) {
10	return seq->enddisp - seq->startdisp;
11	}
12
13	return seq->len;
14	}
15
16	+static FCurve seq_effect_speed_speed_factor_curve_get(Scene scene, Sequence *seq)
17	+{
18	+ return id_data_find_fcurve(&scene->id, seq, &RNA_Sequence, "speed_factor", 0, NULL);
19	+}
20	+
21	/* Build frame map when speed in mode #SEQ_SPEED_MULTIPLY is animated.
22	- * This is, because `target_frame` value is integrated over time.
23	- * Returns true if multiply property is animated and `frameMap` must be used. */
24	-static bool seq_effect_speed_rebuild_map(Scene scene, Sequence seq, bool force)
25	+ * This is, because `target_frame` value is integrated over time. */
26	+static void seq_effect_speed_rebuild_map(Scene scene, Sequence seq)
27	{
28	if ((seq->seq1 == NULL) \|\| (seq->len < 1)) {
29	- return false; /* Make coverity happy and check for (CID 598) input strip... */
30	+ return; /* Make coverity happy and check for (CID 598) input strip... */
31	}
32
33	- FCurve *fcu = id_data_find_fcurve(&scene->id, seq, &RNA_Sequence, "speed_factor", 0, NULL);
34	+ FCurve *fcu = seq_effect_speed_speed_factor_curve_get(scene, seq);
35	if (fcu == NULL) {
36	- return false;
37	+ return;
38	}
39
40	SpeedControlVars v = (SpeedControlVars )seq->effectdata;
41	- if (v->frameMap != NULL && force == false) {
42	- return true;
43	- }
44	-
45	if (v->frameMap) {
46	MEM_freeN(v->frameMap);
47	}
48
49	const int effect_strip_length = seq->enddisp - seq->startdisp;
50	v->frameMap = MEM_callocN(sizeof(float) * effect_strip_length, __func__);
51	v->frameMap[0] = 0.0f;
52
53	float target_frame = 0;
54	for (int frame_index = 1; frame_index < effect_strip_length; frame_index++) {
55	target_frame += evaluate_fcurve(fcu, seq->startdisp + frame_index);
56	v->frameMap[frame_index] = target_frame;
57	}
58	+}
59	+
60	+static void seq_effect_speed_frame_map_ensure(Scene scene, Sequence seq, FCurve *fcu)
61	+{
62	+ SpeedControlVars v = (SpeedControlVars )seq->effectdata;
63	+ if (v->frameMap != NULL) {
64	+ return;
65	+ }
66
67	- return true;
68	+ seq_effect_speed_rebuild_map(scene, seq);
69	}
70
71	/* Override timeline_frame when rendering speed effect input. */
72	static float seq_speed_effect_target_frame_get(Scene *scene,
73	Sequence *seq_speed,
74	float timeline_frame,
75	int input)
76	{
77	if (seq_speed->seq1 == NULL) {
78	return 0.0f;
79	@@ -3193,21 +3201,23 @@ static float seq_speed_effect_target_frame_get(Scene *scene,
80
81	float target_frame = 0.0f;
82	switch (s->speed_control_type) {
83	case SEQ_SPEED_STRETCH:
84	const float target_content_length = seq_effect_speed_get_strip_content_length(source);
85	const float target_strip_length = source->enddisp - source->startdisp;
86	const float ratio = target_content_length / target_strip_length;
87	target_frame = frame_index * ratio;
88	break;
89	case SEQ_SPEED_MULTIPLY:
90	- if (seq_effect_speed_rebuild_map(scene, seq_speed, false)) {
91	+ FCurve *fcu = seq_effect_speed_speed_factor_curve_get(scene, seq_speed);
92	+ if (fcu != NULL) {
93	+ seq_effect_speed_frame_map_ensure(scene, seq_speed, fcu);
94	target_frame = s->frameMap[frame_index];
95	}
96	else {
97	target_frame = frame_index * s->speed_fader;
98	}
99	break;
100	case SEQ_SPEED_LENGTH:
101	target_frame = seq_effect_speed_get_strip_content_length(source) *
102	(s->speed_fader_length / 100.0f);
103	break;
104	diff --git a/source/blender/sequencer/intern/effects.h b/source/blender/sequencer/intern/effects.h
105	index ab5fa54413b..25ba4d8956e 100644
106	--- a/source/blender/sequencer/intern/effects.h
107	+++ b/source/blender/sequencer/intern/effects.h
108	@@ -32,19 +32,19 @@ struct SeqRenderData;
109	struct Sequence;
110
111	/* **********************************************************************
112	* sequencer.c
113	*
114	* Sequencer editing functions
115	* **********************************************************************
116	*/
117
118	struct SeqEffectHandle seq_effect_get_sequence_blend(struct Sequence *seq);
119	-bool seq_effect_speed_rebuild_map(struct Scene scene, struct Sequence seq, bool force);
120	+void seq_effect_speed_rebuild_map(struct Scene scene, struct Sequence seq);
121	float seq_speed_effect_target_frame_get(struct Scene *scene,
122	struct Sequence *seq,
123	float timeline_frame,
124	int input);
125
126	#ifdef __cplusplus
127	}
128	#endif
129	diff --git a/source/blender/sequencer/intern/strip_relations.c b/source/blender/sequencer/intern/strip_relations.c
130	index 7c5a3f031db..409b5f6a2e4 100644
131	--- a/source/blender/sequencer/intern/strip_relations.c
132	+++ b/source/blender/sequencer/intern/strip_relations.c
133	@@ -111,21 +111,21 @@ static void sequence_invalidate_cache(Scene *scene,
134	bool invalidate_self,
135	int invalidate_types)
136	{
137	Editing *ed = scene->ed;
138
139	if (invalidate_self) {
140	seq_cache_cleanup_sequence(scene, seq, seq, invalidate_types, false);
141	}
142
143	if (seq->effectdata && seq->type == SEQ_TYPE_SPEED) {
144	- seq_effect_speed_rebuild_map(scene, seq, true);
145	+ seq_effect_speed_rebuild_map(scene, seq);
146	}
147
148	sequence_do_invalidate_dependent(scene, seq, &ed->seqbase);
149	DEG_id_tag_update(&scene->id, ID_RECALC_SEQUENCER_STRIPS);
150	SEQ_prefetch_stop(scene);
151	}
152
153	/* Find metastrips that contain invalidated_seq and invalidate them. */
154	static bool seq_relations_find_and_invalidate_metas(Scene *scene,
155	Sequence *invalidated_seq,
156	@@ -261,21 +261,21 @@ void SEQ_relations_free_imbuf(Scene scene, ListBase seqbase, bool for_render)
157	for (seq = seqbase->first; seq; seq = seq->next) {
158	if (for_render && SEQ_time_strip_intersects_frame(seq, CFRA)) {
159	continue;
160	}
161
162	if (seq->strip) {
163	if (seq->type == SEQ_TYPE_MOVIE) {
164	SEQ_relations_sequence_free_anim(seq);
165	}
166	if (seq->type == SEQ_TYPE_SPEED) {
167	- seq_effect_speed_rebuild_map(scene, seq, true);
168	+ seq_effect_speed_rebuild_map(scene, seq);
169	}
170	}
171	if (seq->type == SEQ_TYPE_META) {
172	SEQ_relations_free_imbuf(scene, &seq->seqbase, for_render);
173	}
174	if (seq->type == SEQ_TYPE_SCENE) {
175	/* FIXME: recurse downwards,
176	* but do recurse protection somehow! */
177	}
178	}
179	@@ -318,21 +318,21 @@ static bool update_changed_seq_recurs(
180	free_imbuf = true;
181	}
182	}
183
184	if (free_imbuf) {
185	if (ibuf_change) {
186	if (seq->type == SEQ_TYPE_MOVIE) {
187	SEQ_relations_sequence_free_anim(seq);
188	}
189	else if (seq->type == SEQ_TYPE_SPEED) {
190	- seq_effect_speed_rebuild_map(scene, seq, true);
191	+ seq_effect_speed_rebuild_map(scene, seq);
192	}
193	}
194
195	if (len_change) {
196	SEQ_time_update_sequence(scene, seq);
197	}
198	}
199
200	return free_imbuf;
201	}

But not sure if it's much better. in this case id_data_find_fcurve is called twice when you render strip for first time, which is not ideal, but not the end of the world either. I could use optional argument to avoid this, but I would rather not

In D11939#311651, @Peter Fog (tintwotin) wrote:

When not using interpolation I'm getting flicker and render errors with this patch:
(Using Frame Number going from a key with the full length to zero)

This seems to be caused by seek bug rather with this patch, but you should see that also without the patch. Can you share .blend file with movie?

Clarify seq_effect_speed_rebuild_map return value

Harbormaster completed remote builds in B16107: Diff 40101.Jul 29 2021, 8:15 AM

Actually I first applied Germanos patch and did a gif(with no flicker): https://developer.blender.org/D6110#311650

Then I applied your patch on top of it, and then there was flicker. As mentioned it's just animating any value, it shouldn't be related to the source material.

Use static qualifier

Harbormaster completed remote builds in B16108: Diff 40102.Jul 29 2021, 8:25 AM

In D11939#311698, @Peter Fog (tintwotin) wrote:

Actually I first applied Germanos patch and did a gif(with no flicker): https://developer.blender.org/D6110#311650

Then I applied your patch on top of it, and then there was flicker. As mentioned it's just animating any value, it shouldn't be related to the source material.

I can't reproduce the issue, that's why I suspect seeking issues. perhaps I used wrong values?

I'm using this clip: https://www.pexels.com/video/aerial-shot-over-a-white-sand-beach-4135116/
In this .blend file:

speed4.blend603 KBDownload

Oops, seq_effect_speed_rebuild_map is not static

Harbormaster completed remote builds in B16110: Diff 40109.Jul 29 2021, 12:00 PM

Nor seq_speed_effect_target_frame_get shoul be static :/

Harbormaster completed remote builds in B16111: Diff 40110.Jul 29 2021, 12:05 PM

In D11939#311766, @Peter Fog (tintwotin) wrote:

I'm using this clip: https://www.pexels.com/video/aerial-shot-over-a-white-sand-beach-4135116/
In this .blend file:

speed4.blend603 KBDownload

Can't reproduce even with provided files... I will try to apply fcurve patch too, but perhaps this could be some weird GPU glitch?
Does this happen if you maximize preview area (press Ctrl+space)?

@Peter Fog (tintwotin) I re-checked the file and still can't reproduce bug, but I think it is related to prefetch being enabled. There is task to address this issue T90041.

In D11939#311693, @Richard Antalik (ISS) wrote:

I don't think we can avoid using id_data_find_fcurve. But I still don't want to overuse it. that's why seq_effect_speed_rebuild_map currently returns whether frameMap should be used or not.

This should be documented better though

So it could be like this

P2297 (An Untitled Masterwork)
1 diff --git a/source/blender/sequencer/intern/effects.c b/source/blender/sequencer/intern/effects.c
2 index d1d91097cd1..7589e254af4 100644
3 --- a/source/blender/sequencer/intern/effects.c
4 +++ b/source/blender/sequencer/intern/effects.c
5 @@ -3133,54 +3133,62 @@ static int early_out_speed(Sequence *UNUSED(seq), float UNUSED(facf0), float UNU
6 */
7 static int seq_effect_speed_get_strip_content_length(const Sequence *seq)
8 {
9 if ((seq->type & SEQ_TYPE_EFFECT) != 0 && SEQ_effect_get_num_inputs(seq->type) == 0) {
10 return seq->enddisp - seq->startdisp;
11 }
12
13 return seq->len;
14 }
15
16 +static FCurve *seq_effect_speed_speed_factor_curve_get(Scene *scene, Sequence *seq)
17 +{
18 + return id_data_find_fcurve(&scene->id, seq, &RNA_Sequence, "speed_factor", 0, NULL);
19 +}
20 +
21 /* Build frame map when speed in mode #SEQ_SPEED_MULTIPLY is animated.
22 - * This is, because `target_frame` value is integrated over time.
23 - * Returns true if multiply property is animated and `frameMap` must be used. */
24 -static bool seq_effect_speed_rebuild_map(Scene *scene, Sequence *seq, bool force)
25 + * This is, because `target_frame` value is integrated over time. */
26 +static void seq_effect_speed_rebuild_map(Scene *scene, Sequence *seq)
27 {
28 if ((seq->seq1 == NULL) || (seq->len < 1)) {
29 - return false; /* Make coverity happy and check for (CID 598) input strip... */
30 + return; /* Make coverity happy and check for (CID 598) input strip... */
31 }
32
33 - FCurve *fcu = id_data_find_fcurve(&scene->id, seq, &RNA_Sequence, "speed_factor", 0, NULL);
34 + FCurve *fcu = seq_effect_speed_speed_factor_curve_get(scene, seq);
35 if (fcu == NULL) {
36 - return false;
37 + return;
38 }
39
40 SpeedControlVars *v = (SpeedControlVars *)seq->effectdata;
41 - if (v->frameMap != NULL && force == false) {
42 - return true;
43 - }
44 -
45 if (v->frameMap) {
46 MEM_freeN(v->frameMap);
47 }
48
49 const int effect_strip_length = seq->enddisp - seq->startdisp;
50 v->frameMap = MEM_callocN(sizeof(float) * effect_strip_length, __func__);
51 v->frameMap[0] = 0.0f;
52
53 float target_frame = 0;
54 for (int frame_index = 1; frame_index < effect_strip_length; frame_index++) {
55 target_frame += evaluate_fcurve(fcu, seq->startdisp + frame_index);
56 v->frameMap[frame_index] = target_frame;
57 }
58 +}
59 +
60 +static void seq_effect_speed_frame_map_ensure(Scene *scene, Sequence *seq, FCurve *fcu)
61 +{
62 + SpeedControlVars *v = (SpeedControlVars *)seq->effectdata;
63 + if (v->frameMap != NULL) {
64 + return;
65 + }
66
67 - return true;
68 + seq_effect_speed_rebuild_map(scene, seq);
69 }
70
71 /* Override timeline_frame when rendering speed effect input. */
72 static float seq_speed_effect_target_frame_get(Scene *scene,
73 Sequence *seq_speed,
74 float timeline_frame,
75 int input)
76 {
77 if (seq_speed->seq1 == NULL) {
78 return 0.0f;
79 @@ -3193,21 +3201,23 @@ static float seq_speed_effect_target_frame_get(Scene *scene,
80
81 float target_frame = 0.0f;
82 switch (s->speed_control_type) {
83 case SEQ_SPEED_STRETCH:
84 const float target_content_length = seq_effect_speed_get_strip_content_length(source);
85 const float target_strip_length = source->enddisp - source->startdisp;
86 const float ratio = target_content_length / target_strip_length;
87 target_frame = frame_index * ratio;
88 break;
89 case SEQ_SPEED_MULTIPLY:
90 - if (seq_effect_speed_rebuild_map(scene, seq_speed, false)) {
91 + FCurve *fcu = seq_effect_speed_speed_factor_curve_get(scene, seq_speed);
92 + if (fcu != NULL) {
93 + seq_effect_speed_frame_map_ensure(scene, seq_speed, fcu);
94 target_frame = s->frameMap[frame_index];
95 }
96 else {
97 target_frame = frame_index * s->speed_fader;
98 }
99 break;
100 case SEQ_SPEED_LENGTH:
101 target_frame = seq_effect_speed_get_strip_content_length(source) *
102 (s->speed_fader_length / 100.0f);
103 break;
104 diff --git a/source/blender/sequencer/intern/effects.h b/source/blender/sequencer/intern/effects.h
105 index ab5fa54413b..25ba4d8956e 100644
106 --- a/source/blender/sequencer/intern/effects.h
107 +++ b/source/blender/sequencer/intern/effects.h
108 @@ -32,19 +32,19 @@ struct SeqRenderData;
109 struct Sequence;
110
111 /* **********************************************************************
112 * sequencer.c
113 *
114 * Sequencer editing functions
115 * **********************************************************************
116 */
117
118 struct SeqEffectHandle seq_effect_get_sequence_blend(struct Sequence *seq);
119 -bool seq_effect_speed_rebuild_map(struct Scene *scene, struct Sequence *seq, bool force);
120 +void seq_effect_speed_rebuild_map(struct Scene *scene, struct Sequence *seq);
121 float seq_speed_effect_target_frame_get(struct Scene *scene,
122 struct Sequence *seq,
123 float timeline_frame,
124 int input);
125
126 #ifdef __cplusplus
127 }
128 #endif
129 diff --git a/source/blender/sequencer/intern/strip_relations.c b/source/blender/sequencer/intern/strip_relations.c
130 index 7c5a3f031db..409b5f6a2e4 100644
131 --- a/source/blender/sequencer/intern/strip_relations.c
132 +++ b/source/blender/sequencer/intern/strip_relations.c
133 @@ -111,21 +111,21 @@ static void sequence_invalidate_cache(Scene *scene,
134 bool invalidate_self,
135 int invalidate_types)
136 {
137 Editing *ed = scene->ed;
138
139 if (invalidate_self) {
140 seq_cache_cleanup_sequence(scene, seq, seq, invalidate_types, false);
141 }
142
143 if (seq->effectdata && seq->type == SEQ_TYPE_SPEED) {
144 - seq_effect_speed_rebuild_map(scene, seq, true);
145 + seq_effect_speed_rebuild_map(scene, seq);
146 }
147
148 sequence_do_invalidate_dependent(scene, seq, &ed->seqbase);
149 DEG_id_tag_update(&scene->id, ID_RECALC_SEQUENCER_STRIPS);
150 SEQ_prefetch_stop(scene);
151 }
152
153 /* Find metastrips that contain invalidated_seq and invalidate them. */
154 static bool seq_relations_find_and_invalidate_metas(Scene *scene,
155 Sequence *invalidated_seq,
156 @@ -261,21 +261,21 @@ void SEQ_relations_free_imbuf(Scene *scene, ListBase *seqbase, bool for_render)
157 for (seq = seqbase->first; seq; seq = seq->next) {
158 if (for_render && SEQ_time_strip_intersects_frame(seq, CFRA)) {
159 continue;
160 }
161
162 if (seq->strip) {
163 if (seq->type == SEQ_TYPE_MOVIE) {
164 SEQ_relations_sequence_free_anim(seq);
165 }
166 if (seq->type == SEQ_TYPE_SPEED) {
167 - seq_effect_speed_rebuild_map(scene, seq, true);
168 + seq_effect_speed_rebuild_map(scene, seq);
169 }
170 }
171 if (seq->type == SEQ_TYPE_META) {
172 SEQ_relations_free_imbuf(scene, &seq->seqbase, for_render);
173 }
174 if (seq->type == SEQ_TYPE_SCENE) {
175 /* FIXME: recurse downwards,
176 * but do recurse protection somehow! */
177 }
178 }
179 @@ -318,21 +318,21 @@ static bool update_changed_seq_recurs(
180 free_imbuf = true;
181 }
182 }
183
184 if (free_imbuf) {
185 if (ibuf_change) {
186 if (seq->type == SEQ_TYPE_MOVIE) {
187 SEQ_relations_sequence_free_anim(seq);
188 }
189 else if (seq->type == SEQ_TYPE_SPEED) {
190 - seq_effect_speed_rebuild_map(scene, seq, true);
191 + seq_effect_speed_rebuild_map(scene, seq);
192 }
193 }
194
195 if (len_change) {
196 SEQ_time_update_sequence(scene, seq);
197 }
198 }
199
200 return free_imbuf;
201 }

But not sure if it's much better. in this case id_data_find_fcurve is called twice when you render strip for first time, which is not ideal, but not the end of the world either. I could use optional argument to avoid this, but I would rather not

Personally I prefer code like this, but if that way it avoids calling id_data_find_fcurve twice, then it seems to be valid not to make this change.

Overall the code looks good.
We just need to check if the problem reported by @Peter Fog (tintwotin) is really a bug.

source/blender/sequencer/intern/effects.c
3159	Here you can use `MEM_mallocN`.
3193	I have the impression that here we could just check if there is `s->frameMap`. target_frame = s->frameMap ? s->frameMap[frame_index] : frame_index * s->speed_fader; (But I'm not sure).

This revision is now accepted and ready to land.Jul 29 2021, 2:21 PM

Peter Fog (tintwotin) added projects: Video Sequencer, VFX & Video.Jul 30 2021, 9:06 AM

Use snippet in P2297
Use MEM_mallocN

Harbormaster completed remote builds in B16315: Diff 40512.Aug 10 2021, 3:33 AM

I have used code from P2297 here, because it's much cleaner I think. The fact that curve is queried twice is much less problematic IMO. Movie strips for example do have much more insane initialization procedure.

source/blender/sequencer/intern/effects.c
3193	`frameMap` is only initialized if there is `FCurve`, otherwise it's `NULL`.

The UI value doesn't update during playback anymore(unless you hover with the mouse cursor):

It "feels" slower, but I can't confirm, since the speed strips in 3.0 files aren't compatible with 2.93. Maybe it is worth to do a proper speed test between this patch and the previous implementation?

Nb. on stack exchange there is a report on Frame Interpolation having a serious memory-leak: https://blender.stackexchange.com/questions/231333/vse-memory-usage-keeps-climbing-to-50gib-before-eventual-crash

In D11939#315449, @Peter Fog (tintwotin) wrote:

The UI value doesn't update during playback anymore(unless you hover with the mouse cursor):

This happens with all animated properties.

It "feels" slower, but I can't confirm, since the speed strips in 3.0 files aren't compatible with 2.93. Maybe it is worth to do a proper speed test between this patch and the previous implementation?

Math involved during rendering haven't changed, so both cases should be directly comarable.

Nb. on stack exchange there is a report on Frame Interpolation having a serious memory-leak: https://blender.stackexchange.com/questions/231333/vse-memory-usage-keeps-climbing-to-50gib-before-eventual-crash

I know, I have this written down to investigate. It seems plausible, but I still haven't checked. Solution definitely won't be easy.

In D11939#315776, @Richard Antalik (ISS) wrote:

In D11939#315449, @Peter Fog (tintwotin) wrote:

The UI value doesn't update during playback anymore(unless you hover with the mouse cursor):

This happens with all animated properties.

If this is the case, it is crucial that all animateable values in the VSE have the option to get their f-curves drawn on the strips, or else users are working completely in the "dark" animating anything in the VSE.
Drawing and manipulation of keyframes on strips should also be considered.

Germano Cavalcante (mano-wii) accepted this revision.Aug 12 2021, 3:13 PM

Closed by commit rB929d7597b345: VSE: Cleanup speed effect math (authored by Richard Antalik (ISS)). · Explain WhyAug 24 2021, 1:12 AM

This revision was automatically updated to reflect the committed changes.

Richard Antalik (ISS) added a commit: rB929d7597b345: VSE: Cleanup speed effect math.

VSE: Cleanup speed effect math
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 41037

source/blender/blenkernel/intern/scene.c

source/blender/makesdna/DNA_sequence_types.h

source/blender/sequencer/intern/effects.h

source/blender/sequencer/intern/effects.c

source/blender/sequencer/intern/render.c

source/blender/sequencer/intern/strip_relations.c

VSE: Cleanup speed effect mathClosedPublicActions