OpenTimelineIO

OpenTimelineIO copied to clipboard

Following on from this weeks AAF call, this is the main process being used to correctly calculate the source timecode values for the SourceClip component.

@jminor @timlehr @stefanschulze

To give a brief recap, the source timecode values are calculated by looking at a chain of Media Objects (Mobs) and summing and converting a selection of values. The chain of mobs will be a combination of CompositionMobs, MasterMobs, and SourceMobs. There can be a source clip Origin, source clip Start, and a mob slot timecode object, all of which can effect the frame count that ends up being the source start value.

The function _find_mob_chain_and_timecode() condenses a few things in to one process for efficiency:

""" This is combining several processes in to one for efficiency:
     - Traversing the SourceClip's Mob structure using the clip_id to return all
       the necessary Mobs.
     - Extracting the necessary Start/Origin/Offset values for each object, and
       converts the rate if necessary. This value can come from several different
       places.
     - Searching for Operation Group objects that may contain information on how to
       correctly calculate the starting frame value.
     - Storing the individual SourceClips so that at the end of the process the
       correct Length can be derived from the MasterMob."""

Because all the relevant media objects for this particular source clip are returned from this process, there are several efficiencies that can be gained further on in this part of the _transcribe() function. I haven't included any of those here though, as I just want to focus on the source timecode values for now. I can't completely separate the pull requests, as the further changes are reliant on those mobs being available in that particular way.

This

This approach directly addresses the the situation I outlined in my issue from 2019 - #523

An example of how/why this approach is needed... when a TapeName value is added to a clip, it changes the final SourceMob in the chain from a mob with an ImportDescriptor essence to a mob with a TapeDescriptor essence. This then changes where one of the required start frame values is being stored, from in the last mob in the chain, to the second to last mob in the chain - a SourceMob with a CDCIDescriptor essence.

@andrewmoore-nz are you still on this issue?

@timlehr This pull request was supposed to be a jumping off point to look at addressing the AAF parsing issues present in the current version of the parser, which we have fixed in the version currently being used in Matchbox. I'm still very keen to get these changes rolled in.

@jminor @reinecke can we reserve some time for this in an upcoming TSC?

@jminor @reinecke What needs to happen to get some traction on this? It's been well over two years since Roger Sacilotto confirmed in our call with him that this is the correct approach, so would be great to actually get this rolled in and available for people to use, and some further development can continue.

Hi! I've tagged this for TSC review. Up until now it's been marked as a Draft/Work In Progress, so I've changed the status to Open.

I think the core fix in this PR is this:

An example of how/why this approach is needed... when a TapeName value is added to a clip, it changes the final SourceMob in the chain from a mob with an ImportDescriptor essence to a mob with a TapeDescriptor essence. This then changes where one of the required start frame values is being stored, from in the last mob in the chain, to the second to last mob in the chain - a SourceMob with a CDCIDescriptor essence.

If there was a test case added around this that fails with the existing code, but accepts with the patch, I think we'll be good to go.

@meshula There are a few test clips from back in 2019 that should cover that test case pretty easily so I'll add that to the PR this weekend when I get a chance to look at it again.

This was marked as a Draft/Work in Progress as that's what it is meant to be. We had a call back in September discussing this topic, and this Draft PR was supposed to be a jumping off point for a discussion about the problem, which never happened. If this PR specifically was going to be merged, it will need some further work to clean up other, now redundant, parts of the code.

Gotcha, I understood your question on getting traction to mean specifically this PR, as opposed to re-starting a conversation.