Fix error handling on play without load

[addie9000]

When play without load like below, no error reported from native platform.

final player = AudioPlayer();
await player.setAudioSource(<NotFoundAudioSource>, preload = false);
player.playbackEventStream.listen((d) => print('data: $d'), onError: (e) => print('error: $e'));
await player.play();
// should print 'error: PlatformException(...)' after play, but it doesn't.

This is because before playbackEventStream reports error, platform is deactivated and the stream is canceled.
The deactivation is caused by error handling on duration report logic in setPlatform.

just_audio/just_audio/lib/just_audio.dart

Line 1356 in 2a22b69

await _setPlatformActive(false)?.catchError((dynamic e) {});

Also fixed stuck in playing after such error occurs.

ref #390

[ryanheise]

I'm not sure if I would want to automatically set playing to false here, that logic should be left to the app to decide once they catch the error.

[addie9000]

I thought the same thing. But is it possible that send play request occurs error and still in playing?

I think it's more likely as same as revert state if failed to activate the audio session.
https://github.com/ryanheise/just_audio/blob/master/just_audio/lib/just_audio.dart#L867-L868

[ryanheise]

The only methods that should set playing are play, pause and stop. Now if this code is happening as part of play, then in principle it is fine, but this line is still in the wrong place. The error needs to be propagated back to the play method body where it can be caught and THEN playing set to false.

I'm still a bit conflicted about this, though. Reverting the state is not as good as never setting the state in the first place. What I did to handle the audio session error is a bit of a kludge to allow the UI to get immediate state feedback synchronously before trying to activate the audio session asynchronously. The other half of my conflicted brain is thinking that I should just remove the reversion code and leave it dangling in the playing state there, too, but report the exception to the app so that it can decide what to do in that situation. The reason being is that the state model was designed so that the playing state is only ever set in direct response to a request by the app. The reversion was not actually the original request. Although perhaps play could have a parameter to configure whether this reversion happens or not.

[addie9000]

I got it! Yes, probably the playing state should only be changed by method call, however error recovery and auto state changing is also useful.

Anyway, this PR's main purpose is to fix error dropping on play without load. So how about this?

• revert playing state changing for this PR.
• add new enhancement issue about auto playing state recovery option as a reminder.

[ryanheise]

Sounds good 👍

[addie9000]

I've done all two things above.

[ryanheise]

Same comment applies here.

[addie9000]

@ryanheise
Personally I do not use 'play without load' anymore, thus this is not blocking problem for me.
However I believe this could be helpful to someone who (like #390, and myself in the future) uses 'play without load', so if this PR looks OK, could you merge it?

[ryanheise]

I'm not sure if I fully understand whether this is correct. The original code would only call _setPlatformActive(false) if load failed. Your new code will do it for any error, even for example when load initially succeeds, the first 2 items in a ConcatenatingAudioSource play without error, but the 3rd item produces an error. I don't think we want the platform to be deactivated in that case. Similarly, if load succeeds, and it starts playing, but then the network connection is lost and we get an error, we also don't want the platform to be deactivated then. The only time we do want to do this is when the platform was originally inactive, and the attempt to transition to active via a load failed, in which case it should be returned to inactive.

Did I misunderstand something?

[addie9000]

Ah, probably you are right.

So, the problem is that we cannot detect any error on play after non-preloaded setAudioSource.
On the other hand, it seems that at least we can detect the error on load with the same circumstance, because load returns durationCompleter.future which report duration load error. (The source error of hiding platform error by set platform inactive.)
How about change the play like below to report duration load error?

just_audio/just_audio/lib/just_audio.dart

Lines 874 to 877 in b1a7d50

	// If the native platform wasn't already active, activating it will
	// implicitly restore the playing state and send a play request.
	_setPlatformActive(true, playCompleter: playCompleter)
	?.catchError((dynamic e) {});

// If the native platform wasn't already active, activating it will
// implicitly restore the playing state and send a play request.
_setPlatformActive(true, playCompleter: playCompleter)
  ?.catchError((dynamic e) {
     // report error to play completer.
     playCompleter.completeError(e);
});

I haven't tested anything about this, if the way of correction seems right, I'll test it and update this PR.

[ryanheise]

Maybe something like that will work, it's worth a try.

[dickermoshe]

@ryanheise
This bug still persists many years later.
Many exceptions which are emitted by Line 1501

just_audio/just_audio/lib/just_audio.dart

Lines 1496 to 1512 in b5fad79

	if (audioSource != null) {
	try {
	final initialSeekValues = _initialSeekValues ??
	_InitialSeekValues(position: position, index: currentIndex);
	_initialSeekValues = null;
	final duration = await _load(platform, _audioSource!,
	initialSeekValues: initialSeekValues);
	if (checkInterruption()) return platform;
	durationCompleter.complete(duration);
	} catch (e, stackTrace) {
	await _setPlatformActive(false)
	?.catchError((dynamic e) async => null);
	durationCompleter.completeError(e, stackTrace);
	}
	} else {
	durationCompleter.complete(null);
	}

are caught by catchError and don't have their errors emitted.

A simple fix was to do this instead:

} catch (e, stackTrace) {
          durationCompleter.completeError(e, stackTrace);
          await Future.delayed(Duration.zero);
          await _setPlatformActive(false)
              ?.catchError((dynamic e) async => null);
        }

1. Complete the error 1st
2. Wait for the completed error to be emitted by the stream
3. Update the platform

I will try to write an integration test to confirm this. The but the error reporting has been giving me heartache for a while now.