-
Notifications
You must be signed in to change notification settings - Fork 338
OTIO Editing API Design #719
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
c91a30b
e1e5c08
eaefe8a
e501b4d
8bbb1ce
9b59fe9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,354 @@ | ||
| Designing an Edit API | ||
| ===================== | ||
|
|
||
| # Design Overview | ||
| The industry is full of timeline management software but nearly all of them include a fundamental set of tools for manipulation. The most well known being: | ||
|
|
||
| - Overwrite | ||
| - Trim | ||
| - Cut | ||
| - Slip | ||
| - Slide | ||
| - Ripple | ||
| - Roll | ||
| - Fill (3/4 Point Edit) | ||
|
|
||
| OpenTimelineIO, while not seeking to become a fully realized NLE, could benefit from having a simple toolkit to help automate building/modifying timelines based on time rather than index alone. | ||
|
|
||
| ## Structure | ||
| The underlying implementation for these edit commands will be in C++, possibly under a new library called `openedit`. This has the benefit of connecting with other languages more simply (Python, Swift, C, etc.) and, hopefully, we only need expose the functionality to the bindings. | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean that you'd want a separate c++ DSO? To sit next to OpenTime.so and OpenTimelineIO.so? I kind of think it just belongs in the regular OpenTimelineIO library, but with its own headers. Or are you saying you'd just wrap this subset of the library and not need bindings to OpenTimelineIO? I would probably just put them under OpenTimelineIO, possibly combined with other operations into an 'algorithms' folder: Things like flatten are likely peers of this and I'd like to avoid too many namespaces for people to hunt through. In python, I was thinking a similar arrangement: Curious to hear your thoughts.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I completely agree with this. I didn't want to make this my first move without larger consensus but this will work super well and clean up a lot of clunky code I was sketching out. As @reinecke suggested, it'll be worth pointing out that no new schemas should be allowed under |
||
| ``` | ||
| OpenTimelineIO/ | ||
| `- src/ | ||
| `- opentime/ | ||
| `- opentimelineio/ | ||
| `- openedit/ | ||
| `- ... | ||
| ``` | ||
|
|
||
| The benefit for the extra library is the edit suite will inevitably grow and will begin crowding the core model, which ultimately should be able to run without it. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we should assert here that opentime and opentimelineio encapsulate the stored data model and openedit implements operations on that data model but should not define any new schema objects. |
||
|
|
||
| ## Dependencies / Namespaces | ||
| The `openedit` library will have to communicate with both `opentime` and `openedit`. | ||
|
|
||
| This will warrant either a new namespace `namespace openedit { ... }` or using `opentimelineio` again. The latter makes some of the bellow comments less challenging but again, might pollute an otherwise clean namespace. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo 'below' |
||
|
|
||
| ### ErrorStatus | ||
| This will present challenges when interfacing with the `ErrorStatus` of the different namespaces. That said, with a simple wrapper for this, we could have the instances auto convert as required. | ||
|
|
||
| ### Retainer | ||
| Ideally, we continue to use the `Retainer<>` where possible to make sure editing is reference counted. Namespaces might make the names a bit clunky, seeing `SerializedObject::Retainer<>` all over: | ||
|
|
||
| ``` | ||
| namespace openedit { OPENEDIT_VERSION { | ||
|
|
||
| using namespace opentimelineio::OPENTIMELINEIO_VERSION; | ||
| auto item = SerializedObject::Retainer<Item>(...); | ||
|
|
||
| } } | ||
| ``` | ||
|
|
||
| So perhaps establishing some common aliases early might help? | ||
| ``` | ||
| using RetainedItem = SerializedObject::Retainer<Item>; | ||
| using RetainedComposition = SerializedObject::Retainer<Composition>; | ||
| ... | ||
| ``` | ||
|
Comment on lines
+40
to
+35
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| # Editing Commands | ||
| Let's go through some of the most common commands and what the python API might look like: | ||
|
|
||
| ## Overwrite | ||
|
|
||
|  | ||
| - Anything overlapping is destroyed on contact. This may split an item. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be nice to specify things more pedantically using the nomenclature we've established in the recent Allen's Interval Algebra commit. eg.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You've specified the rest of the API more formally. I felt, given our new algebraic nomenclature, overlapping wasn't specific enough, also destroyed on contact was open to a bit of interpretation..
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's probably also worth clarifying intent for the audiences of this. I'm expecting it will be consumed by:
I think the formal definitions will be key for the implementers and useful reference for maintainers. The human-friendly descriptions should be targeted at being approachable for API consumers and provide a framing of the mental model for future maintainers.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed - I've got ASCII art and more explicit instructions per-command on the way (taking in the additional notes where possible). I'll add the audience information to the top along with the replaced "elevator pitch" that @ssteinbach recommended which sums it up beautifully 👍 |
||
|
|
||
| #### API | ||
| ```py | ||
| """ | ||
| Args: | ||
| item: Item that we're going to place onto the track | ||
| track: Track that this item will belong to afterwards | ||
| track_time: RationalTime of our track to put this item at | ||
| fill_template: Optional[Item] that will be cloned where required | ||
| to fill in the event that this overwrite extends the end of the | ||
| composition's limit | ||
| """ | ||
| otio.algorithms.overwrite( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It may help code clarity if the function names are a bit more explicit like |
||
| item: Item, | ||
| track: Track, | ||
| track_time: RationalTime, | ||
| fill_template: Item = None, # Default to Gap | ||
| ) | ||
| ``` | ||
| - In the image, Clip `C` is `item` and `track` is the what `A` and `B` are on. | ||
|
|
||
| ---- | ||
|
|
||
| ## Insert | ||
|  | ||
|
meshula marked this conversation as resolved.
|
||
| - Items past the in point are shifted by the new items duration. | ||
| - Item may be split if required. | ||
|
|
||
| #### API | ||
| ```py | ||
| """ | ||
| Args: | ||
| <same as overwrite> | ||
| """ | ||
| otio.algorithms.insert( | ||
| item: Item, | ||
| track: Track, | ||
| composition_time: RationalTime, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be |
||
| fill_template: Item = None, # Default to Gap | ||
| ) | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## Trim | ||
|  | ||
| - Adjust a single item's start time or duration. | ||
| - Do not affect other clips. | ||
| - Fill now-empty time with gap or template. | ||
| - Clamps source_range to non-gap boundary (can only overwrite gaps) | ||
|
|
||
|
Comment on lines
+110
to
+184
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if source range isn't set, does the available range become the source range before modifying the values? This is assuming the item is in track, is it an error condition if the item has no parent? If the item is directly in a stack, is a track inserted wrapping the item so that gap can be put before or after the item?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I hope that the common commands are as simple as possible - with explicitly defined implementation or course. Part of that would be, possibly, having it assume the source_range is the available_range if not set before. Implementing it as an error would totally work too but do we gain anything with that? |
||
| #### API | ||
| ```py | ||
| """ | ||
| Args: | ||
| item: Item to apply trim to | ||
| delta_in: RationalTime that the item's source_range().start_time() | ||
| will be adjusted by | ||
| delta_out: RationalTime that the item's | ||
| source_range().end_time_exclusive() will be adjusted by | ||
| """ | ||
| otio.algorithms.trim( | ||
| item: Item, | ||
| delta_in: RationalTime = None, #< Duration of change | ||
| delta_out: RationalTime = None, #< Duration of change | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be useful to also document error case behavior (e.x.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes - Error cases are something I definitely need to focus on more. I'll try to get more of the edge cases documented for my next push |
||
| fill_template: Item = None, #< Default to Gap | ||
| ) | ||
| ``` | ||
| > Not convinced on the argument names for this | ||
|
|
||
| --- | ||
|
|
||
| ## Slice | ||
|  | ||
| - Slice an item, generating a clone of self and augment both item's source_range. | ||
|
|
||
| #### API | ||
| ```py | ||
| """ | ||
| Args: | ||
| item: otio.schema.Item - to cut | ||
| at_time: otio.schema.RationalTime - time, based on the | ||
| coordinates to slice at | ||
| coordinates: Enumerator for the variable calculations that | ||
| can be done to the at_time. | ||
|
|
||
| Example: | ||
| Let item = A | ||
| let at_time = 25 @ 24fps | ||
|
|
||
| 0 N | ||
| | ------------------------------------------- | | ||
| | [0 GAP 20][0 A 50] | | ||
| | ------------------------------------------- | | ||
|
|
||
| if coordinates == Local: | ||
| | ------------------------------------------- | | ||
| | [0 GAP 20][0 A 25][26 A 50] | | ||
| | ------------------------------------------- | | ||
|
|
||
| if coordinates == Parent: | ||
| | ------------------------------------------- | | ||
| | [0 GAP 20][0 A 5][6 A 50] | | ||
| | ------------------------------------------- | | ||
|
|
||
| if coordinates == Global: | ||
| - This only matters if the parent is a track in a compound | ||
| stack, at which point we might be better off having the | ||
| user just convert from one time to the other | ||
| """ | ||
| otio.algorithms.slice( | ||
| item: Item, | ||
| at_time: RationalTime, | ||
| coordinates: otio.algorithms.Coordinates.(Local|Parent|Global*) | ||
| ) | ||
| ``` | ||
| > I'm not sure if we have a coordinate enumerator already? | ||
|
|
||
| --- | ||
|
|
||
|
|
||
| ## Slip | ||
|  | ||
| - Adjust the start_time of an item's source_range. | ||
| - Do not affect surrounding items. | ||
| - Clamp to available_range of media (if available) | ||
|
|
||
| #### API | ||
| ```py | ||
| """ | ||
| Args: | ||
| item: Item that we're slipping | ||
| delta: RationalTime to adjust the range by. | ||
| """ | ||
| otio.algorithms.slip( | ||
| item: Item, | ||
| delta: RationalTime, | ||
| ) | ||
| ``` | ||
| - In the example image, `C` is `item` and `delta` is the arrow's vector. | ||
|
|
||
| > Effectively this is already quite simple in the core API but would be nice to have here. | ||
|
Comment on lines
+187
to
+348
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suspect the value of some of these functions might be that they hide the underlying complexity of whether or not source_range is set. Maybe you split these functions into a "maybe" and "Definitely" pile? that will help prioritize them and see if people come out with concerns or use cases. This might be a controversial position, but I think I'd also prefer to not have things named too close even if they do mean a specific thing (slip/slide for example)... unless that verb means the same thing in all major editing packages. |
||
|
|
||
| --- | ||
|
|
||
|
|
||
| ## Slide | ||
|  | ||
| - Adjust start time of item and trim adjacent items to fill | ||
| - Do not change main item's duration, only adjacent | ||
| - Clamp to available range of adjacent items (if available) | ||
|
|
||
| #### API | ||
| ```py | ||
| """ | ||
| Args: | ||
| item: The item to push and pull around, which may in turn affect the | ||
| items around it. | ||
| delta: The delta that we're looking to push this item about. | ||
| """ | ||
| otio.algorithms.slide( | ||
| item: Item, | ||
| delta: RationalTime, # Relative like slide | ||
| ) | ||
| ``` | ||
| - In the example image, `C` is `item` and `delta` is the arrow's vector. The edit command does the rest of the item management for us. | ||
|
|
||
| --- | ||
|
|
||
| ## Ripple | ||
|  | ||
| - Adjust a source_range without adjusting any other items | ||
| - This effectively shifts all currently adjacent items to stay at the edges | ||
| - No items _before_ the item are moved/affected | ||
| - (Impl detail for otio, this is the same as adjusting the source_range of the item) | ||
|
|
||
| #### API | ||
| ```py | ||
| """ | ||
| Args: | ||
| item: The item to initiate the edit on | ||
| delta_in: see trim() | ||
| delta_out: see trim() | ||
| """ | ||
| otio.algorithms.ripple( | ||
| item: Item, | ||
| delta_in: RationalTime = None, #< Duration of change | ||
| delta_out: RationalTime = None, #< Duration of change | ||
| ) | ||
| ``` | ||
| - In the example image, `A` is item and `delta_out` is a negative RationalTime. The edit command shifts `B` without adjusting it's source_range. | ||
|
|
||
| > Again, no crazy about the argument names | ||
|
|
||
| --- | ||
|
|
||
|
|
||
| ## Roll | ||
|  | ||
| - Any trim-like action results in adjacent items source_range being adjusted to fit | ||
| - No new items are ever created | ||
| - Clamped to available media (if available) | ||
|
|
||
| #### API | ||
| ````py | ||
| """ | ||
| Args: | ||
| item: The item to initiate the edit on | ||
| delta_in: see trim() | ||
| delta_out: see trim() | ||
| """ | ||
| otio.algorithms.roll( | ||
| item: Item, | ||
| delta_in: RationalTime = None, #< Duration of change | ||
| delta_out: RationalTime = None, #< Duration of change | ||
| ) | ||
| ```` | ||
| - In the example image, `A` is the item and `delta_out` is a negative RationalTime. The edit command augments `B` without changing it's `source_range().end_time_exclusive()` | ||
| - If `delta_in` is supplied, the item to the left of `A` would be modified (if any) - otherwise we may need to add a `Gap` / fill template. | ||
|
|
||
| --- | ||
|
|
||
| ## 3/4 Point Edit | ||
|  | ||
| - The most complex - this "fills" a gap based on a source in/out point _or_ track in/out point | ||
| - Often used to patch in items as edit is built | ||
| - Note: This can be accomplished by a conjunction of commands above | ||
|
|
||
| ### API | ||
| ```py | ||
| """ | ||
| Args: | ||
| item: The item to place onto the track | ||
| track: Track that will now own this item | ||
| replace: Item (or RT?) that we will effectively replace | ||
| reference_point: For 4 point editing, the reference point dictates what | ||
| transform to use when running the fill. | ||
|
|
||
| Options: | ||
| - Source : Don't modify the source, overwrite if required | ||
| - Sequence : Don't modify the sequence, trim item if required | ||
| - Fit : Apply Time Effect | ||
| """ | ||
| otio.algorithms.fill( | ||
| item: Item, | ||
| track: Track, | ||
| replace: Item = None, # Alternatively, we could have a parent-coord | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels like this should maybe be a |
||
| # RationalTime to find the item for us | ||
| reference_point: otio.algorithms.ReferencePoint.Source | ||
| ) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one's a bit tricky! |
||
| ``` | ||
| - This may still require some additional tinkering. | ||
|
|
||
|
|
||
| # Expanded Implementation | ||
|
|
||
| ## Proposal | ||
| For editing commands, we should strive to do all validation and assert that everything "works" before committing anything on the timeline. A simple atomic command structure will provide that level of sophistication. | ||
|
|
||
| ```cpp | ||
| class EditEvent { | ||
| public: | ||
| EditEventKind kind; // e.g. Insert, Append, Remove, Modify | ||
| Retainer<Composition> parent; | ||
| Retainer<Item> composable; | ||
|
|
||
| // ... Additional fields to execute the above as required | ||
|
|
||
| bool run(ErrorStatus* error_status); | ||
| bool revert(); | ||
| }; | ||
| ``` | ||
| An event is atomic in nature and does "one thing" to an `Item`. Each edit maneuver (e.g. `otio.algorithms.overwrite(...)`) would generate a vector of these events that can be played forward and, possibly, backward. The result of them collectively is the commands result. | ||
|
Comment on lines
+320
to
+612
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets factor this out into a second PR. It is an orthogonal concept and also relevant to other non-edit api operations in OTIO, so we should design that as its own piece.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I totally see it being used more widely. I was hoping to build the common commands on top of this system. A command like If the general feeling is they shouldn't use it, then I'll write them as direct procedures. |
||
|
|
||
| ```cpp | ||
| for (EditEvent &event: events) { | ||
| event.run(error_status); | ||
| if (*error_status) { | ||
| for (auto it = completed_events.rbegin(); /*...*/) | ||
| (*it).revert(); | ||
| break; | ||
| } | ||
| completed_events.push_back(event); | ||
| } | ||
| ``` | ||
|
|
||
| ## Overall | ||
| Many of the commands mentioned have common code paths and math that is required. We can streamline many of the placement commands into a single call with different options. We can then expose that as a raw edit platform while giving users the common algorithms for ease-of-use. | ||
|
meshula marked this conversation as resolved.
|
||
|
|
||
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 this could be rephrased to be closer to OpenTimelineIO's scope... maybe something like:
I don't think they need to be enumerated here, since you're going to be covering them in more detail later.