fix(audio): stop emitting a transient idle during a source change
Root cause of the disappearing media notification, and it is self-inflicted
on EVERY Android version — no plugin patch involved.
`audio_service`'s `_observePlaybackState` (audio_service.dart:1131-1136) calls
`AudioService._stop()` on ANY transition into `idle` from a non-idle state.
That reaches `stopService()` -> `deactivateMediaSession()` ->
`notificationManager.cancel(NOTIFICATION_ID)`. The notification is re-posted
at exactly one place, `internalStartForeground()`, reachable only from the
`!wasPlaying && playing` edge in `setState()`, and its FIRST statement is
`ContextCompat.startForegroundService(...)` — which on API 31+ throws
`ForegroundServiceStartNotAllowedException` whenever the process is not in a
foreground state.
Every station change walked straight into that. `_cambiarFuente` pushes
`loading`, then `_recrearPlayer` disposes the old `AudioPlayer` and builds a
FRESH one; a fresh player's first `playerStateStream` event is always `idle`,
and the listener forwarded it verbatim. So `loading -> idle` tore the
foreground service down mid-source-change, and recovery depended on the
following `playing: true` edge restarting it. Screen off, lock screen, or an
Android Auto / Bluetooth-initiated start is precisely where the platform
refuses that restart: audio keeps playing, the notification never returns.
That is exactly what the user reports.
The mapping decision moves out of the private `_mapProcState` into a pure
top-level `mapearEstadoProceso(proc, {required bool cambiandoFuente})`, so the
one line that decides whether the foreground service dies is unit-testable
without instantiating the handler (which needs MethodChannels). It is
byte-for-byte identical to the old switch in every case except `idle` while a
source change is in flight, which now maps to `loading`. The test asserts the
full ProcessingState x cambiandoFuente matrix against a literal transcription
of the previous mapping, and asserts both directions explicitly: a real stop
still yields `idle`, a source-change idle yields `loading`, and `idle` is the
only case where the two branches differ at all.
The only risk this introduces is a `_cambiandoFuente` stuck at `true`: a real
user stop would be masked away from `idle`, the service would never stop, and
the notification would become unkillable. So the flag is cleared by four
independent mechanisms rather than one audited path:
- a `finally` around the whole body of `_cambiarFuente`, which covers normal
completion, BOTH `revision != _revisionFuente` early returns, every
`rethrow` out of a catch clause, and any non-`Exception` `Error` that none
of the three clauses matches;
- eagerly at the top of each of the three catch clauses — needed on top of
the `finally` because `_gestionarErrorReproduccion` calls `_player.stop()`
WITHOUT awaiting it, so that `idle` could otherwise land while the mask
was still up;
- right after `setUrl` resolves, before anything below can await, since the
fresh player's transient `idle` is already behind us at that point;
- at the start of `stop()` — before `_player.stop()` — and at the start of
`_gestionarErrorReproduccion`, which makes the invariant total: the flag
is `false` before every single `_player.stop()` call in this class.
`stop()` matters most: `BaseAudioHandler.stop()` is empty, so the handler
never pushes `idle` itself — teardown is driven entirely by the player's
emission. A stop landing while a station change was still in flight would
otherwise be masked and the notification would survive the stop.
Audited: two `_player.stop()` call sites exist and both are preceded by a
clear; `_recrearPlayer` has exactly one caller and it is guarded; the old
player cannot emit during `_recrearPlayer` because its subscriptions are
cancelled first.
This commit is contained in:
@@ -120,6 +120,57 @@ Emisora emisoraDesdeMediaItem(MediaItem mediaItem) {
|
||||
);
|
||||
}
|
||||
|
||||
/// Maps a `just_audio` [ProcessingState] to the `audio_service`
|
||||
/// [AudioProcessingState] pushed into `playbackState`. Identical to the
|
||||
/// previous private `_mapProcState` in every case EXCEPT one:
|
||||
/// [ProcessingState.idle] maps to [AudioProcessingState.loading] while
|
||||
/// [cambiandoFuente] is `true`.
|
||||
///
|
||||
/// Why that single exception exists — this is the media-notification
|
||||
/// regression, not a cosmetic tweak:
|
||||
///
|
||||
/// `audio_service`'s `_observePlaybackState` (`audio_service.dart:1131-1136`)
|
||||
/// calls `AudioService._stop()` — which reaches `stopService()` and cancels
|
||||
/// the notification through `deactivateMediaSession()` — on ANY transition
|
||||
/// into `idle` from a non-idle state. The notification is posted at exactly
|
||||
/// one place, `internalStartForeground()`, reachable only from the
|
||||
/// `!wasPlaying && playing` edge, and its FIRST statement is
|
||||
/// `ContextCompat.startForegroundService(...)`, which throws
|
||||
/// `ForegroundServiceStartNotAllowedException` on API 31+ whenever the
|
||||
/// process is not in a foreground state.
|
||||
///
|
||||
/// Every station change walked straight into that: `_cambiarFuente` pushes
|
||||
/// `loading`, then `_recrearPlayer` disposes the old [AudioPlayer] and builds
|
||||
/// a FRESH one, and a fresh player's first `playerStateStream` event is
|
||||
/// always `idle`. Forwarded verbatim, that is a `loading -> idle` transition,
|
||||
/// so the foreground service was torn down mid-source-change and the app then
|
||||
/// depended on the following `playing: true` edge to restart it. With the
|
||||
/// screen off, on the lock screen, or on an Android Auto / Bluetooth-initiated
|
||||
/// start, that restart is exactly the case the platform refuses — audio keeps
|
||||
/// playing, the notification never comes back. Self-inflicted, on every API
|
||||
/// level, no plugin patch needed: just stop emitting the transient `idle`.
|
||||
///
|
||||
/// A genuine user stop is unaffected: `stop()` clears the flag BEFORE
|
||||
/// `_player.stop()`, so its `idle` still reaches `playbackState` as a real
|
||||
/// `idle` and still tears the service down. Pure — no handler dependency — so
|
||||
/// the full [ProcessingState] x [cambiandoFuente] matrix is unit-testable
|
||||
/// directly.
|
||||
AudioProcessingState mapearEstadoProceso(
|
||||
ProcessingState proc, {
|
||||
required bool cambiandoFuente,
|
||||
}) {
|
||||
if (cambiandoFuente && proc == ProcessingState.idle) {
|
||||
return AudioProcessingState.loading;
|
||||
}
|
||||
return switch (proc) {
|
||||
ProcessingState.idle => AudioProcessingState.idle,
|
||||
ProcessingState.loading => AudioProcessingState.loading,
|
||||
ProcessingState.buffering => AudioProcessingState.buffering,
|
||||
ProcessingState.ready => AudioProcessingState.ready,
|
||||
ProcessingState.completed => AudioProcessingState.completed,
|
||||
};
|
||||
}
|
||||
|
||||
/// Custom-action names for the equalizer's `PlaybackStateCompat` custom
|
||||
/// actions on the now-playing screen (Design "EQ custom actions", item 4).
|
||||
/// Public consts so tests and this file's own `customAction` dispatch share
|
||||
@@ -416,6 +467,24 @@ class PluriWaveAudioHandler extends BaseAudioHandler
|
||||
Future<void> _colaCambioFuente = Future<void>.value();
|
||||
int _revisionFuente = 0;
|
||||
|
||||
/// `true` only for the window inside [_cambiarFuente] where the OLD player
|
||||
/// has been disposed and the FRESH one has not loaded its URL yet — the
|
||||
/// window in which `playerStateStream` unavoidably emits a transient
|
||||
/// `idle` that is NOT a stop. [mapearEstadoProceso] masks that one `idle`
|
||||
/// as `loading` so `audio_service` does not tear the foreground service
|
||||
/// (and with it the media notification) down mid-source-change; see that
|
||||
/// function's doc for the full mechanism.
|
||||
///
|
||||
/// A value stuck at `true` is the ONLY risk this flag introduces: a real
|
||||
/// user stop would then be masked away from `idle` and the service would
|
||||
/// never stop, leaving an unkillable notification. It is therefore cleared
|
||||
/// by a `finally` in [_cambiarFuente] (which covers normal completion,
|
||||
/// both revision-mismatch `return`s, every `rethrow`, and any non-`Exception`
|
||||
/// `Error` that no catch clause matches), AND eagerly at the top of every
|
||||
/// catch clause, AND at the start of [stop] and [_gestionarErrorReproduccion]
|
||||
/// — i.e. before every single `_player.stop()` call in this class.
|
||||
bool _cambiandoFuente = false;
|
||||
|
||||
/// Active local-music queue (Design "single load-bearing invariant"):
|
||||
/// `null` means "not local-queue playback" — the ONLY gate the
|
||||
/// auto-advance/skip/isolation logic reads. Radio never sets this field.
|
||||
@@ -522,7 +591,10 @@ class PluriWaveAudioHandler extends BaseAudioHandler
|
||||
if (colaActiva) MediaAction.skipToNext,
|
||||
},
|
||||
androidCompactActionIndices: [colaActiva ? 1 : 0],
|
||||
processingState: _mapProcState(proc),
|
||||
processingState: mapearEstadoProceso(
|
||||
proc,
|
||||
cambiandoFuente: _cambiandoFuente,
|
||||
),
|
||||
playing: playing,
|
||||
bufferedPosition: _player.bufferedPosition,
|
||||
speed: _player.speed,
|
||||
@@ -608,6 +680,11 @@ class PluriWaveAudioHandler extends BaseAudioHandler
|
||||
/// only retry exhaustion (or non-network errors) falls through to the
|
||||
/// existing error path, so the user sees a single error — no spam per retry.
|
||||
void _gestionarErrorReproduccion(Object error) {
|
||||
// Terminal-error path also ends in `_player.stop()` below, and it is
|
||||
// reachable from `_eventosSub`'s `onError` WHILE a source change is still
|
||||
// in flight. Dropping the mask here keeps the invariant total: the flag
|
||||
// is `false` before every `_player.stop()` call in this class.
|
||||
_cambiandoFuente = false;
|
||||
if (_intentarReconexion(error)) return;
|
||||
|
||||
String mensaje;
|
||||
@@ -740,16 +817,6 @@ class PluriWaveAudioHandler extends BaseAudioHandler
|
||||
return _textos.audioErrorCannotPlay;
|
||||
}
|
||||
|
||||
AudioProcessingState _mapProcState(ProcessingState state) {
|
||||
return switch (state) {
|
||||
ProcessingState.idle => AudioProcessingState.idle,
|
||||
ProcessingState.loading => AudioProcessingState.loading,
|
||||
ProcessingState.buffering => AudioProcessingState.buffering,
|
||||
ProcessingState.ready => AudioProcessingState.ready,
|
||||
ProcessingState.completed => AudioProcessingState.completed,
|
||||
};
|
||||
}
|
||||
|
||||
/// Public entry point for EVERY external play (phone `reproducir`, car
|
||||
/// `emisora:`/`grupo:`/`pista:` non-path). ALWAYS clears the
|
||||
/// local queue FIRST (Design ADR-2, the single load-bearing invariant:
|
||||
@@ -889,16 +956,31 @@ class PluriWaveAudioHandler extends BaseAudioHandler
|
||||
errorMessage: null,
|
||||
),
|
||||
);
|
||||
// Opens the masking window BEFORE `_recrearPlayer`, which is what
|
||||
// disposes the old player and constructs the fresh one whose first
|
||||
// `playerStateStream` event is the transient `idle` we must not forward
|
||||
// (see [mapearEstadoProceso]).
|
||||
_cambiandoFuente = true;
|
||||
try {
|
||||
await _recrearPlayer();
|
||||
if (revision != _revisionFuente) return;
|
||||
|
||||
await _player.setUrl(mediaItem.id).timeout(_timeoutCambioFuente);
|
||||
// Source swap complete: the fresh player's transient `idle` is behind
|
||||
// us, so stop masking immediately — before anything below can await —
|
||||
// and let a real `idle` through again from here on.
|
||||
_cambiandoFuente = false;
|
||||
if (revision != _revisionFuente) return;
|
||||
|
||||
_iniciarPlaySinBloquear(mediaItem, revision);
|
||||
unawaited(_activarEcualizador());
|
||||
} on PlayerException catch (e) {
|
||||
// Cleared BEFORE `_gestionarErrorReproduccion`, not just by the
|
||||
// `finally`: that method calls `_player.stop()` without awaiting it, so
|
||||
// the resulting `idle` could otherwise land while the mask was still
|
||||
// up and be rewritten to `loading`. Same reason in the two clauses
|
||||
// below.
|
||||
_cambiandoFuente = false;
|
||||
if (revision == _revisionFuente) {
|
||||
_gestionarErrorReproduccion(e);
|
||||
// Reconnect engaged: complete normally so callers do not surface a
|
||||
@@ -907,6 +989,7 @@ class PluriWaveAudioHandler extends BaseAudioHandler
|
||||
}
|
||||
throw Exception(_mensajeAmigable(e));
|
||||
} on TimeoutException catch (e) {
|
||||
_cambiandoFuente = false;
|
||||
// A real network drop usually surfaces as our 12s source timeout:
|
||||
// route it through the reconnect machine instead of a terminal error.
|
||||
if (revision == _revisionFuente) {
|
||||
@@ -915,6 +998,7 @@ class PluriWaveAudioHandler extends BaseAudioHandler
|
||||
}
|
||||
rethrow;
|
||||
} on Exception catch (e, stackTrace) {
|
||||
_cambiandoFuente = false;
|
||||
developer.log(
|
||||
'[PluriWave] Error inesperado en playMediaItem: $e',
|
||||
name: 'ServicioAudio',
|
||||
@@ -933,6 +1017,15 @@ class PluriWaveAudioHandler extends BaseAudioHandler
|
||||
this.mediaItem.add(null);
|
||||
}
|
||||
rethrow;
|
||||
} finally {
|
||||
// Leak-proof backstop. Dart runs `finally` on EVERY exit from the
|
||||
// block above: normal completion, both `revision != _revisionFuente`
|
||||
// early returns, every `throw`/`rethrow` out of a catch clause, and —
|
||||
// crucially — any `Error` (as opposed to `Exception`) that none of the
|
||||
// three clauses catches. The flag must never depend on a single
|
||||
// hand-audited exit path, because a `_cambiandoFuente` stuck at `true`
|
||||
// would mask a REAL stop's `idle` and leave the notification unkillable.
|
||||
_cambiandoFuente = false;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1147,6 +1240,17 @@ class PluriWaveAudioHandler extends BaseAudioHandler
|
||||
// logic above — `onTaskRemoved` (which calls stop()) inherits this for
|
||||
// free (Phase 3 task 3.5).
|
||||
_desactivarCola();
|
||||
// Genuine user stop: drop the source-change mask BEFORE `_player.stop()`,
|
||||
// so its `idle` reaches `playbackState` as a REAL `idle` and
|
||||
// `audio_service` tears the foreground service down as it always has.
|
||||
// `stop()` never pushes `idle` itself — `BaseAudioHandler.stop()` is
|
||||
// empty and the teardown is driven entirely by the player's emission —
|
||||
// so a stop landing while a station change is still in flight would
|
||||
// otherwise be masked to `loading` and the notification would become
|
||||
// unkillable. Paired with `_revisionFuente++` below, which invalidates
|
||||
// that in-flight change; its `finally` only re-clears the flag, and
|
||||
// nothing re-arms it (the single `= true` assignment already ran).
|
||||
_cambiandoFuente = false;
|
||||
_revisionFuente++;
|
||||
await _player.stop();
|
||||
emisoraActual = null;
|
||||
|
||||
Reference in New Issue
Block a user