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

GStreamer Editing Services XML adapter #412

Merged
merged 1 commit into from Feb 13, 2019

Conversation

@thiblahute
Copy link
Contributor

commented Jan 23, 2019

This implement the basic for a "xges" adapter, the native GStreamer Editing Services serialization format.

Implemented features:

  • Clip serialization/de-serialization
  • Transitions

Missing features

  • Stacks inside Tracks, we are missing nested timelines in GES
  • GESGroups, basically I can't find any way to represent them in otio. Groups are simply a way to represent the fact that several clips are meant to be manipulated together in a timeline.
  • Effects, otio doesn't seem to have that feature yet
  • Keyframes, otio doesn't seem to have that feature yet

I have already implemented a GESFormatter to be able to take advantage of the otio formatter in any project using GES. I am not sure if it belongs to otio itself or GES, what would you prefer?

Also note that in GES each and every timestamp is a GstClockTime (a.k.a long unsigned int) representing times in nanoseconds, this lead to big approximation because of the nature of times representation in otio. Note that this "restriction" in GES is being worked on and we plan on introducing a mode where we would work with rational time exclusively (see this issue).

@thiblahute thiblahute force-pushed the thiblahute:xges branch from c8426f4 to 7fc4b85 Jan 23, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Jan 23, 2019

Codecov Report

Merging #412 into master will decrease coverage by 0.19%.
The diff coverage is 84.08%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #412     +/-   ##
=========================================
- Coverage   86.92%   86.72%   -0.2%     
=========================================
  Files          63       65      +2     
  Lines        5605     6026    +421     
=========================================
+ Hits         4872     5226    +354     
- Misses        733      800     +67
Impacted Files Coverage Δ
opentimelineio_contrib/adapters/fcpx_xml.py 92.19% <ø> (ø) ⬆️
opentimelineio_contrib/adapters/xges.py 83.12% <83.12%> (ø)
...lineio_contrib/adapters/tests/test_xges_adapter.py 96.66% <96.66%> (ø)

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 7cb24b8...7fc4b85. Read the comment docs.

@codecov-io

This comment has been minimized.

Copy link

commented Jan 23, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@bdbd4cb). Click here to learn what that means.
The diff coverage is 84.16%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #412   +/-   ##
=========================================
  Coverage          ?   86.65%           
=========================================
  Files             ?       65           
  Lines             ?     6033           
  Branches          ?        0           
=========================================
  Hits              ?     5228           
  Misses            ?      805           
  Partials          ?        0
Impacted Files Coverage Δ
opentimelineio_contrib/adapters/xges.py 83.2% <83.2%> (ø)
...lineio_contrib/adapters/tests/test_xges_adapter.py 96.66% <96.66%> (ø)

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 bdbd4cb...ed6d73e. Read the comment docs.

@thiblahute

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

Anything missing to get a review and get that merged?

@jminor
Copy link
Collaborator

left a comment

This looks really good. Sorry for the delayed review. I left some comments. The only critical thing is getting the license text to match the rest of OTIO.

Also, a heads up that some time before we get to 1.0 we will likely move the entire contrib area into a separate repo, but you don't need to worry about that until much later.

opentimelineio_contrib/adapters/xges.py Outdated Show resolved Hide resolved
opentimelineio_contrib/adapters/xges.py Outdated Show resolved Hide resolved
opentimelineio_contrib/adapters/xges.py Show resolved Hide resolved
opentimelineio_contrib/adapters/xges.py Outdated Show resolved Hide resolved

@thiblahute thiblahute force-pushed the thiblahute:xges branch from 7fc4b85 to 1e1cccb Feb 6, 2019

@thiblahute

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

Also, a heads up that some time before we get to 1.0 we will likely move the entire contrib area into a separate repo, but you don't need to worry about that until much later.

A bit sad from my point of view, but I understand.

Also, ooc, how do you choose what adapters deserve to be in "core" ?

@thiblahute thiblahute force-pushed the thiblahute:xges branch from 1e1cccb to 9d734bb Feb 6, 2019

@thiblahute

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

I do not get how that patch could lead to those travis failures.

@jminor

This comment has been minimized.

Copy link
Collaborator

commented Feb 6, 2019

Something changed upstream in pyaaf2. One other PR is also affected. We're in-progress on a patch to pin to a specific version of that package which will avoid this. Sorry for the hassle.

@jminor

This comment has been minimized.

Copy link
Collaborator

commented Feb 7, 2019

#419 is fixed now. If you rebase onto the latest master, then the build problem will go away.

@thiblahute thiblahute force-pushed the thiblahute:xges branch from 9d734bb to b6941f2 Feb 7, 2019

@thiblahute

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

#419 is fixed now. If you rebase onto the latest master, then the build problem will go away.

Thanks! It is done, let's see how it goes.

@thiblahute thiblahute force-pushed the thiblahute:xges branch from 1224ca1 to ed6d73e Feb 12, 2019

@thiblahute

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

I think I addressed all your comments, is anything else missing?

@jminor

This comment has been minimized.

Copy link
Collaborator

commented Feb 12, 2019

Yes, the code all looks good. I'm checking on the copyright question - the person I need to check with is out until 2/15. That's the only thing holding this up. Sorry about that.

If you're willing to use the Pixar copyright, we can accept this today. We make sure to keep the commit attributed to you & to mention you in release notes.

@jminor

This comment has been minimized.

Copy link
Collaborator

commented Feb 12, 2019

Oops, I just heard back. It is fine to have a non-Pixar copyright in the new file(s), but there should be just one per file, and since your CLA is on behalf of Igalia, it should be that one. Does that work for you?

@thiblahute thiblahute force-pushed the thiblahute:xges branch from ed6d73e to 891f4cd Feb 13, 2019

@thiblahute

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

Oops, I just heard back. It is fine to have a non-Pixar copyright in the new file(s), but there should be just one per file, and since your CLA is on behalf of Igalia, it should be that one. Does that work for you?

Done just that.

@jminor

jminor approved these changes Feb 13, 2019

@jminor jminor merged commit 7e59d37 into PixarAnimationStudios:master Feb 13, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jminor

This comment has been minimized.

Copy link
Collaborator

commented Feb 13, 2019

Thanks for the contribution to OTIO :)

@ssteinbach ssteinbach added this to the Public Beta 10 milestone Apr 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.