Skip to content

AAF Parser - Calculating Source Values#1063

Closed
andrewmoore-nz wants to merge 1 commit intoAcademySoftwareFoundation:mainfrom
andrewmoore-nz:AAF_Src_Timecode_Parsing
Closed

AAF Parser - Calculating Source Values#1063
andrewmoore-nz wants to merge 1 commit intoAcademySoftwareFoundation:mainfrom
andrewmoore-nz:AAF_Src_Timecode_Parsing

Conversation

@andrewmoore-nz
Copy link
Copy Markdown
Contributor

@andrewmoore-nz andrewmoore-nz commented Sep 12, 2021

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

@andrewmoore-nz
Committing current version of Matchbox AAF source timecode parsing ro…
f25a71b 
…utine for investigation and discussion with the group.
@andrewmoore-nz andrewmoore-nz changed the title Committing current version of Matchbox AAF source timecode parsing ro… AAF Parser - Calculating Source Values Sep 12, 2021
@andrewmoore-nz
Copy link
Copy Markdown
Contributor Author

andrewmoore-nz commented Sep 12, 2021

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 andrewmoore-nz marked this pull request as draft September 13, 2021 00:26
@timlehr
Copy link
Copy Markdown
Contributor

timlehr commented Feb 4, 2022

@andrewmoore-nz are you still on this issue?

@andrewmoore-nz
Copy link
Copy Markdown
Contributor Author

andrewmoore-nz commented Feb 7, 2022

@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.

@timlehr
Copy link
Copy Markdown
Contributor

timlehr commented Feb 7, 2022

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

@andrewmoore-nz
Copy link
Copy Markdown
Contributor Author

andrewmoore-nz commented Mar 13, 2022

@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.

@meshula meshula marked this pull request as ready for review March 13, 2022 21:15
@meshula
Copy link
Copy Markdown
Collaborator

meshula commented Mar 13, 2022

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.

@andrewmoore-nz
Copy link
Copy Markdown
Contributor Author

andrewmoore-nz commented Mar 15, 2022

@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.

@meshula
Copy link
Copy Markdown
Collaborator

meshula commented Mar 15, 2022

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

@meshula meshula marked this pull request as draft March 15, 2022 20:01
@meshula meshula added TSC Slated for discussion at the next TSC meeting and removed TSC Review labels Mar 16, 2022
jminor
jminor reviewed Mar 31, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@jminor jminor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At a high level this all makes sense. I can't claim that I understand all the details, but I trust that if this passes the unit tests, especially for the wider range of sample AAFs that you have, then this is a nice step forwards.
Sorry for the extremely long delay before looking at it in detail.

contrib/opentimelineio_contrib/adapters/advanced_authoring_format.py
time_effect = op_stack.effects[0] if len(op_stack.effects) > 0 else None

time_scalar = time_effect.time_scalar if hasattr(time_effect, 'time_scalar') else None
if time_scalar:
Copy link
Copy Markdown
Collaborator

@jminor jminor Mar 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you checking for None or 0 or both? If it was a freeze frame, the time_scalar will be 0 which Python treats as false.

Copy link
Copy Markdown
Collaborator

@jminor jminor Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 would only come up via a FreezeFrame. If that was the case, we should already have the right first frame. So 0 and None should both skip this code block.
WBN to have a unit test for that, or at least verify manually to confirm this.

contrib/opentimelineio_contrib/adapters/advanced_authoring_format.py
alt = alt[0] if len(alt) > 0 else None
if alt is not None:
return _find_source_clip(item.alternates.value[0], op_group_found)
# return _find_source_clip(item.selected.value, op_group_found)
Copy link
Copy Markdown
Collaborator

@jminor jminor Mar 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If alt was None, this should return something. Also, should the Selector's "Selected" item be used here instead of the first alternate?

jminor
jminor requested changes Apr 19, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@jminor jminor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you could turn some of these informal code review notes from our video conf into comments in the code that would be great.

contrib/opentimelineio_contrib/adapters/advanced_authoring_format.py
mobs.append(mob)
else:
continue
def _find_source_clip(item, op_group_found=None):
Copy link
Copy Markdown
Collaborator

@jminor jminor Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From Zoom code review: we're wondering what item is passed in here?

Copy link
Copy Markdown
Collaborator

@jminor jminor Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the segment of the 1st slot of the source clip from _find_mob_chain_and_timecode, which might just be a SourceClip, but we might need to dig into the structure under it to find the SourceClip.

Copy link
Copy Markdown
Collaborator

@jminor jminor Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if/elif chain is attempting to handle the variety of AAFs in our test suite (both in OTIO, and from Matchbox) but we are still unclear of the full breadth of structures we might find within a SourceClip.

contrib/opentimelineio_contrib/adapters/advanced_authoring_format.py
return None
slot = [s for s in mob.slots if s.slot_id == slot_id]
slot = slot[0] if len(slot) > 0 else None
result = _find_source_clip(slot.segment) if slot else (None, None)
Copy link
Copy Markdown
Collaborator

@jminor jminor Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From Zoom code review: this is where the code attempts to drill into the SourceClip to find the details of what's inside it

contrib/opentimelineio_contrib/adapters/advanced_authoring_format.py

source_start = int(metadata.get("StartTime", "0"))
if source_start_multiplier:
source_start = int(source_start * source_start_multiplier)
Copy link
Copy Markdown
Collaborator

@jminor jminor Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the multiplier just from varying frame rate clips, or is this from timewarp effects in the composition?
0 or None means it was not found, and should be ignored.

contrib/opentimelineio_contrib/adapters/advanced_authoring_format.py
time_effect = op_stack.effects[0] if len(op_stack.effects) > 0 else None

time_scalar = time_effect.time_scalar if hasattr(time_effect, 'time_scalar') else None
if time_scalar:
Copy link
Copy Markdown
Collaborator

@jminor jminor Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 would only come up via a FreezeFrame. If that was the case, we should already have the right first frame. So 0 and None should both skip this code block.
WBN to have a unit test for that, or at least verify manually to confirm this.

contrib/opentimelineio_contrib/adapters/advanced_authoring_format.py
time_effect = op_stack.effects[0] if len(op_stack.effects) > 0 else None

time_scalar = time_effect.time_scalar if hasattr(time_effect, 'time_scalar') else None
if time_scalar:
Copy link
Copy Markdown
Collaborator

@jminor jminor Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if time_scalar:
if time_scalar is �None or time_scalar == 0: # FreezeFrames have a time_scale of 0

@timlehr timlehr mentioned this pull request May 2, 2022
Closed
@jminor jminor mentioned this pull request Jun 13, 2022
Open
@jminor jminor mentioned this pull request Jun 29, 2022
Closed
@jminor jminor mentioned this pull request Apr 11, 2024
Merged
@timlehr
Copy link
Copy Markdown
Contributor

timlehr commented Mar 27, 2025

I think we can close this one in favour of OpenTimelineIO/otio-aaf-adapter#44.

@jminor
Copy link
Copy Markdown
Collaborator

jminor commented Mar 28, 2025

Closing in favor of OpenTimelineIO/otio-aaf-adapter#44

@jminor jminor closed this Mar 28, 2025
@github-project-automation github-project-automation bot moved this from In progress to Done in OTIO - AAF Adapter Mar 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jminor jminor jminor requested changes

Assignees

No one assigned

Labels

TSC Slated for discussion at the next TSC meeting

Projects

OTIO - AAF Adapter
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@andrewmoore-nz @timlehr @meshula @jminor @ssteinbach