[jakubjezek001]

Changelog Description

New shared ayon_editorial.otio.builder: TimelineBuilder, ClipSpec, MediaSpec and MarkerSpec build an OTIO timeline from plain data, replacing the shared skeleton of the Flame, Hiero and Resolve exporters. New otio_compare (library ayon_editorial.otio.compare and tools/otio_compare.py) compares two OTIO files and is used for every host adoption step (P2.7 to P2.9).

Additional info

• The builder never reads a host object, path or rate source, and never rounds. Where the hosts disagree, the behaviour stays a parameter instead of being unified: gap_mode (record for Flame and Hiero, track_end for Resolve), ClipSpec.offset_at_clip_rate (Resolve conforms the source offset in a different order, which differs for mixed rates: 86503.5035 vs 86503.4865 for 23.976 media on a 24 timeline), sequence_fallback_padded, marker rate and anchor.
• Quirks kept on purpose (documented in TimelineBuilder): record gaps can be zero or negative on overlaps and skip the - 1 after a previous out of 0; track_end assumes the timeline rate.
• Resolve template: the audio duration is decoded at 24 fps, as the host does today (10 s at 25 fps give 240 frames, not 250), so the adopted export stays equal to the current one. The shared otio.time.timecode_to_frames is right; P2.8 decides, a test pins the difference.
• compat.has_image_sequence_reference() added; on the 0.15 floor ImageSequenceReference always exists, so the fallback is kept but never runs there.
• otio_compare: dict key order ignored, rate of a zero time ignored, other rates strict, int vs float is a difference, --ignore takes globs where only * and ? are wildcards. Exit codes 0 equal, 1 different, 2 unreadable. The logic is in the package so a host Python console can run before/after without the checkout.
• Design by the deep tier (the manifest asks for it); implementation by the standard tier; the diff was reviewed before this PR and its one real finding (bracket in --ignore) is fixed.

How the builder is checked without a host: tests/resources/host_exports/{flame,hiero,resolve}_reference.py copy each host exporter verbatim (only host reads replaced, marked HOST-READ; the header records the host commit and git blob). The builder, fed by <host>_specs.py from the same synthetic input, is compared with each reference by otio_compare and each reference with a golden .otio written on OTIO 0.15. test_host_reference_sync.py checks the blobs recorded in the headers always and, with AYON_EDITORIAL_HOST_ROOT set to the folder holding the host checkouts, that the host files still have them (not run in CI). The <host>_specs.py files are the templates P2.7 to P2.9 copy into the host.

These references are modelled on the code, not exported from a host. The real proof is the before/after export of a human in each host (P2.7 to P2.9).

Found while writing the references:

• Flame's get_reformatted_filename in otio/utils.py calls .search on a plain string and would raise; it is only reached on OTIO without ImageSequenceReference, so it is dead today.
• Resolve's Start TC conversion fails on OTIO 0.18 for a rounded 23.976 rate (SMPTE timecode does not support it) and works on 0.15. If Resolve reports the rounded rate, newer OTIO breaks the export there; the reference uses the unrounded rate.

Testing notes:

1. ./tools/manage.sh run-tests (also run on Python 3.9 with OTIO 0.15, 0.16 and 0.17)
2. Optional: AYON_EDITORIAL_HOST_ROOT=<folder with the host checkouts> ./tools/manage.sh run-tests runs the host-sync checks.
3. python tools/otio_compare.py a.otio b.otio

Dependency

Refs #18

The issue stays open: its criterion "equal to fixture exports from each host" is met by references modelled on the host code, not by real exports. Close it when a human has done the before/after exports of P2.7 to P2.9.

Checklist

• ruff check and pytest pass
• python create_package.py --skip-zip builds
• No frozen contract changed
• Settings key renamed only with a conversion function (none renamed)
• Layer rules kept (tests/test_dcc_agnostic.py)

[jakubjezek001]

Cross-review finding 73-1: hiero_specs.py hardcodes hiero_source_type "VideoTrack" for every track, the host writes "AudioTrack" for audio tracks

tests/resources/host_exports/hiero_specs.py:227 (_marker_specs(track.get("tags", []), "VideoTrack", None))

The host stores item.__class__.__name__ as hiero_source_type (hiero_reference.py, create_otio_markers), so a tag on an audio track gets "AudioTrack". The P2.7 template passes the constant "VideoTrack" for all tracks. The fixture has no tagged audio track, so test_builder_hiero_matches_reference stays green. hiero_source_type is what the tag import uses to pick the target, so P2.7 would copy this template and silently change the marker.

Failing input (OTIO 0.18.1, py3.9): give the audio track of hiero_input.json "tags": [{"name": "AudNote", "icon": "icons:TagGreen.png", "in_time": 3, "metadata": {"tag.length": "2"}}], then compare_timelines(hiero_reference.export(data), hiero_specs.build(data)):

tracks.children[2:Track "Audio 1"].markers[0:Marker "AudNote"].metadata.hiero_source_type: changed "AudioTrack" != "VideoTrack"

Fix: pass the class name from the track kind ("AudioTrack" / "VideoTrack") and add a tagged audio track to the fixture and golden.

Severity: should-fix (template for P2.7, fixture gap hides it).

[jakubjezek001]

Cross-review finding 73-2: no test fails when compare stops at the shorter list (a missing or extra clip would compare equal)

client/ayon_editorial/otio/compare.py:205 (walk_lists)

compare is the gate for P2.7 to P2.9, and a dropped clip is the most likely adoption bug, yet nothing pins the length case. Mutation check: change range(max(len(left), len(right))) to range(min(...)) and run the whole suite (pytest tests, 460 tests on OTIO 0.15/0.17/0.18): everything passes. test_compare_timelines_missing_and_added_keys covers dict keys only, test_compare_timelines_list_order_matters covers equal lengths.

Failing input after the mutation: two timelines where the right one has one clip less in tracks.children[0].children; otio_compare a.otio b.otio exits 0.

Fix: add a test for a missing and an added list element (expect missing / added at ...children[1:Clip "x"]), also for markers and effects lists.

Severity: should-fix (the code is right today, the guard is missing).

[jakubjezek001]

Cross-review finding 73-3: otio_compare breaks its exit-code contract (1 different / 2 unreadable) for a missing OTIO and for non-OTIO JSON

client/ayon_editorial/otio/compare.py:24 (top level import opentimelineio), compare.py:296 (compare_files)

1. On an interpreter without opentimelineio the import fails before main(): Python exits with 1, the code that means "different". Anything wrapping the tool (CI step, adoption script) reads that as a real difference. Repro: /usr/bin/python3 tools/otio_compare.py a.otio b.otio; echo $? gives ModuleNotFoundError and 1.
2. otio.adapters.read_from_file returns a plain dict/list for valid JSON that is not OTIO, and normalise accepts it. Two identical non-OTIO files are "equal": echo '{"x":1}' > d.otio; otio_compare d.otio d.otio exits 0 (same for [], OTIO 0.15, 0.17, 0.18.1). A truncated or wrong export compared with the same wrong export passes the gate. It should be exit 2, "not an OTIO timeline".

Fix: catch the import error in the shim (tools/otio_compare.py) and main, return 2; reject results of read_from_file that are not otio.core.SerializableObject.

Severity: minor.

[jakubjezek001]

Cross-review finding 73-4: tools/otio_compare.py itself is untested; tests/AGENTS.md says its exit codes are

tools/otio_compare.py:19 (sys.exit(main())), tests/AGENTS.md row for test_otio_compare.py

test_otio_compare.py only calls compare.main(). Mutation check: replace sys.exit(main()) with main() in the shim and run pytest tests: all green, while the command now exits 0 for different files (so P2.7 to P2.9 scripts see "equal"). The path setup of the shim (package stub, Path(__file__)) is not exercised either.

Fix: one subprocess test: [sys.executable, "tools/otio_compare.py", a, b] with an equal pair (0), a different pair (1) and a missing file (2), run from a different cwd.

Severity: minor.

[jakubjezek001]

Cross-review finding 73-5: resolve_specs.py copies the host's 24 fps timecode bug for audio duration, against the time.py decision, and the fixture cannot see it

tests/resources/host_exports/resolve_specs.py:22 (_frames_from_timecode, used at line 69)

The host timecode_to_frames ignores its framerate and decodes at 24. otio/time.py (P2.2) deliberately follows Flame so "Hiero and Resolve become correct at any other rate". The Resolve template, which P2.8 copies, mirrors the bug instead of calling time.timecode_to_frames, and the PR body does not list it among the quirks kept on purpose (only the docstring of the spec does). The fixture has only 24 fps audio, so builder, reference and golden agree and nothing records the choice.

Failing input: properties["FPS"]="25"; properties["Duration"]="00:00:10:00" on the music clip of resolve_input.json:

reference and specs available duration: RationalTime(240, 25)
time.timecode_to_frames("00:00:10:00", 25) = 250

Ten seconds become 240 frames, a wrong available range (and wrong audio source range) for any non-24 Resolve audio. If it is kept for byte-identical adoption, list it in the PR body and add a 25 fps audio clip to the fixture so P2.8 has to decide; otherwise use the shared helper and say so.

Severity: should-fix (undeclared divergence, bug carried into the template).

[jakubjezek001]

Cross-review finding 73-6: reference provenance is inconsistent (Resolve utils blob, unmarked edits) and the sync check never runs in CI

tests/test_host_reference_sync.py:41, tests/resources/host_exports/resolve_reference.py:1-29, flame_reference.py line 903

I diffed every reference against the host blobs at the recorded commits: the copied code is verbatim, apart from these:

• The Resolve header says utils.py is copied from "the same commit" (59328ce), where its blob is ed00ac2. The sync test pins 450ec94, the blob at current develop, which has other lines (logging import, JSON guard in unwrap_resolve_otio_marker). Checking out the recorded commit makes test_host_file_is_the_copied_one[resolve-utils] fail, and the recorded line ranges (9-12, 24-53, 56-74) are those of ed00ac2. Flame utils.py has the same "same commit" wording with no blob in the header.
• flame_reference.py:903 appends # codespell:ignore to a host comment, not marked HOST-READ, although the header says every changed line is. Harmless, but the claim "verbatim, only HOST-READ lines changed" is not exact.
• hostsync tests are skipped unless AYON_EDITORIAL_HOST_ROOT is set, and no workflow sets it, so "fails when a host file changed" holds only on a developer machine. Whole-file blobs also trip on unrelated host edits (for example the marker unwrap that is not copied), so a warning there will be ignored.

Fix: record the blob that matches the recorded commit (or move the recorded commit forward), record a blob for each helper file in each header, mark or drop the codespell edit, and either run the sync job in CI against the host develop or hash only the copied line ranges.

Severity: minor.

[jakubjezek001]

Cross-review finding 73-7: tests/AGENTS.md is stale for this PR, and an unrelated roadmap status is flipped

tests/AGENTS.md:16 (table "Existing tests"), .github/roadmap.yml:194

• The table lists test_otio_builder.py and test_otio_compare.py but not test_otio_builder_{flame,hiero,resolve}.py, test_host_reference_{flame,hiero,resolve}.py, test_host_reference_sync.py, nor resources/host_exports/ (inputs, references, specs, goldens). The "Fixtures" section still says "one folder per host". The new hostsync marker and AYON_EDITORIAL_HOST_ROOT are only documented in the PR body, the issue AC "module map updated" is met for the client map only.
• roadmap.yml changes P1.1 from In review to Done in this PR, which has nothing to do with P2.6.

Severity: minor.

[jakubjezek001]

Cross-review finding 73-8: acceptance criteria 1 and 2 of #18 are not literally met, yet the PR says Close #18

Issue #18 asks for: "Builds timelines equal (by otio_compare) to fixture exports from each host" and "Tests written first (characterization tests when porting host code)".

• The builder is compared with <host>_reference.py, the host code run on a synthetic dict through fake host classes, not with an export from a host. The PR body says so ("modelled on the code, not exported from a host"), and the fakes encode assumptions that I could not check without the hosts. Examples: Hiero TrackItem.framerate() returns the source clip's rate, a Hiero track has no framerate (so track tags use the timeline rate), linkedItems() is modelled by item name (duplicate names in a track would all match), Resolve GetSetting("timelineFrameRate") is a number (the real API usually returns a string; the host passes it unconverted to value_rescaled_to). A wrong assumption makes builder, reference and golden agree on the wrong output.
• Order: d30e3c4 adds the builder with its own tests; the characterization fixtures (9a98907) come after it, so the builder was not written against them.

I did not find a divergence between builder and reference on 900 randomised numeric variants of the three inputs (record gaps, offsets, speeds, overlaps) or on a multi-channel Resolve audio track, so I do not claim a behaviour bug here. The deliberate host quirks (negative record gap folds an overlap onto record_in, no - 1 after previous out 0, track_end in timeline units) are justified as host compatibility for the adoption steps.

Fix: either keep Close #18 and tick the criteria with a note that the host before/after export of P2.7 to P2.9 is the real fixture (human sign-off), or change the PR to Part of #18/amend the criteria so the issue does not close on references alone.

Severity: should-fix (process, needs an explicit decision).

[jakubjezek001]

Author answers to 73-1 to 73-8, fixed in 8ba3d9c (head of this PR) unless noted.

• 73-1: fixed. The fixture has a tagged audio track (AudNote), the Hiero specs pick AudioTrack / VideoTrack from the track kind, the golden was regenerated by the reference on OTIO 0.15.
• 73-2: fixed. Tests for a missing and an added list element (clips, markers, effects). Checked the mutation: max -> min now fails 4 tests.
• 73-3: fixed. compare_files raises ValueError("... not an OTIO timeline") for JSON that is not an OTIO object (exit 2 in main); tools/otio_compare.py exits 2 with a message when opentimelineio cannot be imported.
• 73-4: fixed. Subprocess tests of the shim from another folder: 0 equal, 1 different, 2 missing file, and 2 without OpenTimelineIO.
• 73-5: kept on purpose, now declared. The template copies the host's 24 fps decoding so the adopted Resolve export stays equal to today's. It is listed in the PR body as a kept quirk, the specs docstring says it is wrong, and test_resolve_audio_duration_is_decoded_at_24_fps pins 240 vs 250 so P2.8 has to decide.
• 73-6: partly. The reviewer's line ranges are those of the develop blob, not of ed00ac2: the Resolve header's utils.py ranges (9-12, 24-53, 56-74) are for blob 450ec94, which the sync test pins, and the three copied functions are identical in ed00ac2 at the recorded commit (the diff only adds the logging import and a JSON guard elsewhere). The header said "same commit", which was wrong; Flame and Resolve headers now record the blob of utils.py and the sync test checks the header for them. The codespell:ignore edit is named in the Flame header. Not done: running the sync in CI (it needs the host checkouts); the description now says it is not run in CI.
• 73-7: fixed for tests/AGENTS.md (new tests, host_exports/, the hostsync marker). The P1.1 -> Done line in roadmap.yml stays: P1.1 (PR [Editorial P1.1]: Shared OpenTimelineIO install pre-launch hook #65) is merged, so In review was stale; it lives in this PR's manifest commit.
• 73-8: agreed, it was a process decision. The description now says Refs instead of Close, and why; the issue stays open until a human has done the before/after exports of P2.7 to P2.9. Left as is: the fixtures were added after the builder commit; the references are compared on about 900 variants by the reviewer and no divergence was found, but they do not replace a real export. This deviates from the usual Close ending; tell me if you want Close back.

[jakubjezek001]

Follow-up to the review comment on #69 about shot and version names: the host fixtures now use shot0010_v0010_[1001-1024].exr (Flame), shot0020_v0010.[0001-0100].exr (Resolve) and shot0020_v0010.%04d.exr (Hiero) for their sequences. Reference and builder still agree with no difference, and the regenerated goldens differ only in name_prefix. No code change.