From 949b5cb73f220696feb65ff49659ec5a6cfe4b97 Mon Sep 17 00:00:00 2001 From: Tommy Chow Date: Wed, 10 Jun 2026 23:44:16 -0700 Subject: [PATCH] fix(video): close recovery gaps found in deep player review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Expire _isQualitySwitching after a 5s grace window: a resolution-only switch never interrupts playback so no event clears the flag, which disabled stall classification (and the 8s recovery) for the rest of the session — including from session start for saved qualities - Cap latency-driven recovery: route full refreshes through the existing attempt cap + exponential backoff, and stop intervening entirely after 3 consecutive high readings (device clock skew would otherwise loop seek/refresh forever, every 3s in PiP) - Respect user pause in the 4xx and media-services-reset auto-refresh paths: defer the refresh until resume instead of force-resuming audio - Guard the _onPlayerError liveness check with _initGeneration so a slow stream-info fetch can't pin a stale error overlay on recovered playback - Confirm offline with a second /streams check before tearing down playback state — Helix transiently drops live streams from results - Capture the controller in _setStreamQualityIndex and re-check after awaits instead of _controller! (dispose race during quality switch) Co-Authored-By: Claude Fable 5 --- .../channel/video/native_video_store.dart | 81 ++++++++++++++++--- .../channel/video/stream_info_poller.dart | 18 ++++- .../channel/video/video_timing_constants.dart | 17 ++++ 3 files changed, 103 insertions(+), 13 deletions(-) diff --git a/lib/screens/channel/video/native_video_store.dart b/lib/screens/channel/video/native_video_store.dart index 53e6fc66..c4a00f12 100644 --- a/lib/screens/channel/video/native_video_store.dart +++ b/lib/screens/channel/video/native_video_store.dart @@ -76,6 +76,12 @@ abstract class NativeVideoStoreBase DateTime? _lastPlayingStart; int _shortPlayBounceCount = 0; var _isQualitySwitching = false; + Timer? _qualitySwitchGraceTimer; + + /// Set when an automatic refresh was suppressed because the user had + /// paused (e.g. the playback session expired mid-pause). Consumed by + /// [handlePausePlay] to refresh instead of playing a dead session. + var _pendingRefreshOnResume = false; var _initGeneration = 0; static final _bareResolutionRe = RegExp(r'^(\d+)p$'); @@ -536,6 +542,12 @@ abstract class NativeVideoStoreBase statusCode >= 400 && statusCode < 500 && !_isWithinRecoveryCap()) { + if (_userPaused) { + // Don't yank a user-paused stream back to life — the refresh would + // reload with autoplay. Defer until they resume. + _pendingRefreshOnResume = true; + return; + } debugPrint( 'NativeVideoStore: $statusCode on HLS fetch, refreshing with fresh ' 'token (attempt ${_totalRefreshAttempts + 1}/${VideoTimingConstants.maxRefreshAttempts})', @@ -547,8 +559,9 @@ abstract class NativeVideoStoreBase // Check stream liveness before showing the error — an offline channel // is expected to 404 and the overlay handles it. + final generation = _initGeneration; updateStreamInfo(forceUpdate: true).then((_) { - if (_disposed) return; + if (_disposed || generation != _initGeneration) return; runInAction(() { if (_streamInfo != null) { _recordNativePlaybackError( @@ -603,6 +616,14 @@ abstract class NativeVideoStoreBase return; } + if (_userPaused) { + // The player is orphaned either way, but don't auto-resume a stream + // the user paused — rebuild when they tap play. + _disposeController(); + _pendingRefreshOnResume = true; + return; + } + _totalRefreshAttempts++; debugPrint( 'NativeVideoStore: media services reset, rebuilding player ' @@ -795,6 +816,15 @@ abstract class NativeVideoStoreBase !_loading && !_userPaused) { _highLatencyCount++; + // Still high after several interventions — the reading can't be + // trusted (device clock skew) or the device genuinely can't keep up. + // Either way, looping seeks/refreshes forever makes playback worse. + // The counter survives refreshes and only resets on a healthy + // reading, so a persistent condition stops recovery permanently. + if (_highLatencyCount > + VideoTimingConstants.maxHighLatencyRecoveries) { + return; + } if (_isInPipMode || _highLatencyCount == 1) { // During PiP, only seek to live edge — handleRefresh() disposes // the controller which kills PiP. @@ -803,13 +833,16 @@ abstract class NativeVideoStoreBase ); _controller?.seekToLiveEdge(); _controller?.play(); - } else { + } else if (!_isWithinRecoveryCap() && + _backoffDelayRemaining() == Duration.zero) { debugPrint( - 'NativeVideoStore: latency still high (${rounded}s), refreshing', + 'NativeVideoStore: latency still high (${rounded}s), refreshing ' + '(attempt ${_totalRefreshAttempts + 1}/${VideoTimingConstants.maxRefreshAttempts})', ); - _highLatencyCount = 0; + _totalRefreshAttempts++; _handleRefresh(userInitiated: false); } + // Within the refresh cap/backoff window — wait for the next poll. return; } _highLatencyCount = 0; @@ -867,6 +900,13 @@ abstract class NativeVideoStoreBase @override void handlePausePlay() { + if (_pendingRefreshOnResume) { + // The playback session died while user-paused (token expiry, media + // services reset) — playing the dead session would no-op or error. + _pendingRefreshOnResume = false; + _handleRefresh(userInitiated: false); + return; + } if (_controller == null) return; if (_paused) { _userPaused = false; @@ -913,8 +953,11 @@ abstract class NativeVideoStoreBase _isStalled = false; _hasPlayedOnce = false; _userPaused = false; + _pendingRefreshOnResume = false; _stallRecoveryAttempt = 0; - _highLatencyCount = 0; + // _highLatencyCount intentionally survives refreshes so the + // latency-recovery cap can stop a persistent loop; it resets on the + // next healthy latency reading. _firstTimeSettingQuality = true; _pendingQualityIndex = null; _isQualitySwitching = false; @@ -1009,20 +1052,36 @@ abstract class NativeVideoStoreBase @action Future _setStreamQualityIndex(int index) async { _streamQualityIndex = index; - if (_controller == null) return; + final controller = _controller; + if (controller == null) return; _isQualitySwitching = true; + // A resolution-only switch never interrupts playback, so no play/pause + // event arrives to clear the flag — expire it after a grace window or + // it sticks for the session and disables stall classification. + _qualitySwitchGraceTimer?.cancel(); + _qualitySwitchGraceTimer = Timer( + VideoTimingConstants.qualitySwitchGrace, + () { + if (_disposed) return; + runInAction(() => _isQualitySwitching = false); + }, + ); if (index == 0) { // 'Auto' — reset to adaptive quality - await _controller!.setQuality(NativeVideoPlayerQuality.auto()); + await controller.setQuality(NativeVideoPlayerQuality.auto()); } else { final qualityIndex = index - 1; if (qualityIndex >= 0 && qualityIndex < _qualityObjects.length) { - await _controller!.setQuality(_qualityObjects[qualityIndex]); + await controller.setQuality(_qualityObjects[qualityIndex]); } } + // The controller can be disposed or replaced during the platform call + // (user backs out, chat-only toggle, media services reset). + if (_disposed || !identical(controller, _controller)) return; + // Toggle background PiP based on audio-only mode. // Only call when the mode actually changes to avoid unnecessary // method channel round-trips on every quality switch. @@ -1035,9 +1094,9 @@ abstract class NativeVideoStoreBase _isAudioOnlyMode = isAudioOnly; if (Platform.isIOS) { if (isAudioOnly) { - _controller!.disableAutomaticInlinePip(); + controller.disableAutomaticInlinePip(); } else { - _controller!.enableAutomaticInlinePip(); + controller.enableAutomaticInlinePip(); } } else if (Platform.isAndroid && _autoPipAvailable == true) { if (isAudioOnly) { @@ -1139,6 +1198,7 @@ abstract class NativeVideoStoreBase _stallRecoveryTimer?.cancel(); _initRetryTimer?.cancel(); _healthyPlaybackTimer?.cancel(); + _qualitySwitchGraceTimer?.cancel(); _pipSub?.cancel(); _pipSub = null; _qualitiesSub?.cancel(); @@ -1157,6 +1217,7 @@ abstract class NativeVideoStoreBase void _resetPlayerStateCommon() { _hasPlayedOnce = false; _userPaused = false; + _pendingRefreshOnResume = false; _paused = true; _isStalled = false; _isQualitySwitching = false; diff --git a/lib/screens/channel/video/stream_info_poller.dart b/lib/screens/channel/video/stream_info_poller.dart index 2f3718d6..5917ffed 100644 --- a/lib/screens/channel/video/stream_info_poller.dart +++ b/lib/screens/channel/video/stream_info_poller.dart @@ -63,11 +63,23 @@ class StreamInfoPoller { final info = await twitchApi.getStream(userLogin: userLogin); return StreamInfoResult(stream: info); } catch (_) { + // Helix can transiently drop a live stream from /streams (or 500 on + // one endpoint while others succeed). A false "confirmed offline" + // tears down latency polling and chat delay on healthy playback, so + // require a second failed check before falling back to /channels. try { - final channel = await twitchApi.getChannel(userId: userId); - return StreamInfoResult(offlineChannel: channel); + await Future.delayed( + VideoTimingConstants.offlineConfirmationDelay, + ); + final info = await twitchApi.getStream(userLogin: userLogin); + return StreamInfoResult(stream: info); } catch (_) { - return const StreamInfoResult(); + try { + final channel = await twitchApi.getChannel(userId: userId); + return StreamInfoResult(offlineChannel: channel); + } catch (_) { + return const StreamInfoResult(); + } } } } diff --git a/lib/screens/channel/video/video_timing_constants.dart b/lib/screens/channel/video/video_timing_constants.dart index 6dfba15f..bd0cc24f 100644 --- a/lib/screens/channel/video/video_timing_constants.dart +++ b/lib/screens/channel/video/video_timing_constants.dart @@ -28,6 +28,23 @@ class VideoTimingConstants { static const int highLatencyThresholdSeconds = 30; static const int chatSyncDriftToleranceSeconds = 2; + /// Maximum consecutive high-latency interventions (seek/refresh) before + /// giving up on latency-driven recovery. Persistent high readings usually + /// mean device clock skew or a device that can't keep up — looping + /// recoveries forever makes playback worse, not better. + static const int maxHighLatencyRecoveries = 3; + + /// How long a quality switch may suppress stall classification before the + /// flag expires. A resolution-only switch never interrupts playback, so no + /// play/pause event arrives to clear it — without this backstop the flag + /// sticks for the whole session and disables stall recovery. + static const qualitySwitchGrace = Duration(seconds: 5); + + /// Delay before re-checking /streams when the first check reports offline. + /// Helix transiently drops live streams from results; a single failed + /// check isn't enough to tear down healthy playback. + static const offlineConfirmationDelay = Duration(seconds: 2); + // Overlay static const overlayAutoHide = Duration(seconds: 5); static const overlayQuickHide = Duration(seconds: 3);