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
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- Fix-T70415 (branched from master)
- Build Status
Buildable 5948 Build 5948: arc lint + arc unit
Event Timeline
Comment Actions
- 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)
Comment Actions
- 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.
| 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. |
Comment Actions
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.
Comment Actions
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 | ||
| 1026 | seq_proxysize_to_scale_factor() has type of double, so conversion here seems to be redundant | |
Comment Actions
- 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
Comment Actions
if (!ibuf && proxy_size != IMB_PROXY_NONE) { is needed but changes will be made to fix this.
Comment Actions
Also please look at unnecessary whitespace changes and apply clang-format.
| source/blender/blenkernel/intern/sequencer.c | ||
|---|---|---|
| 3848 | 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. | |
| 3858 | (psize != IMB_PROXY_NONE); would be nicer | |
| source/blender/editors/space_sequencer/sequencer_draw.c | ||
| 1027 | 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. | |
| 1030–1033 | You could compare sseq->render_size to eSpaceSeq_Proxy_RenderSize items | |
| 1041–1042 | These + 0.5f are used to "round" floats to nearest integer rather than nearest lower integer. | |
Comment Actions
- 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.
Comment Actions
I don’t know why clang format wasn’t working. Visual studios usually shows a popup whenever I re open the project.
Comment Actions
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.
Comment Actions
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
Comment Actions
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.