In D2872#67562, @Sergey Sharybin (sergey) wrote:

Is it really needed as two different effects? To me it seems more like a "blend" effect, which will support any of blending modes from BLI_math_color_blend.h. Even if we currently only will support two blend modes, still don't see why it's beneficial to have different effect type for those.

Sorry, I didn't know about BLI_math_color_blend.h file, I will point the effect for those.

Concerning the multiple x single effect, I think the same way, but it would change the way Blender works today, if you look carefully today we do have different effects for Add, Subtract, and Multiply. My initial idea was to add them only as blend modes I extended it to effects when Peter suggested it. To refactor those I think I should add 'Add, Subtract, and Multiply' to the same effect, preserve the old ones for compatibility, and still add them separately to the blend modes and sure it would take a lot longer once today all blend modes are separated effects.

I am very sorry again but I would like to hear your thoughts about that before coding this change.

Warm regards.

Hello, I've used the VSE many times for doing color grading and finishing of animation done in blender, including many professional projects.
Having screen and overlay modes was simply my top secret feature request for the VSE , this is making some simple things possible like adding grain or light effect that don't blow out the image.
Other modes can be great to have but I think these ones are the most important missing ones.

About having blending modes as effects strips, I don't really see the need here, as you can change blending mode of any strips you like. It's IMO doing the same thing in a less efficient way.
Maybe there is a reason that I'm not aware of. I suspect this is just old stuff that stays here for compatibility. It's not like cross or gamma cross that I found useful even if you can achieve the same result with keyframes.

Thanks Nokiam for the patch and I look forward using this feature soon !

Hi szap, thanks for the support! Blender is a great software, I believe that it deserves to have this basic blend modes in VSE. I work with video editing in my spare time and all other professional editing softwares have those, but I do like blender and decided to add them myself. I really hope it gets accepted!

IMO I think as Sergey and Aaron suggested , having one effect with multiple blend modes is much better. It use less space in menus for a similar effect.
Also, many people don't know each blend mode and what they do, so they like to test them one by one. It would be easier with only one strip.

Maybe it's better to keep multiply and add for backward compatibility. I don't know how important is that in our particular case.
If that was my decision I would get rid of them, unless many people told me they are using them, but better ask someone more involved in blender development. I'm just a tourist here !

Thanks,

I also prefer one "blend" effect. Regarding the compatibility issue, this should be fixable with versioning code that will automatically convert the old effect strip types to the new one.

I did some tests with GCC 6.3 (Debian Stretch) with a simple test-file (that makes it easier to understand the generated assembler output):

Sadly, only codepath number 2 gets the proper treatment by the optimizer (is turned into MMX/SSE-Code and the virtual function call is removed).

The difference in speed is around 450% which sounds worth the effort :)

The good news: you don't have to change much to make your code look like path 2.
I think, you can still get away with one switch-case statement, but you have to explicitly call your filtering function with the appropriate kernel directly otherwise, the optimizer won't get it.

Yieks, that's a lot of duplicated code!

I tested with GCC-4.7 and you are right, none of my solutions including code path 2 will work properly with the version of GCC that is used for the official release builds
(granted, https://wiki.blender.org/index.php/Dev:Doc/Building_Blender is still up to date).

I don't know, what the others are thinking, but in that case, maybe use a macro?

That isn't pretty either, but clearly preferable to crazy code duplication (tm) IMHO...

... like code path number 3 in the new version:

which does the trick in GCC 4.7 (as well as current versions).

In D2872#68299, @Peter Schlaile (schlaile) wrote:

... like code path number 3 in the new version:

test.c2 KBDownload

which does the trick in GCC 4.7 (as well as current versions).

I also vote for the macro version. This file contains a macro version as proposed so that the others can analyze.

Warm regards!

@Sybren A. Stüvel (sybren): because that doesn't work.

Older versions of GCC won't properly optimize the code and the result is round about 400% slower.

Please read the discussion above and feel free to fiddle with my test code.

@Sergey Sharybin (sergey): the only two clean solutions would be:

• port the sequencer to C++ and use templates
• make sure, release builds are generated with a recent version of GCC (larger than version 6, probably version 5 will do too, have to test).

If the later is the case, I'm totally with you, if it isn't, I'm not really fond of the idea of a really slow working effect pipeline...

Hi @Peter Schlaile (schlaile),

IMHO It seems to me that building in a higher gcc version would be a feasible solution addressing the code readability problem and performance at same time. If its not the case, maybe opening an exception to use macros here would be the less of two evils.

VSE is lightweight and fast, I totally agree that we must keep it that way.

Can I help somehow with the gcc version? If someone would be so kind as to pointing me in the right direction I would be glad to work on that.

Warm regards.

If you define the function receiving the function pointer with BLI_INLINE , older compiler versions will likely work fine as well.

The official Blender 2.79 Linux release seems to have been compiled with GCC 7.1.0, so I don't think it's a problem anyway.

@Brecht Van Lommel (brecht) : thanks for validating the compiler version, maybe someone should update the blender wiki then!

So we can use the inline functions and everything will work out nicely!

@Brecht Van Lommel (brecht) : BTW, inline will not work for older compilers, I tried that, but it doesn't matter luckily :)

Did you try BLI_INLINE specifically, which expands to static inline __attribute__((always_inline)) __attribute__((__unused__))?

Regardless, it's still safer to use it, since newer compiler versions might change their inlining heuristics.

Wiki updated.

Hi guys,

Is there any preference for BLI_INLINE instead of MINLINE? They are the exact same thing.

Regards

Use BLI_INLINE, it seems more commonly used for this type of thing.

Can we move forward with the review now? Also this might be beyond this patch but the old blend strips should be removed and versioning code to replace the strip with this new one with the correct mode.

Updated Diff with the changes requested by @Sybren A. Stüvel (sybren) .

Committed now, with some minor tweaks to the enum identifiers.