#841 Track range_of_all_children applies TimeEffect effect to duratio…#842
#841 Track range_of_all_children applies TimeEffect effect to duratio…#842splidje wants to merge 7 commits into
Conversation
…eEffect effect to duration of clip.
reinecke
left a comment
reinecke
left a comment
There was a problem hiding this comment.
Hi @splidje,
Thanks for jumping in!
I think there is some misleading terminology in OTIO when TimeWarp is applied to a clip. Here is the code in python I use if I have a clip in a timeline that has a LinearTimeWarp applied and I want to calculate the original source frame range:
time_warps = [
effect for effect in clip.effects
if isinstance(effect, otio.schema.LinearTimeWarp)
]
time_scalar = functools.reduce(
lambda x, y: (x * y.time_scalar if y.time_scalar != 0 else 0),
time_warps,
1,
)
effect_transform = otio.opentime.TimeTransform(scale=time_scalar)
computed_duration = effect_transform.applied_to(clip.source_range.duration)
This works well with EDLs I've been using. The way I think of it is that the retimed clip is acting like it's own new piece of media and if you want to evaluate the range in the original media you have to do so backwards through the time warps.
@ssteinbach @meshula @jminor Would you mind checking my mental model on this stuff, I've always been a little fuzzy about the mental model for TimeWarps.
Depending on the answer to that, if we decide to move forward we'd want to have some unittesting in place before accepting the PR.
| } | ||
|
|
||
| virtual TimeRange output_range(TimeRange input_range, ErrorStatus* error_status) const { | ||
| return TimeRange(input_range.start_time(), input_range.duration() / _time_scalar); |
There was a problem hiding this comment.
Here you could construct aTimeTransform and then use .applied_to(input_range.duration() to compute the scaled duration.
Also, another thing to consider is what happens when _time_scalar is zero (like for FreezeFrame). Perhaps, if it doesn't exist, a unittest for the FreezeFame case should be added.
| friend RationalTime operator/ (RationalTime lhs, double rhs) { | ||
| return RationalTime {lhs._value / rhs, lhs._rate}; | ||
| } | ||
|
|
There was a problem hiding this comment.
We originally omitted overloading / and * operators for RationalTIme because we felt that the meaning of division and multiplication in the context of RationalTime could potentially be ambiguous. Instead we provided TimeTransform to make sure it was explicit how these operations work.
| } | ||
| else if (auto item = dynamic_cast<Item*>(child.value)) { | ||
| auto last_range = TimeRange(last_end_time, item->trimmed_range(error_status).duration()); | ||
| auto output_range = TimeRange(RationalTime(0), item->trimmed_range(error_status).duration()); |
There was a problem hiding this comment.
Since considerable work is potentially being done in the following for loop, should error_status be right after instead of later below?
|
Thank you for the review. I wasn't aware of TimeTransform. I'll make those changes. It makes sense in my head that a clip has if you had the mapping (output -> source): |
|
I'm probably being naive though. As you say, freeze frame is an anomaly. Should it be separate from a time effect as it has special behaviour, or should time effect be designed to encompass freeze frame? Not sure. A freeze frame needs a duration. Actually, it's just occurred to me that it does fit the model I described above. A time effect provides two things:
so a freeze frame would just return its duration property for duration, and the mapping always returns the same source frame. |
Codecov Report
@@ Coverage Diff @@
## master #842 +/- ##
==========================================
- Coverage 84.18% 83.84% -0.35%
==========================================
Files 74 74
Lines 3061 3076 +15
==========================================
+ Hits 2577 2579 +2
- Misses 484 497 +13
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
…imeEffect to differentiate it from LinearTimeWarp. Given it a duration property.
…tive item duration. Commiting to save WIP.
…uration in a track. So a TimeEffect actually has no effect on changing an item's duration. I still have FreezeFrame as a direct subclass of TimeEffect, rather than LinearTimeWarp - it doesn't need a time_scalar property of 0, we know that already about it.
4 participants
Fixes #841Track range_of_all_children applies TimeEffect effect to duration of clip.