Parser Rebuild #9

reinecke · 2023-12-11T20:54:44Z

Overview

EDL can be a tricky format to get right in the details - our original parser worked well in controlled situations, but also had issues like not adopting global_start_time support, not fully baked transition support, and brittleness when sources were in frame rates that didn't match the timeline rate.

This refactor separates the reader code into two separate disciplines:

A parser that yields an edl statement per line with parsed fields but does not resolve timing and represents the facts of exactly what's in the EDL as closely as possible.
The Reader which interprets the statements grouped by event into their resolved timing, converts them to OpenTimelineIO schema objects, and applies them to a timeline

Changes and Enhancements

Adapter Args

The adapter has been updated with new controls on parsing behavior based on our more current knowledge of the format:

ignore_timecode_mismatch is still present, but is always hard-wired True and raises DeprecationWarning when used. Will be removed in a future release.
(NEW!) ignore_invalid_timecode_errors - When a timecode can't be interpreted due to something like Frame rate mismatch. Timecode '19:13:21:28' has frames beyond 23, the start timecode in the source will be rounded up to zero frames in the next second. This helps address issues that can occur from doing things like using a 30 fps source in a 24 fps timeline without retimes.
(NEW!) discard_unsupported_events - enables a mode where events that can't be interpreted (like KEY edits) are skipped and the event is noted in timeline.metadata["cmx_3600"]["unsupported_events"].

Note that setting ignore_invalid_timecode_errors and discard_unsupported_events to False (the default) will preserve the old behavior of erroring when correctness can't be trusted.

Parsing

Transitions now resolve properly according to references in the CMX spec (EDL Adapter incorrectly processing EDL that ends with "hanging dissolve event" AcademySoftwareFoundation/OpenTimelineIO#977) (Transition duration incorrect in CMX 3600 AcademySoftwareFoundation/OpenTimelineIO#895) (maybe EDL parser doesn't handle adjacent dissolves AcademySoftwareFoundation/OpenTimelineIO#137)
Uses Timeline.global_start_time rather than Track.available_range to signal the timeline start time (cmx3600 adapter assumes tracks start at 00:00:00:00 #6) (EDL Adapter: should author and read timeline::global_start_time attribute rather than source_range.start_time of track AcademySoftwareFoundation/OpenTimelineIO#1385) (maybe EDL with offset record time breaks trimmed_range_in_parent. AcademySoftwareFoundation/OpenTimelineIO#346)
Timeline events are no longer merged if they aren't from the same reel or their source range is discontinuous (in the old parser clips adjacent to transitions could be merged even if they came from separate sources) (CMX 3600 does not handle correctly transitions AcademySoftwareFoundation/OpenTimelineIO#1328) (cmx_3600 adapter incorrectly processing three-clip transitions AcademySoftwareFoundation/OpenTimelineIO#912)
Includes more contextual information in parsed clips (event numbers, original EDL timecode values, etc)
OTIO clip durations now primarily driven from timeline timecode rather than source timecode (this should avoid A/V sync issues due to potentially accumulating errors)
Adds the ability to use a timing approximation when timecode doesn’t match the provided EDL rate (This is helpful when the source is at a different frame rate than the timeline rate)
Most EDLParseException instances now include more contextual information
Now attempts to fallback to ISO-8859-1 encoding in read_from_file when the operating system default string encoding fails - this has anecdotally been successful in handling some EDLs that would otherwise fail
Video events that overwrite previous events in the timeline are now placed on a video track "above" the previous event's track to simulate an "overwrite". Overlapping audio events still raise an exception. Anecdotally this is most common in EDLs from Adobe Premiere.

TODOs

Write tests around specific cited issues
Update README documentation with new adapter args
Flesh out docstrings in code
Evaluate whether or not we should cull out parts of code targeted at SMPTE 258M EDL
Evaluate if using a duration comparison based heuristic (interpret source timecode at all the known timecode rates until we find a source duration that is very close to the timeline duration) we can reliably infer mismatched source rates where we're currently using the ignore_invalid_timecode_errors behavior

Signed-off-by: Eric Reinecke <[email protected]>

…from_file Signed-off-by: Eric Reinecke <[email protected]>

Signed-off-by: Eric Reinecke <[email protected]>

… other than a comment Signed-off-by: Eric Reinecke <[email protected]>

Signed-off-by: Eric Reinecke <[email protected]>

…ead of floats Signed-off-by: Eric Reinecke <[email protected]>

Signed-off-by: Eric Reinecke <[email protected]>

… given before clip parsing. This better flags non-edl files Signed-off-by: Eric Reinecke <[email protected]>

Signed-off-by: Eric Reinecke <[email protected]>

…a transition event Signed-off-by: Eric Reinecke <[email protected]>

Signed-off-by: Eric Reinecke <[email protected]>

douglascomet

Overall this is a significant improvement over the previous implementation and probably get merged as-is, especially since you mentioned this is already being used in production at Netflix.

Most of my comments are questions on your approach or highlighting small fixes. However, a docstring pass would be greatly appreciated before being merged.

Admittedly the cmx_3600_reader module is very dense. Like several operations are done in a single line or in a comprehension, which could be difficult to debug.

Also, you mentioned several times about m2 issues. Just to check, are you referring to m2 processors or something else? If it is the processors I'm surprised to here this is an issue to have to work around in the first place.

douglascomet · 2024-05-22T00:28:05Z

src/otio_cmx3600_adapter/cmx_3600_reader.py

+            elif key == "media_reference":
+                # These are the comments that will drive the media ref
+                self.handled[key] = statement.data


Why handle this if its identical to the else clause?

It's a fair point. I think the idea is to explicitly call-out how we address media_reference so that in future if more needs to happen it already has a place carved out to live.

douglascomet · 2024-05-22T00:28:56Z

src/otio_cmx3600_adapter/cmx_3600_reader.py

+    def parsed_asc_sat(self, sat_data: str) -> float:
+        return float(sat_data)
+
+    def parsed_asc_sop(self, sop_data: str):


Missing return type annotation

douglascomet · 2024-05-22T00:30:09Z

src/otio_cmx3600_adapter/cmx_3600_reader.py

+        )
+        marker.marked_range = otio.opentime.TimeRange(
+            start_time=marker_time,
+            duration=opentime.RationalTime(),


Didn't know we could instantiate a RationalTime without specifying any parameters.

Yeah, it's a short-cut for zero time. Arguably this should be changed to:
RationalTime(rate=self.edl_rate) for consistency-sake.

douglascomet · 2024-05-22T00:31:11Z

src/otio_cmx3600_adapter/cmx_3600_reader.py

+    ) -> Optional[otio.schema.Marker]:
+        # An example EDL locator line looks like this:
+        # * LOC: 01:00:01:14 RED     ANIM FIX NEEDED
+        m = re.match(r"(\d\d:\d\d:\d\d:\d\d)\s+(\w*)(\s+|$)(.*)", statement.data)


Somewhat of a style preference to suggest using a more descriptive variable name than m.

Single-char variables are bad, this is good feedback. For some reason I have this nasty habit with re Match objects.

douglascomet · 2024-05-22T00:33:16Z

src/otio_cmx3600_adapter/cmx_3600_reader.py

+                elif (
+                    statement_identifier is NoteFormStatement.NoteFormIdentifiers.SPLIT
+                ):
+                    # SPLIT notes appy to the following event


typo

Suggested change

# SPLIT notes appy to the following event

# SPLIT notes apply to the following event

douglascomet · 2024-05-22T01:33:18Z

src/otio_cmx3600_adapter/edl_statement.py

+class SourceMode(Enum):
+    """
+    Which source type the edit applies to.
+    """
+
+    VIDEO = "V"
+    AUDIO = "A"
+    BOTH = "B"
+    AUDIO_2 = "A2"
+    AUDIO_2_VIDEO = "A2/Y"
+    AUDIO_1_AUDIO_2 = "AA"
+    AUDIO_1_AUDIO_2_VIDEO = "AA/V"
+
+
+class EffectType(Enum):
+    """
+    Type of effect.
+    """
+
+    # Single source effects
+    CUT = "C"
+    SYNC_ROLL = "R"
+
+    # Multiple source effects
+    DISSOLVE = "D"
+    WIPE = "W"
+    KEY = "K"
+    KEY_BACKGROUND = "KB"
+    KEY_REMOVE = "KO"
+    MATTE = "M"
+    FOREGROUND_FILLER = "F"
+    QUAD_SPLIT = "Q"
+    NONADDITIVE_MIX = "N"
+    AUDIO_MIX = "X"
+
+
+class SpecialSource(Enum):
+    # BLACK/BL and BARS are called out as "Special Source Identifiers" in
+    # the documents referenced here:
+    # https://github.com/AcademySoftwareFoundation/OpenTimelineIO#cmx3600-edl
+    AUX = "AX"
+    BLACK = "BLACK"
+    BARS = "BARS"
+
+    @classmethod
+    def _missing_(cls, value):
+        if value == "BL":
+            return cls.BLACK
+        elif value == "SMPTEBars":
+            return cls.BARS


fwiw, these could be benefit from being represented as StrEnums and get the benefits of strings and enums.

To account for older Python versions, this could be implemented as follows:

try: # Python Built-Ins from enum import StrEnum except ImportError: class StrEnum(str, enum.Enum): """ Mixin of ``str`` and ``Enum`` to make a compatible implementation of `StrEnum` for Py2. Sourced from : * https://www.cosmicpython.com/blog/2020-10-27-i-hate-enums.html * https://stackoverflow.com/questions/75040733/is-there-a-way-to-use-strenum-in-earlier-python-versions """ def __str__(self): # type: () -> str return str.__str__(self)

douglascomet · 2024-05-22T01:34:54Z

src/otio_cmx3600_adapter/edl_statement.py

+    This is used for all well-known note form statements.
+    """
+
+    class NoteFormIdentifiers(Enum):


Same suggestion of using StrEnum for this.

douglascomet · 2024-05-22T01:36:36Z

src/otio_cmx3600_adapter/exceptions.py

+    line_number: Optional[int] = None
+    event_number: Optional[str] = None


Out of curiosity, why are these defined in the class scope and populated in the instance scope?

I've been writing too much Java lately 😄 I should pull these "declarations"

douglascomet · 2024-05-22T01:37:37Z

tests/test_cmx_3600_adapter.py

+            # snap the metadata value back to what it originally was
+            # We could consider overriding the value in the M2 speed with what
+            # the TC says, but it's probably best to stay true to what the EDL
+            # was signalling, who knows what TC rounding methods are used.


typo

Suggested change

# was signalling, who knows what TC rounding methods are used.

# was signaling, who knows what TC rounding methods are used.

douglascomet · 2024-05-22T01:43:01Z

src/otio_cmx3600_adapter/cmx_3600_writer.py

+    def __init__(
+            self, tracks, rate, style, global_start_time=None, reelname_len=8
+    ):


Could we also add a parameter to specify the zfill of the event number? If I wanted to do a round trip from a 32 EDL from AVID to OTIO back to EDL, the event number would be different between the 2 EDLs.

In this pass my goal was to not break the writer, but I didn't focus on addressing issues with it. It might be a good idea to collect some issues about things like this the writer could do a lot better (making "dialects" more of a first-class citizen in the implementation would fall in this category too).

reinecke · 2024-06-13T19:24:20Z

src/otio_cmx3600_adapter/cmx_3600_reader.py

+        """
+        Timeline timing:
+        - for each clip in event, use rec start for timeline placement
+        - If a clip has a rec end time different from the start, use it
+
+        Clip Timing:
+        - After the timeline has been resolved and all event statements have been
+            processed, back calculate source duration through any time warps from the
+            timeline timing
+        - The source of truth for source timing is always the Timecode values
+        """


Looks like I left a stray "note to self" comment in here. This should be turned into # comments so they aren't part of the docstring or removed if this is already captured piecemeal in the code below.

reinecke · 2024-06-13T19:27:51Z

@douglascomet Thanks for the feedback!

The M2 issues are with M2 motion effects - see page 20 in the cmx spec.

A docstring pass is a good idea - I need to make sure I share some of the ideas I had behind this stuff to help future us.

The code density stuff is good feedback - we can think about driving sections of this toward being more approachable.

I'll try to carve some time for this to try and get it out in the coming weeks.

reinecke added 20 commits December 11, 2023 15:27

Initial rebuild/refactor of parsing code and corrections to tests

be25c48

Signed-off-by: Eric Reinecke <[email protected]>

Reshuffled module structure and created dedicated adapter.py entrypoint

a5da948

Signed-off-by: Eric Reinecke <[email protected]>

Optimization pass to match/beat performance of previous reader

3cfdd3f

Signed-off-by: Eric Reinecke <[email protected]>

SPLITs are now attached to following event, lint and cleanup pass

24d9d0a

Signed-off-by: Eric Reinecke <[email protected]>

Added better handling around invalid timecodes

1fe6ba4

Signed-off-by: Eric Reinecke <[email protected]>

Fixed parsing for some edge case standard form lines

c54f18e

Signed-off-by: Eric Reinecke <[email protected]>

More robustness for certain cases

276459f

Signed-off-by: Eric Reinecke <[email protected]>

Added basic handling for EDLs in alternate encodings when using read_…

368ff00

…from_file Signed-off-by: Eric Reinecke <[email protected]>

Bumped to version 2.0.0.dev1

a8ff37e

Signed-off-by: Eric Reinecke <[email protected]>

Now errors on some less common kinds of M2 effects

db7185e

Signed-off-by: Eric Reinecke <[email protected]>

Fixed issue where commented out lines may be interpreted as something…

fd33371

… other than a comment Signed-off-by: Eric Reinecke <[email protected]>

Improved M2 effect handling for more cases

0db2599

Signed-off-by: Eric Reinecke <[email protected]>

Added support for video events that overwrite previous events

b240127

Signed-off-by: Eric Reinecke <[email protected]>

Marked ignore_timecode_mismatch adapter arg deprecated

8b5de41

Signed-off-by: Eric Reinecke <[email protected]>

Fixed an issue where RationalTime might be created with Decimals inst…

dc03db4

…ead of floats Signed-off-by: Eric Reinecke <[email protected]>

Restored behavior where AX reel names are no longer persisted

d983b92

Signed-off-by: Eric Reinecke <[email protected]>

Restored an EDL parse exception if an uhandled statement/directive is…

86b9157

… given before clip parsing. This better flags non-edl files Signed-off-by: Eric Reinecke <[email protected]>

Added feature to skip unsupported statements/do best effort parsing

0b6f250

Signed-off-by: Eric Reinecke <[email protected]>

Updated to match old behavior of attaching locators to both sides of …

3af9b9c

…a transition event Signed-off-by: Eric Reinecke <[email protected]>

Added contextual data to EDLParserError where available

5e86a53

Signed-off-by: Eric Reinecke <[email protected]>

reinecke force-pushed the feature/parser-rebuild branch from 1ddd739 to 5e86a53 Compare December 11, 2023 23:27

reinecke added 5 commits December 11, 2023 15:49

Linting pass

3c29b1d

Signed-off-by: Eric Reinecke <[email protected]>

Linting pass on tests

20c01a9

Signed-off-by: Eric Reinecke <[email protected]>

One more lint fix

87b27e0

Signed-off-by: Eric Reinecke <[email protected]>

Added cached_property backward compatability

1aa7492

Signed-off-by: Eric Reinecke <[email protected]>

Made tests avoid children_if to avoid OTIO version dependence

2077f17

Signed-off-by: Eric Reinecke <[email protected]>

douglascomet approved these changes May 22, 2024

View reviewed changes

reinecke commented Jun 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parser Rebuild #9

Parser Rebuild #9

reinecke commented Dec 11, 2023 •

edited

Loading

douglascomet left a comment

douglascomet May 22, 2024

reinecke Jun 13, 2024

douglascomet May 22, 2024

douglascomet May 22, 2024

reinecke Jun 13, 2024

douglascomet May 22, 2024

reinecke Jun 13, 2024

douglascomet May 22, 2024

douglascomet May 22, 2024

douglascomet May 22, 2024

douglascomet May 22, 2024

reinecke Jun 13, 2024 •

edited

Loading

douglascomet May 22, 2024

douglascomet May 22, 2024

reinecke Jun 13, 2024

reinecke Jun 13, 2024

reinecke commented Jun 13, 2024

	# SPLIT notes appy to the following event
	# SPLIT notes apply to the following event

		line_number: Optional[int] = None
		event_number: Optional[str] = None

	# was signalling, who knows what TC rounding methods are used.
	# was signaling, who knows what TC rounding methods are used.

Parser Rebuild #9

Are you sure you want to change the base?

Parser Rebuild #9

Conversation

reinecke commented Dec 11, 2023 • edited Loading

Overview

Changes and Enhancements

Adapter Args

Parsing

TODOs

douglascomet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reinecke Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reinecke commented Jun 13, 2024

reinecke commented Dec 11, 2023 •

edited

Loading

reinecke Jun 13, 2024 •

edited

Loading