shidarin / cdl_convert

Converts between common ASC Color Decision List (CDL) formats
Other
60 stars 17 forks source link

Parsing a .CDL with a ColorCorrection node without an id tag fails #46

Closed gregcotten closed 9 years ago

gregcotten commented 9 years ago

Is this how it should work or can we gracefully generate a unique ID for it instead? Or no ID at all even?

Below is an example export out of Arri's Amira Color Tool. Don't know if they are just exporting it incorrectly or what.

<?xml version="1.0" encoding="UTF-8"?> 
<!-- ASC CDL Parameter written by ARRI Color Tool --> 
<ColorDecisionList xmlns="urn:ASC:CDL:v1.01">
    <InputDescription>Corresponding to Look File</InputDescription>
    <ColorDecision>
        <ColorCorrection>
            <SOPNode>
                <Description>ASC CDL Parameters to be applied to LogC data</Description>
                <Slope> 1 1 1 </Slope>
                <Offset> 0 0 0 </Offset>
                <Power> 1 1 1 </Power>
            </SOPNode>
            <SatNode>
                <Saturation> 1 </Saturation>
            </SatNode>
        </ColorCorrection>
    </ColorDecision>
</ColorDecisionList>
gregcotten commented 9 years ago

It would need to change here to:

try:
    cc_id = root.attrib['id']
except KeyError:
    import uuid
    cc_id = str(uuid.uuid4())
    #raise ValueError('No id found on ColorCorrection')
shidarin commented 9 years ago

It is actually off of the ASC CDL standard to have a CC without an ID, according to the standard all ASC CDL CCs must have an ID field.

We'll continue to raise an exception if the config.HALT_ON_ERROR value is set to True (False by default, True when using the --check flag), otherwise we'll generate one with uuid as suggested. This generation will actually happen in the ColorCorrection class itself, allowing it to be generated with id=None, and the generation logic living on the class itself.

gregcotten commented 9 years ago

If only I could find any documents for the said CDL standard!

I figured it was off-spec but if Arri isn't getting it right you know the ASC didn't do a good job making the spec accessible!

shidarin commented 9 years ago

Actually scratch the UUID stuff- we already have this in the ColorCorrect class:

        elif not id:
            if config.HALT_ON_ERROR:
                raise ValueError('Blank id given to ColorCorrection.')
            else:
                id = str(len(ColorCorrection.members) + 1).rjust(3, '0')

Which does everything I was about to code in, except the UUID part, which now that I remember, I didn't use because I figured a numerically increasing number was probably easier to understand.

So I just changed the parse_cc to read:

    try:
        cc_id = root.attrib['id']
    except KeyError:
        if config.HALT_ON_ERROR:
            raise ValueError('No id found on ColorCorrection')
        else:
            cc_id = None

The full ASC CDL spec can be obtained by emailing asc-cdl at theasc dot com.

I'll email ARRI about their CDL generation.

gregcotten commented 9 years ago

Makes sense!

gregcotten commented 9 years ago

Actually wait. You could have a collision issue if another color correction node with a specified id is ID'd the same as your auto-generated ID (such as id="001")