Add Screen and Overlay blend mode to VSE
ClosedPublic

Authored by Maikon Araujo (Nokiam) on Oct 8 2017, 6:44 AM.

Details

Summary

The same way we have add, multiply and subtract blend modes on vse strips, I would like to add Screen and Overlay as they are very useful.

Updated: After a lot of feedbacks and productive discussion from you guys, I added a single Color Mix effect and the default strip Blend Mode having the blend modes defined em 'BLI_math_color_blend.h' as suggested by @Sergey Sharybin (sergey).

Warm Regards and thanks for blender!!


Diff Detail

Repository
rB Blender
There are a very large number of changes, so older changes are hidden. Show Older Changes
source/blender/blenkernel/intern/seqeffects.c
1270

Consider making it a typedef for functor perhaps, is used in lots of places and typing it every is a bit verbose.

1287

Spaces, rt2, rt1, facf1, ...

This revision now requires changes to proceed.Oct 11 2017, 10:13 AM

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.

I just updated the diff with changes concerning the code style and the 'BLI_math_color_blend.h' functions.

Thanks again.

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!

Fixed a little bug in do_screen_effect.

I really would like to know what is preferable to have: one effect or multiple effects? I just studied the code a little more and I believe that I can implement both, once we have BLI_math_color_blend.h it is easy to add all the other blend modes as well.

Warm Regards.

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.

Hi guys,

As the most of you suggested, I implemented the Color Mix Effect having the most used blend modes, and updated the blend modes to add Hue, Saturation, Soft Light, Darken, Lighten, Dodge, etc. I kept the Add, Subtract e Mul effect to avoid breaking any user project.

Thanks for Blender!
Warm Regards,

Blend Mode

Blend Effect

source/blender/blenkernel/intern/seqeffects.c
1268

Changed for inline functions. Actually I used the inline functions from BLI_math_color_blend.h once they are already defined there. Thank you for your feedback!

1270

Tried to incorporate your suggestions as you can see in the new files. Thanks again!

1277

Sorry again, I got confused. All functions in the last diff are following this rule. Sorry again, and thank you for the review.

1285

I hope it is fixed now. Thanks again.

1287

Tried to fix all wrong spaces. Thanks again.

1288

I hope this is ok in this new diff. Thank you!

1303

All "ifs" have braces now. Thank you.

1343

Best tip ever!!! It made very easy to incorporate all the other blend modes once they are all standardized there. Thank you very much! I hope you like the last diff. Warm Regards!

1376

I hope it is ok now. Thank you.

1384

I tried to fix that. Thank you.

1423

I hope the code is more readable now. Thank you.

1427

I tried to get rid of all commented code. Thank you.

source/blender/editors/space_sequencer/sequencer_edit.c
99

All fixed now. Thank you.

source/blender/makesdna/DNA_sequence_types.h
521

If you look at SEQ_TYPE_ADJUSTMENT and the other ones above, the new types are indented, Gaussian Blur and Text are the exceptions. How should I proceed here? Just checking to do the best thing. Thank you for your feedback!

source/blender/makesrna/intern/rna_sequencer.c
1346

Fixed the spaces. Thank you.

1421

Fixed the spaces. Thank you.

2415

I hope it is better now. Thank you.

source/blender/makesrna/intern/rna_sequencer_api.c
478

I hope it is ok now. Thank you

release/scripts/startup/bl_ui/space_sequencer.py
756

I would call this "Blend Mode"

Maikon Araujo (Nokiam) marked an inline comment as done.Oct 13 2017, 3:44 PM
Maikon Araujo (Nokiam) added inline comments.
release/scripts/startup/bl_ui/space_sequencer.py
756

I tried to avoid confusion with the no effect Blend Mode, but once this one is shown inside the Effect section I do agree it is a lot better. Thank you.

Maikon Araujo (Nokiam) marked an inline comment as done.

Adjusted the label to Blend Mode as Aaron Carlisle suggested.

Maikon Araujo (Nokiam) edited the summary of this revision. (Show Details)

Fixed the layout for the Color Mix effect Blend Mode property. And added linear burn, pin light, linear light, and vivid light. Thanks again for all the feedbacks!!!


Peter Schlaile (schlaile) requested changes to this revision.Oct 15 2017, 10:37 PM
Peter Schlaile (schlaile) added inline comments.
source/blender/blenkernel/intern/seqeffects.c
1382

you might want to rename the function to something recognizable. like "do_blend_effect_float" or something.
After all, those are all effects here :)

I'm also not sure, if the compiler gets the inline optimizations with your function pointers right.

Please make sure, that you don't generate a virtual function call for every pixel you are processing...

Everything said here also goes for the byte version.

1423

the comment above regarding function naming and compiler optimizations...

This revision now requires changes to proceed.Oct 15 2017, 10:37 PM

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.

Maikon Araujo (Nokiam) updated this revision to Diff 9487.EditedOct 29 2017, 10:01 PM

Hi @Peter Schlaile (schlaile),

I appreciate the tests you have made, thanks a lot. I was afraid if it would be optimized for all possible compilers (like for windows, mac, etc), so I went the hard way of explicit calling all the inline functions.

Do you think it is worthy to make it look like path 2, or is it ok to leave like this?

Thanks again and warm regards.

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).

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

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!

Hi @Peter Schlaile (schlaile), @Sergey Sharybin (sergey), and @Sybren A. Stüvel (sybren).

Just updated the code to also add the last two effects from BLI_math_color_blend.h (difference and exclusion), also fixed a bug: the ColorMix effect wasn't being serialized on saving.

Looking forward to hear your opinions about having the version with a lot of duplicated code, this one with macros (also ugly), or something else you guys may suggest to make the code better while achieving the performance from inline functions as tested by @Peter Schlaile (schlaile).

Thanks again for the help!
Warm Regards!

I'm happy with the macro solution.

Sybren A. Stüvel (sybren) requested changes to this revision.Nov 2 2017, 4:34 PM

I don't like the code generation with macro's. Why not use higher-order functions and pass function pointers instead?

This revision now requires changes to proceed.Nov 2 2017, 4:34 PM

@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) requested changes to this revision.Nov 2 2017, 4:48 PM

I do not see how code-generate rather a huge function using a macro considered a good solution. This is most horrible solution from maintainability point of view. Just pass color function as a functor.

@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 guys,

In case of you decide to discard the macro solution for the inline one, here is a new diff with inline functions.

Warm regards!

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.

Changed to BLI_INLINE.

Best Regards,

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.

Sybren A. Stüvel (sybren) requested changes to this revision.Nov 21 2017, 10:05 AM
Sybren A. Stüvel (sybren) added inline comments.
source/blender/blenkernel/intern/seqeffects.c
1271

Please don't call things "helper" -- it conveys no meaningful information. Also see The Helper Anti-Pattern. Make sure that your comments actually describe what functions do, or why they are useful.

1286

Isn't this just a for (x=x0; x>0; x--) loop? Why desconstruct that into an assignment and a while-loop?

source/blender/makesdna/DNA_sequence_types.h
306

If this factor has a limited or otherwise expected range (like [0, 1] or (-1, 1]), please put that into a comment.

529

Sounds like a sheep ;-)

Anyway, I thought we were going for one strip type, and not have different types for different mix effects. Or am I confusing things here? Since the patch wasn't submitted with 'arc', there is no context available around the diffs.

By the looks of the code further down, it seems that these new SEQ_TYPE_XXX enum values are used for selecting the mix type, and not the strip type. Having two very different uses for the same enum type is confusing.

This revision now requires changes to proceed.Nov 21 2017, 10:05 AM
Maikon Araujo (Nokiam) marked 4 inline comments as done.

Diff updated.

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

source/blender/blenkernel/intern/seqeffects.c
1271

Removed The Helper Anti-Christ ;) . I left the comment to indicate where the blend mode functions begin, this is the way the whole file appears to be organized, so I kept things the way they are. For each effect it has the effect's name in the beginning.

1286

It is! Just changed for your suggestion, it was like that because I had copied this function from the others effects in this same file, they all use the while(x--).

source/blender/makesdna/DNA_sequence_types.h
529

It is only one strip type (SEQ_TYPE_COLORMIX), but VSE uses the same enum for effects and blend modes, and sorry but it is out of the scope of this proposal to change that right now. About the blend mode, all kinds of strips have this blend mode property which blends the strip with the channel below. If you think that the effect works like do_effect(channalA, channelB) the blend mode works like do_effect(channalA, this).

So, the way it works today, it has in fact the same enum for two things, and I implemented it accordingly. Having one effect strip carrying all blend modes available at the moment. Sorry for not using arc, I will dig into it next time!

This revision was automatically updated to reflect the committed changes.

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