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

Example RV OTIO reading application plugin #565

Merged

Conversation

avrata
Copy link
Contributor

@avrata avrata commented Sep 6, 2019

Re-creating #395 as an example RV plugin for loading an OTIO file into the current RV context.

@codecov-io
Copy link

codecov-io commented Sep 6, 2019

Codecov Report

Merging #565 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #565      +/-   ##
=========================================
- Coverage   81.61%   81.6%   -0.01%     
=========================================
  Files          72      72              
  Lines        2730    2729       -1     
=========================================
- Hits         2228    2227       -1     
  Misses        502     502
Flag Coverage Δ
#py27 81.58% <ø> (ø) ⬆️
#py36 81.58% <ø> (ø) ⬆️
#py37 81.58% <ø> (ø) ⬆️
Impacted Files Coverage Δ
...ntimelineio/opentimelineio-bindings/otio_utils.cpp 87.2% <0%> (-0.3%) ⬇️
src/opentimelineio/deserialization.cpp 53.19% <0%> (ø) ⬆️
src/opentimelineio/stackAlgorithm.cpp 82.97% <0%> (+0.37%) ⬆️

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 1ac88cb...2785a65. Read the comment docs.

@eric-desruisseaux-adsk
Copy link
Contributor

Hi, my name is Eric Desruisseaux, I am a SW developer at Autodesk. I am very happy to see this PR, good job! I think the approach makes great sense. I have a few questions.

  1. Did you think of how this could scale to support custom SchemaDef? One way to do this could be to expose the WRITE_TYPE_MAP map and let the user append its own "Schema --> function" entry. Assuming that the $OTIO_PLUGIN_MANIFEST_PATH path is properly set, this would require very minimal work per custom schema and would allow customization.

  2. Did you try loading timelines with nested compositions? The create_rv_node_from_otio function seems to support arbitrary structures, but I am wondering if you've successfully done it.

  3. Did you think of how you'd write the reverse logic? RV to OTIO? Since the RV graph topology is arbitrary and OTIO supports nested tracks, an RV-to-OTIO package could support almost all RV graph cases but I guess we could start with a fixed number of topologies, ie.: Stack of Sequences of Sources, Stack of Sources, Sequence of Sources. Any thoughts?

  4. Metadata is supported in _create_item (for Clips and Gaps), but I think OTIO supports metadata on all objects. It would be nice if the plug-in supported it too.

  5. Are there any known limitations which should be discussed (and ideally documented in the plug-in or elsewhere)?

Again, great work, this is very nice!

Eric

@eric-desruisseaux-adsk
Copy link
Contributor

Another question :

  1. The package calls otio.adapters.read_from_file. If I understand properly, this can be implemented by any registered adapter in opentimelineio. Does it mean that any format supported by adapters could be loaded through the package (ie.: native OTIO, but also FCP, AAF, etc.)?

@ssteinbach
Copy link
Collaborator

Another question :

  1. The package calls otio.adapters.read_from_file. If I understand properly, this can be implemented by any registered adapter in opentimelineio. Does it mean that any format supported by adapters could be loaded through the package (ie.: native OTIO, but also FCP, AAF, etc.)?

Yes, otio.adapters.read_from_file will dispatch to the adapter plugins based on file suffix.

@avrata
Copy link
Contributor Author

avrata commented Sep 18, 2019

Yes, otio.adapters.read_from_file will dispatch to the adapter plugins based on file suffix.

Exactly, there is this code:

if ext in otio.adapters.suffixes_with_defined_adapters(read=True):

That'll have the plugin only "wake up" when it sees extensions that OTIO supports. There is an interesting question though of who would "win" in the case where there is any overlap with other supported extensions from other plugins or the built in file reading logic since this is not using the standard way file format plugins work.

@jminor
Copy link
Collaborator

jminor commented Sep 19, 2019

@eric-desruisseaux-adsk here are responses to your numbered questions:

  1. I agree that would be great if there was a clean way to override or extend the WRITE_TYPE_MAP entries. Internally we are using a fork of this code which uses a special Pixar-specific node for dissolves. If there was a mechanism to override/extend then we wouldn't need a fork.

  2. We did some testing with multi-track and nesting, but not an exhaustive test. Most of our production use cases are single track. I think @ssteinbach's RV adapter might have more testing around that, but I'm not sure.

  3. I believe that @JoshBurnell and @swallitsch built the inverse (RV to OTIO plugin). OTIO is missing the notion of a Layout, but perhaps this is just a Stack with metadata about placement?

  4. Ideally RV (and any other app) would carry OTIO metadata around blindly and round-trip it back out again. This would allow production metadata to flow through RV seamlessly.

  5. Yes, there are some known limitations. In particular, image sequences are not formally handled, but can work if you use RV syntax in a MediaReference's target_url. We should discuss better handling for that.

@eric-desruisseaux-adsk
Copy link
Contributor

Thanks @jminor for your answers!

  1. We've implemented support for layouts by adding custom metadata at the media-reference level. We injected some resolution-independent coordinates and sizes that we call bounds. Bounds are basically 4 doubles : centerX, centerY, sizeX, sizeY. Those are in an arbitrary units so pixel density has no impact on the way the source is displayed. This is similar to the concept of Data Window in OpenEXR. It differs from a 2D Transform (effect) in that it is seen as source-metadata, but ultimately it is still implemented by a RVTransform2D node in the source group.

At some point (unrelated to this RV package) it would be nice to have a discussion on what would be the best way to make this data OTIO standard.

@eidmi
Copy link

eidmi commented Sep 19, 2019

Which relates to the discussion we had previously in representing position and size in a universal coordinate system.

@jminor
Copy link
Collaborator

jminor commented Sep 19, 2019

Yes, we'd like to hear more details about your universal coordinate system. Are you available to discuss at our next OTIO Technical Steering Committee meeting (video conf) Sept 26th?

Separate from that, do you see any of items 1-6 as blockers to accepting this PR, or smaller changes that should be addressed? You should be able to check out @avrata's branch to try it yourself.

@eric-desruisseaux-adsk
Copy link
Contributor

eric-desruisseaux-adsk commented Sep 19, 2019

I already checked it out and played with it in RV, I tried it with our OTIO and it worked well, except of course for all the missing private schemas. The only thing that I think should be added (now or in a follow-up) is the support of the metadata on all nodes (as expected based on the OTIO spec).

The rest was more some curiosity/question I had.

Thanks!

Eric

@jminor
Copy link
Collaborator

jminor commented Sep 19, 2019

Great. @avrata is going to look at the metadata support.

@avrata
Copy link
Contributor Author

avrata commented Sep 24, 2019

I took a quick pass at adding metadata to each rv node if the associated otio item has any here: f7af52c If this looks interesting I can look into trying to update the external rv adapter with this same pattern as well. I did change the property location of the RV metadata with this a little to something that made more sense to me (.otio.metadata), but that's easy to revert if people are attached to the old one.

Copy link
Collaborator

@ssteinbach ssteinbach left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @avrata. I had two really small questions.

@ssteinbach ssteinbach added this to the Public Beta 12 milestone Sep 24, 2019
@eric-desruisseaux-adsk
Copy link
Contributor

Thanks @avrata!

@avrata avrata changed the title [WIP] Example RV OTIO reading application plugin Example RV OTIO reading application plugin Sep 24, 2019
@ssteinbach ssteinbach merged commit 74c0951 into AcademySoftwareFoundation:master Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants