Add ALE adapter round-trip CDL values

[Viraj-Rana008]

Fixes #1050

Summary
Appended ASC_SOP and ASC_SAT values for the round-trip of data.

To provide a similar behavior as EDL adapter.

[codecov-commenter]

Codecov Report

Merging #1054 (cdef7f0) into master (8bc311f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1054   +/-   ##
=======================================
  Coverage   79.05%   79.05%           
=======================================
  Files         113      113           
  Lines        6122     6122           
=======================================
  Hits         4840     4840           
  Misses       1282     1282           

Flag	Coverage Δ
unittests	79.05% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

---

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 8bc311f...cdef7f0. Read the comment docs.

[reinecke]

Hi @Viraj-Rana008,

Thanks for contributing! It looks like the changes might be in the read code instead of the writer code - also it looks like it's somewhat matching EDL syntax instead of ALE syntax.

It might be best to start by adding a unittest to make sure CDL data round-trips properly and that will help make clearer whether or not the behavior is correct.

[reinecke]

Looks like it's coming along! Looks like the next big thing is to add unittesting to make sure this behaves as expected.

[jminor]

Waiting on tests to be added.

[jminor]

@Viraj-Rana008 could you add some tests for this new feature?
Also, there is a page in the documentation called "Feature Matrix" which should be updated to show that ALE supports Color Decision Lists.

[Viraj-Rana008]

Yes, I'll work on it.

[Viraj-Rana008]

Can you provide some direction? Won't "test_ale_roundtrip" suffice for the changes?

[reinecke]

Hi @Viraj-Rana008,
Because the ALE adapter would do blind pass-through deserializing metadata into the ALE namespace, then serialize anything in that namespace back out I think it's possible that while the functionality changed as expected there could be a regression we wouldn't know about that the round-trip would hide. My personal rule-of-thumb is that if functionality changes and the unit tests don't, then it means our tests aren't validating the change we made - the code could accidentally get reverted in a merge error and no one would know.

An ALE CDL write test should be able to cover it. You could create a SerializableCollection with a single clip that has both ALE and CDL metadata, then use write_to_string to output and check the result. The nice part is that if there are other specialized kinds of metadata that would need to be translated, that same test could be expanded to handle them.

[reinecke]

This change should move to OpenTimelineIO/otio-ale-adapter.