Page MenuHome
Differential D7417

VSE: Add smooth frame interpolation option to speed control effect strip
ClosedPublic
Actions

Authored by Israel Medina (imedina) on Apr 14 2020, 5:05 AM.
Tags
Subscribers
Campbell Barton (campbellbarton)
Sergey Sharybin (sergey)
William Reynish (billreynish)
Video Sequencer
Tokens
"Love" token, awarded by jorsh."Love" token, awarded by ImaginationStrikes."Burninate" token, awarded by kaimueri."Love" token, awarded by sybren."Love" token, awarded by pablovazquez."Love" token, awarded by Andrea_Monzini.

Details

Reviewers
Richard Antalik (ISS)
Sergey Sharybin (sergey)
Campbell Barton (campbellbarton)
Commits
rB99cb6dbe6525: VSE: Add frame interpolation option to speed effect
Summary

When using the speed control effect strip, with this patch an option for selecting smooth frame interpolation (a.k.a frame blending) is revealed in the strip sidebar tab.
To avoid compatibility issues, the default value is no interpolation at all.

Demo:

Diff Detail

Repository
rB Blender

Event Timeline

Israel Medina (imedina) requested review of this revision.Apr 14 2020, 5:05 AM
Israel Medina (imedina) created this revision.
Israel Medina (imedina) added a reviewer: Richard Antalik (ISS).Apr 14 2020, 5:07 AM
Andrea Monzini (Andrea_Monzini) awarded a token.Apr 14 2020, 8:19 PM
Sergey Sharybin (sergey) requested changes to this revision.Apr 15 2020, 12:14 PM
Sergey Sharybin (sergey) added a subscriber: Sergey Sharybin (sergey).

Nice looking feature! :)

Main concern here is that there is fair enough of speed effect related logic in a generic place. There should be a way to have everything implemented in the speed effect implementation.

source/blender/blenkernel/intern/sequencer.c
2973

Why is this only handled in the EARLY_DO_EFFECT branch? Is it possible to pass required information to the speed effect's render function instead?

2976–2977

speed_cfra_float and speed_cfra_int is more clear.

This revision now requires changes to proceed.Apr 15 2020, 12:14 PM
Campbell Barton (campbellbarton) added a subscriber: Campbell Barton (campbellbarton).Apr 15 2020, 12:15 PM
Campbell Barton (campbellbarton) added inline comments.
source/blender/blenkernel/intern/sequencer.c
2977

Underscores at the beginning of names are normally for hidden internal vars, prefer speed_cfra_int, or (speed_cfra_fl and speed_cfra).

Richard Antalik (ISS) added a comment.EditedApr 15 2020, 12:54 PM
In D7417#178222, @Sergey Sharybin (sergey) wrote:

Nice looking feature! :)

Main concern here is that there is fair enough of speed effect related logic in a generic place. There should be a way to have everything implemented in the speed effect implementation.

Same thoughts, here. This is design issue I will have to look into. Since I would like to support time remapping for movie strips (to support arbitrary frame rates) in future as well and "WYSIWYG" style of editing based on preview This time stuff may be better decoupled from speed effect, and rather only used by it.

I have talked about this issue with @Israel Medina (imedina) that I will need a bit of time to cathch up and have some time to consider what and how to do.

Another issue is that we should set up for multithreaded execution of do_cross_effect_***.

Israel Medina (imedina) updated this revision to Diff 23770.EditedApr 15 2020, 5:27 PM
Israel Medina (imedina) marked 2 inline comments as done.

Renamed speed_cfra and _speed_cfra to speed_cfra_float and speed_cfra_int respectively.

Israel Medina (imedina) added a comment.Apr 15 2020, 5:28 PM
This comment was removed by Israel Medina (imedina).
Pablo Vazquez (pablovazquez) awarded a token.Apr 17 2020, 6:14 PM
Sybren A. Stüvel (sybren) awarded a token.Apr 17 2020, 6:14 PM
Kai Müri (kaimueri) awarded a token.Apr 17 2020, 7:43 PM
Reston Stanton (ImaginationStrikes) awarded a token.Apr 17 2020, 10:10 PM
jorge (jorsh) awarded a token.Apr 18 2020, 8:22 PM
Richard Antalik (ISS) updated this revision to Diff 23955.Apr 22 2020, 3:26 AM
  • Use mutithreaded cross effect for SEQ_SPEED_CONTROL_SMOOTH_INTERPOLATION
  • Move as much speed fx logic as possible to seqeffects.c
  • Cleanup: Use default RNA property caption, use undersores in variable names
  • Cleanup: change name to BKE_sequencer_speed_effect_target_frame_get
  • Cleanup: interpolation enum captions and values
Harbormaster completed remote builds in B7668: Diff 23955.Apr 22 2020, 3:26 AM
Campbell Barton (campbellbarton) added a comment.Apr 22 2020, 5:53 AM

Leave this one for @Richard Antalik (ISS), only left some minor notes.

source/blender/makesrna/intern/rna_sequencer.c
2711–2715

The name smooth is sometimes used to describe a curve shape, in this case: fade, crossfade or blend probably make more sense.

2747

Tooltip reads strangely, could read "Method of blending between frames at low speed"

Richard Antalik (ISS) updated this revision to Diff 23959.Apr 22 2020, 10:59 AM
  • Cleanup: Missed renaming of BKE_sequencer_speed_effect_target_frame_get
  • Fix warnings and move versioning to 2.9 file
  • Fix checking for inputs
  • Another fixes...
  • Use more descriptive captions for RNA props
Harbormaster completed remote builds in B7671: Diff 23959.Apr 22 2020, 10:59 AM
Richard Antalik (ISS) added a comment.Apr 22 2020, 12:42 PM

I have updated this patch - cross effect is now multithreaded, moved as much speed effect code as possible to seqeffects.c and did some general cleanup.

Now effect strip has only small footprint in seq_render_effect_strip_impl(), so I don't think this is outstanding issue really.
To fully remove this exception, we would have to render effect inputs by effects, which seems reasonable to me. I guess it would be good to rectify this once VSE render pipeline is moved from BKE,. Otherwise we can make private header to not make functions "public" if they don't really need to be.

source/blender/makesrna/intern/rna_sequencer.c
2743

Does this really need to be enum? I mean do you think, it is likely that another interpolation method can be implemented?

Israel Medina (imedina) added inline comments.Apr 23 2020, 2:56 AM
source/blender/makesrna/intern/rna_sequencer.c
2743

Yes, for more quality demanding cases, a pixel-warp-based motion interpolation method would be nice to have in the future.

Israel Medina (imedina) added inline comments.Apr 23 2020, 5:58 AM
source/blender/blenkernel/intern/seqeffects.c
64

Is this intentionally marked as non-static? Because in the current definition of this function, it is set as static, and it fails to compile if it's not consistent.

Campbell Barton (campbellbarton) updated this revision to Diff 23995.Apr 23 2020, 7:29 AM
  • Versioning code wasn't correct and isn't needed, remove.
  • Match names for RNA enum and internal DNA value.
  • Correct compiler warnings.
Harbormaster completed remote builds in B7685: Diff 23995.Apr 23 2020, 7:29 AM
Richard Antalik (ISS) added a subscriber: William Reynish (billreynish).Apr 23 2020, 12:06 PM

I have one more issue, otherwise this looks OK I would say.

In regards to whether use enum or checkbox, I have discussed this on chat with @William Reynish (billreynish) https://blender.chat/channel/ui?msg=952eJNAjFCuDAJRLr
Checkbox would be prefferable until there are multiple options, but I don't have very srtong opinion here, his opinion doen't sound too strong either.

release/scripts/startup/bl_ui/space_sequencer.py
1068

Move this element to bottom of settings, so it isn't placed inbetween settings related to speed

Israel Medina (imedina) added a comment.Apr 28 2020, 10:27 PM
In D7417#180305, @Richard Antalik (ISS) wrote:

I have one more issue, otherwise this looks OK I would say.

In regards to whether use enum or checkbox, I have discussed this on chat with @William Reynish (billreynish) https://blender.chat/channel/ui?msg=952eJNAjFCuDAJRLr
Checkbox would be prefferable until there are multiple options, but I don't have very srtong opinion here, his opinion doen't sound too strong either.

I agree, for now a checkbox should be enough.

Campbell Barton (campbellbarton) accepted this revision.Apr 29 2020, 10:11 AM
Sergey Sharybin (sergey) accepted this revision.Apr 29 2020, 10:12 AM

My concerns seems to be adressed. So to avoid confusing that might be seen that I expect some changes marking as expected.

Final decision, apply and such I am leaving to Richard :)

This revision is now accepted and ready to land.Apr 29 2020, 10:12 AM
Closed by commit rB99cb6dbe6525: VSE: Add frame interpolation option to speed effect (authored by Israel Medina (imedina), committed by Richard Antalik (ISS)). · Explain WhyApr 30 2020, 9:52 PM
This revision was automatically updated to reflect the committed changes.
Richard Antalik (ISS) added a commit: rB99cb6dbe6525: VSE: Add frame interpolation option to speed effect.

Revision Contents
Changeset List

PathSize
release/
scripts/
startup/
bl_ui/
1 line
source/
blender/
blenkernel/
intern/
48 lines
57 lines
blenloader/
intern/
1 line
makesdna/
8 lines
makesrna/
intern/
15 lines

Diff 23770

View Options

release/scripts/startup/bl_ui/space_sequencer.py

Loading...
View Options

source/blender/blenkernel/intern/seqeffects.c

Loading...
View Options

source/blender/blenkernel/intern/sequencer.c

Loading...
View Options

source/blender/blenloader/intern/versioning_280.c

Loading...
View Options

source/blender/makesdna/DNA_sequence_types.h

Loading...
View Options

source/blender/makesrna/intern/rna_sequencer.c

Loading...