Details

Reviewers

Sergey Sharybin (sergey)
Sybren A. Stüvel (sybren)

Summary

Calculate nearest power of two render size based on image size in
preview area when "Proxy Render Size" is set to "Automatic".

Downscaling is implemented for movies on IO level - see IMB_Downscale
and IMB_anim_absolute(). Movies are also downscaled by factor which
is power of two. Currently up to 32x - IMB_DOWNSCALE_32X

This means that with smaller preview sizes optimal image sizes are used
resulting in lower memory usage and higher performance.

Demo 8K video preview performance + debug info how it works internally:

Diff Detail

Repository

rB Blender

Branch

arcpatch-D9414_1 (branched from master)

Build Status

Buildable 12525
Build 12525: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

In D9414#238162, @Sergey Sharybin (sergey) wrote:

Wouldn't it be better if we scaled to exact size we need? Then we can skip one scaling step alltogether. Even if we had to use different approach for images.

The exact size you wouldn't really know in advance, as it will depend on settings and other factors. You don't want to re-implement that on the IO level.

Well if you can scale to arbitrary size, in this patricular case you know exact size.
But I can see why this is not what you meant in design description, I am not going to argue this here, I can present you performance of each implementation and then we can decide.

For testing I used 1920 x 1080 render size and I used proxy size to quickly switch between sizes. Technically it is the same thing as if you resized actual preview.

Not sure this is really fair comparison, as proxies don't perform runtime scaling of data on CPU?

It is fair comparison - it downscales images if proxy doesn't exist. This is because sequencer requires all images to have same size.
So in described setup, 8K source is downscaled by 4x for 100% size, 8x for 50% size and 16x for 25% size.

Design was not clear to me and I didn't really understand how it would fit into cache design.

There was no finished/signed-off design yet. So we do need to take a step a back on this topic and finalize the design.

It all starts from the idea that processing FullHD media for a typical setup of video editor is too much. If you drop resolution to its half, you drop memory and processing by 4 times. So lets try to exploit it!

If you can pick up and wrap up the design -- perfect. Otherwise i'll make sure it is finalized in the coming day or so.

Got it, There is also D9481 that is waiting for downscaling to be implemented so it doesn't store big images in cache. So I will change approach here and start working on using preview size to get optimal render size in rendering context

Change prescaling API to use pre-defined scale factors.
Set render resolution based on preview size and zoom - SEQ_RENDER_SIZE_AUTOMATIC setting

TODO:

Investigate why zoom is calculated incorrectly when preview is not set to auto scale to fit
Invalidate cache when zooming in/out, currently image is updated only during playback
Clarify design/approach what should happen when workspace has more previews.

Harbormaster completed remote builds in B11470: Diff 31468.Dec 1 2020, 5:18 AM

Fix method of caclulating zoom
Add last_downscale_index to anim struct so we get image with correct size when requesting image with different scale. No changes to sequencer cache were necessary.

Harbormaster completed remote builds in B11494: Diff 31503.Dec 2 2020, 1:44 AM

Cleanup

Harbormaster completed remote builds in B11495: Diff 31504.Dec 2 2020, 3:31 AM

Richard Antalik (ISS) retitled this revision from Scale movies to custom resolution with sws_scale to VSE: Render in size nearest to preview image.Dec 2 2020, 4:10 AM

Richard Antalik (ISS) edited the summary of this revision. (Show Details)

Richard Antalik (ISS) added a reviewer: Sergey Sharybin (sergey).

Bit more cleanup

Harbormaster completed remote builds in B11496: Diff 31505.Dec 2 2020, 4:10 AM

@Sergey Sharybin (sergey) This patch should be good for review, here are some points I could have done differently, I would like to hear your feedback.

There can be new function like IMB_anim_absolute that will accept downscale argument, so we don't have to modify existing API
Values of IMB_Downscale are a bit counter intuitive. This is only beneficial so we can alloc/free sws_scale contexts in loop and access img_convert_ctx[downscale_index]. This could be reworked as well.
When fetching same frame in different scale, we run only ffmpeg_postprocess() which is quite fast, probably faster than fetching largest image from sequencer cache and downscaling. So performance with more than 1 preview is good. Worst case is when there is 1X downscale and 2X downscale previews (2 largest images) - about 12% drop in performance. Possible optimization would be to check for all preview areas, and use largest one to determine render size for all areas.
This implementation ignores proxies which are often used for movies. Should we try to use them?

Fix warnings

Harbormaster completed remote builds in B11497: Diff 31506.Dec 2 2020, 5:11 AM

Peter Fog (tintwotin) awarded a token.Dec 2 2020, 5:59 AM

The best moving forward strategy here would be something like:

Ask @Francesco Siddi (fsiddi) to have some ground-truth file which will allow to verify that Milestone 1 is fully done.
Test the master branch against, test this patch, see how it helps.

Still think it could be made to work for image sequences. And probably API can be streamlined a bit, but that can happen after initial pass is finalized.

Store Raw images except proxies by default

Harbormaster completed remote builds in B11614: Diff 31731.Dec 8 2020, 1:28 PM

@Dalai Felinto (dfelinto) asked me to take over the review from @Sergey Sharybin (sergey). I've left some inline notes where I think improvements could be made.
It would indeed be nice if someone (@Francesco Siddi (fsiddi)?) could provide some test material.

source/blender/editors/space_sequencer/sequencer_draw.c
1327–1330	I think it would be nice to put this code into its own function. That way it can have two return parameters (`r_rectx` and `r_recty`) and return `downscale_factor`. The declaration of `float downscale_factor` here can then also become `const`, or maybe directly assigned to `context.downscale_factor`.
source/blender/imbuf/intern/anim_movie.c
1191	Flip the condition, then you can avoid indenting the block, and avoid an else-after-return. if (anim->last_downscale_index != downscale_index) { return ffmpeg_fill_ibuf(anim, downscale_index); } ... // rest of the code
source/blender/makesrna/intern/rna_space.c
5093	It might be nice to add a description for the tooltip that explains that "Automatic" will pick the size based on the viewport size.
source/blender/sequencer/intern/effects.c
3935	I think `preview_scale_factor` can be `const`.
source/blender/sequencer/intern/render.c
275	Avoid `else` after `return`. Probably a `switch` with two `case`s and a `default` is clearer here.
783–787	Why the change in logic here? In the previous code `BKE_sequencer_cache_put()` was only called if `use_preprocess == false`, but now it only depends on `is_proxy_image`.
1166	`index` can be `const`

This revision now requires changes to proceed.Dec 15 2020, 5:56 PM

Address inlines

Harbormaster completed remote builds in B11761: Diff 32029.Dec 15 2020, 8:02 PM

@Sybren A. Stüvel (sybren) Thanks for taking over.

I can provide testing material that I used in video. This was mostly based on design task T80278

time8K.zip33 MBDownload

I think that @Francesco Siddi (fsiddi) had issues mainly with raw images not being in cache, so I have cheated a bit and included that change in this patch, but on the other hand I want this patch to allow caching raw images.
As I said in inline this still requires to enable caching raw images in cache settings manually though.

source/blender/sequencer/intern/render.c
783–787	We want to put raw images in cache as part of design change. These images can be very large though and use a lot of RAM. So I did not want to make this change before this patch is committed. This should be separate commit really. It is here only so it can be tested easily. However It requires manual tweak that would be done by D9745, so I am not sure if this helps with testing really.

Andrea Monzini (Andrea_Monzini) awarded a token.Dec 16 2020, 10:49 PM

Richard Antalik (ISS) mentioned this in D9879: [WIP] This is merged patch for testing..Dec 17 2020, 1:50 AM

Fix downscale used if preview size is not set to "Automatic"

Harbormaster completed remote builds in B11790: Diff 32091.Dec 17 2020, 3:15 AM

rB571362642201 changed how we did scaling, so there are some issues currently, I will have to resolve this and test later today.

I will have to store downscale index in cache key, so when it is retrieved we have information about image original size. We could use IMB_anim_get_image_height() but these are not available for images, so I am better off making this change in cache right away.
Also I need to drag used downscale_index through pipeline same way as is_proxy_image. This is not very nice. I have a plan how to deal with that, but unfortunately preprocessing is possible even for strips it was not really meant for, so this will have to be rather elaborate change I can't do here.

In D9414#246426, @Richard Antalik (ISS) wrote:

I will have to store downscale index in cache key, so when it is retrieved we have information about image original size.

Would it make sense to bundle those into some struct, then?

Set used downscale_index - initialized in seq_render_strip()
Store downscale_index in SeqCacheItem and disk cache header. index is stored in cache item because original key can't be retreived with BLI_ghash_lookup()

Harbormaster completed remote builds in B11802: Diff 32112.Dec 17 2020, 6:13 PM

In D9414#246449, @Sybren A. Stüvel (sybren) wrote:

In D9414#246426, @Richard Antalik (ISS) wrote:

I will have to store downscale index in cache key, so when it is retrieved we have information about image original size.

Would it make sense to bundle those into some struct, then?

Not quite sure what do you mean. Bundle into struct as that we pass as argument to BKE_sequencer_cache_put()? Otherwise this value is bundled in SeqCacheItem

I would rather cleanup BKE_sequencer_cache_put() arguments a bit. I want to remove cost as it is useless, timeline_frame can be part of context really, skip_disk_cache should be handled by cache internally if possible. BKE_sequencer_cache_put() could also be specialized by type to avoid passing it explicitly and pass downscale_index only when needed - for RAW images. I can try to make these changes in master and rebase this patch on them.

I could also try to linearize preprocessing it is already done partially. Personally I would rather do it after this patch, because I need to investigate how effects hijack this path and how to make it linear for them. And whether is should remain as pre-processing or it should be post-processing with same features.

In any case now this patch works as it should, I just have to update versioning to wipe existing disk cache.

Fix one too many arguments

Harbormaster completed remote builds in B11803: Diff 32113.Dec 17 2020, 6:36 PM

Fix null ptr access

Harbormaster completed remote builds in B11804: Diff 32114.Dec 17 2020, 6:40 PM

@Richard Antalik (ISS) Testing this for the first time(Diff 32112 & Diff 32114), and it seems to be scaling unrelated to "canvas size". Is this intended behaviour(it doesn't seem so when comparing with the video on top)?
(Proxy resolutions also seems to be scaled by changing the proportions in unrelated to "canvas", I guess it shouldn't?)

In D9414#246625, @Peter Fog (tintwotin) wrote:

@Richard Antalik (ISS) Testing this for the first time(Diff 32112 & Diff 32114), and it seems to be scaling unrelated to "canvas size". Is this intended behaviour(it doesn't seem so when comparing with the video on top)?
(Proxy resolutions also seems to be scaled by changing the proportions in unrelated to "canvas", I guess it shouldn't?)

Checking this, I think I can reproduce something similar on existing project with proxies, but not quite your case. Can you share .blend file?

Fix hack I used for testing

Harbormaster completed remote builds in B11808: Diff 32121.Dec 17 2020, 8:16 PM

Peter Fog (tintwotin) added projects: Video Sequencer, VFX & Video.Dec 19 2020, 12:37 AM

Merge branch 'master' into arcpatch-D9414
Merge branch 'master' into arcpatch-D9414

Harbormaster completed remote builds in B11832: Diff 32191.Dec 20 2020, 4:17 AM

The bug I reported above has been fixed now.

Richard Antalik (ISS) mentioned this in T80278: VSE 2.0: Performance, Cache System.Jan 11 2021, 10:00 AM

Pablo Vazquez (pablovazquez) awarded a token.Jan 11 2021, 1:06 PM

Liam Scaife (Voltist) added a subscriber: Liam Scaife (Voltist).Jan 21 2021, 5:24 AM

Rebase on master, fix some issues, re-tested functionality. Found issue that changing render size while zooming should reset prefetch. Will go over code again to make sure all is ready for review

Harbormaster completed remote builds in B12525: Diff 33401.Feb 1 2021, 6:53 PM

Fix prefetch using incorrect downscale

Harbormaster completed remote builds in B12542: Diff 33427.Feb 2 2021, 2:15 PM

Fix last fix - don't check for original depsgraph, because prefetch uses own

Harbormaster completed remote builds in B12544: Diff 33430.Feb 2 2021, 2:33 PM

Here is patch adding downscale for images P1944

Problem is, that before rendering image strip, it's size is unknown. With movies we keep handle available during runtime, where checking width and height is very cheap. With images and mainly image sequences this is problematic.

Possible solutions:

If image sequence frames are assumed to be uniform in size, size information can be held in first StripElem struct. Regardless of where this value will be stored, it needs to be managed, which is not great.
If image sequence frames are allowed to differ in size, it is still possible to use downscaled image, but scaling needs to be done in sequencer. So after rendering the image, if original image actual resolution doesn't match expected resolution, sequencer can use original image instead. This is most reliable method, but it means that downscaling will be handled outside of ImBuf API.
- Perhaps sequencer can provide expected size to IMB_loadiffname_downscale() as argument and function can fallback on original image. If IMB_Downscale index was part of ImBuf sequencer would know whether image is original or not.

Solution 2.1 would also simplify sequencer code, as IMB_Downscale wouldn't have to be passed as argument to multitude of rendering and cache functions.

With images and mainly image sequences this is problematic.

Why? And is this a problem of the patch overall? Or something that could be handled in a followup patch? How does P1944 relate to this patch? Are you suggesting it should become part of this patch, or is it indeed intended as a follwup?

Rebase

Harbormaster completed remote builds in B14679: Diff 37336.May 21 2021, 3:42 AM

Richard Antalik (ISS) mentioned this in D11336: [WIP]VSE: Always use automatic rendersize.May 21 2021, 7:04 AM

Mal Duffin (mal_cando) awarded a token.May 21 2021, 10:53 AM

Gavriel Lavi (Gavriel5578) awarded a token.May 26 2021, 11:22 PM

Maybe this is a side-effect of the new transform settings, but with a 1920x1080 render resolution and switching to Automatic on a 4k movie strip, the strip doesn't scale to fit anymore:

Btw. maybe a more descriptive name could be found instead af "Automatic"?

In D9414#291776, @Peter Fog (tintwotin) wrote:

Maybe this is a side-effect of the new transform settings, but with a 1920x1080 render resolution and switching to Automatic on a 4k movie strip, the strip doesn't scale to fit anymore:

There probably will be some bugs still, I just updated patch so it builds. Need to evaluate some fundamental design issues here and in further direction as other changes may be necessary.

VSE: Render in size nearest to preview image
Needs ReviewPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 33401

source/blender/blenkernel/intern/image.c

source/blender/blenkernel/intern/movieclip.c

source/blender/editors/space_sequencer/sequencer_draw.c

source/blender/editors/space_sequencer/space_sequencer.c

source/blender/imbuf/IMB_imbuf.h

source/blender/imbuf/intern/IMB_anim.h

source/blender/imbuf/intern/anim_movie.c

source/blender/imbuf/intern/indexer.c

source/blender/imbuf/intern/thumbs.c

source/blender/makesdna/DNA_space_types.h

source/blender/makesrna/intern/rna_space.c

source/blender/sequencer/SEQ_render.h

source/blender/sequencer/intern/effects.c

source/blender/sequencer/intern/image_cache.h

source/blender/sequencer/intern/image_cache.c

source/blender/sequencer/intern/prefetch.c

source/blender/sequencer/intern/proxy.c

source/blender/sequencer/intern/render.h

source/blender/sequencer/intern/render.c

source/blender/windowmanager/intern/wm_playanim.c

VSE: Render in size nearest to preview imageNeeds ReviewPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 33401

source/blender/blenkernel/intern/image.c

source/blender/blenkernel/intern/movieclip.c

source/blender/editors/space_sequencer/sequencer_draw.c

source/blender/editors/space_sequencer/space_sequencer.c

source/blender/imbuf/IMB_imbuf.h

source/blender/imbuf/intern/IMB_anim.h

source/blender/imbuf/intern/anim_movie.c

source/blender/imbuf/intern/indexer.c

source/blender/imbuf/intern/thumbs.c

source/blender/makesdna/DNA_space_types.h

source/blender/makesrna/intern/rna_space.c

source/blender/sequencer/SEQ_render.h

source/blender/sequencer/intern/effects.c

source/blender/sequencer/intern/image_cache.h

source/blender/sequencer/intern/image_cache.c

source/blender/sequencer/intern/prefetch.c

source/blender/sequencer/intern/proxy.c

source/blender/sequencer/intern/render.h

source/blender/sequencer/intern/render.c

source/blender/windowmanager/intern/wm_playanim.c

VSE: Render in size nearest to preview image
Needs ReviewPublic
Actions

Revision Contents
Changeset List