Conversation
A card offload knows its source: everything under one root, and the only
question is whether it arrived intact. This is the other job, the one that
turns up when a cut comes back from an editor. Which files does this timeline
need, and are they all here?
offloader resolve --timeline "01 Chairs Row V6.xml" --search-root E:\ChairsDoc
offloader offload --timeline "..." --search-root E:\ChairsDoc --dest E:\...
Reading timelines is OpenTimelineIO's job and nothing else's, behind a new
[timeline] extra. The adapters are not in OTIO's core distribution, so a bare
install reads .otio and none of the formats an NLE exports; the extra names
otio-fcp-adapter explicitly.
OTIO is trusted with media references and not with structure. Measured against
a Premiere-exported FCP 7 XML of 913 clipitems, it recovered all 334 unique
references with none missed and none invented, including the clipitems whose
<file> is an id reference carrying no path of its own. On the same file it
reported 29.97 fps and 12 audio tracks for a sequence that declares 24 and 23.
So no rate, duration or track count from the adapter reaches a caller.
The resolver is the part worth having. Six statuses, and the order of the
tests is the design: a generated reference stops at once; anything under a
search root with that basename wins whatever path the timeline asked for; a
camera original may be satisfied by a proxy if the frame counts agree; only
then is a readable file a gap.
It refuses to choose between two files that share a name. On the conform this
was written against, 20 basenames had more than one copy under the search
root. Seven were byte-identical and harmless. Thirteen were different files,
eleven of them MISSING MEDIA stand-in slates from an earlier pass sitting
beside the real archival footage that arrived later. Relinking by filename
picks one at random, and when it picks a slate the clip reports as online.
The closest-path candidate is offered as an explicitly labelled guess; it was
right all thirteen times, which is a reason to show it and not a reason to act
on it unsupervised.
Byte-identical duplicates resolve normally and are not copied again. On that
job that avoided 1.4 GB of pointless transfer and, more to the point, avoided
manufacturing 32 filename collisions on a drive that had none.
A proxy standing in for a camera original is checked frame for frame. A proxy
one frame short moves every edit point after it. Where the count cannot be
established the note says unverified rather than implying agreement; where it
is established and disagrees, nothing is substituted.
Engine: OffloadOptions.selection takes an explicit file set in place of
scanning one root, so a job can span volumes. Each file carries its own
destination-relative path and the engine infers no layout.
The safety rule had to change, and narrowing it was the point.
assert_safe_destinations refuses a destination inside the source, which is
right for a card because the source root is the card. It is wrong for a
selection, where the search roots are a library being read and collecting a
cut's gaps into a folder on that same drive is the ordinary case. A selection
is held instead to the property that actually matters and is checkable
exactly: no file being read may sit at or beneath somewhere being written. A
destination-relative path that is absolute, or climbs out with .., is refused
before the job starts rather than partway through it.
33 tests. The eight that exercise the selection path fail without the engine
change. Verified end to end against two real timelines: 334 references
reconciled file by file against an independent hand-built list, zero missed
and zero extra, 15 proxy substitutions all frame-verified.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
CI on macOS and Linux caught this; Windows passed. After the scheme and any localhost authority, what is left always begins with a slash: /E:/Media/a.mov, or /Volumes/Edit/a.mov. That slash belongs to a POSIX path and must stay, but on a Windows path it sits in front of the drive letter and must go. Stripping it unconditionally turned /Volumes/Edit/a.mov into the relative path Volumes/Edit/a.mov. The failure is close to invisible, which is why it earns a long comment. A relative path still resolves by basename, so a resolve run looks entirely correct; what breaks is every test of whether a file is where the timeline says it is, so a reachable file reads as missing rather than as a gap. An editor cutting on a Mac addresses every clip that way, so this is the common case, not an edge. Tested as a pure function over both platforms' forms, including UNC and percent-escapes, so the coverage no longer depends on which runner it lands on. Re-checked against the two production timelines: 327 references, zero status changes.
JobControl has existed since the desktop app needed transport buttons, and run() checks it once per 8 MiB chunk, but cli.py never passed one. So a job started in a terminal could only be killed: the capability was there and nothing reached it. --control-file PATH wires a FileControl to a file, and "offloader control PATH --pause|--resume|--cancel" drives it from anywhere. A file rather than a signal or a keypress because Windows has almost no signal support beyond SIGINT, a 1.9 TB offload is started detached or over ssh with no console to type into, and a file needs no port and no daemon, survives the terminal closing, and can be read to see what a job is doing. The failure modes are the design. The file holds one word; anything else - empty, garbled, half-written, or momentarily unreadable because another process has it open - is no opinion, and leaves the job in whatever state it is already in. Inferring cancel from a damaged control file would let a stray byte stop an offload that is nine hours in, and a small text file is exactly what a sync client or an editor rewrites in two steps. The control command writes through a staging file and renames it into place, so a job polling between chunks cannot read a truncated instruction. Starting a job claims the path by writing run to it first, or a stale pause from a previous job would stop the next one before it copied a byte with nothing on screen to say why. A paused job keeps polling, so it can still be cancelled; JobControl alone would block inside Event.wait() and never see it. 11 tests. The cancel tests drive the transition from the progress callback rather than after a sleep, because a dozen 400 KB files finish faster than any wall-clock guess; the suite was run five times over to confirm it. One of them also covers the handler never being registered in main(), which argparse hides: --help works from the parser alone, so the command looks fine until it is actually run, which is how that shipped broken for an hour here.
owenpkent
left a comment
owenpkent
left a comment
There was a problem hiding this comment.
Reviewed timeline parsing/resolution, selected-file copying, source/destination validation, control-file handling, CLI verdicts, and their tests. Two malformed-control-file defects are detailed inline. Additional timeline completeness/missing-media findings and synthetic reproductions are supplied privately under SECURITY.md.
The current head also has a merge-blocking CI problem: Ubuntu 3.10, Ubuntu 3.13, Ubuntu with ffmpeg, and macOS fail the new pause/cancel integration tests. For example, the macOS job fails at test_control.py:215 and :264. The small fixtures can finish before FileControl's minimum 50 ms polling interval expires, even when the test requests poll=0.0. Make the tests deterministic with an injected clock/poll mechanism or a synchronized blocking reader, then require the affected platform jobs to pass. Windows passing by timing does not establish the intended behavior.
Local validation here used focused synthetic scripts against this exact head and read-only inspection of the existing CI failure logs. A new full local suite was not run.
| return self.RUN | ||
| except OSError: | ||
| return None | ||
| word = text.strip().lower().split() |
There was a problem hiding this comment.
[P2] Validate the entire control-file state instead of its first token.
Splitting the text and accepting word[0] treats malformed content such as cancel pending upload as an actual cancellation command. A direct call to FileControl.read() on that content returns cancel, although the CLI/docs promise that anything other than the single states run, pause, or cancel is no change of state.
An editor/sync operation that leaves trailing content can therefore stop a running offload instead of being ignored. Compare the entire stripped, case-normalized value against STATES. Add regressions for trailing tokens after all three valid words, alongside the existing whitespace/case tests.
| except FileNotFoundError: | ||
| # Deleting the file releases the job rather than stranding it. | ||
| return self.RUN | ||
| except OSError: |
There was a problem hiding this comment.
[P2] Treat invalid UTF-8 control contents as no state change.
read_text(encoding='utf-8') can raise UnicodeDecodeError, which is not an OSError and escapes this handler. I reproduced this with a control file containing the two bytes FF FE: FileControl.read() raises instead of returning None. A damaged control file can consequently abort a checkpoint/transfer even though malformed or temporarily unreadable control content is documented as non-authoritative.
Catch decoding errors with the read errors and preserve the current state. Add a regression using invalid UTF-8 bytes while a job is running or paused, asserting that the checkpoint neither changes state nor raises.
Two ways a damaged control file could stop a running offload, both of them the opposite of what the file's own docstring promises. read() compared word[0] after splitting, so "cancel pending upload" - the kind of thing an editor or a sync client leaves behind - read as a cancellation. It now compares the whole stripped, case-folded value against the three states, so anything with a token after it is no opinion. read_text(encoding="utf-8") raises UnicodeDecodeError, which is not an OSError and escaped the handler beside it. A control file holding two bytes of FF FE aborted the checkpoint instead of being read as damage. Caught now alongside the read errors, leaving the job in whatever state it was in. The pause/cancel integration tests were failing on Ubuntu 3.10, Ubuntu 3.13, Ubuntu with ffmpeg and macOS while passing on Windows, which is timing rather than behaviour. poll has a deliberate 50 ms floor, so poll=0.0 did not mean what those tests assumed: sync() skips the read until the interval elapses, and ten 400 KB files can finish between two reads without ever seeing the word written midway. FileControl now takes an injectable clock, defaulting to time.monotonic, so a test can decide the limiter has elapsed instead of hoping the machine is slow enough. Production behaviour is unchanged and the floor stays. The pause tests also write their instruction from the progress callback rather than from the main thread after start, the way the cancel test already did, and write it once: several events fire per file, and rewriting pause after the resume put the job straight back to sleep. Five regressions: a trailing token after each of the three valid words, invalid UTF-8, and a paused job held through both kinds of damage without cancelling. The control suite runs in 2.1s rather than 22.5s, which is the wall-clock the races were costing, and was run five times over to confirm it.
1 participant
Stacked on #5, so it targets
proxies-first. Review that one first.A card offload knows its source: everything under one root, and the only
question is whether it arrived intact. This is the other job, the one that
turns up when a cut comes back from an editor.
Which files does this timeline need, and are they all here?
resolvecopies nothing and answers the question.offloadcopies thedifference, with the same verified copy, checksums and reports as a card.
What OpenTimelineIO is trusted with
Reading. Nothing else, and the boundary is measured rather than assumed.
Against a Premiere-exported FCP 7 XML of 913 clipitems, the
fcp_xmladapterrecovered all 334 unique media references, none missed and none invented —
including the clipitems whose
<file>is an id reference carrying no path ofits own. A sweep that reads only
pathurlfinds a source's first use andsilently misses every later one, which is how a file gets left out of a
package.
Its reading of structure, on the same file, is wrong:
So nothing here reads a rate, duration, track or timecode from the adapter, and
none reaches a caller. If that ever changes it needs its own measurement first.
The adapters are also not in OTIO's core distribution — a bare
pip install opentimelineioreads.otioand none of the formats an NLEactually exports — so the
[timeline]extra namesotio-fcp-adapterexplicitly, and
docs/timeline.mdcarries the table for FCPXML, EDL and AAF.Why
ambiguousrefuses instead of choosingThis is the part that earns the feature.
On the conform it was written against, 20 basenames had more than one copy
under the search root. Seven were byte-identical, which is harmless. The
other thirteen were different files sharing a name:
sitting beside the real archival footage that arrived weeks later.
(
f2/260223_009.WAVandf3/260223_009.WAV) — different recordings thathappen to be named alike.
Relinking by filename picks one at random. When it picks a slate, the clip
reports as online, and the cut looks fine until someone watches it.
So the resolver reports every candidate and copies nothing. It offers the
candidate whose trailing path components match the timeline's for longest, as
an explicitly labelled guess. That heuristic was right all thirteen times,
which is a good reason to show it and not a good enough reason to act on it
unsupervised.
Byte-identical duplicates are not ambiguous: they resolve to one copy and are
not offloaded again. On that job this avoided 1.4 GB of pointless transfer and,
more to the point, avoided manufacturing 32 filename collisions on a drive that
had none.
Proxies may stand in, but only frame for frame
A timeline referencing 55 GB of BRAW usually references it because that is what
the camera wrote, not because the cut needs it — the editor cut proxies, and
the proxies are already on the drive.
substitutedmeans a stem-matched proxy was found and the frame countsagree. Relink a clip to a file one frame shorter and every edit point after
it moves, quietly. The count comes from
probe.py, which reads BRAW out of thecontainer because ffprobe cannot open the format at all. Where a count cannot
be established the note says
unverifiedrather than implying agreement; whereit is established and disagrees, nothing is substituted and the reference is
reported
ambiguous.Engine: an explicit file set
OffloadOptions.selectiontakes(source, relative, root)in place ofscanning one root, so a job can span volumes. Each file carries its own
destination-relative path; the engine infers no layout.
The safety rule had to change, and narrowing it was the point.
assert_safe_destinationsrefuses a destination inside the source. That isright for a card, where the source root is the card and writing into it is
how you lose it. It is the wrong question for a selection, where the search
roots are a library being read and collecting a cut's gaps into a folder on
that same drive is the ordinary case, not a mistake:
A selection is held instead to the property that actually matters and is
checkable exactly: no file being read may sit at or beneath somewhere being
written. That admits the case above and still refuses the one that destroys
data. A destination-relative path that is absolute, or that climbs out with
.., is refused before the job starts rather than partway through it. Twoselected files landing on one path are refused by the existing collision check,
which matters more here than for a card: drawing from several volumes at once
makes a name clash the normal case rather than the unlucky one.
Testing
33 new tests, 630 in the suite, ruff clean.
Per CONTRIBUTING, the eight tests that exercise the selection path were checked
to fail without the engine change —
engine.pystashed, suite re-run, alleight failed, restored.
The resolver tests need no OpenTimelineIO: they build
Referenceobjectsdirectly, because what is under test is the decision, not the parse. Only the
reader tests need the adapter, and they skip without it.
Verified end to end against two real timelines: 334 references reconciled file
by file against an independently hand-built list — zero missed, zero extra —
15 proxy substitutions all frame-verified, and the ambiguity list reproduced
exactly. Running it also corrected two counts in the hand pass it was checked
against, which is the argument for having it.
Not in scope
between two files that share a name has a person's judgement in it, and a
tool that made that choice silently would be worse than no tool.
presentandsubstitutedarenot offloaded, because copying them would put a second file of that name on
the drive and create the ambiguity this module exists to detect.