Details

Reviewers

Richard Antalik (ISS)
Bastien Montagne (mont29)

Commits

rBaaa85ad2e0f8: VSE: Implement sanity check for files with more channels than supported
rB14b8f6c25b29: VSE: Implement sanity check for files with more channels than supported
rBa64782b13341: VSE: Implement sanity check for files with more channels than supported

Summary

This is a follow up to 8fecc2a8525467ee2fbbaae16ddbbc10b3050d46.

This makes sure future .blend files that have more channels than the
limit won't break Blender.

It can be partially backported to LTS as P2442.

This is part of https://developer.blender.org/D12645

Diff Detail

Repository

rB Blender

Branch

vse-sanity-checks (branched from master)

Build Status

Buildable 17339
Build 17339: arc lint + arc unit

Event Timeline

Dalai Felinto (dfelinto) requested review of this revision.Sep 27 2021, 3:07 PM

Dalai Felinto (dfelinto) created this revision.

Harbormaster completed remote builds in B17339: Diff 42487.Sep 27 2021, 3:07 PM

Dalai Felinto (dfelinto) added projects: Video Sequencer, VFX & Video.Sep 27 2021, 3:12 PM

Bastien Montagne (mont29) added inline comments.Sep 27 2021, 3:38 PM

source/blender/blenkernel/intern/scene.c
999–1000	Can be replaced by a single call to `BLI_freelinkN`

Also, this is still missing report to user that we nuked some data on load.

This revision now requires changes to proceed.Sep 27 2021, 3:38 PM

In D12648#330686, @Bastien Montagne (mont29) wrote:

Also, this is still missing report to user that we nuked some data on load.

Hm I wonder if we can even handle multiple warnings (users will have the warning about opening a file from a newer Blender already). But I will look at that.

We can handle as many reports as we want here, but would indeed rather suggest adding a field to BlendFileReadReport.count, and then do a synthetic report in file_read_reports_finalize.

From review: use BLI_freelinkN.

Harbormaster completed remote builds in B17341: Diff 42491.Sep 27 2021, 3:44 PM

Dalai Felinto (dfelinto) planned changes to this revision.Sep 27 2021, 3:45 PM

From review: use reports

Harbormaster completed remote builds in B17361: Diff 42529.Sep 28 2021, 9:31 AM

For the records, since there is no blend file report reader in 2.93, my suggestion would be to only do P2442. So it only prevents the crash, but it doesn't inform users that data was removed.

Dalai Felinto (dfelinto) edited the summary of this revision. (Show Details)Sep 28 2021, 9:36 AM

Bastien Montagne (mont29) requested changes to this revision.Sep 28 2021, 9:42 AM

Bastien Montagne (mont29) added inline comments.

source/blender/blenkernel/intern/scene.c
998	For sake of completeness, I would check for all possible invalid values (currently should also check for `seq->machine < 1` if I am not mistaken). This check could also be made a utils function of sequencer, it's done in several other places in the code, but that should be a separate cleanup commit anyway.
source/blender/windowmanager/intern/wm_files.c
877 ↗	(On Diff #42529)	No final `.` in reports messages (just like for tooltips) currently.

This revision now requires changes to proceed.Sep 28 2021, 9:42 AM

In D12648#330995, @Dalai Felinto (dfelinto) wrote:

For the records, since there is no blend file report reader in 2.93, my suggestion would be to only do P2442. So it only prevents the crash, but it doesn't inform users that data was removed.

Agreed on not using reports in 2.93LTS yes.

Seems to be working correctly now.

From review: check for < 1 frames, and remove .

Harbormaster completed remote builds in B17363: Diff 42531.Sep 28 2021, 10:18 AM

Mark comments as done.

(for the records, the other place with the sanity check is SEQ_transform_seqbase_shuffle_ex, so yes would be nice to unify them as a separate commit)

LGTM

This revision is now accepted and ready to land.Sep 28 2021, 10:21 AM

Closed by commit rBa64782b13341: VSE: Implement sanity check for files with more channels than supported (authored by Dalai Felinto (dfelinto)). · Explain WhySep 28 2021, 10:23 AM

This revision was automatically updated to reflect the committed changes.

Dalai Felinto (dfelinto) added a commit: rBa64782b13341: VSE: Implement sanity check for files with more channels than supported.

Jeroen Bakker (jbakker) added a commit: rB14b8f6c25b29: VSE: Implement sanity check for files with more channels than supported.Oct 29 2021, 10:36 AM

Philipp Oeser (lichtwerk) added a commit: rBaaa85ad2e0f8: VSE: Implement sanity check for files with more channels than supported.Nov 2 2021, 1:52 PM