fix(feed): cap playback length with native clipping - #6429
Merged
Conversation
#5551 removed maxLoopDuration to kill the audible loop seam its Dart-side seekTo(zero) produced, and the product rule that setting also enforced went with it: nothing in the feed plays longer than a Vine. A 60s file referenced by a foreign client's kind-34236 event has played in full ever since — there is no duration guard anywhere in the app today. Restore the cap as a native clip end so the loop point lives in the platform player instead of a Dart roundtrip: Android clips the MediaItem and repeats it, iOS trims the composition AVPlayerLooper loops over. The cap is 7s rather than VideoEditorConstants.maxDuration, which is the 6.3s recording limit — classic Vine assets measure 6.500-6.533s, so reusing it would cut the musical loop point off every one of them. Every path that re-opens a player applies the cap (first load, cache hit, source failover, HTTP-202 processing retry). The contract test counts clip constructions against cap applications so a future path cannot skip it silently. AVFoundation clamps what insertTimeRange actually inserts, but the Swift side computed clipDuration from the requested end regardless, so a capped clip would have reported 7.0s for a 6.533s asset and desynced every duration readout. Clamp endTime to the asset duration; ExoPlayer already clamps its ClippingConfiguration the same way.
This comment has been minimized.
This comment has been minimized.
The feed now caps every clip at 7s without knowing the source length, so a backend that reports the requested end as the duration inflates it for every shorter video. Android does this in ClippingTimeline and Apple got the clamp in the previous commit; the web backend returned `clipEnd - clipStart` unconditionally. On app.divine.video that made every feed video report 7.0s. Auto-advance arms at `duration - 1s`, so a 3s video never reached 6s, never armed, and the feed stopped advancing — plus every progress readout was wrong. The clamp lives in a plain Dart file rather than inline in the `_web.dart` backend: that file cannot be imported from the VM, and `element.duration` is read-only in a browser test, so an inline clamp would have stayed untestable.
The cap guards matched raw file text, so commenting out the cap left them green: `// end: widget.maxPlaybackDuration` keeps the substring, both sides of the parity count stay equal, and the feed plays uncapped. Verified by mutation before and after. They now read through `scripts/lib/dart_code_only.awk`, the same filter the design-system ratchets use, instead of a second hand-rolled stripper. The clip regex widens to `VideoClip[.(]` so the unnamed constructor and the asset/memory helpers cannot drop out of both counts together. The app-side wiring moves from a grep to an assertion on the pumped InfiniteVideoFeed, which no textual edit can defeat. The Apple clamp gets a source contract next to the existing composition-guard one — it has no Dart runtime surface and the package runs no Swift job, so nothing else would notice a refactor dropping it.
This comment has been minimized.
This comment has been minimized.
hm21
marked this pull request as ready for review
July 27, 2026 08:55
Mobile PR PreviewPreview refreshed for Last refresh:
|
rabble
approved these changes
Jul 27, 2026
rabble
left a comment
rabble
left a comment
Member
There was a problem hiding this comment.
LGTM. 7s native clip-end cap threaded through all four feed open paths; Swift clamps end to asset duration; web duration resolver clamps; parity contract test pins every clip site. Cross-PR note: textually conflicts with #6430 in source_loader.dart, infinite_video_feed.dart, and DivineVideoPlayerInstance.swift — semantically compatible (both only shorten the end), but whichever merges second will need a rebase at those three sites. Nit (non-blocking): feed_looping_contract_test.dart shells out to awk with CWD-relative paths — fine in CI (runs from mobile/) but non-hermetic; consider resolving paths from the script location.
7 tasks
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
2 participants
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.
Description
Restores the feed-side guarantee that nothing plays longer than a Vine, implemented as native clipping rather than the Dart-side
seekTo(Duration.zero)that #5551 removed.Why the rule went missing. The old
maxLoopDuration: VideoEditorConstants.maxDurationdid two jobs at once: it enforced the product rule (no 30/60s videos in the feed, including kind-34236 events from foreign clients pointing at arbitrarily long files) and it implemented that rule as a Dart seek. The seek was the direct cause of the audible loop seam in #5544, so #5551 removed the whole setting — and the product rule left with it. There is no duration guard anywhere in the app today; a 60s file referenced by another Nostr client plays in full.Why clipping works where the seek didn't. The loop point now lives in the native player rather than in a Dart roundtrip, so there is no seam to produce. Android sets
MediaItem.ClippingConfiguration.endPositionMsandREPEAT_MODE_ALLrepeats the clipped item; iOS trims theAVMutableCompositionviainsertTimeRangeandAVPlayerLooperloops the trimmed composition. Both were already plumbed through the method channel asstartMs/endMs— this PR only starts using them from the feed.Why 7s and not 6.3s.
VideoEditorConstants.maxDurationis the recording limit and stays 6.3s. Classic Vine assets are not 6.3s — measured withffprobeonmedia.divine.videothey run 6.500–6.533s. Re-clipping at the recording limit would cut ~200ms off every classic Vine and destroy the musical loop point on exactly the beatbox content #5544 and #6386 complain about. The newAppConstants.maxFeedPlaybackDurationis a separate, named playback constant set to 7s.Why the cap applies to every source, not just non-Divine ones. The issue floated capping only foreign sources. Divine's own export is already bounded at 6.3s (
renderVideo(maxOutputDuration:)) and classic Vines are ≤6.533s, so a 7s cap is a no-op for both. A source-conditional branch would buy nothing and leave a hole for anything that slips past the source check.Why all four open paths. First load, cache hit, source failover and the HTTP-202 processing retry each re-open the player independently, so a missed site would let a long video escape the cap on that path alone. The contract test counts
VideoClipconstructions against cap applications, so a future path cannot skip it silently.Why the Swift change. ExoPlayer clamps
endPositionMsto the real duration itself (ClippingTimeline:if (endUs > window.durationUs) endUs = window.durationUs). AVFoundation clamps whatinsertTimeRangeactually inserts, but the Swift side computedclipDurationfrom the requested end regardless — so every 6.533s Vine would have reported a 7.0s duration while looping at 6.533s, desyncing every duration readout.endTimeis now clamped to the asset duration, matching ExoPlayer.maxLoopDurationand its seek path are untouched and still unused by the feed; the existing contract assertion thatfeed_videos.dartcontains nomaxLoopDuration:stays green.Related Issue: Closes #6421
Out of Scope
maxLoopDurationparameter andsubscribeToLoopEnforcementfrominfinite_video_feed. Pre-existing dead-in-production code, unrelated to this fix, and deleting it would touch 16 test references in the package.media_kit) backend does not clamp a clip end past the source duration, so it reports the requested length. Linux is a dev-only desktop target that CI does not build, and clamping there would cost a duration probe per clip on the editor path. Documented onVideoClip.endas an Android/Apple guarantee instead.Verification
flutter analyze lib test integration_test— clean (app), plus both touched packages.dart format— clean.infinite_video_feed: full suite, 231 tests, 100.00% line coverage (the package's CI gate).divine_video_player: full suite, 257 tests. Coverage 99.49%, byte-identical to the same run on a clean tree — the four uncovered lines are pre-existing const-constructor declarations.test/widgets/video_feed_item/+test/screens/feed/, 460 tests.source_loader_test.dart); cap wired infeed_videos.dart, applied at every clip site, and clearing classic Vine length (feed_looping_contract_test.dart).end:application on purpose and confirmed the new contract test goes red.ClippingMediaSource$ClippingTimelinefrom the media3 1.10.0 AAR rather than trusting the docs.Manual test plan (pending — draft until these pass)
AVPlayerLooperstill gapless over the trimmed composition.Type of Change