VSE: reduce effects code duplication, making gaussian blur faster in the process #116089
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#116089
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "aras_p/blender:vse-fx-cleanup"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Now that the code is in C++, quite some duplication between "byte" and "float" effect code paths can be reduced (easier than it was in C times). So I did that, removing about 400 lines of code.
In that process I accidentally made Gaussian Blur faster, since while reducing the amount of code I noticed it was doing some things sub-optimally (calculated kernel tables for each job, etc.). Applying 100x100 gaussian blur on 4K UHD resolution image strip on Ryzen 5950X went 630ms -> 450ms.
@blender-bot build
WIP: VSE: reduce effects code duplication, making gaussian blur faster in the processto VSE: reduce effects code duplication, making gaussian blur faster in the processThis is really nice change, I really appreciate it! I would like to discuss the inline stuff, perhaps it would be good idea to ask on chat to better understand the intentions behind it.
@ -946,4 +863,0 @@
using IMB_blend_func_byte = void (*)(uchar *dst, const uchar *src1, const uchar *src2);
using IMB_blend_func_float = void (*)(float *dst, const float *src1, const float *src2);
BLI_INLINE void apply_blend_function_byte(float fac,
From what I read,
BLI_INLINE
has an effect mostly on how the code compiles, not necessarily on performance. I have seen some benchmark linked from random stackoverflow article, but not sure if they are benchmarking code performance, or compiler performance. https://indico.cern.ch/event/386232/sessions/159923/attachments/771039/1057534/always_inline_performance.pdfThe benchmark also suggests effects on binary size, but guessing it won't have any major effect here.
So the question is, why did this code use this attribute/qualifier and whether it really needs to be there. From what I have read, I would guess it does not really need to be there, but would like to hear your opinion on this.
I think in this particular case it does not matter at all.
"inline" is a hint to the compiler, saying like "oh whenever anything calls this function, please to try to just literally paste the whole function into the call site, instead of actually calling the function". It's only a hint, but generally it makes sense to do that for functions that are extremely short, think one or two lines of code with a handful of operations. This one is not, it's two nested loops, a bunch of operations inside the loop, and call into yet another blend function. The amount of overhead from "call the function" compared to the amount of work done inside the function (working on thousands or tends of thousands of pixels!) is absolutely tiny.
So I have no idea why would anyone mark it as inline. My only guess is that maybe back at some point the function used to not work on many pixels, but rather on one pixel at a time, and it was marked inline, but then it got changed to have these two loops, and the inline stayed by accident. But this theory is hard to prove since the code seems to have been (re)added by Ton 15 years ago with "blender 2.5 is back with sequencer code!", so presumably the code existed from even before that, just without a trace in git.
@ -373,0 +321,4 @@
for (int y = 0; y < height; y++) {
for (int x = 0; x < width; x++) {
float4 col2 = load_premul_pixel(src2);
if (col2.w <= 0.0f) {
Later in the code you access
float4
values array by index, here by it's member. Personally I like index more, sincepixel[4]
is more familiar thanpixel.w
It's a matter of getting used to it I guess, so it's a nitpick really.
I don't have an opinion either way.
.w
for me personally is a bit easier to read than[3]
, but that's a taste/familiarity thing. And yeah I could have used.x
....w
below too. Do you want me to change it one or another way?Pressed the wrong button on x/y/z/w or index discussion, I think it should stay as it is. Would be perhaps nicer to have rgba members for pixels, but that's just cosmetics really..