-
Notifications
You must be signed in to change notification settings - Fork 268
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement blend modes (#320) #791
base: develop
Are you sure you want to change the base?
Conversation
@nilp0inter Thanks for submitting this! I think it's a great idea. Some initial thoughts, I'll follow up with inline comments afterwards, to address specifics. One minor factor here is this note from the Qt 5.5 docs:
Our images, notably, are not any type of
So it appears that at some point between Qt 5.5 and 5.15, they removed that limitation. Whatever that cutoff point is, AIUI these modes won't work with any older Qt version. That's not necessarily a dealbreaker — we can always just document the limitation, but it's something we should be aware of. And probably try to figure out where the version cutoff is. I initially went into the archived docs to check whether |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I had some small suggestions... 😉
(Really, don't hate me.)
src/Clip.h
Outdated
@@ -157,6 +157,7 @@ namespace openshot { | |||
openshot::AnchorType anchor; ///< The anchor determines what parent a clip should snap to | |||
openshot::FrameDisplayType display; ///< The format to display the frame number (if any) | |||
openshot::VolumeMixType mixing; ///< What strategy should be followed when mixing audio with other clips | |||
openshot::BlendType blend; ///< What strategy should be followed when mixing video with other clips |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should make this an openshot::Keyframe
property. There are plenty of good reasons why someone might want to change the blend mode used on a particular Clip halfway through, and it would provide a much greater range of "special effects" type flexibility if they weren't locked into a single mode for the clip duration.
In practice it doesn't change a whole lot, making it a Keyframe property — it just means that it has to be queried on a frame-by-frame basis, you'd look up the value for the current frame with blend.GetValue(frame->number)
, instead of reading it directly.
Everything else involving the Keyframe processing / interface is already taken care of by the Keyframe
class and the JSON metadata you're already setting.
Oh, the declaration should be moved down with the others around line 280, though. You can probably just plop it right in after alpha
, they're pretty related.
src/Enums.h
Outdated
enum BlendType | ||
{ | ||
BLEND_SOURCEOVER = QPainter::CompositionMode_SourceOver, ///< This is the default mode. The alpha of the current clip is used to blend the pixel on top of the lower layer. | ||
BLEND_DESTINATIONOVER = QPainter::CompositionMode_DestinationOver, ///< The alpha of the lower layer is used to blend it on top of the current clip pixels. This mode is the inverse of BLEND_SOURCEOVER. | ||
BLEND_CLEAR = QPainter::CompositionMode_Clear, ///< The pixels in the lower layer are cleared (set to fully transparent) independent of the current clip. | ||
BLEND_SOURCE = QPainter::CompositionMode_Source, ///< The output is the current clip pixel. (This means a basic copy operation and is identical to SourceOver when the current clip pixel is opaque). | ||
BLEND_DESTINATION = QPainter::CompositionMode_Destination, ///< The output is the lower layer pixel. This means that the blending has no effect. This mode is the inverse of BLEND_SOURCE. | ||
BLEND_SOURCEIN = QPainter::CompositionMode_SourceIn, ///< The output is the current clip,where the alpha is reduced by that of the lower layer. | ||
BLEND_DESTINATIONIN = QPainter::CompositionMode_DestinationIn, ///< The output is the lower layer,where the alpha is reduced by that of the current clip. This mode is the inverse of BLEND_SOURCEIN. | ||
BLEND_SOURCEOUT = QPainter::CompositionMode_SourceOut, ///< The output is the current clip,where the alpha is reduced by the inverse of lower layer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so don't hate me... but I don't think we should piggyback directly off QPainter
like this.
It's not JUST because I'd rather avoid including <QPainter>
in one of our headers (though that's true, especially one that's loaded everywhere), it's also because tying our Blend mode values directly to their enum has a good chance of biting us later on. The FFmpeg code is a spaghetti mass of #ifdef
s to try and work around version changes in the underlying API, and I'd rather avoid ending up with the same thing here. Using QPainter::CompositionMode_*
directly means:
- If Qt ever changes their API, and either deprecates or alters these values, we have project files out "in the wild" that contain unsupported data which we might then have to special-case for.
- If we ever decide to handle blend-mode support differently, or if we switch away from Qt to something else entirely, then we have a bunch of project files out in the wild that contain data we don't have a definition for... or we're forced to continue importing
QPainter
just for its enum values even though we no longer use it. - If we define our own stable set of values, we have more freedom to deviate from Qt's definition. Like, we may want to drop some of these —
BLEND_CLEAR
, at least, seems questionable — and our Keyframe implementation isn't really compatible with value ranges that have holes in them.
So, I'd suggest dropping the assignments and just let the compiler number the set, same as the other enums:
enum BlendType | |
{ | |
BLEND_SOURCEOVER = QPainter::CompositionMode_SourceOver, ///< This is the default mode. The alpha of the current clip is used to blend the pixel on top of the lower layer. | |
BLEND_DESTINATIONOVER = QPainter::CompositionMode_DestinationOver, ///< The alpha of the lower layer is used to blend it on top of the current clip pixels. This mode is the inverse of BLEND_SOURCEOVER. | |
BLEND_CLEAR = QPainter::CompositionMode_Clear, ///< The pixels in the lower layer are cleared (set to fully transparent) independent of the current clip. | |
BLEND_SOURCE = QPainter::CompositionMode_Source, ///< The output is the current clip pixel. (This means a basic copy operation and is identical to SourceOver when the current clip pixel is opaque). | |
BLEND_DESTINATION = QPainter::CompositionMode_Destination, ///< The output is the lower layer pixel. This means that the blending has no effect. This mode is the inverse of BLEND_SOURCE. | |
BLEND_SOURCEIN = QPainter::CompositionMode_SourceIn, ///< The output is the current clip,where the alpha is reduced by that of the lower layer. | |
BLEND_DESTINATIONIN = QPainter::CompositionMode_DestinationIn, ///< The output is the lower layer,where the alpha is reduced by that of the current clip. This mode is the inverse of BLEND_SOURCEIN. | |
BLEND_SOURCEOUT = QPainter::CompositionMode_SourceOut, ///< The output is the current clip,where the alpha is reduced by the inverse of lower layer. | |
enum BlendType | |
{ | |
BLEND_SOURCEOVER, ///< This is the default mode. The alpha of the current clip is used to blend the pixel on top of the lower layer. | |
BLEND_DESTINATIONOVER, ///< The alpha of the lower layer is used to blend it on top of the current clip pixels. This mode is the inverse of BLEND_SOURCEOVER. | |
BLEND_CLEAR, ///< The pixels in the lower layer are cleared (set to fully transparent) independent of the current clip. | |
BLEND_SOURCE, ///< The output is the current clip pixel. (This means a basic copy operation and is identical to BLEND_SOURCEOVER when the current clip pixel is opaque). | |
BLEND_DESTINATION, ///< The output is the lower layer pixel. This means that the blending has no effect. This mode is the inverse of BLEND_SOURCE. | |
BLEND_SOURCEIN, ///< The output is the current clip,where the alpha is reduced by that of the lower layer. | |
BLEND_DESTINATIONIN, ///< The output is the lower layer,where the alpha is reduced by that of the current clip. This mode is the inverse of BLEND_SOURCEIN. | |
BLEND_SOURCEOUT, ///< The output is the current clip,where the alpha is reduced by the inverse of lower layer. |
...and so on for the rest of the enum list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I say "don't hate me" because this clearly required a lot of tedious effort. And here I come, cavalierly asking, "It's great, can you just change everything?" So... yeah, sorry about that.)
src/Enums.h
Outdated
BLEND_DESTINATIONOUT = QPainter::CompositionMode_DestinationOut, ///< The output is the lower layer,where the alpha is reduced by the inverse of the current clip. This mode is the inverse of BLEND_SOURCEOUT. | ||
BLEND_SOURCEATOP = QPainter::CompositionMode_SourceAtop, ///< The current clip pixel is blended on top of the lower layer,with the alpha of the current clip pixel reduced by the alpha of the lower layer pixel. | ||
BLEND_DESTINATIONATOP = QPainter::CompositionMode_DestinationAtop, ///< The lower layer pixel is blended on top of the current clip,with the alpha of the lower layer pixel is reduced by the alpha of the lower layer pixel. This mode is the inverse of BLEND_SOURCEATOP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the types that Qt had in CamelCase, because our enum labels are all-caps I think it's better to insert underscores between words, for readability. IOW, BLEND_DESTINATION_ATOP
, BLEND_SOURCE_OUT
, etc.
src/Enums.h
Outdated
@@ -13,6 +13,7 @@ | |||
#ifndef OPENSHOT_ENUMS_H | |||
#define OPENSHOT_ENUMS_H | |||
|
|||
#include <QPainter> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't be necessary here, if we let the enum assign new values. (The mapping to QPainter::CompositionMode_*
will happen in Clip.cpp
, where <QPainter>
is already included because we're using it there.)
src/Clip.cpp
Outdated
@@ -907,6 +935,7 @@ Json::Value Clip::JsonValue() const { | |||
root["anchor"] = anchor; | |||
root["display"] = display; | |||
root["mixing"] = mixing; | |||
root["blend"] = blend; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So with a Keyframe property, like the others you'd want to set this to the JSON representation of its current data:
root["blend"] = blend; | |
root["blend"] = blend.JsonValue(); |
src/Clip.cpp
Outdated
if (!root["blend"].isNull()) | ||
blend = (BlendType) root["blend"].asInt(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this becomes...
if (!root["blend"].isNull()) | |
blend = (BlendType) root["blend"].asInt(); | |
if (!root["blend"].isNull()) | |
blend.setJsonValue(root["blend"]); |
src/Clip.cpp
Outdated
@@ -761,6 +762,7 @@ std::string Clip::PropertiesJSON(int64_t requested_frame) const { | |||
root["scale"] = add_property_json("Scale", scale, "int", "", NULL, 0, 3, false, requested_frame); | |||
root["display"] = add_property_json("Frame Number", display, "int", "", NULL, 0, 3, false, requested_frame); | |||
root["mixing"] = add_property_json("Volume Mixing", mixing, "int", "", NULL, 0, 2, false, requested_frame); | |||
root["blend"] = add_property_json("Blend Mode", blend, "int", "", NULL, 0, 23, false, requested_frame); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would move down into the Keyframe property section and become...
root["blend"] = add_property_json("Blend Mode", blend, "int", "", NULL, 0, 23, false, requested_frame); | |
root["blend"] = add_property_json("Blend Mode", blend.GetValue(requested_frame), "int", "", &blend, 0, 23, false, requested_frame); |
src/Clip.cpp
Outdated
// Add video blend choices (dropdown style) | ||
root["blend"]["choices"].append(add_property_choice_json("Source Over", BLEND_SOURCEOVER, blend)); | ||
root["blend"]["choices"].append(add_property_choice_json("Destination Over", BLEND_DESTINATIONOVER, blend)); | ||
root["blend"]["choices"].append(add_property_choice_json("Clear", BLEND_CLEAR, blend)); | ||
root["blend"]["choices"].append(add_property_choice_json("Source", BLEND_SOURCE, blend)); | ||
root["blend"]["choices"].append(add_property_choice_json("Destination", BLEND_DESTINATION, blend)); | ||
root["blend"]["choices"].append(add_property_choice_json("Source In", BLEND_SOURCEIN, blend)); | ||
root["blend"]["choices"].append(add_property_choice_json("Destination In", BLEND_DESTINATIONIN, blend)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And then this whole section moves down below that (so that root["blend"]
is already defined before we start adding choices). But other than moving it the only change needed is that all of the blend
third arguments would become blend.GetValue(requested_frame)
instead.
src/Clip.cpp
Outdated
painter.setCompositionMode(QPainter::CompositionMode_SourceOver); | ||
painter.setCompositionMode((QPainter::CompositionMode) blend); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, here's where you'd need to make the last change. Instead of passing the value of blend
to the QPainter instance directly, you'll need to map it from our enum to theirs.
Probably easiest to write a small utility method for this, but ultimately up to you. But assuming you do write one, let's call it
QPainter::CompositionMode Clip::blendToQPainter(openshot::BlendMode blend) {
// ...
}
There's a bunch of different ways you can do the mapping (including building a std::map<openshot::BlendMode, QPainter::CompositionMode>
to index the Qt modes by our enum), but there are some advantages to sticking with the basics.
If you use a basic switch(blend)
to drive the conversion, and you don't give it a default:
branch, then the compiler will (with the right settings) emit a compile-time warning if any of the BlendMode
enum values are missed by the switch
statement. (So, you should include a case for BLEND_SOURCE_OVER
explicitly, even though it's the default. Just to silence those warnings.)
The compiler's monitoring of switch
usage becomes especially handy further down the road, when someone decides to change or add elements to the enum
, and forgets to update the code that uses it.
But basically that's it, just fill it with a bunch of cases like
case BLEND_DESTINATION_ATOP:
painter_mode = QPainter::CompositionMode_DestinationAtop;
break;
case BLEND_HARD_LIGHT:
painter_mode = QPainter::CompositionMode_HardLight;
break;
Lather rinse, repeat, and finally return painter_mode;
. The return value of blendToQPainter()
you could even pass directly to painter.setCompositionMode()
— you don't really need it for anything else. But that'a entirely up to you.
Re: tests... tests would be good, for sure, if you're willing to work on them. They certainly don't have to be exhaustive, but at least a couple that confirm the basic modes are working as expected would be good for peace of mind. (This PR may also crater coverage without it... though, I don't actually think so, since all of the enum values are accessed by Lines 1027 to 1028 in 0d1ae39
Anyway, as far as writing tests, the existing My knee-jerk thought is that the tests should go into Wherever they get put, in spirit the tests would be a hybrid of the existing Timeline and ChromaKey tests. Actually, for starters you'd want to test any conversion method(s) you add to the Clip class, since it's easy to whip one of those off. If there are any issues with the conversion, they'll be the source of hard-to-find bugs later on. (And there you have it, the entire argument for unit-testing condensed into an easy-to-swallow pill form.) For the blend tests, you'd start off like the
I would strongly suggest constructing test data more like the ChromaKey tests, though:
|
I have no idea why the CI jobs are all failing, but it's some sort of Github Actions issue. They're not even getting far enough into the process to load the repo code, never mind compile it. I'm sure it'll clear up shortly. |
We seem to be back in business. |
Codecov Report
@@ Coverage Diff @@
## develop #791 +/- ##
===========================================
+ Coverage 48.92% 49.04% +0.12%
===========================================
Files 184 184
Lines 15815 15733 -82
===========================================
- Hits 7738 7717 -21
+ Misses 8077 8016 -61
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Thanks @nilp0inter for working on this. Really look forward to seeing this added as a new feature. |
Thank you @ferdnyc for the thorough review, it is really appreciated. I'll be back working on this over the weekend. |
@nilp0inter Now that I've finally fixed all of the dumb typos and Qt versioning vagaries that were preventing it from passing CI, my
(One caveat: When I say " |
Any update on this one? Would be a great feature! |
0d1ae39
to
9451380
Compare
@jeffski , thank you for the remainder. I absolutely forgot about this. I am updating my branch and working on the proposed changes right now. |
@ferdnyc , I've changed the code according to your comments (thank you very much for the detailed explanation, btw) and tested it manually using openshot-qt. Everything appears to be working. Regarding the tests, I'd like to give it a try, and will do in the following days. Please, comment if you see something wrong with the actual implementation. |
Merge conflicts have been detected on this PR, please resolve. |
This pull request implement blend modes as per #320 .
I tested it manually and it seems to work OK. I'd like to add some automated tests, but I'd need some guidance to do it (I am not a C++ programmer).
Thanks for the great software!