CDL is not unified among adapters

[vvzen]

Hi everyone,
and thanks for this great initiative!
I was wondering why the CDL data is stored in the cdl property of the metadata dictionary when using the EDL adapter while when using the ALE adapter it’s instead stored in the CDL property of the ALE property of the metadata.

Here's a sample of an ALE metadata:
{'ALE': {'CDL': '(0.8714 0.9334 0.9947) (-0.0870 -0.0922 -0.0808) (0.9988 1.0218 1.0101) (0.9000)'

And here's an example of an EDL metadata:
{'asc_sop': {'slope': [0.8044, 0.9091, 0.9958], 'power': [1.0282, 1.0281, 1.0275], 'offset': [0.0278, -0.0074, -0.0484]}, 'asc_sat': 1.0}

I'm writing a thin wrapper on top of OTIO for exporting CDLs, having a unified interface would greatly reduce the amount code that is adapter specific.
Is there a specific reason for this?
Maybe this issue can be linked to #170 since having the cdl in the metadata leads to this kind of non unified approach.. (instead of storing it as a Clip Effect, for example)

Thanks!

[jminor]

That looks like an oversight. They should certainly be unified.

I would guess that the EDL code is more widely used than ALE, so conforming to that would likely be least disruptive.

In the long run, we would like to evolve CDL support into an official schema, and include support in all of the adapters.

[reinecke]

I think the issue is that the ALE adapter doesn't seem to support ASC CDL explicitly. I think it's getting the metadata via this fallback:
https://github.com/PixarAnimationStudios/OpenTimelineIO/blob/6d3268764f6b6d84db2274271eabd0ea28033990/contrib/opentimelineio_contrib/adapters/ale.py#L121

It would be great to port what we have in the CMX adapter to the ALE adapter.

[vvzen]

I see. I’d love to open a PR implementing the same CDL structure of the EDL, if needed.
I’ll also check if this slight inconsistency appears only between the ALE and the EDL.

[reinecke]

@vvzen Do you happen to have an ALE with CDL data we could use for unittests?

[reinecke]

For reference, here is the CMX reader code:
https://github.com/PixarAnimationStudios/OpenTimelineIO/blob/6d3268764f6b6d84db2274271eabd0ea28033990/src/py-opentimelineio/opentimelineio/adapters/cmx_3600.py#L408-L445

And the CMX writer code:
https://github.com/PixarAnimationStudios/OpenTimelineIO/blob/master/src/py-opentimelineio/opentimelineio/adapters/cmx_3600.py#L1219-L1239

[vvzen]

@reinecke I do have tons of ALE samples which I 'm currently using to unit test the small local patch that I made to the (pure python) ale adapter. I'll ask if it's fine sharing them since it's client material (or I'll just make them shareable). Where should I send them?
Today I'll run your tests, sign the contributing pdf and then I'll open a PR here so you can have a look.

[reinecke]

Thanks for putting together a PR!
As long as you have one that is a representative example, you could include it in contrib/opentimelineio_contrib/adapters/tests/sample_data/ so you can reference it from the adapter unittests.

Are there different flavors of how the data comes across in these, or is it pretty regular? If they're all pretty much the same, including one sample case should be fine. If you have more files you don't mind sharing though, feel free to attach them to this issue.

[vvzen]

Hi @reinecke !
I've opened the PR and included one ALE sample and test since they were all pretty similar.
I also see that in the EDL adapter you're providing a no-op CDL whenever you can't find the CDL data. Is this something we want to do also in the ALE? I feel that it may be more useful to signal that no CDL was found in the file/clip, rather than providing a default one. What do you think?
Thanks

[jminor]

I agree that it can be important to distinguish whether CDL data was missing versus default is likely to be important in some workflows.

[reinecke]

I agree, I'd prefer it didn't exist in the OTIO if it doesn't exist in ALE or EDL.

[vvzen]

This has been merged so I'll close it myself, thanks!