Page MenuHome

Increase VSE strip channels limit from 32 to 128
ClosedPublic

Authored by Dalai Felinto (dfelinto) on Sep 27 2021, 12:24 PM.

Details

Summary

The original limit dates back from 2002 when Blender went open source.
After that many years some productions (e.g., Sprite Fright) are already
experiencing limitations for complex edits.

The future plans is to support an initial shorter (2?) number of
channels with support to "unlimited" channels.

Finally, I'm bumping the minimum file requirement since files with more
than 32 channels won't work well in old Blender versions.

In a future commit I will implement a sanitization so that we only read (and write)
128 channels. Making sure future changes of this number won't corrupt Blender.

Diff Detail

Repository
rB Blender

Event Timeline

Dalai Felinto (dfelinto) requested review of this revision.Sep 27 2021, 12:24 PM
Dalai Felinto (dfelinto) created this revision.
Dalai Felinto (dfelinto) planned changes to this revision.Sep 27 2021, 12:24 PM

Subversion bump - still needs the sanity check for read/write

Implement strip sanitizer

The sanitize check needs to do >, not >=

When trying to artificially lower the number of strips to test, I'm getting a strange draw artifact:

It goes away after any edit happens, so I think this can be ignored. This is a sanitize check anyways to prevent crashes.

Channel numbers become unreadable whit that amount channels T81064:


Also, a file with strips in 128 channels opened in 2.93 will make it crash.

source/blender/sequencer/intern/sequencer.c
1023–1027 ↗(On Diff #42479)

I don't quite understand this part, if you want to skip metas while iterating then you can use LISTBASE_FOREACH? It is not quite evident why changing strip type is harmless, and even why you don't check inside metas - there can be strips with large channel number. I guess metas have own writer?

Also I noticed there must be hard limit set to 32 in transform code since I can't move strip beyond that channel.

Richard Antalik (ISS) requested changes to this revision.Sep 27 2021, 2:09 PM

I wanted to check drawing glitch, but I used meta strips, and they are actually converted into color strips, which causes crash.


So if you try to open this file with MAXSEQ set to 64, it will crash.

This revision now requires changes to proceed.Sep 27 2021, 2:09 PM

Strips in 128 channels which crashes Blender: 2.93:


Also notice that the channel numbers are unreadable in the left margin.

Don't try to be too clever converting meta to colors

source/blender/sequencer/intern/sequencer.c
1016–1021 ↗(On Diff #42481)

Now thinking about this, this is not bulletproof too, because strip above threshold can have effects under itself and it can be used as mask for other strip. Best is to use SEQ_edit_flag_for_removal and SEQ_edit_remove_flagged_sequences which will handle all relationships.

Bastien Montagne (mont29) requested changes to this revision.Sep 27 2021, 2:48 PM

I don't think this is the proper way to handle this sanitizing...

  • Read code should check and immediately free invalid Sequences (in link_recurs_seq() (don't agree that keeping unfreed memory around is acceptable, unless there is absolutely no other way to do). Also, should report when this happen (using BlendDataReader data member).
  • Write code should not change existing data (ever, never). Instead, a simple assert should be fine (since read code already sanitizes things, having invalid data at that point is plain bug).

But indeed think this can be dealt with separately, in its own patch.

This revision now requires changes to proceed.Sep 27 2021, 2:48 PM
This revision was not accepted when it landed; it landed in state Needs Revision.Sep 27 2021, 2:59 PM
This revision was automatically updated to reflect the committed changes.

@Dalai Felinto (dfelinto) Currently it is only possible to view around 30 channels, so there seems to be a limitation there too:

@tintwontin Indeed. But I think this should be re-done to have a hard-limit based on pixel size, not on the number of strips. As it is I consider what we have bad already, but also unrelated to this patch.

@tintwontin Indeed. But I think this should be re-done to have a hard-limit based on pixel size, not on the number of strips. As it is I consider what we have bad already, but also unrelated to this patch.

It's in a much worse state now, than it was just recently: https://developer.blender.org/T91724