Skip to content
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

Refactor FCP XML adapter - add generator and effect support #494

Merged

Conversation

Projects
None yet
5 participants
@reinecke
Copy link
Collaborator

commented Apr 16, 2019

This is a refactor of the FCP XML adapter adapter. The goal of this refactor was to:

  • Address some timing accuracy issues that could be encountered
  • Add support for handling effects and generators
  • Add support for parsing and roundtripping FCP XML metadata

This is expected to address the following issues (I would love confirmation from the relevant parties):

@codecov-io

This comment has been minimized.

Copy link

commented Apr 16, 2019

Codecov Report

Merging #494 into master will increase coverage by 0.18%.
The diff coverage is 92.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #494      +/-   ##
==========================================
+ Coverage   88.37%   88.55%   +0.18%     
==========================================
  Files          68       68              
  Lines        6969     7255     +286     
==========================================
+ Hits         6159     6425     +266     
- Misses        810      830      +20
Impacted Files Coverage Δ
opentimelineio/adapters/fcp_xml.py 92% <92.45%> (-0.32%) ⬇️
opentimelineio/schema/clip.py 91.42% <0%> (-2.86%) ⬇️
opentimelineio/schema/schemadef.py 100% <0%> (+26.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d77adfe...7122c45. Read the comment docs.

@apetrynet

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

Hey!
I'm afraid this doesn't resolve #265

Running otioconvert xml->xml on a file with offline clips the resulting xml yields an empty timeline when re-importing in Hiero.
I tried looking at the output file and found some differences, but uncertain what exactly is causing the different behavior. Timecode and In/Out points differ for instance, but manually tweaking these still don't produce a timeline containing clips. I don't know if this is true for other applications.
Here's the file I tested with if you want to check it out.

@reinecke

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 17, 2019

Thanks @apetrynet for taking a look! I'll take a quick pass and see if I can resolve.

@reinecke

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 17, 2019

@apetrynet I definitely see the start timecode on file elements not roundtripping, I'll take a look a that. In terms of not loading, would you mind trying this file: https://gist.github.com/reinecke/5b4bbb89ab6dc3717f8d3f053708fd58

It's a wild hunch I'd like to validate but I don't have Hiero on my machine to try with :/

@apetrynet

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

@reinecke I actually tried removing the project tags earlier today. No go I'm afraid. Unfortunately I won't be able to double check your file until Tuesday when I'm back at work.

@reinecke

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 17, 2019

All I did was try removing the project tags, so I'm guessing that file won't work then :/
I noticed there were cases where Resolve had an issue but this file seemed to work fine there. I'll keep looking.

One thing I notice in the file you provided is that while in and out values are floats, the start and end values are ints. There could be an issue arising there.

@reinecke

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 17, 2019

@apetrynet If you get a chance, I updated the gist. In fixing the start timecode issue in the file elements for missing references I also discovered duration elements were being omit. With luck, this may help Heiro out a little.

@apetrynet

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

@reinecke sounds good! I'll check when I get back to work and report back.

@apetrynet

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

@reinecke Your latest gist works like a charm! Good job!

@reinecke reinecke force-pushed the reinecke:feature/fcp_xml_refactor branch from bb20e16 to a3afe75 May 7, 2019

@ssteinbach
Copy link
Member

left a comment

This is a huge amount of work, thanks @reinecke! I appreciate the extra tests you've added as well. I had some small questions and I think you're still working on some aspect of this?

Show resolved Hide resolved opentimelineio/adapters/fcp_xml.py Outdated
Show resolved Hide resolved opentimelineio/adapters/fcp_xml.py Outdated
Show resolved Hide resolved opentimelineio/adapters/fcp_xml.py Outdated

for kind in content_types:
_insert_new_sub_element(file_media_e, kind)
audio_exts = {'.wav', '.aac', '.mp3', '.aif', '.aiff', '.m4a'}

This comment has been minimized.

Copy link
@ssteinbach

ssteinbach May 7, 2019

Member

Just so I understand, FCP doesn't explicitly mark a track as audio or video? Its implicit based on what the thing is pointed at?

This comment has been minimized.

Copy link
@wrosecrans

wrosecrans May 7, 2019

In the Final Cut XML, the structure of the tags for tracks basically look like

<project> <children> <sequence> 
    <media> <video>
            <track/> <track/> ...
    </video>
    <audio>
            <track/> <track/> ...
    </audio> </media>
</sequence> </children> </project>

where "sequence" == "a timeline", and a particular track element is in either a "video" or "audio" element, and only has one kind of media one it. (But one file can wind up referenced in both a video and audio track, linked so you can drag them as a unified thing in the UI of something like Premiere.) If you use OTIO to populate only the video track with a quicktime file, it shows up as silent. Likewise, if you put a .wav file in a video track, it won't work right, so needs to be filtered out from video tracks.

All that said, this spot in fcp_xml.py is for making a file, which can potentially exist outside of a track, and sort of has its own semantics, because xmeml wants to rot your brain.

When a video has a , the clipitem has a which has its own element that has to have both and elements describing the format if the actual file does -- even if the track that contains the element is only a video track. I have seen Premiere fail to link a Quicktime file if the XML omits the the tag, even if it is only in a track, because in sees the file has two audio channels, but it thinks the file referenced in the timeline is supposed to have none, and thinks there is a mismatch.

I think that answers your question, but I fear I may have answered your question by making it even less clear. :)

This comment has been minimized.

Copy link
@reinecke

reinecke May 7, 2019

Author Collaborator

Also, this whole area of logic feels a bit CBB to me. However, the solve in unclear to me at the moment.

@reinecke

This comment has been minimized.

Copy link
Collaborator Author

commented May 8, 2019

Updated based on review notes. If it checks out, the PR is ready to merge from my POV.

@ssteinbach
Copy link
Member

left a comment

Looks good to me, thanks!

@ssteinbach ssteinbach merged commit a48bdce into PixarAnimationStudios:master May 8, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.