Page MenuHome

VSE playback rate control
Needs ReviewPublic

Authored by Olly Funkster (Funkster) on Dec 11 2018, 8:06 PM.

Details

Reviewers
Brecht Van Lommel (brecht)
Group Reviewers
Video Sequencer
Summary

This patch adds a playback rate override control to sequencer strips that have visual data (movie, movie clip, scene, meta), which allows playback of the source material at a rate that is different to the rate of the scene that it is in.

It also sets the override to match that of an imported movie at the point of adding the new strip.

As a result, getting audio and video strips to match length when using source material of mixed framerates is trivial, and changing the playback rate is as easy as typing a new number in the override rate box - and the length of the strip is optionally adjusted to keep the same source material in view.

Demo here (a bit old, but basic functionality is the same) https://www.youtube.com/watch?v=Jebas1ICYdg

Diff Detail

Repository
rB Blender

Event Timeline

Olly Funkster (Funkster) created this object with edit policy "Olly Funkster (Funkster)".

I would like to see this implemented.

I have just one question:
Let's say, that I have project of 100fps and source of 10fps, and I want do do a cut on frame 5. Is that possible?
That is possibility of doing exact cuts.
Second question would be, if speed fx can work with this, but we can modify speed fx...

I am asking this, because I have patch D3496, that handles sped up sounds to behave "more correctly" (e.g. to respect set pitch vs timeline timebase). This patch is mess, because I didn't want to touch Sequence struct, so I had to do a lot of conversions.

Solution to exact cuts can be using floats for Sequence struct members, that affects timeline position. That is start, len, offsets and so on.
Such change would require changes in UI:

  • ability to place playhead inbetween 1 frame (move playhead one "sample" / time interval)
  • cut position to be represented by timelineFrameNr +SubSampleNr
  • I would say that scrubbing should also in this case respect subsampling

This would be considerably more work to be done, but it would be good IMO...

Let's say, that I have project of 100fps and source of 10fps, and I want do do a cut on frame 5. Is that possible?

In that particular case, doing the cut on frame 50 of the strip would result in a cut on frame 5 of the source, but only because they're a nice multiple of each other. If you made the cut on frame 45, you would get 5 (scene) frames of frame 4 of the source before frame 5 was presented.

Maybe in the case of playback significantly slower than scene rate, we could draw source frame markers on the movie strip so you could see where to make cuts?

Let's say, that I have project of 100fps and source of 10fps, and I want do do a cut on frame 5. Is that possible?

In that particular case, doing the cut on frame 50 of the strip would result in a cut on frame 5 of the source, but only because they're a nice multiple of each other. If you made the cut on frame 45, you would get 5 (scene) frames of frame 4 of the source before frame 5 was presented.

Sorry I was thinking of reverse scenario - 10fps project and 100 fps source, and cut on frame 5 of source. That would be cut at frame 0.5
Or with my all float proposal that would be cut at 0 +5

If I understand this correctly, this means that this patch will change cut operator behavior, so user will need to calculate cut position manually. I would like to keep cut operator WYSIWIG as it is now. And consistent.

Maybe in the case of playback significantly slower than scene rate, we could draw source frame markers on the movie strip so you could see where to make cuts?

May be also a way. It would be good idea to start discussion about this issue.
Or just do it, but make it so user doesn't notice :)

With sound patch that I mentioned earlier there was another magic, that user could animate pitch (FPS) so drawing that is quite a challenge.

Aren't there also formats, where you can mix framerates within one file?
We don't have to support obscure file formats however.

Ah, yes that would be impossible with this patch as-is. If all the start / end offsets etc. were made into floats then it would behave as you want with 0.5 in it.

Aren't there also formats, where you can mix framerates within one file?

I think so yes, and also formats where the presentation time of each frame can be specified individually. I think that's beyond the scope of the VSE though - blender itself, and any decent video camera, has constant framerate for a given clip.

If I understand this correctly, this means that this patch will change cut operator behavior, so user will need to calculate cut position manually.

In your example, we could over-ride the "slip" operator (or whatever it's called, press s) to allow slipping of source frames rather than scene frames, and if you are zoomed in far enough we could show source frames on the strip in between scene frame markers so that the user could slip the source until frame 5 is at the beginning of the strip.

Anyway, I think we can merge this, and deal with partial frames implementation later. Unless @Olly Funkster (Funkster) would like to do that :)
There is subframe in RenderData struct - maybe use that?

typedef struct RenderData {

	...
	int cfra, sfra, efra;	/* frames as in 'images' */
	float subframe;			/* subframe offset from cfra, in 0.0-1.0 */
	int psfra, pefra;		/* start+end frames of preview range */
	...

In this context, if any changes are to be made, I would also like to create more modular code. In this case create module, that handles strip manipulation, offsets and stuff like that.
so cut operator can look like

cut(){
	move_right_handle(seq1, -50, HARD);
	move_left_handle(seq2, 1050, HARD);
}

I think, that this has to be done at some point. It may even accelerate development further.

I think partial frames can wait, I would expect 99% of users of rate control wouldn't need such exact timing control. If someone is happy to merge it, fire away!

Sergey Sharybin (sergey) changed the visibility from "All Users" to "Public (No Login Required)".Dec 14 2018, 4:19 PM
Brecht Van Lommel (brecht) requested changes to this revision.Dec 14 2018, 4:27 PM

@Richard Antalik (ISS), this needs a much closer review and testing before being merged. This is complicated stuff, but there are broken things here even from a quick look.

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

playbackrate -> playback_rate

890–895

Don't add a bunch of commented out code like this. Either remove it or leave a comment about why it should be there.

903

Same comment.

source/blender/blenkernel/BKE_sequencer.h
392

OVER -> OVERRIDE, no need for ambiguous abbreviation.

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

Group all IMB_ #includes together, not randomly between other includes.

Don't include intern/ files, they are supposed to be internal to the module. Either the header should be moved, or more likely some function should be added to the public API to access stuff.

3067

Don't create a RenderData, just define the floats as in BKE_sequence_get_fps for example.

3495

This feature is in the UI but not implemented? Then it should not be in this patch.

5438

Same comment as above.

5439

Above the return value of this function is checked, here it isn't and can lead to uninitialized variables.

5458

Comment should explain why it's useful.

5460

We should understand what happens before commiting hacks.

source/blender/editors/space_sequencer/sequencer_add.c
71

Same comment as above.

424

clim -> clip

426

Same comment as above, not checking return value.

Since this is repeated multiple times, maybe a utility function is needed to deduplicate code.

775

Things like this TODO comment need to be solved before committing.

777

You should not leave the label string empty.

If the operator needs a custom UI layout, there is an operator method for that.

source/blender/makesrna/intern/rna_sequencer.c
132–133

You can't use static variables to pass data from set to update functions, it does not work in general. There needs to be another solution.

Is it even needed to have a separate update function, or can it all be in set?

139–140

Follow code style if () {

163

Move the logic in this code to the core sequencer code, and try to get the playback rate stuff wrapped into a few functions.

The RNA code should mostly be a thin wrapper over code elsewhere.

1728

Comments should not be from a personal point like this, here can just be left out.

This revision now requires changes to proceed.Dec 14 2018, 4:27 PM

Yes, this is messy situation. similar to D3496.
In that patch I said, that I do not recommend committing.

I think, that I will require some underlying framework for these 2 patches to be applied.
Then I will change D3496 as a "template" of how this should look.

My earlier comments apply more to functionality then code itself.

And thanks for sample reviews :)

Addressed all review comments from @Brecht Van Lommel (brecht) apart from the one about custom UI layout, for which I would appreciate guidance.

Olly Funkster (Funkster) marked 18 inline comments as done.Dec 15 2018, 4:18 PM
Olly Funkster (Funkster) added inline comments.
source/blender/blenkernel/intern/sequencer.c
5460

I have removed the hack since I hadn't meant to submit it in the first place. I will investigate the preseek behaviour separately.

source/blender/editors/space_sequencer/sequencer_add.c
777

I could do with some pointers on this one... the UI code doesn't really make much sense to me (I'm an embedded engineer most of the time). Any chance you could point me at something that does a custom UI layout that I can use as an example? Thanks!

Olly Funkster (Funkster) marked an inline comment as done.Dec 15 2018, 4:20 PM
Brecht Van Lommel (brecht) requested changes to this revision.Dec 16 2018, 3:46 AM
Brecht Van Lommel (brecht) added inline comments.
source/blender/blenkernel/intern/sequencer.c
5469

Simpler would be fabsf(error_factor - 1.0f) < 0.0001f. Using #defines here doesn't make it much more clear.

source/blender/editors/space_sequencer/sequencer_add.c
777

See ot->ui = sequencer_add_draw for an example of an operator doing its own UI layout.

source/blender/makesdna/DNA_sequence_types.h
126–128

add SEQ_ prefix.

131–133

Use enum.

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

Still using intern/ include here.

This revision now requires changes to proceed.Dec 16 2018, 3:46 AM
Olly Funkster (Funkster) marked an inline comment as done.Dec 16 2018, 9:08 PM
Olly Funkster (Funkster) added inline comments.
source/blender/blenkernel/intern/sequencer.c
5469

fabsf makes perfect sense. Can I still use a define in the place of the 0.0001f? I'm allergic to magic numbers.

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

A #define elsewhere mostly makes sense if you plan to use it in multiple places, or if it's together with other values that you might want to tweak together. Since this is only intended to be used in one place, it seems better to just define it locally like:

const float error_threshold = 0.0001f;

Added SEQ_ prefix to the enums that lacked it; made some defines into enums; changed override rate threshold to use fabsf(); set add movie strip left-hand UI to use vertical alignment so that there is room for the label on the option box for mismatch behaviour.

Olly Funkster (Funkster) marked 5 inline comments as done.Dec 17 2018, 4:42 PM

Latest adjustments to suit. If this approach on the UI isn't acceptable I'll try something more complicated, but I thought if the simpler, less-code option is okay I would go with it.

source/blender/editors/space_sequencer/sequencer_add.c
777

Current patch sets everything to vertical arrangement, is this okay? There is now room for the label on the drop-down option, but the start frame and channel options now take up more room.

source/blender/editors/space_sequencer/sequencer_add.c
777

This patch would be applied in 2.8, where this layout is quite different. So it needs to be updated for that. In 2.8 the panel is wider by default, though some of the names are still quite long.

Perhaps they can be shortened still? Some ideas:

  • "Framerate Mismatch Action" -> "On Framerate Mismatch".
  • "Playback at Scene rate" -> "Use Scene Rate"
  • "Override Playback Rate" -> "Repeart or Drop Frames"
  • "Set Scene Framerate to match Movie" -> "Change Scene Rate"

If you want to test it against a (unofficial) 2.80 VSE panel, you can get it here: https://devtalk.blender.org/t/can-we-in-collaboration-produce-a-working-suggestion-for-improving-the-user-interface-of-the-vse/3264/77

Oh, sorry I didn't notice it is the import options(so the link above won't make a difference. In 2.80 it seems like all panels are right aligned - however only the image input settings are right aligned, movie and sound import options are left aligned. Maybe they all should be right aligned?

Here is timeline API proposal: T59540
It may be quite a lot of work, but not difficult.

I like to write a lot, and couldn't sleep...

I may add:
When testing, srtips like to behave different when:

  • strip is uncut
  • left side of cut strip
  • right side of cut strip
  • middle side of cut strip
  • is soft cut
  • is hard cut
  • has still frames on the end
  • has still frames at start
  • has still frames on both sides
  • ...
  • any combination of these

Don't know if you have tried such tests, just saying, that it may be better to invest time somewhere else at this point.

I will get to this eventually, it will take time. Don't know how long, we are missing so many essential things.

Updated to build against 2.8 / current master.

I'll attempt to address the issues found in testing soon.