Page MenuHome

Fix-T70415 Fix 100% proxy files playing with poor performance as proxy
AcceptedPublic

Authored by Eitan (EitanSomething) on Dec 5 2019, 2:37 PM.

Details

Summary

Refactor of the preview_render_size enum system.

The old system was using hack that caused poor performance when using 100% proxy

There is a perforce improvement of 66% when using 100% proxy with no resolution mismatch

Diff Detail

Repository
rB Blender
Branch
arcpatch-D6368 (branched from master)
Build Status
Buildable 6045
Build 6045: arc lint + arc unit

Event Timeline

  • When changing 'Proxy Render Size' and no proxy files have been encoded, the image quality is changed, but the play back rate stays the same. If it has no impact on the play back rate, then maybe it is not needed?
  • When there are encoded proxy files, and the 'Proxy Render Size' is set to 'No proxy, full render' or 'Scene render size' it is actually using the 25% proxy file - and not the source file. This means that it is currently not possible to compare the play rates of the proxy files played as proxies and as clips played directly in the time line(No proxy, full render). (When in 'No proxy, full render', use Strip > Movie Strip > Set Render Resolution to see the current resolution matches the 25% proxy files)
Eitan (EitanSomething) updated this revision to Diff 20114.EditedDec 5 2019, 9:57 PM
  • Fixed When there are encoded proxy files, and the 'Proxy Render Size' is set to 'No proxy, full render' or 'Scene render size' it is actually using the 25% proxy file - and not the source file. This means that it is currently not possible to compare the play rates of the proxy files played as proxies and as clips played directly in the time line(No proxy, full render). (When in 'No proxy, full render', use Strip > Movie Strip > Set Render Resolution to see the current resolution matches the 25% proxy files)
  • Fixed When changing 'Proxy Render Size' and no proxy files have been encoded, the image quality is changed, but the play back rate stays the same.
Brecht Van Lommel (brecht) added inline comments.
source/blender/makesdna/DNA_space_types.h
630–636 ↗(On Diff #20114)

Changing enum values in DNA breaks backwards compatibility.

source/blender/makesdna/DNA_space_types.h
630–636 ↗(On Diff #20114)

Can this be fixed with versioning code

source/blender/makesdna/DNA_space_types.h
630–636 ↗(On Diff #20114)

It can be, at the cost of some extra code complexity.

But why do the values need to change? You already seem to be changing code to use the proper SEQ_PROXY_RENDER_SIZE_* rather than assuming specific values, so then it would not matter what the actual value is.

  • revert eSpaceSeq_Proxy_RenderSize to original numbers
  • Merge branch 'master' of git://git.blender.org/blender into Fix-T70415

Execelent work here. This patch is a solid performance improvement.

In official 2.81 a 1080p clip with a 100% proxy files plays at 18 fps.
With this patch it plays around 40 fps.

At 75% 2.81 plays at 30 fps.
Patch at 50 fps.

Eitan (EitanSomething) edited the summary of this revision. (Show Details)Dec 6 2019, 6:50 PM
Eitan (EitanSomething) edited the summary of this revision. (Show Details)Dec 6 2019, 7:18 PM
  • revert if (!ibuf || proxy_size == IMB_PROXY_NONE) {

Personally I wanted to change values in eSpaceSeq_Proxy_RenderSize to avoid any confusion in future and discourage use of values directly. But it is not that big deal lo leave it as is.

Kind of unrelated, but please change titles and description of your patches so devs can immediately know what this is supposed to do and also so it can be used as commit message(ideally)
https://wiki.blender.org/wiki/Style_Guide/Commit_Messages

I will have to return to this patch once more to review in greater detail, but overall this seems fine.

source/blender/blenkernel/BKE_sequencer.h
211

functions in this header file need to have BKE_sequencer_ prefix

source/blender/editors/space_sequencer/sequencer_draw.c
1025–1026

seq_proxysize_to_scale_factor() has type of double, so conversion here seems to be redundant

Eitan (EitanSomething) updated this revision to Diff 20164.EditedDec 9 2019, 3:53 PM
  • remove unnecessary checks

Removed them because the only time seq_render_movie_strip is called is when It couldn't find the proxy file therefore it will be IMB_PROXY_NONE

Eitan (EitanSomething) planned changes to this revision.Dec 9 2019, 10:43 PM

if (!ibuf && proxy_size != IMB_PROXY_NONE) { is needed but changes will be made to fix this.

  • Merge branch 'master' of git://git.blender.org/blender into Fix-T70415
Eitan (EitanSomething) retitled this revision from Fix-T70415 to Fix-T70415 Fox proxy files playing with poor performance in the timeline.Dec 10 2019, 7:40 PM
Eitan (EitanSomething) retitled this revision from Fix-T70415 Fox proxy files playing with poor performance in the timeline to Fix-T70415 Fix proxy files playing with poor performance in the timeline.
  • Merge branch 'master' of git://git.blender.org/blender into Fix-T70415
Eitan (EitanSomething) edited the summary of this revision. (Show Details)Dec 12 2019, 5:47 PM
Eitan (EitanSomething) retitled this revision from Fix-T70415 Fix proxy files playing with poor performance in the timeline to Fix-T70415 Fix 100% proxy files playing with poor performance in the timeline.Dec 12 2019, 6:03 PM
Eitan (EitanSomething) retitled this revision from Fix-T70415 Fix 100% proxy files playing with poor performance in the timeline to Fix-T70415 Fix 100% proxy files playing with poor performance as proxy .Dec 12 2019, 6:06 PM
Richard Antalik (ISS) requested changes to this revision.Dec 16 2019, 2:16 AM

Also please look at unnecessary whitespace changes and apply clang-format.

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

seq_proxy_fetch() had code, that can use proxies from custom dir/file. You need either to move that code to seq_render_movie_strip() or don't change it in this patch.

3877–3878

(psize != IMB_PROXY_NONE); would be nicer

source/blender/editors/space_sequencer/sequencer_draw.c
1026

This variable could be removed entirely, if you change SeqRenderData item int preview_render_size to eSpaceSeq_Proxy_RenderSize proxy_size and you can set this value to sseq->render_size directly. Or IMB_Proxy_Size equivalent of that.
This may simplify also some code downstream.

1028–1029

You could compare sseq->render_size to eSpaceSeq_Proxy_RenderSize items

1040–1042

These + 0.5f are used to "round" floats to nearest integer rather than nearest lower integer.
Any reason why you removed this other than it looks ugly?

This revision now requires changes to proceed.Dec 16 2019, 2:16 AM
Eitan (EitanSomething) marked an inline comment as done.
Eitan (EitanSomething) retitled this revision from Fix-T70415 Fix 100% proxy files playing with poor performance as proxy to Fix-T70415 Fix 100% proxy files playing with poor performance as proxy.
Eitan (EitanSomething) edited the summary of this revision. (Show Details)
  • cleanup code
Eitan (EitanSomething) marked 6 inline comments as done.Dec 16 2019, 6:05 PM
  • Revert BKE_sequencer_proxysize_to_scale_factor to double, fix some issues, cleanup
  • use context to get proxy size
  • fix unused var
  • apply clang-format
  • cleanup diff

Decided to chip in some code.

  • Revert BKE_sequencer_proxysize_to_scale_factor to double, fix some issues, cleanup
  • use context to get proxy size
  • fix unused var
  • apply clang-format
  • cleanup diff

Decided to chip in some code.

I don’t know why clang format wasn’t working. Visual studios usually shows a popup whenever I re open the project.

Richard Antalik (ISS) requested changes to this revision.Dec 18 2019, 2:50 AM

I definitely don't like what I have done to seq_proxy_get_fname(). It definitely deserves explicit function to get filename. Movieclip uses rendersize_to_number() for this. Perhaps both editors could use IMB_Proxy_Size to number function and move it to IMB_imbuf.h.
As this patch stands right now it won't work for image strip proxies.

Now I remember, that I wanted to define proper types for proxysize conversion functions, but it seems to be not possible if function is declared in BKE_sequencer.h.

This revision now requires changes to proceed.Dec 18 2019, 2:50 AM

I don’t know why clang format wasn’t working. Visual studios usually shows a popup whenever I re open the project.

Got same issue when recently moved to new machine.

Follow this link, there you can define own supplied clang-format.exe (Blender-git\lib\win64_vc15\llvm\bin) https://docs.microsoft.com/en-us/visualstudio/ide/reference/options-text-editor-c-cpp-formatting?view=vs-2019

I don’t know why clang format wasn’t working. Visual studios usually shows a popup whenever I re open the project.

Got same issue when recently moved to new machine.
Follow this link, there you can define own supplied clang-format.exe (Blender-git\lib\win64_vc15\llvm\bin) https://docs.microsoft.com/en-us/visualstudio/ide/reference/options-text-editor-c-cpp-formatting?view=vs-2019

Already did this

Why won't this work for image strips?

Why won't this work for image strips?

Image strips are using completely different system. Not sure what you meant by this question.

I have looked at possibility of further centralizing IMB_Proxy_Size to filename suffix and I don't think that this is very practical to do in this patch, So I will accept this in this form.

This revision is now accepted and ready to land.Wed, Jan 1, 2:58 AM

If this patch is accepted, could it be committed, please?