Color-separated Saturation

[mkarg]

Initial draft of color-separated saturation effect.

See OpenShot/openshot-qt#3060.

[mkarg]

Works well on Linux, still couldn't test it on Windows.

The math is extremely verbose by intention to clearly describe the idea of the algorithm. In fact, it should be discussed whether to keep it that way, or whether to rewrite it in one of two alternative goals:

• Math could be reduced, but makes it possibly harder to understand then.
• Refactor math into a new function adjustSaturation() which prevents having the same formula applied four times.
• or simply keep it as is

@ferdnyc WDYT?

[ferdnyc]

@mkarg This looks cool, I think it'll be great to have available in OpenShot!

TBH I'm not really fussed about the verbosity of the math-code, since any superfluous terms will most likely just end up optimizing away. My main question is whether it makes sense to keep (and apply) both the common/grayscale saturation and the per-channel saturation?

As things stand the required parameters increase not just from 1 to 3, but from 1 to FOUR. And there's a confusing interplay between the common saturation value and the per-channel saturation value.

I was initially going to suggest that the common saturation be eliminated, and the single-value constructor become just an overload that initializes all three channels the same. i.e.:

// Declaration

public:
    Keyframe saturation_R;
    Keyframe saturation_G;
    Keyframe saturation_B;

    Saturation();
    Saturation(Keyframe new_saturation);
    Saturation(Keyframe new_saturation_R,
               Keyframe new_saturation_G,
               Keyframe new_saturation_B);

// Implementation, via C++11 delegating constructors

// Actual constructor
Saturation::Saturation(Keyframe new_saturation_R,
                       Keyframe new_saturation_G,
                       Keyframe new_saturation_B)
    : saturation_R(new_saturation_R),
      saturation_G(new_saturation_G),
      saturation_B(new_saturation_B)
{
    InitEffectInfo();
    // ..Other property initialization
}

// Delegating overload, 1-parameter version
Saturation::Saturation(Keyframe new_saturation)
    : Saturation(new_saturation, new_saturation, new_saturation) {}

// Delegating overload, 0-parameter version
Saturation::Saturation() : Saturation(1.0, 1.0, 1.0) {}

...But that still doesn't solve the OpenShot case, since the JSON interface it uses means that it's all-or-nothing, you're expected to supply values for the entire defined set of parameters.

What would you think about making the three-channel version a separate effect, maybe "Chroma Saturation" or "RGB Saturation" or whatever? It's admittedly a bit reductive, since they'd do largely the same thing. But since we don't support "switchable" parameters or whatever, doing it as a separate effect would allow the user to choose which interface they want based on how much control they need.

They could use the standard Saturation effect if they just want to adjust all of the channels at once, or they could pick the new effect if they need the ability to adjust channels separately (and then they'd have to supply three individual channel saturation values for each point, with no common-saturation parameter).

[ferdnyc]

(P.S: I see no reason to worry about this working on Windows (or MacOS), it's not doing anything even remotely OS-dependent.)

[mkarg]

I had the same idea before opening the PR when I inspected the existing effects. So I already discussed the idea with my wife which is my beta tester here (she is a long-term AfterEffects professional and creative video artist) and her answer was clear: The four sliders should stay in one single effect! Why? Because creative artists do not decide to use either greyscale or RGB, they use both at the same time. The reason is rather simple: You first pull the greyscale to get a common baseline, the adjust the colours, then possibly re-adjust the common baseline again, and so on, until you're happy with the result. If you split this into separate effects you have to go back-and-forth between effects all the time which drives you crazy. If you only have the RGB effect, then things are even worse, because you cannot have a color-neutral reduction at all: you definitively will screw the RGB balance when you're forced to do that with three separate sliders. Hence, she convinced me, that the way AfterEffects does it, is really the optimum: A single effect with lots of sliders. I tried it out, and she is right. In fact, there is nothing confusing, the system acts completely intuitive and correct. BTW, AfterEffects even has three more sliders (CMY) to make it even more convenient to pull two-of-three sliders (like RG, GB, BR) -- but to keep things simple, I'll defer that idea for later... ;-)

[ferdnyc]

👍 OK then, I'll buy that!

I can certainly believe that a heavily-instrumented system like that is a professional editor's best friend, especially if it's as intelligently designed and productivity-optimized as I have no doubt AfterEffects' tools would surely be.

I do still have some slight concerns. But, TBH I'm content to just get them out there, then push them down, soldier on anyway, and hope for the best. But mostly:

1. Even though you say that you didn't find it to be an issue (and I'm not doubting you on that at all), I worry whether tools geared towards a pro-level editor's use are still accessible to less advanced/technical users. We're not AfterEffects and aren't trying to be (we'd fail miserably if we did), and robbing our less technical users of a simple one-slider saturation control still feels like a bit of a gamble to me.

   If you say the four- (or more-)slider version of the effect is not just useful, but more useful than a three-slider version, I can completely accept that. But I'm still thinking it might be worth it to keep the one-slider Saturation plugin as-is, for users who want simply that with no complications. And then add a new Advanced Saturation effect, for those who want to use the four-slider version. (Or more, even! One big advantage to segregating the new features, it gives us freedom to dump as much as we can think of into it. No matter how complex or advanced it gets, users who aren't interested in the new tools won't be affected in any way.)

2. My other worry is... well, like I said, we're not AfterEffects. I have no doubt their tools are great to use, in no small part because they're effective and performant, presumably built from heavily-optimized and hardware-accelerated algorithms that're designed to wring every last CPU cycle out of the systems they run on.

   I don't think I'm catching anyone by surprise if I say that OpenShot's code is... ... ... not that. Very not that. Your implementation is very clean and should have a negligible impact on performance, so there's very little cause for concern as I expect it'll run with performance/efficiency that's effectively unchanged from the existing implementation. Buuuut, as I'm sure you noticed, the bar there is pretty low, and the existing implementation is... not what i'd call "ideal". So, I don't expect the experience to be quite comparable to AfterEffects, even if we were to provide all the same tools/features

Still, edging closer isn't a bad thing, and it gives us a target to work towards!

I still need to actually build and test this (haven't gotten around to it quite yet), and I'll go through and submit whatever code-review comments I have as soon as I can. (I already looked through it well enough to know there's not much. Might make things a bit more readable if the function statement for the four-value constructor weren't like 300 characters wide and all on a single line, that was the main thing that jumped out at me. 😉 )

[mkarg]

Thanks for reviewing it. I'm glad to fix everything you like me to. :-)

Regarding your concerns, I really do share them, but let me tell you this (which might surprise you):

1. For the average joe, nothing changes. He can simply ignore the three additional sliders. I built the code backwards compatible, so existing projects will produce the same result, when just not touching the R/G/B sliders in this effect. Nobody is forced to touch these new sliders; the defaults in place have a neutral setting, so there is no need at all to mess with them. It is just an additional option one could use or simply ignore, as the filter works 100% as before when just using the common saturation slider. Anyways, I see your point. So my counter proposals for keeping the original effect would be: (A) We could produce an expert variant of the saturation filter. (B) We could provide a filter named "Black And White" -- as this is what it is named in AfterEffects, so those user would find it most easily; that would be a small bit of more sliders to 100% fulfil what it does, but we could add those new sliders (C/M/Y) in a later PR as those a negligible comfort only. (C) We could provide a new feature to OpenShot which allows to separate the "normal" sliders from the "expert" sliders, so the average joe simply does not see the new sliders, and experts switch their GUI into the expert mode to see all of them.

2. You won't believe that, but this is the whole true story: My wive runs her business using Adobe for years, even gave Adobe workshops. At one day she noticed that 80% of her daily work can be done with open source products. Since that day, we started to replace Adobe products by open source products. If ther is something missing that she often needs, I write the code to provide it. At one time we discovered open shot and she gave it a try with one of their creative video projects. The result was: She said there are just few features I have to add to make her (and in turn lots of her students) actually replace AfterEffects + PremierePro by solely OpenShot. Why? Because most Adobe users simply do not need the full power nor all the features of that products. In some cases OpenShot was rendering exports even many times faster than the Adobe couple (in fact, more than ten times faster than Adobe, on her professional HP rendering monster). So yes, OpenShot might not be AfterEffects and you might not intend to become AfterEffects, but for lots of people OpenShot is just few steps away from the power and feature set needed to replace it for them (and every day they are more people, thanks to Adobe's product and price prolitics). So I am here to fulfil that vision, and help her and her students to live an Adobe-free live. That's my driver and as a day-job CDO I know we can reach that target.

[ferdnyc]

Re your point 2, I absolutely agree with you on the commercial-vs.-open-source thing, in fact one of the primary motivators for many open source projects is the fact that they can achieve effective functional parity for 95% of the needs of 90% of the userbase of a similar commercial product. The simple fact is, as you say, most people don't need all that "stuff" that keeps getting added to commercial software products as they grow, and the few who can't do without them are mostly business users (no matter what the software), exactly the userbase commercial software products (and commercial software prices) are increasingly targeted to.

(The only thing I don't understand is the Adobe suite rendering so much more slowly than OpenShot; ffmpeg is good, sure, but I'd expect their code to be at least as good... or that they'd just get a commercial license to incorporate ffmpeg itself, if they couldn't achieve parity with it. I have to think that the lower speed has to come with some benefits in trade, most probably more efficient video compression. OpenShot's export process for almost all formats currently renders fixed-bitrate video. Which is fine if you're mastering a Blu-Ray disc, but creates much larger files than are possible if the goal is any sort of streaming/online delivery. However, rendering with more efficient encoding does take longer.)

On this, though:

For the average joe, nothing changes. He can simply ignore the three additional sliders.

See, I consider those two statements inherently contradictory. Three new sliders have appeared in the interface, which more than fits the definition of "something changed". Of course the user doesn't have to use them, but they at LEAST have to ignore them, which means they have to first figure out that they can simply ignore them. For the user, changes start with what they see, what the program tells them and shows them. Whether or not they'll then have to make any changes in how they interact with the program... is actually a secondary consideration. (And therefore at least twice as hard to sell them on, when it's the case.)

I mean, don't get me wrong. I personally love knobs, and my default position is that I want to have s many as possible, so I can tweak everything it's possible to tweak. But I've also grown all too familiar with the other side of the userbase, the users who just Like Things The Way They Are™ and who greet changes with a combination of fear, suspicion, and frustration. Any changes at all, even the ones they eventually grow to rely on, and will ultimately prefer to the previous status quo.

I don't believe those people should be allowed to get in the way of all progress, by any means. (As I said, they're often eventual converts to the changes that they knee-jerk despised.) I simply find that it's often easiest if we can find some way that progress can be made without setting them off, and that allows them to discover and embrace the changes at their own pace, rather than having them thrust upon them. Clearly I'm either remarkably accommodating, or a coward*.

* - (It's definitely the second one.)

[mutagennix]

@ferdnyc I have heard about you from jonoomph when I spoke to him several day ago. Thank you for all your work on this project. I am a designer by profession but I will try to assist where I can. In regards to the sliders, I would suggest to simply create a "Basic" and "Advanced" tabs/toggle or select dropdown above the slider. Make "Basic" the default selection, showing only one slider and when a user selects "Advanced", then show multiple sliders. I created a UI mockup to demonstrate this.

[ferdnyc]

@mutagennix Oh, absolutely, if the current properties interface supported anything like that, I'd agree it'd be the way to go. But it doesn't, currently, at all, and since the properties are defined by the effects themselves in libopenshot, stored in a structure, and communicated to the UI via JSON text, it's not just the UI itself that would need to be implemented, but also (first) a grammar for communicating it to the frontend.

I'm not saying it's a worse solution — heck, it's a better solution, without question — but it's also a solution that isn't currently available without some non-trivial development on both the library and UI sides. A separate effect could be created ''today''.

Anyway, this may be a moot point, as it turns out we may be forced to make it a new effect. More on that shortly.

Impressions

So, I did finally get a chance to try this out. I absolutely see what you mean about the fourth slider still being useful, in fact it's absolutely required! It interacts with the individual-channel controls in surprisingly complex ways, some of them a bit unexpected. It takes some time to wrap your head around, but the possibilities for making use of those interactions are vast.

For instance, taking a sample frame from everyone's favorite test video, "Big Buck Bunny", this is the original frame, untouched:

Setting Saturation to 0.0 has the expected effect:

But when Saturation is 0.0, and the RGB channel saturations are set to (0.2, 0.2, 0.5), the result is somewhat unexpected:

For comparison, here's Saturation 0.0, RGB Saturation (0.0, 0.0, 1.0):

With the common saturation minimized, the channel saturation controls become something of an "overall tint" control, affecting every pixel in the image.

By going in the other direction with the common saturation, that effect can be reversed to create something of a "spot color" effect where only pixels of a certain color will be shaded.

For instance, here's Saturation 3.0, RGB Saturation (0.0, 0.0, 1.0):

(This frame is a bit too light, the effect is even more pronounced on darker images that don't "white out" so much when desaturated.)

So, that's actually quite cool, and not at all what I was expecting.

Property adjustment and keyframing

Beyond that, everything works as expected. The four individual values can be keyframed at any point along the clip, they interpolate smoothly (if told to), and the image is adjusted accordingly.

Saving and loading the effect parameters in the project file is effective.

Forwards & backwards compatibility

Saving and loading project files in different versions of OpenShot, though, is where we hit a snag.

Creating a project file with a four-slider Saturation effect, then loading it into OpenShot 2.4.4, is actually fairly workable. The data for the extra sliders is largely ignored, and all of the common-Saturation data is applied to the single Saturation effect correctly. Because the keyframes for the other three properties are still there, green pips are shown for them on the clip despite there being no apparent keyframe at that location, but that's minor. The channel-saturation data is also retained in the project file even if edited and re-saved from OpenShot 2.4.4, so it won't be lost when loading back to a newer version.

Unfortunately, loading an older project file into an OpenShot with the four-slider version of the Saturation effect turns out to break fairly badly. The effect loads the common-saturation data correctly, and shows the channel-saturation values all initialized to 1.0, but that's deceptive.

Because the project data contains no existing values for saturation_R, saturation_G, or saturation_B, any attempts to adjust those values on a pre-existing Saturation effect will cause them to instantly reset to 0.0, and they cannot be adjusted from that point — no new values will successfully apply. Worse, the attempted values end up being stored directly onto the properties themselves, rather than as keyframes, so the interface goes a little bit crazy trying to process that. If the project file is then resaved, the values are stored the same way, leading to a corrupted project file, for instance:

Corrupted effect data from resaved legacy project file when loaded with newer effect

{ "class_name": "Saturation", "description": "Adjust the color saturation.", "duration": 0.0, "end": 0.0, "has_audio": false, "has_video": true, "id": "UG9UNVHC07", "layer": 0, "name": "Color Saturation", "order": 0, "position": 0.0, "saturation": { "Points": [ { "co": { "X": 1.0, "Y": 0.39473684210526316 }, "interpolation": 2 }, { "co": { "X": 24, "Y": 1.263157894736842 }, "interpolation": 1 }, { "co": { "X": 65, "Y": 0.5789473684210527 }, "interpolation": 1 } ] }, "short_name": "", "start": 0.0, "type": "Saturation", "saturation_B": 2.3421052631578947, "saturation_R": 2.4473684210526314 }

That's quite obviously a bug, either in OpenShot's UI implementation or its project data management, and a pretty ugly one at that (as is any bug that corrupts data). But as long as it's there, it looks like modifying existing effects to add parameters is a no-go. I think it'll need to be done as a new effect, which either complements or replaces the older one.

I'll go through the PR diff now and submit any code-review comments I can think of.

[ferdnyc]

@mutagennix

I have heard about you from jonoomph when I spoke to him several day ago.

Hmmm, ominous. Well, if it was mostly-bad then it was definitely all true!

[ferdnyc]

(Honestly, the documentation comment updates are the only change I'm adamant about, and the only reason I'm even marking this "Request changes" at all.)

[mkarg]

(The only thing I don't understand is the Adobe suite rendering so much more slowly than OpenShot;

AfterEffects (at least the latest version we obtained, which is definitively some years old) always loads all clips completely into RAM and explodes it there (all frames of all clips are kept in RAM always in an uncompressed way). When RAM is low, it starts to swap memory to disk. As it progresses with rendering, it has to do that in circles, hence it swapps in what was just swapped out. Swapping is a very bad thing, even with SSDs. For example, today we loaded some 4k clips into it (5 MP4 clips 3.5 GiB each = 17,5 GiB on disk results in use of approx. 60 GiB physical RAM), applied just two simple effects, and started to render two minutes of final MP4 video output. One should think the CPU is heavily working now, but it isn't. We have 20 cores, 64 GiB of RAM, 1 TiB PCI-SSD (non-SATA M2-RAID special HP booster hardware), server-class mainboard, etc. (it is a HP Z 600 professional graphics workstation). AfterEffects runs for three hours @ CPU at 8%, RAM at 98%) -- it simply waits for ongoing swapping!

[mkarg]

So, that's actually quite cool, and not at all what I was expecting.

Yes, it is cool, but in fact, this is not intended, but simply (so I assume) a side effect of not mixing the "common" slider and distinct "R, G, B" sliders. Recently you said you want to have separate effects, so the new filter has just three sliders. Hence I coded a variant (which is the baseline of this PR) which simply does exactly that: It first applies the "old" effect (which, as you know, weighs colors with distinct factors), ontop (i. e. after applying those factors) it applies the "new" effect after that. It simply works as if we would have two effects applied in sequence. But f(g(x)) is not the same as g(f(x)): the cummutative law does not apply to effect sequences! So those factors now have the effect that R, G and B are "drifted" before the new effect "sees" them. It is simply a bug in this PR; instead the effect needs to do avg(g(x), f(x)). This bug (the color drift) is exactly what will happen with separate effects. If we instead would have had the permission to actually extend the existing effect, we could mix correctly, so the old and new sliders see the same original color. No more color drift would happen then -- which you cannot reach with separate effects applied in sequence.

[mkarg]

That's quite obviously a bug, either in OpenShot's UI implementation or its project data management, and a pretty ugly one at that (as is any bug that corrupts data). But as long as it's there, it looks like modifying existing effects to add parameters is a no-go. I think it'll need to be done as a new effect, which either complements or replaces the older one.

I cannot follow that conclusion. If it is a bug of openshot-qt then we should simply fix that bug first. But it should not be a reason to change our decision of the effects themselves.

[ferdnyc]

Recently you said you want to have separate effects, so the new filter has just three sliders.

I never said I wanted that. At all. I was immediately on board with the four-slider version, even before I'd tried it. In initial discussions I raised the question of whether a 3-slider plugin was sufficient (before the separate-effects discussion even started, and before I understood the interactions of the four parameters sufficiently). You explained why that wasn't equivalent, and I accepted that:

If you say the four- (or more-)slider version of the effect is not just useful, but more useful than a three-slider version, I can completely accept that. But I'm still thinking it might be worth it to keep the one-slider Saturation plugin as-is, for users who want simply that with no complications. And then add a new Advanced Saturation effect, for those who want to use the four-slider version. (Or more, even!

...And most definitely intended to be used only one or the other, never both. Combining separate effects would require processing each frame twice, which would have a serious performance impact.

[ferdnyc]

That's quite obviously a bug, either in OpenShot's UI implementation or its project data management, and a pretty ugly one at that (as is any bug that corrupts data). But as long as it's there, it looks like modifying existing effects to add parameters is a no-go. I think it'll need to be done as a new effect, which either complements or replaces the older one.

I cannot follow that conclusion. If it is a bug of openshot-qt then we should simply fix that bug first. But it should not be a reason to change our decision of the effects themselves.

If you want to take on fixing that bug a prerequisite for merging this PR, then by all means. It's not a bug that currently affects anything, since the only way to trigger it is to change the set of parameters for an effect. But sure, it'd be good to have that fixed.

(I also don't want to assert with too much confidence that the bug is located in OpenShot-qt. It's in the properties handling, clearly — either in openshot-qt (whether in terms of the project data or the communication of property changes to libopenshot), or in libopenshot (on the other end of that communication)... or even both.)

[mkarg]

@ferdnyc All requested changes fixed. :-)

[jonoomph]

Hi! This PR looks great so far. Are we getting close to a final version that is ready for merging?

[mkarg]

Hi! This PR looks great so far. Are we getting close to a final version that is ready for merging?

I do not see there is anything open within this PR that keeps you from merging.

[jonoomph]

LGTM, sorry this took forever and day to get to. I don't see any big issues with it, and I think it will be awesome to include in OpenShot! Thanks again for your help! Please make some additional effects!!! Thanks.

[jonoomph]

@mkarg If you are interested in helping more deeply with OpenShot, please give me a quick email: jonathan@openshot.org. 👍

[codecov]

Codecov Report

Merging #368 into develop will decrease coverage by 0.19%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop     #368      +/-   ##
===========================================
- Coverage    49.39%   49.19%   -0.20%     
===========================================
  Files          129      129              
  Lines        10198    10238      +40     
===========================================
  Hits          5037     5037              
- Misses        5161     5201      +40     

Impacted Files	Coverage Δ
include/effects/Saturation.h	0.00% <ø> (ø)
src/effects/Saturation.cpp	0.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 414a2cd...a7fe41c. Read the comment docs.

[jonoomph]

Fixed some merge regressions... now for the real merge