Fix playback crashes, desync, and stalls found in a real-footage stability review - #79
Merged
Merged
Conversation
Flyleaf interrupts the demuxer's in-flight read on every seek. When that read is FFmpeg's concat demuxer opening the next chunk file, the half-opened input makes the next av_seek_frame crash the process with an access violation. Scrubbing a real 11-chunk clip crashed 3 of 8 stress runs with interrupts on and 0 of 12 with them off; the same footage stream-copied into one file per camera never crashed. Interrupts only help cut short slow network reads, and every input here is a local file.
Flyleaf's default only publishes CurTime when the whole second changes, so the seek bar and time readout stepped once a second, and the controller's end-of-clip checks worked from a position up to a second stale. Players now publish on the engine's refresh tick, which batches every player's time into one UI update, and the tick runs every 100 ms.
The controller had no single owner for its state. Flyleaf raises end-of-stream and failures on its own play thread, the controller mutated state from there, and the view-model forwarded those changes with a synchronous Dispatcher.Invoke. Meanwhile Flyleaf's Play and Pause spin on the calling UI thread until that play thread exits, so a Space press as a clip ended or hit a corrupt chunk could deadlock the app. Recovery then ran on the thread pool, driving players concurrently with the UI thread. Request ids, ended counters, opening flags, and locks had been added one race at a time. FlyleafCameraPlayer now runs every blocking Flyleaf call on the thread pool, serialized per camera, and posts every event back to the UI dispatcher, dropping events from media that has since been closed. Seeks complete when Flyleaf presents the frame rather than when the request is queued. VideoPlayerController keeps its public API but is rebuilt around sessions: each opened clip is a session with its own cancellation, every player command runs as a serialized operation, and player events queue behind the running operation and re-check the player's real state before acting. Repositioning pauses every camera, seeks them all to the same instant, and starts them together, which also keeps seeks off Flyleaf's play-thread path. The scrub gesture holds playback paused and resumes on release. Found with a harness driving the real window against real footage: - Side cameras drifted 0.7 to 1.2 s from the front after quick play/pause presses and never caught up. - After a clip ended, seeks moved only the front camera. - Clip switches stopped every player three times, one after another; players now close once, in parallel, and time from click to playing went from 2.1 s to about 0.7 s. - Flyleaf's Stop resets the renderer on a D3D device shared by every camera, so closes are serialized globally to avoid a crash in the swap chain release. The fake player now models an ended player ignoring Play, a failure closing the player, and a closed player ignoring seeks, and the controller tests await an idle barrier instead of polling.
Each seek pauses, seeks, and resumes every camera, which takes a few hundred milliseconds. A held arrow key or fast clicks queued dozens of them, and the video kept jumping for seconds after the input stopped. A seek that is still waiting to run is now retargeted instead of queued again, so only the latest target runs. Arrow keys go through SeekByAsync, which measures from where a waiting seek is headed, so every press still moves five seconds.
Tesla cameras drop frames in different places, so stepping each camera on its own let them drift apart: ten steps forward and back left the rear camera a third of a second off the front on real footage. Backward steps could also get stuck, because Flyleaf's accurate seek presents the first frame no earlier than half a frame before the target, and a one-frame step can't cross a gap where a frame was dropped. The front now steps and the side cameras follow its clock: forward, they step too and are reseeked only if they slipped more than a frame and a half; backward, they seek straight to the front's new frame. A backward step that doesn't move retries with a wider step until it crosses the gap. On real footage, forward and backward steps now visit exactly the same frames in reverse, with every camera within a frame of the front.
Space while a clip was loading called PlayAsync, which saw a session that wasn't open yet and started the whole open over, throwing away the build and camera opens already in flight. The clip plays as soon as it is ready, so play during an open is now a no-op; a clip whose open failed still reopens.
Selecting a clip yields to the UI before it loads. Pressing Stop in that window stopped nothing, and the load then started and played the clip anyway, with the loading overlay left stuck on. Stop now cancels the pending selection load and clears the loading state.
After clicking any control-bar button, keyboard focus stayed on it, so Space clicked that button again instead of toggling playback (for example, dropping the speed a second step), and buttons ate the arrow keys for focus navigation. Shortcuts were handled on the bubbling KeyDown, and the handler also set e.Handled after an await, when WPF had already finished routing the key. Shortcuts now run from the tunneling PreviewKeyDown, ahead of the focused control. The view-model resolves the key synchronously and reports whether it was a shortcut, so the view can mark it handled before any work is awaited. Keys that aren't shortcuts, and every key while typing in the search box, still reach the focused control.
Sampling the UI thread during the one-second stall after the clip list loaded showed it inside ThumbnailConverter every time: for a UriSource, WPF asks URLMON which security zone the file belongs to, a COM round trip per thumbnail. Reading the file through StreamSource skips the zone check and brought the stall from about 1.0 s to about 0.75 s on a 174-clip library. IgnoreImageCache is dropped with it, since the image cache is keyed by URI and WPF throws evicting a null key when there is only a stream; the converter's catch-all turned that into blank thumbnails. The new test decodes a real PNG and also checks the file isn't left locked, which would block deleting the clip.
The file-name pattern was unanchored and took everything after the timestamp as the camera. On a real library: - Copying or merging a drive leaves "-2" twins such as front-2.mp4, which became extra cameras named "front-2" in 28 clips, so the builder probed and wrote playlists for twice the files. - macOS "._" resource-fork files matched too, and on NTFS the "._" twin enumerates first, so keep-first handed the player a 4 KB metadata file and the chunk was dropped as unreadable. The pattern is now anchored, case-insensitive, and parses the "-N" suffix as a copy number of the same camera. When a camera has several files at one timestamp, the original wins unless it is empty, in which case the copy is used, and a chunk whose only front file is a copy is kept.
Mp4DurationReader only caught IO and access errors, so a corrupt header could throw out of the media source build and fail the whole clip with a playback error instead of skipping one chunk: - A 64-bit duration too large for TimeSpan threw OverflowException. - A box size near the Int64 limit wrapped the next read position negative and threw ArgumentOutOfRangeException. Boxes that claim to run past their parent are now rejected and those exceptions mean "no readable duration". The builder also rejects a chunk claiming more than ten minutes, since a header saying hours would otherwise stretch the timeline with footage that isn't there.
An IOException reading event.json (a bad sector, or the file held open elsewhere) escaped CamEvent.FromFile, and CamClip.TryMap then dropped the whole folder, hiding playable footage because its optional metadata couldn't be read. The clip now loads without event details and the failure is logged.
- The two event-jump tests ran with no media open and only checked that the seek bar fraction was assigned, and they duplicated each other. One test now opens a clip and checks that pressing E moves the players to the event moment. - SelectingClip_TriggersPlaybackLoading never reached playback; it is renamed to what it checks. - Two coalescer tests slept 50 ms to prove a seek didn't happen. The gate's continuation runs inline on SetResult, so the assertion can follow immediately with no delay.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A stability review of playback, driven through the real window with real Flyleaf players against a 253 GB TeslaCam library, plus manual testing. Each fix is its own commit so history stays readable; merge without squashing to keep it.
Playback
VideoPlayerControllerandFlyleafCameraPlayeraround one owner thread (public API unchanged). Flyleaf raises end and failure events on its play thread while its Play/Pause spin on the UI thread, which could deadlock; events are now posted to the UI thread, every blocking Flyleaf call runs off it, and player commands run as serialized operations per clip session. Every reposition pauses all cameras, seeks them to one instant, and starts them together.UI
Data
-2duplicate files and macOS._files no longer become fake cameras or replace the real front video.event.jsonno longer hides the clip.Tests
The fake player now models an ended player ignoring Play, failures closing the player, and closed players ignoring seeks, and controller tests await an idle barrier instead of polling. Tests that passed without exercising their behavior were replaced. 393 tests, each commit builds and passes on its own.
How to verify