fix(video-player): loop where both tracks still have content - #6430
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
9227dfb to
a7d1567
Compare
This comment has been minimized.
This comment has been minimized.
|
@hm21 I took over the review fixes for the clamp policy and pushed commit a7d1567. What changed:
Verification I ran:
I did not approve this yet because the remaining action is native device verification on the repro assets in the PR body, especially the pathological 3df3a7a7… asset. Please verify those on device and merge when you're satisfied with the playback behavior. |
rabble
left a comment
rabble
left a comment
There was a problem hiding this comment.
LGTM. Opt-in trimToCommonTrackEnd clamps the loop point to the shorter track's end, bounded <=500ms and <=10% of playable duration; Android probes local files only; Apple fail-open catch present; feed opts in at all clip sites; editor multi-clip paths untouched. Cross-PR note: textually conflicts with #6429 at three shared sites — semantically composable (effectiveEndMs = min(endMs, commonEndMs) on both platforms), but the second to merge needs a rebase. Nit (non-blocking): DivineVideoPlayerInstance.kt:427 runs MediaExtractor.setDataSource synchronously on the platform thread per clip — typically <5ms for a local header parse, fine as-is, but the docstring undersells that the local probe is I/O; consider moving it off main or documenting the measured cost.
An mp4's declared duration is its longest track, and capture and export pipelines routinely stop the audio and video tracks tens of milliseconds apart. Playing to the container duration therefore ends on a stretch where one track has already run out — silence, or a frozen last frame. On the feed's looping player that stretch is replayed every cycle, which is the seam reported in #6386. A survey of 622 published uploads plus 58 archived Vines found 12-19% of assets with a >=40ms mismatch, the worst at 280ms. It is a property of the asset, not of the player, so it cannot be fixed by re-encoding what is already published: Blossom is content-addressed, and the sha256 is the identity. Clips can now opt into ending where every track still has content, and feed playback does. Apple takes the minimum of the two track ends when building the composition, which costs nothing because both tracks are already loaded there. Android reads the per-track durations with MediaExtractor and passes the minimum to the clipping configuration — for local files only, because probing a remote source would block playback start on a network round trip, and the feed prefetches to disk anyway. Clamping only ever shortens; an explicit trim that ends earlier still wins.
a7d1567 to
f963cfc
Compare
Mobile PR PreviewPreview refreshed for Last refresh:
|
3 participants
Description
An mp4's declared duration is its longest track, and capture and export pipelines routinely stop the audio and video tracks tens of milliseconds apart. Playing to the container duration therefore ends on a stretch where one track has already run out — silence, or a frozen last frame. On the feed's looping player that stretch is replayed every single cycle. That is the seam behind #6386.
I measured it before changing anything.
ffprobeover 622 published Divine uploads and 58 archived Vines (audio track length minus video track length, which is exactly the seam):So it is a property of the asset, not of the player — which is why it never reproduces on a random scroll through the feed, and why it needs a fix that does not depend on re-encoding. Published assets cannot be repaired anyway: Blossom is content-addressed and the sha256 is the identity, so a re-encode is a different file and a different event.
Clips can now opt into ending where every track still has content, and feed playback does. Clamping only ever shortens, and an explicit trim that ends earlier still wins.
The clamp is intentionally bounded so malformed assets fail open instead of collapsing into tiny loops. Native players trim only when the removed tail is at most 500 ms and at most 10% of the playable duration. That keeps the measured seam cases, while leaving the pathological
3df3a7a7…asset at its container duration.Apple takes the minimum of the two track ends when building the composition, then applies the bounded policy. If optional track-range loading fails, playback continues untrimmed rather than failing composition.
Android reads the per-track durations with
MediaExtractorand passes the bounded minimum to theClippingConfiguration. The ExoPlayer timeline only exposes the container duration (the longest track), andFormatcarries no duration, so there is no cheaper source for this. It is applied to local files only: probing a remote source means a network round trip on the platform thread before playback can start. The feed prefetches to disk, so the common path is local; an uncached first play keeps the old behaviour rather than paying a startup stall.Related Issue: Closes #6386
Out of Scope
The export side. iOS is not just inheriting bad source files, it also creates the mismatch:
VideoSequenceBuilderinpro_video_editorclamps the clip range to the video track's range and then reuses that same range for the audio insert without intersecting the audio track. When the source audio is shorter,insertTimeRangesilently truncates. That matches the measured signature exactly — 192 of 225 non-zero iOS deltas are "audio shorter", and the 38 % that land on exactly 0 ms are the clips whose audio fills the range anyway. Android's residual is different in kind: never exactly 0 (2 %), but bounded and sign-balanced (21 ms median, 121 ms worst) — Media3 Transformer per-track frame quantisation, not a defect.The fix for that is hm21/pro_video_editor#182 (2.10.0, unreleased); wiring it here needs that release and a constraint bump. It only helps new uploads, so it does not overlap with this PR.
One genuinely broken asset turned up in the survey:
3df3a7a7…, 6.37 s of video carrying 9 frames with 0.37 s of audio. This PR now treats that kind of mismatch as malformed and keeps the container duration.Verification
flutter analyze packages/divine_video_player/lib packages/divine_video_player/test packages/infinite_video_feed/lib packages/infinite_video_feed/test/src/utils/source_loader_test.dart— cleanflutter testinpackages/divine_video_player— 262 passedflutter test packages/divine_video_player/test/src/video_clip_test.dart packages/divine_video_player/test/src/loop_seam_trim_contract_test.dart— passed after rebaseflutter test test/src/utils/source_loader_test.dartinpackages/infinite_video_feed— passed after rebaseswiftc -parse packages/divine_video_player/darwin/divine_video_player/Sources/divine_video_player/DivineVideoPlayerInstance.swift— cleanAndroid Kotlin compile was not locally runnable in this checkout: there is no Gradle wrapper, and the standalone plugin Gradle project cannot resolve Flutter's
dev.flutter.flutter-plugin-dependenciesplugin from the local Flutter SDK. CI's Android unit/build checks should cover this path.Device verification still recommended before merge because the observable behavior is native playback. Best repro assets, worst first:
06c68e6645cfecd2a3a306f7ddff7bf0b289c5c1fbba1d10bbc05a338c35d2af(-280 ms, audible gap)91909f52ded5efd8b5d715daf673bdf849e2f47eaa86fd69fa6414d62ebdb60d(-148 ms)4804ef2c11b02d5d2aa14286e0fe1f5a4f673029f10f714fd7977d93b401c1af(+108 ms, visible frozen frame)3df3a7a7…pathological short-audio asset, which should now keep its container durationWorth checking alongside: that an editor trim still lands where the user set it, and that a stop-motion clip held past a short sound is not shortened.
Type of Change