High level interface

[AdrianEddy]

I'm designing a high level plugin interface for the bindings, and my general idea is to have something like:

struct PortablePlugin { }
impl AdobePlugin for PortablePlugin {
    fn can_load(_host_name: &str, _host_version: &str) -> bool { true }
    fn new() -> Self { Self {} }

    fn handle_command(&mut self, cmd: Command) {
        match cmd {
            Command::About { mut out_data, .. } => {
                out_data.set_return_msg("Test plugin."));
            }
            Command::Render { in_data, mut params, output, .. } => {
                // ...
            }
            Command::GpuDeviceSetup { in_data, mut out_data, extra/*: GPUDeviceSetupExtra*/ } => { ... }
            // ...
        }
    }
}

register_plugin!(PortablePlugin);

basically the idea is to only expose relevant and valid data in each command, including all extras properly casted. I have that implemented so far, but I have a few concerns I wanted to discuss:

Struct instances

1. Should we create and maintain global instance for user? What I imagine is that PortablePlugin struct would be stored in global_data and would be a single object alive for the entire time the project is open (and any plugin instance is added to the layer)
2. Should we create and maintain sequence instance for user? That would be a separate struct, which is unique per every layer where the plugin is applied. This would be stored in sequence_data and I imagine I would require the user to make this struct serde::Serialize and serde::Deserialize and automatically handle flattening the data with bincode. Also that struct should be Send + Sync I believe, because AE can call the commands from any thread at any time? Is that correct?

I believe these two points are very useful for almost all plugins and at the same time quite time-consuming to implement properly by the user. Pros:

• User can focus on just his actual plugin code without worrying about complicated details of Handles and flattening
• Much easier to get started with writing plugins

Cons:

• A bit of memory overhead, since the instances would be always created. But since we're talking about video processing plugins here, any overhead like that is nothing compared to actual pixels processing so it should be fine. We could maybe make this optional with a register_plugin macro parameter or something

Parameters

1. What do you think to create something like ParameterRegistry, which would be a manager for user params, with the enum-based getter/setter provided by user, and all parameters would be defined and retrieved using this enum. Something like:

   enum MyParams {
   MixChannels
   };
   fn handle_command(&mut self, cmd: Command) {
   match cmd {
       Command::ParamsSetup { mut params } => {
           params.add_param(MyParams::MixChannels, 
               "Mix channels", 
               ae::FloatSliderDef::new()
                   .set_value(10.0)
                   .precision(1)
                   .display_flags(ae::ValueDisplayFlag::PERCENT)
           );
       }
       Command::Render { mut params, .. } => {
           let slider = params.get::<ae::Param::FloatSlider>(MyParams::MixChannels);
           // slider.value()
       }
   }
   }
   register_plugin!(PortablePlugin, MyParams);

   That struct would maintain params array, indexes and counts (and would be stored in global instance data), so users won't have to worry about it.

I'll have more things to discuss once I get further, but let's start with these.

Let me know your thoughts, thanks!

CC @virtualritz @mobile-bungalow

[mobile-bungalow]

To add to the cons of struct instances for global, sequence, and frame data as well as simulation cache.

3.) Complex trait bounds: There might not be a reasonable set of trait bounds that can be applied to all instances of any of these associated types. For instance, should global data be Send + Sync? Likely yes if MFR is enable, but in many other cases no.

4.) using DST's is necessary for serializing text or other file formats to disk so Enums are out, this likely means that we will need both SequenceData and SequenceDataFlat associated types and methods for getting and setting each on our abstraction.

These both seem to be indicators of a premature abstraction, However, maintaining a memory and type safe interface without the associated types would be hard. I Feel find constraining the trait bounds on global and sequence data to the more stringent multithreaded use cases, simply because allowing for trait bounds based on out_flags and out_flags2 seems very opaque to the user and would likely rely on build script or codegen trickery.

On parameter registration.

A registry is a good idea, something to help the user maintain consistency in parameter layouts between versions is ideal. Consider that parameters might be created dynamically in a loop, and renamed and revealed based on user input. I think providing a slice of wrapper enums and allowing the user to user constant or dynamic offsets if that suits their use case might be wise.

[AdrianEddy]

3.) Complex trait bounds: There might not be a reasonable set of trait bounds that can be applied to all instances of any of these associated types. For instance, should global data be Send + Sync? Likely yes if MFR is enable, but in many other cases no.

Right, if MFR is enabled then global data also needs to be Send + Sync. However, since we have the flags in the build script, we can require that trait only when there's OutFlags2::SupportsThreadedRendering in PiPL. Besides, if user won't use any specific global data, he doesn't have to care about it, and when he does and enables MFR, then he needs to ensure his types are safe anyway, so it's better to have that handled for him by the bindings. In any case, there are certain requirements for the plugin imposed by Adobe, and I feel like Rust as a language is able to express and enforce them, versus expecting the user to write correct and thread-safe C code

4.) using DST's is necessary for serializing text or other file formats to disk so Enums are out, this likely means that we will need both SequenceData and SequenceDataFlat associated types and methods for getting and setting each on our abstraction.

Hmm, not sure if I understand this one. Wouldn't the requirement for the user sequence struct to be serde::Serialize and serde::Deserialize handle everything? If there's anything that's impossible to serialize he can always add #[serde::skip] or add #[serde::serialize_with="custom_func"] and handle all edge cases like that?

Also, isn't sequence data always flattened and restored per thread? I mean that the calls done by AE from different threads don't actually use the same pointer, but rather create a copy (using flatten/un-flatted) per every thread? In this case the user's SequenceData struct wouldn't have to be Send+Sync at all, since all threads would have it's own copy. I haven't confirmed this yet though, so not sure if that's the case

[virtualritz]

Also, isn't sequence data always flattened and restored per thread? I mean that the calls done by AE from different threads don't actually use the same pointer, but rather create a copy (using flatten/un-flatted) per every thread? In this case the user's SequenceData struct wouldn't have to be Send+Sync at all, since all threads would have it's own copy. I haven't confirmed this yet though, so not sure if that's the case.

I don't think so. See the resp. Compute Cache section in the Ae SDK docs. Quote:

Without the Compute Cache, the effect will need to add the PF_OutFlag2_MUTABLE_RENDER_SEQUENCE_DATA_SLOWER flag which will create unique copies of sequence_data per render thread. Each render thread may then need to perform the time-consuming calculations independently and won’t be able to share the results between the render threads.

However, I will ask on the AE SDK mailing list if this could be handled in a third way, by keeping our own mutul acces primitive behind the Rust API that exposes sequence data. I would think this is an option and they just do not mention it since it's a PITA to do in C/C++.

[AdrianEddy]

So I've implemented most of what I described and reduced the portable example to this:

use after_effects as ae;
use after_effects_sys as ae_sys;
use ae::register_plugin;

fn detect_host(in_data: ae::InData) -> String {
    let app = ....;

    format!("Running in {app}")
}

#[derive(Eq, PartialEq, Hash)]
enum PortableParams {
    Slider,
}

#[derive(Default)]
struct PortablePlugin { }

#[derive(Default, serde::Serialize, serde::Deserialize)]
struct PortableInstance { }

impl AdobePluginGlobal for PortablePlugin {
    fn can_load(_host_name: &str, _host_version: &str) -> bool {
        true
    }

    fn params_setup(&self, params: &mut ae::Parameters<PortableParams>) -> Result<(), Error> {
        params.add_param(PortableParams::Slider, "Mix channels", *ae::FloatSliderDef::new()
            .set_valid_min(0.0)
            .set_slider_min(0.0)
            .set_valid_max(200.0)
            .set_slider_max(200.0)
            .set_value(10.0)
            .set_default(10.0)
            .precision(1)
            .display_flags(ae::ValueDisplayFlag::PERCENT),
        );
        Ok(())
    }

    fn handle_command(&self, cmd: ae::Command, in_data: ae::InData, mut out_data: ae::OutData) -> Result<(), ae::Error> {
        match cmd {
            ae::Command::About => {
                out_data.set_return_msg("Portable, v3.3\rThis example shows how to detect and respond to different hosts.\rCopyright 2007-2023 Adobe Inc.");
            }
            ae::Command::SequenceSetup => {
                out_data.set_return_msg(&detect_host(in_data));
            }
            _ => { }
        }
        Ok(())
    }
}

impl AdobePluginInstance for PortableInstance {
    fn flatten(&self) -> Result<Vec<u8>, Error> {
        Ok(bincode::serialize(self).unwrap())
    }
    fn unflatten(bytes: &[u8]) -> Result<Self, Error> {
        Ok(bincode::deserialize::<Self>(bytes).unwrap())
    }

    fn render(&self, in_data: ae::InData, in_layer: &ae::Layer, out_layer: &mut ae::Layer, params: &ae::Parameters<PortableParams>) -> Result<(), ae::Error> {
        let slider_value = if let Some(ae::Param::FloatSlider(slider)) = params.get(PortableParams::Slider) {
            slider.value()
        } else {
            0.0
        };

        // If the slider is 0 just make a direct copy.
        if slider_value < 0.001 {
            out_layer.copy_from(in_layer, None, None)?;
        } else {
            let extent_hint = in_data.extent_hint();
            let out_extent_hint = out_layer.extent_hint();
            // clear all pixels outside extent_hint.
            if extent_hint.left != out_extent_hint.left
                || extent_hint.top != out_extent_hint.top
                || extent_hint.right != out_extent_hint.right
                || extent_hint.bottom != out_extent_hint.bottom
            {
                out_layer.fill(Pixel::default(), Some(out_extent_hint))?;
            }

            // iterate over image data.
            let progress_height = extent_hint.top - extent_hint.bottom;
            in_layer.iterate(out_layer, 0, progress_height, extent_hint, |_x: i32, _y: i32, pixel: ae::Pixel| -> Result<ae::Pixel, Error> {

                // Mix the values. The higher the slider, the more we blend the channel with the average of all channels
                let average = (pixel.red as f64 + pixel.green as f64 + pixel.blue as f64) / 3.0;
                // let midway_calc = (slider_value * average) + (200.0 - slider_value) * pixel.red as f64;

                Ok(ae::Pixel {
                    alpha: pixel.alpha,
                    red:   (((slider_value * average) + (100.0 - slider_value) * pixel.red   as f64) / 100.0).min(ae_sys::PF_MAX_CHAN8 as f64) as u8,
                    green: (((slider_value * average) + (100.0 - slider_value) * pixel.green as f64) / 100.0).min(ae_sys::PF_MAX_CHAN8 as f64) as u8,
                    blue:  (((slider_value * average) + (100.0 - slider_value) * pixel.blue  as f64) / 100.0).min(ae_sys::PF_MAX_CHAN8 as f64) as u8,
                })
            })?;
        }

        Ok(())
    }

    fn handle_command(&mut self, cmd: ae::Command, in_data: ae::InData, _out_data: ae::OutData) -> Result<(), ae::Error> {
        Ok(())
    }
}

register_plugin!(PortablePlugin, PortableInstance, PortableParams);

This already works, but I'm still working on polishing the api implementation

I'm leaning towards having different trait methods like render() on the AdobePluginInstance trait instead of Command enum, because that command is always relevant to a particular plugin instance (kept in sequence_data), and we can enforce const or mut access if it's a method. I figured out a way to make the trait dynamic, for example fn smart_render() method is only required if PiPL in build.rs has OutFlags2::SupportsSmartRender, fn do_dialog() is only required if PiPL has OutFlags::IDoDialog etc, so this will be additionally enforced by the compiler depending on your flags

Also we need to hand over flattening to the user (through flatten/unflatten methods), because using bincode for all will break any existing projects if you simply add a new struct field, so users should have full control over such scenario

Also, if you pass () instead of PortableInstance to register_plugin, sequence_data will not be allocated or handled for you at all.

There's a slight problem with using pf::Handle, because the whole point of it is to make AE in charge of the memory, but due to the fact that Rust doesn't have placement-new, Rust always allocates the struct memory first, before copying it to AE-managed memory region, so it's not great. But I guess user structs won't be too large, and we'll document that

[virtualritz]

This looks awesome. I especially like the closure stuff. So very Rust.

Regarding naming: two things. I think I used set_*() a lot (because I was Rust n00b) but most builders now use with_. It may also be possible to use the init struct pattern (or offer it alternatively through a with_config() method).

And I also think we should ruthlessly oxidize all names, including method names. The Ae SDK has a lot of API cruft.

[AdrianEddy]

I think I have wrong understanding of sequence_data. I thought the purpose of this pointer is to have a data associated with the plugin instance added to a clip. Since it is saved with the project and is sent to all commands, that made sense to me. But I observed something different and looking at sources of some other plugins on github, I think I just don't understand what it's for: What I've observed is the following:

1. I'm allocating the sequence_data in SEQUENCE_SETUP
2. AE calls SEQUENCE_FLATTEN and SEQUENCE_RESETUP to create a second instance of the data
3. I'm getting USER_CHANGED_PARAM call with the pointer to the second instance
4. I'm getting RENDER call with the pointer to the first instance. Therefore, any data I modify in the struct when USER_CHANGED_PARAM is not getting sent to RENDER and there are two separate pointers alive even with only one plugin instance on a single clip.

Do you have any insights about that behavior? Where should I keep the plugin internal state related to a particular instance? Is that the job for ParamArbitrary?

I understand that I should check out the params value in FRAME_SETUP or PRE_RENDER, but I'm talking about cases where I want to load some resource from disk selected by the user (I get USER_CHANGED_PARAM when user clicks the button)

[AdrianEddy]

it turned out that in order to keep sequence_data up-to-date, you have to set PF_OutFlag_FORCE_RERENDER in USER_CHANGED_PARAM, and that makes AE call SEQUENCE_FLATTEN and SEQUENCE_RESETUP again to get an updated clone of the data. Looks like the sequence data clone is kept for rendering and IS NOT updated if you change the params in the UI or do anything unless you set PF_OutFlag_FORCE_RERENDER to force it to discard these copies and create new ones

This does make sense, but I didn't find this behavior documented anywhere

[virtualritz]

This does make sense, but I didn't find this behavior documented anywhere

Welcome to the Ae SDK docs. ;)

[virtualritz]

I believe to recall that the idea was that you can use other ways to sync plug-in state with widgets. Or go through sequence_data.

But I have to look at the old AtomKraft codebase or the Rust plug-ins I wrote, to confirm.

It's been two years since I touched Ae plug-in code.

[AdrianEddy]

any idea why I'm getting Unsupported effect control when adding PF_Param_ARBITRARY_DATA?

[virtualritz]

Nope. I have a custom-built Ae curve widget somewhere that is 100% Rust. I.e. something that looks and feels like the one in the Ae Curves filter.

Screenshot is in my uniform-cubic-splines crate.

Would that help you?

[AdrianEddy]

@mobile-bungalow Now it's a good time to start porting your plugin to the new rust API - it seems to work well with the examples I ported, and most of the suites are already wrapped. It'd be good to hear feedback of porting a real plugin before I merge this new API and we publish a new official version.

There's still quite some work to do, but it's nice already

[mobile-bungalow]

Oh wow great news. I will 100% do this.

[AdrianEddy]

Ok so next steps for me are:

• [x] Port the ColorGrid example to verify the ArbitraryData params handling
• [x] Add generic type to ArbitraryData and reduce serializations/deserializations
• [x] Fix Custom UI not working in Premiere
• [x] Add a "safe_mode" where the EffectMain is wrapped in panic::catch_unwind - this currently doesn't work, because the sequence_data handling breaks if the stack unwinding happens, but I'll fix it
• [ ] Finish the Gyroflow plugin, and add any fixes/changes if needed
• [ ] Write basic overview and getting started in README
• [ ] Document some most important functions
• [ ] Release v0.2

The API won't change much, so I think it's a good time to start reviewing this. @virtualritz how would we approach that? Do you even have time to check this, or you just trust me to merge this and release? Do you plan to port any of your existing plugins?

I never thought I'd say this, but wrapping all the suites is actually realistic, there's not that much left. I'm not gonna do this, but we're pretty close

[AdrianEddy]

@virtualritz what's the reason to use FlatHandle in ArbitraryData? Shouldn't it be always typed, using Handle<T>? and serialized only on PF_Arbitrary_FLATTEN_FUNC. Right now it's constantly serializing and deserializing, instead of just using the memory directly. Can I change it to Handle<T>?

Also, is value_owned() actually useful anywhere? Shouldn't the value memory be always managed by AE, and disposed only in PF_Arbitrary_DISPOSE_FUNC?

[AdrianEddy]

@virtualritz so I would like to propose these changes to ArbitraryData: https://github.com/AdrianEddy/after-effects/commit/bbeb3c0d21d6eab6cdb66b9bf7d4a165f6f5f70a

It reduces serializing/deserializing a lot and operates directly on the object memory It also fixes handling ArbitraryCallback when there are more than one params with ArbitraryData

The .value::<T>() interface is especially nice for the end user and it operates on the pointer/reference directly without copying, while handling locking and unlocking automatically

[virtualritz]

https://github.com/AdrianEddy/after-effects/commit/bbeb3c0d21d6eab6cdb66b9bf7d4a165f6f5f70a LGTM!

[AdrianEddy]

And in the debug mode, I propose to catch and log panics like this: https://github.com/AdrianEddy/after-effects/commit/301d67eff00f8ee8ac297b50eedc3f917c487ef7 System logger is also automatically set up in debug mode, using either DbgView on Windows or Console on macOS

[AdrianEddy]

We now have a wgpu example with a shader written in Rust, yay!

This brings us to 14 total examples

[AdrianEddy]

I spent the entire day today chasing after a very peculiar heap corruption. It manifested itself only in a specific case of drawing the custom UI with ArbitraryData params and doing MFR Smart Renders, and I couldn't reproduce it with any of the simple examples, while my plugin was seemingly not doing anything more than the examples

I finally figured out it was due to the global HashMap of Params, which was Rc<RefCell<>>, but used in the global_data pointer. Since the HashMap is only ever written to in ParamsSetup, and all other calls are just reading, I thought just calling .borrow() would be fine. It turned out that just calling .borrow() when there's any multi threading involved, while it's obviously my mistake and it's not correct, it didn't cause any compiler error, any panic or anything easily debuggable (under debugger). It corrupted the heap randomly and it was very hard to chase.

Anyway, it's fixed now and I just wanted to share this interesting bug hunting, maybe it will save you some time if you ever hit something similar

[mobile-bungalow]

My plugin is now ported! I will be cleaning up and publishing asap. Working with the new API was an absolute pleasure. my notes so far.

• I basically copy pasted Adriens gpu interop code, and found copying buffers to and from ram led to significantly faster render times on both macos and windows. I also noted that all gpu buffers are RGBA f32 format, as opposed to the variable color depth formats in ram, which was an interesting surprise on adobes part. As such I have decided to omit the interop code. This might be an implementation bug on my part, but I would be unsurprised if adobe needed to do a lot of extra shuffling behind the scenes to get their data in and out of metal and cuda buffers.

  • Popup params should not be modifiable at run time. This is a bizarre one, but after effects expects the '|' delimited string to only be assigned in param setup, , we could enforce this by adding an extra mutable out variable in the popup constructor and removing the options builder from the popupdef.

[AdrianEddy]

@mobile-bungalow nice! I took a quick look and noticed these:

1. https://github.com/mobile-bungalow/tweak_shader_ae_rs/blob/main/src/param_util.rs#L334-L349 this can be simplified like this: https://github.com/AdrianEddy/after-effects/blob/master/examples/supervisor/src/lib.rs#L320-L339
2. Instead of u8 as Params, you can use enum like this:

   pub enum Params {
   LoadButton,
   UnloadButton,
   Time,
   IsImageFilter,
   UseLayerTime,
   DynamicParam(u8)
   }

---

Popup params should not be modifiable at run time. This is a bizarre one, but after effects expects the '|' delimited string to only be assigned in param setup, , we could enforce this by adding an extra mutable out variable in the popup constructor and removing the options builder from the popupdef.

This is interesting, because Adobe's own Supervisor example, does rename Popup items, however, many of their included examples are broken and this particular thing seems to be broken too :D I found a lot of broken things in their SDK and examples, it's so bad

[mobile-bungalow]

thanks! Those are fair critiques my rewrite was a little slapdash. The adobe SDK is a little nauseating, you did a great job with it though. Next I think i'm going to try embedding depth-anything into a plugin, see how that goes. do you want an update if I finish that?

[doctorpangloss]

how would I achieve the aesthetic goal of "there are multiple effects in the Effects menu"? I don't see any examples achieving this.

[AdrianEddy]

If I understand what you mean correctly, you just set the same group in build.rs in multiple plugins