Details

Reviewers

Commits

rBb45e71e22cc7: VSE: Build proxies only for slow movies

Summary

Don't build proxy file if movie is already fast enough to seek.
To determine movie performance, check if whole GOP can be decoded
in 50 milliseconds.

This test will ensure consistent performance on wide array of machines.

Downside is at most 50ms delay (per strip) when movie is added.
Ideally another test would be performed to detect whether next GOPs
do have same size or not as it may not be safe to assume that movies
will have consistent gop sizes. Even better would be to find biggest
one within small-ish range from start, and perform this test on it.
such test may add about 10ms of additional delay, but provide even more
consistency.

Diff Detail

Repository

rB Blender

Branch

arcpatch-D11671_1 (branched from master)

Build Status

Buildable 19894
Build 19894: arc lint + arc unit

Event Timeline

Richard Antalik (ISS) requested review of this revision.Jun 22 2021, 5:17 PM

Richard Antalik (ISS) created this revision.

Harbormaster completed remote builds in B15362: Diff 38605.Jun 22 2021, 5:17 PM

When finished(if it isn't already), maybe it should just come included when adding any kind of movie file, and not have a checkbox of it's own?
If it is possible to detect vfr with the current ffmpeg solution, then automatic timecode/proxy/intermediate(whatever needed to make them work) file rendering could also be added this way?

Peter Fog (tintwotin) added projects: Video Sequencer, VFX & Video.Jun 25 2021, 10:03 AM

We can indeed be smarter on the automatic proxies creation.

What is the actual issue what this change solves: memory usage? disk usage? responsivness of the UI?

There are few things which I am not sure how they fit together:

For the scrubbing performance, do we need proxies or timecode? Or both?
Proxies can be used to speedup sequencer by minimizing number of pixels which are being processed. So basing the proxy generation on scrubbing performance seems a bit weird to me.
There is also D11336 (which I'm still not sure what is the best design there, that's why didn't reply there yet). How would this change fit together with thing proposed here?

I am also a bit concerned about is the extra option. It feels like moving in opposite direction from making VSE easier to use :(

In D11671#301819, @Sergey Sharybin (sergey) wrote:

We can indeed be smarter on the automatic proxies creation.

What is the actual issue what this change solves: memory usage? disk usage? responsivness of the UI?

It can lead to lower disk usage UI responsiveness should be unaffected, because this algorithm is tuned to ensure about same scrubbing performance as with proxies.

There are few things which I am not sure how they fit together:

For the scrubbing performance, do we need proxies or timecode? Or both?

Only proxy.
Timecode is not used at all with proxies, because it's only usable with original media (converts timeline frame to movie image frame exactly, which is not possible otherwise)

Proxies can be used to speedup sequencer by minimizing number of pixels which are being processed. So basing the proxy generation on scrubbing performance seems a bit weird to me.

That's partially true. On one hand this patch assumes 100% proxy resolution (which is default), but on other scrubbing performance depends mostly on seek+decode+scan time.
At least I should not prevent building downscaled proxies on demand here.

There is also D11336 (which I'm still not sure what is the best design there, that's why didn't reply there yet). How would this change fit together with thing proposed here?

With D11336 your point #2 would be less applicable, because scaling would be done along with pixel format conversion.

I am also a bit concerned about is the extra option. It feels like moving in opposite direction from making VSE easier to use :(

I think this could be quite safely integrated into "Automatic" proxy setup. It would be enabled by default anyway.

As far as implementation goes, I found, that there was issue that when proxy building is in process, CPU cores were occupied and this provided false negatives (as in it thinks that movie decodes slowly). Therefore this process must be done before proxy starts building within same job.

So I will update this patch as I don't think there is any other way to ensure consistent results.

@Sergey Sharybin (sergey) @Richard Antalik (ISS) A couple of problems in the current proxy-system:
One proxy-option was never discussed in public, but it's a standard in other video handling applications, which is rendering proxies to a fixed X value(and Y scaled accordingly), which will result in uniformed size and quality. Currently if you have a project at 1080p, but have 720p and 4k material, at 25% the 720p will look terrible and the 4k at 25% may not be able to play(with ex. effects applied).

Another thing is the double scaling: when selecting 25% on a file without an available proxy file, the resolution will be worse, or in other words there is down-scaling added on top of the rendered proxy files, which will make proxy files look and perform worse when played as proxy files, than the same proxy files added directly to the sequencer and played without proxy switched on. I guess in order to add properly sized text's proxy files are first up-scaled to render resolution, then texts are added and then everything is downscaled + the proxy render resolution. Instead of working in pixels for Text strips font size, they could be working in percentage of the image height, which will make them easier to handle when inserting them on the proxy file, without up and down scaling to/from render resolution.

The VSE proxy/performance enhancing UI is a mess, and needs a serious clean-up/redesign: https://devtalk.blender.org/t/a-complete-redesign-of-the-vse-proxy-system-is-needed/17491/2

There is no indication of which strips have proxies rendered at the current proxy resolution and which ones are missing.

Rewrite whole thing, but principle of operation is the same.

Updating patch because of storm, I want to make changes as some files can fool performance ckecking algorithm.

Harbormaster completed remote builds in B15832: Diff 39566.Jul 14 2021, 9:20 PM

Implement even more robust performance test. This can analyze big portion of file without introducing big delay.

Harbormaster completed remote builds in B15834: Diff 39570.Jul 14 2021, 10:41 PM

@Richard Antalik (ISS), there are no reviewers assigned, so not sure if you expect a review or there is still something you're investigating?

source/blender/imbuf/intern/indexer.c
1150	Is it guaranteed that 10k packets can be decoded fast enough? Shall there be a time-limit provided to this function?

In D11671#322300, @Sergey Sharybin (sergey) wrote:

@Richard Antalik (ISS), there are no reviewers assigned, so not sure if you expect a review or there is still something you're investigating?

Oops I thought I assigned this to you... As far as I remember this should be ready for review as in I don't plan any changes. May need to rebase.

source/blender/imbuf/intern/indexer.c
1150	These packets are not decoded, only analyzed, how many of them are key frames. Then maximum GOP size is calculated. This should be done in few milliseconds (<50 I think)

Peter Fog (tintwotin) removed a subscriber: Peter Fog (tintwotin).Sep 3 2021, 4:42 PM

To confirm: this is only done for automatic proxies (and manual proxy build will not ignore users request) ?

In D11671#363340, @Sergey Sharybin (sergey) wrote:

To confirm: this is only done for automatic proxies (and manual proxy build will not ignore users request) ?

Yes, that is correct.

I think case I think it is an interesting topic of exploration.

Some minor feedback in the code.

source/blender/imbuf/intern/indexer.c
1150	Ok. Is there some specific reason for 10k packets? Would be nice to have a comment about why this number was picked.

Richard Antalik (ISS) added inline comments.Jan 6 2022, 2:40 AM

source/blender/imbuf/intern/indexer.c
1150	I don't think there is any particular reason for this number. It should be as large as possible, but it should be done within reasonable time period. Therefore it would make sense to remove fixed number and base this on time.