Discussion about v2.0 API and architectures changes

[OsaAjani]

Making suite to @keikoro suggestion in issue #1874, I open this issue to be the place we can discuss the changes (general as well as specific implementations) we want to see for v2.0 API and architecture.

If you was part of technical discussions for v2.0 API, please come here: @Zulko @mgaitan @tburrows13 @davidbernat

[Zulko]

Won't have time to write more before the weekend but in a nutshell:

• I like what you're proposing for class-based effects. I agree that it is cleaner although slightly more work to write an effect.
• I'm not sure I like that a function called with_effect can accept a list as well as an effect. I would rather have it called with_effects and if you only have a single effect you pass a single-element list. I tend to prefer having a single type of input now.
• Regarding your idea to do more in __init__(), I would say avoid having logics inside __init__(). Yes it is a lot of boilerplate to be using init just to store parameters, but it's clean to think that an effect is just a description of specs, until it is applied to a clip.
• Idea which can be for later: with this new API you can probably get rid of clip.afx (which was for audio effects) and have all effects be applied via clip.with_effects([BlackAndWhite(), VolumeX(), WithEcho(), etc.])

As you said there is a risk of scope-creep in the work to do towards v2.0. What I would suggest is you do as much as you feel. When you have something you're happy with, submit an MR and let's push it. In the spirit of "shipped is better than perfect", we could even "launch" v2.0 without a fully definitive API, and that we could consider v2.1 as the first stable version of v2).

I'll shorten this issue's name so it's easier to follow in emails.

[OsaAjani]

Hi @Zulko, if you just have the time for this one question. What of this two solutions would you prefer ?

1. Auto-adding all effects as method to VideoClip and AudioClip at runtime.
2. Only adding a hand-few of absolutely essentials effects to VideoClip and AudioClip manually.

I would probably advocate for 2 if we only have 3-5 essentials, for more I would go for 1.

If 2, what would you consider to be absolutely essentials effects in the current list ? resize and crop I guess, anything else ?

In both case, what should be the name of those methods. I have 3 suggestions:

• A) The name of the effect in snake case (for example, effect MultiplySpeed would be accessible as clip.multiply_speed).
• B) The name of the effect in snake case, but preceded by with_ (for example, effect MultiplySpeed would be accessible as clip.with_multiply_speed).
• C) The name of the effect in snake case, but preceded by fx_ (for example, effect MultiplySpeed would be accessible as clip.with_fx_multiply_speed).

If 2, I would personally advocate for going with B.
If 1, I would prefer C, maybe even with_fx, as I would prefer to keep a clear distinction between automagically added filter methods and real methods.

[OsaAjani]

I'm not sure I like that a function called with_effect can accept a list as well as an effect. I would rather have it called with_effects and if you only have a single effect you pass a single-element list. I tend to prefer having a single type of input now.

Yeah, kind of unhappy with the single nature of the name too, but after using it (I had to write quite a few informal test to see if my migration of effect worked as expected), I feel like supporting a list as well as a single effect is really beneficial in terms of readability, both when calling one/multiple effect(s).

Honestly, supporting both is very easy, and because I do type hinting every-time I can, it makes it ultra clear for the user and the IDEs.

This is how it's done:

    def with_effect(self, effects: Union['Effect', List['Effect']]):
        """Return a copy of the current clip with the effects applied

        >>> new_clip = clip.with_effect(vfx.Resize(0.2, method="bilinear"))

        You can also pass multiple effect as a list

        >>> clip.with_effect([vfx.VolumeX(0.5), vfx.Resize(0.3), vfx.Mirrorx()])
        """
        if not isinstance(effects, list) :
            effects = [effects]

        new_clip = self.copy()
        for effect in effects :
            # We always copy effect before using it, see Effect.copy
            # to see why we need to
            effect_copy = effect.copy()
            new_clip = effect_copy.apply(new_clip)

        return new_clip

Regarding your idea to do more in __init__(), I would say avoid having logics inside __init__(). Yes it is a lot of boilerplate to be using init just to store parameters, but it's clean to think that an effect is just a description of specs, until it is applied to a clip.

Yeah, I agree it seems to be the logic in the Python world (I'm coming from PHP, where construct is more frequently used for that). Anyway, I've been mostly using @dataclass for effects, and it makes it a lot simpler and more readable. And if for a reason or another dataclass is not practical for a particular effect, then we just go and write a good old __init__.

Idea which can be for later: with this new API you can probably get rid of clip.afx (which was for audio effects) and have all effects be applied via clip.with_effects([BlackAndWhite(), VolumeX(), WithEcho(), etc.])

Yep, it's already done ^^. Basically, all audio effects can be applied onto VideoClip and only modify audio with the proper decorator, so I also felt like afx was not needed anymore.

As you said there is a risk of scope-creep in the work to do towards v2.0. What I would suggest is you do as much as you feel. When you have something you're happy with, submit an MR and let's push it. In the spirit of "shipped is better than perfect", we could even "launch" v2.0 without a fully definitive API, and that we could consider v2.1 as the first stable version of v2).

I like that !!!

I will finish the adding of effect shortcuts into clip as soon as I have your answer, then I will freeze further change of the API and call it "definitive" ^^. Then I will update unit tests (while keeping all ropes and stools out of hand), and finish updating the doc. And finally, I can do the MR...

I would also like to add a way to tests all docs examples automatically, as well as update the docker so we can have a shared environment for testing.

I hope I can do all of that in the next 1 to 2 weeks. After that I will have to wrap it up anyway.

[Zulko]

Regarding the core effects, I think adding a handful of effects manually is the way.

Some really core effects can have their own short name so they flow well: clip.cropped().rotated().resized()

For other effects, I would say anything that shows that it is outplace (e.g. starting with with_) and that flows well in plain English with_volume_multiplier(), with_speed_multiplier(). Maybe these 5 effects are already enough? I have used fadein, margin and other effects a lot, but wouldn't mind using them as effect objects if need be.

Regarding with_effect, I still think the function should be called with_effects, especially if the internal parameter is called effects. And I would only accept lists. I understand that it's not technically complicated to support both lists and effects, but it will still be

• more explanations to write in the docstring
• more to learn from the user
• more user failures (users who provide a tuple instead of a list and then you get a list of a tuple and then the error makes no sense, etc.)
• more tests to write to catch all cases maybe?
• more confusion for IDEs who analyse types.

Each of these points are light and not very important, but they add up, and in general I have mostly regretted using mixed-type inputs.

For the rest, good calls 👍

[Zulko]

kind of missed that but ideally we wouldn't need effect_copy = effect.copy(), effects would just be a description of specs and an apply recipe to make the effect happen, but they would be immutable, with no internal state, and so no need for copy.

[OsaAjani]

Thanks for taking the time @Zulko :)

Some really core effects can have their own short name so they flow well: clip.cropped().rotated().resized()

I would have liked clip.with_crop().with_rotate().with_resize() better. It flows a little less well, I admit, but it keep the API consistent and it makes it easier to document (the user just have to look at methods starting by with_ and he get all core modifications and only have to remember the one rule, if with_ then out-place).

This way we only have one path for the user to modify a clip, through the usage of with_* methods.

Maybe these 5 effects are already enough?
Yep, seems good to me, those are indeed very core functionalities that are enough when combine with with_subclip to do entire videos not needing transitions.

Regarding with_effect, I still think the function should be called with_effects, especially if the internal parameter is called effects. And I would only accept lists. I understand that it's not technically complicated to support both lists and effects, but it will still be [...]

Okay, I will go with with_effects then. I do agree with a lot of your points, I just liked the fact not having square brackets for one effect only made it more readable. Also, I do agree in the long run mixed-type almost always add burden for maintaining.

kind of missed that but ideally we wouldn't need effect_copy = effect.copy(), effects would just be a description of specs and an apply recipe to make the effect happen, but they would be immutable, with no internal state, and so no need for copy.

That would be in an ideal world indeed, but truth is: working with effect without modifying internal state make things very hard and unintuitive. Because we set effect params on effect instantiation, but clip properties only become accessible on call to apply, we frequently tend to update effect params with default value to a real value coming from clip.

We could avoid doing so by only using local variables and never touch the internal, but it would result in lot of boilerplate and would probably increase usage of closure and other kind of "advanced" techniques that make things less readable (and of course writable) for the average user.

Even though we could consider enforcing strict rules internally to prevent side effect when writing internal code, we really can't assume the end users will do the same when writing custom effect, even if we put it in the doc in red glowing letters.

Trust me on that one, this one line of code will save us hundred of hours of fastidious debug and seemingly inexplicable issues in the long run.

[Zulko]

I would have liked clip.with_crop().with_rotate().with_resize() better.

You are right regarding consistency, but core-core features can be allowed to be a bit irregular. My thinking here is that core examples will be shorter/nicer (which is petty, but counts). Side note that with_resizing or with_resized_frames would be more "plain english" than with_resize.

working with effect without modifying internal state make things very hard and unintuitive

I would say follow your heart, this part of the implementation doesn't affect the user experience and so can be revisited later. If the idea is to allow clips to be re-evaluated in presence of a clip, then I would suggest the following:

def with_effects(effects):
    new_clip = clip.copy()
    for effect in effects :
            updated_effect = effect.updated_by_clip(new_clip)
            new_clip = updated_effect.apply( updated_effect)

This way:

• Effects are actually immutable.
• Effects can get copied with updates via updated_by_clip. This methods is a no-op (return self) in the base effect class, so that most effects don't even need to worry about it.
• The method apply can focus on modifying a clip, not its effect (no side-effects, ahah) unless super-necessary.

[OsaAjani]

Okay, I will go with the short name then.

If we want to go back to with_effects we can indeed do that later. But I really think just copying the effect will be the simplest/most natural way to achieve consistent behavior when reusing an effect.

[Zulko]

Do it your way 👍

[OsaAjani]

Okay, it's done ! I will go back to documentation and probably a lot of unit tests fixing ^^. If anyone want to have a peak at the new system, you can find the current state of the code at https://github.com/OsaAjani/moviepy

[Zulko]

I had a look at the effects, looking good 👍 One interesting thing to try to get an idea of the new API, would be to update the examples in the example folder (maybe only the ones that are reproducible! Dusting off the 10-year-old examples might be a rabbit hole we don't want to get into now).

[OsaAjani]

Yeah I thought about the examples too, but I will not have the time to update them to the new API. The new documentation have quite more code examples along the road though.
For now I'm writing the introduction tutorial to moviepy.

The goal would be to have something more like the panda doc. With a getting started with install, main concept and presentation if moviepy and a 10 minutes tutorial for the users who want to have a rapid understanding of the basis. Then a user guide with more in depth explanation of the different objects, etc. For the users who want to dig deeper and better understand. And finally the api reference with the doc of the function for full understanding.

[davidbernat]

Reading along after a few days of development on my side projects testing speed effects of FFMPEG and similar video tools. Impressive work @OsaAjani, and thank you @Zulko, for driving things so strong forward.

A few comments about Readability, and about Scope Creep:

1. Please consider that using .with_effect() that only accepts one effect and returns a clip is generally a superior super-structure of code. It is slightly verbose chaining together multiple effects, but providing them each on new lines is very, very clear to the reader what is happening. More important, the inside code, and the inheritance models usually stay much simpler. Most importantly, the less "spaghetti glue" code for handling different inputs, the easier this is to maintain across the package when random engineers read the code down the line. I strongly prefer only accepting one effect and CamelCase, as the CamelCase implies Object Orientation encapsulation is atomic and interchangeable across types.
2. I strongly agree with @Zulko about not having logic inside init. Same reasons. (And, to be embarrassingly frank, every time I do do it, I expand the code six months later and need to remove it.)
3. "Some really core effects can have their own short name so they flow well: clip.cropped().rotated().resized()" <== I very strongly disagree with this choice. It implies two tiers of "essential" functionality acting on Clips. Why is cropped(), a spatial function, superior to bw() an equally simple color function? Truth is, just because of convention wandering around other packages. Trust the user to apply the Effect is my opinion, or, mandate that every Effect have a "method name" and create those dynamically (which generally is frowned upon).
4. I would be delighted if we were to state which effects and features are "in scope" v2.01 for @Zulko and which are not. It would help me and other readers get a handle on what @Zulko had in mind when he was "creating" advanced utilities.

@OsaAjani Do you need me to read something tomorrow morning? Some code? Documentation?
Want me to go through all the Effects and see whether I can organize something for you? I have time.