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

Change cmx_3600 adapter to use transition duration frames (Issue #895) #896

Merged
merged 6 commits into from
Mar 5, 2021

Conversation

JoshBurnell
Copy link
Contributor

The cmx_3600.py adapter appears to use a duration for Transitions that doesn't conform to the CMX 3600 spec.

According to this spec (Page 30), the duration is stored in a "RATE" field.

This guide makes it a little easier to read:

75 frame dissolve from reel 123 to reel 123B on video and A1:

Edit # Reel Name  Channel Trans      Dur      Source IN          Source OUT          Record IN         Record OUT
105 123    B     C        03:05:57:17 03:05:57:17 01:00:21:20 01:00:21:20
105 123B   B     D    075 03:15:33:09 03:15:35:24 01:00:21:20 01:00:24:05

However, the cmx_3600.py adapter appears to use the length of the previous clip:
https://github.com/PixarAnimationStudios/OpenTimelineIO/blob/84c8245c070b12d7e627c145435a5db9add0d2de/src/py-opentimelineio/opentimelineio/adapters/cmx_3600.py#L668

Test:

➜  OpenTimelineIO-JoshBurnell git:(issue-895) python3 tests/test_cmx_3600_adapter.py    
..............................
----------------------------------------------------------------------
Ran 30 tests in 0.285s

OK
➜  OpenTimelineIO-JoshBurnell git:(issue-895) python3 tests/test_cmx_3600_adapter.py EDLAdapterTest.test_transition_duration
.
----------------------------------------------------------------------
Ran 1 test in 0.092s

OK

@ssteinbach
Copy link
Collaborator

First of all, kudos for citing a document from 1988! Is there some way to get that as a citation/reference with a comment above the block where you set the transition duration?

@JoshBurnell
Copy link
Contributor Author

lol If you look closely, I bet you can see the perforated edges of the dot matrix paper that that document was printed on. :-P

Great idea. I couldn't find a good reference link directly to something that said "Duration works like so..." but I'll add something.

@JoshBurnell JoshBurnell changed the title #895 Change cmx_3600 adapter to use transition duration frames Change cmx_3600 adapter to use transition duration frames (Issue #895) Feb 25, 2021
@ssteinbach
Copy link
Collaborator

Do you know what the legal status of that document is? Is that something that we or ASWF could host? @jmertic do you know if there is a place for documents like that?

@jmertic
Copy link
Contributor

jmertic commented Feb 25, 2021

Good question! Do you know the org that owns this spec? Or who used to :-)?

@mikekoetter
Copy link
Contributor

TIL the CMX from cmx 3600 comes from CMX Editing Systems (a member of the Chyron group)
and the 3600 was a model of a "Highly popular online videotape editor" :)

@jminor
Copy link
Collaborator

jminor commented Feb 25, 2021

SMPTE ST 258:2004 is the most official spec I know of for EDL
https://ieeexplore.ieee.org/document/7291839

The proper citation format is something like: "ST 258:2004 - SMPTE Standard - For Television — Transfer of Edit Decision Lists," in ST 258:2004 , vol., no., pp.1-37, 6 April 2004, doi: 10.5594/SMPTE.ST258.2004.

@jminor
Copy link
Collaborator

jminor commented Feb 25, 2021

and there are mentions/links to two of the docs mentioned above here: https://opentimelineio.readthedocs.io/en/latest/tutorials/adapters.html

@ssteinbach
Copy link
Collaborator

Ok great, so in that case SMPTE is the organization that hosts the document and we should link to it with the citation that @jminor notes. Thanks everyone!

@jmertic
Copy link
Contributor

jmertic commented Feb 25, 2021

Citation makes complete sense here :-)

One other thing to think about is if within the project there is an implementation of the spec; if so you likely would want to review the copyright/IP policy of the standards owner to ensure your use is allowed in that context. You probably are fine, but just mentioning it :-)

@JoshBurnell
Copy link
Contributor Author

I updated the Citation per Josh's suggestion.

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 -- one more last thing, if you accept the change, we can use assertIsInstance instead of assertTrue(isinstance(...))

tests/test_cmx_3600_adapter.py Outdated Show resolved Hide resolved
@ssteinbach ssteinbach added this to the Public Beta 14 milestone Mar 5, 2021
@ssteinbach ssteinbach merged commit 84af5e5 into AcademySoftwareFoundation:master Mar 5, 2021
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

5 participants