OTIO Editing API Design#719
Conversation
Codecov Report
@@ Coverage Diff @@
## master #719 +/- ##
=======================================
Coverage 81.88% 81.88%
=======================================
Files 72 72
Lines 2755 2755
=======================================
Hits 2256 2256
Misses 499 499
Continue to review full report at Codecov.
|
| ## 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. |
| ## Overwrite | ||
|
|
||
|  | ||
| - Anything overlapping is destroyed on contact. This may split an item. |
There was a problem hiding this comment.
It would be nice to specify things more pedantically using the nomenclature we've established in the recent Allen's Interval Algebra commit. eg.
for all items i
if e contains i, or i begins e, or e ends i : delete i
if i contains e : split i at e.start, yielding {i0, i1}. Split i1 at en.end, yielding {i2, i3}, delete i, i1 and i2, return i0 and i3.
if i overlaps e...
else skip
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
It's probably also worth clarifying intent for the audiences of this. I'm expecting it will be consumed by:
- The implementer(s) of the API
- Future users of the API - TDs and DCC developers
- Future maintainers (not necessarily the people from No. 1)
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.
There was a problem hiding this comment.
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 👍
meshula
left a comment
meshula
left a comment
There was a problem hiding this comment.
I like the looks of this proposal ~ I have a few initial thoughts.
| ## Overwrite | ||
|
|
||
|  | ||
| - Anything overlapping is destroyed on contact. This may split an item. |
There was a problem hiding this comment.
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..
reinecke
left a comment
reinecke
left a comment
There was a problem hiding this comment.
This is a great start!
Once we feel like this is pretty fleshed out I think it could be helpful to come up with some real world example cases and see how they might work using this API. Would these operations be targeting enabling lightweight NLE implementations with OTIO as a backing data model?
Glad to see someone interested in taking this on!
| `- ... | ||
| ``` | ||
|
|
||
| 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. |
There was a problem hiding this comment.
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.
This will hopefully create clarity around how their responsibilities are separate.
| ## Overwrite | ||
|
|
||
|  | ||
| - Anything overlapping is destroyed on contact. This may split an item. |
There was a problem hiding this comment.
It's probably also worth clarifying intent for the audiences of this. I'm expecting it will be consumed by:
- The implementer(s) of the API
- Future users of the API - TDs and DCC developers
- Future maintainers (not necessarily the people from No. 1)
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.
| to fill in the event that this overwrite extends the end of the | ||
| composition's limit | ||
| """ | ||
| otio.algorithms.overwrite( |
There was a problem hiding this comment.
It may help code clarity if the function names are a bit more explicit like overwrite_in_track.
| otio.algorithms.insert( | ||
| item: Item, | ||
| track: Track, | ||
| composition_time: RationalTime, |
There was a problem hiding this comment.
Should this be track_time to match overwrite?
| otio.algorithms.trim( | ||
| item: Item, | ||
| delta_in: RationalTime = None, #< Duration of change | ||
| delta_out: RationalTime = None, #< Duration of change |
There was a problem hiding this comment.
Would it be useful to also document error case behavior (e.x. delta_in + delta_out > clip duration)?
There was a problem hiding this comment.
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
| otio.algorithms.fill( | ||
| item: Item, | ||
| track: Track, | ||
| replace: Item = None, # Alternatively, we could have a parent-coord |
There was a problem hiding this comment.
This feels like this should maybe be a TimeRange within the track. If there is just a single item to operate on the caller can use range_in_parent to get the value they want to use.
| replace: Item = None, # Alternatively, we could have a parent-coord | ||
| # RationalTime to find the item for us | ||
| reference_point: otio.algorithms.ReferencePoint.Source | ||
| ) |
There was a problem hiding this comment.
This one's a bit tricky!
My instinct is that this may be trying to do too much and could yield unexpected results for users. The reference_point being set to Source is effectively the same as just using overwrite, right?
Also there is some clarification needed about whether it's most important to keep the in point or out point of the inserted item.
I wonder if maybe this operation can be broken down into a few more explicit variants instead (e.x. overwrite at time range trimming item, overwrite at time range retiming item, etc.)
ssteinbach
left a comment
ssteinbach
left a comment
There was a problem hiding this comment.
This is a great proposal to start! I had some questions about where you were suggesting putting the code namespace wise, and a bunch of discussion following along with some of the points.
I'd also like to see the edit event api split out into its own PR.
| # 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I totally see it being used more widely. I was hoping to build the common commands on top of this system. A command like overwrite will potentially do quite a few events - something like "sub-edits"
e.g.
- set A.duration = F
- set B.duration = B
- insert C into T' at index 3
If the general feeling is they shouldn't use it, then I'll write them as direct procedures.
| # 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. |
There was a problem hiding this comment.
I think this could be rephrased to be closer to OpenTimelineIO's scope... maybe something like:
OpenTimelineIO includes a suite of tools for operating on objects as raw data structures (append, etc), but is missing a library for performing higher level operations familiar to people from the editing world (Slip, 3/4 point edit, etc). This proposal presents an API for working with OpenTimelineIO objects in those ways.
I don't think they need to be enumerated here, since you're going to be covering them in more detail later.
| 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. | ||
|
|
There was a problem hiding this comment.
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:
- opentimelineio/
edit/
or
algo/
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:
opentimelineio.algorithms.slip()
etc.
Curious to hear your thoughts.
There was a problem hiding this comment.
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 algo/
| ### 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>; | ||
| ... | ||
| ``` |
There was a problem hiding this comment.
RetainedItem seems like a reasonable alias, however, reading this feels like it should all be in the same namespace. The edit library is going to require both OpenTimelineIO and opentime to function, and you'll need constructors, classes, etc from those to setup objects and metadata-- it seems like this might be more analogous to the split in imath between for example ImathBox.h and ImathBoxAlgo.h:
https://github.com/AcademySoftwareFoundation/openexr/blob/master/IlmBase/Imath/ImathBox.h
https://github.com/AcademySoftwareFoundation/openexr/blob/master/IlmBase/Imath/ImathBoxAlgo.h
| ## 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) | ||
|
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
| ## 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. |
There was a problem hiding this comment.
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.
- openedit scrubbed in favor of opentimelineio/algo - better overview statement for the purpose of the project - clarification of audience - ASCII art and additional constraints for various commands (still more to do)
|
|
||
|  | ||
| - Anything overlapping is destroyed on contact. This may split an item. | ||
| - Augment the source range of overlapping items |
There was a problem hiding this comment.
To be honest, I still feel the wording is not as precise as it could be. Elsewhere in OTIO we are adopting Allen's nomenclature, here, because the intent of the words is unambiguous: https://en.wikipedia.org/wiki/Allen's_interval_algebra
The ASCII art that follows makes things quite clear though, so I'm just going to express a preference for Allen as we continue to iterate.
| composition's limit | ||
| track's current duration | ||
|
|
||
| Example: |
6 participants
This is the relevant PR for Issue 771
This lays out the initial design specs for the edit platform within OTIO.