fix(android): Released wrong source in LOW_LATENCY mode

[jasharpe]

Description

Fix issue where source field is updated before player.setSource calls release(), which should affect previous source, not the new source.

This is fixed by moving the field = value line to after player.setSource(value) is called.

Details for the problematic call path:

1. player.setSource(value)
2. SoundPoolPlayer.setSource
3. UrlSource.setForSoundPool
4. SoundPoolPlayer.setUrlSource
5. SoundPoolPlayer.release() (conditioned on if soundId != null)

SoundPoolPlayer.release() is called to free up the previous sound, and it does this based on the values of this.soundId and this.urlSource. These are supposed to be from the PREVIOUS sound (the one to be unloaded), not the CURRENT sound (the one to be loaded).

During the call to release() from setUrlSource, the soundId is correctly the previous version, but because the field = value line occurs BEFORE the call to player.setSource, the this.urlSource is NOT the previous version, it's the current version that's supposed to be loaded.

Ultimately because the soundId and urlSource don't match, this causes the synchronized update lines in release() to corrupt the urlToPlayers / soundIdToPlayer structures because the sound ID and URL source don't match.

I hope that explanation is reasonably clear. I checked the various other codepaths around here to see if having a later update of the source field would cause any problems, and I couldn't find any, but let me know if you can think of some way this ordering change would be bad.

I made the following reproduction to show the problem. Before this change, it results in playing incorrect sounds. With this particular setup, it plays the same sound two times, but it is expected to play two different sounds. After the change, it plays the two different sounds correctly as different sounds.

There are many other repros possible since there is some weird corruption of data structures happening, but this one is reliable for me on a Pixel 3a emulator.

  void soundTest(BuildContext context) async {
    var file1 = "notification_decorative-02.wav";
    var file2 = "hero_simple-celebration-03.wav";

    var player = AudioPlayer()
      ..setPlayerMode(PlayerMode.lowLatency)
      ..setReleaseMode(ReleaseMode.stop);

    await player.stop();
    await player.play(AssetSource(file1));
    await Future.delayed(Duration(milliseconds: 50), () {});
    await player.stop();
    await player.play(AssetSource(file2));
    await player.stop();

    player = AudioPlayer()
      ..setPlayerMode(PlayerMode.lowLatency)
      ..setReleaseMode(ReleaseMode.stop);

    await Future.delayed(Duration(seconds: 1), () {});

    await player.stop();
    await player.play(AssetSource(file1));

    await Future.delayed(Duration(seconds: 1), () {});

    await player.stop();
    await player.play(AssetSource(file2));
  }

Checklist

• The title of my PR starts with a Conventional Commit prefix (fix:, feat:, refactor:,
  docs:, chore:, test:, ci: etc).
• I have read the Contributor Guide and followed the process outlined for submitting PRs.
• I have updated/added tests for ALL new/updated/fixed functionality.
• I have updated/added relevant documentation and added dartdoc comments with ///, where necessary.
• I have updated/added relevant examples in example.

Breaking Change

• Yes, this is a breaking change.
• No, this is not a breaking change.

Related Issues

[Gustl22]

Thank you! Nice catch! Your solution works fine. I went through the original code and noticed that the structure is hard to follow in general. I propose a solution where the old url is saved in SoundPoolPlayer and cannot be altered from outside the class, which was a highly unexpected behavior. If you want I can push my alternative fix on your branch and may can reevaluate it :)

[jasharpe]

I propose a solution where the old url is saved in SoundPoolPlayer and cannot be altered from outside the class

I did think about that and I'm not sure it really makes sense as a trade-off in my opinion. It adds extra state that's used for only one very specific thing. I think having the field unset during the body of the property setter actually makes some sense - it seems to me that in the context of player.setSource(value) it's fairly clear that the passed in value should be used inside of setSource instead of the value of the field.

Plus, running the field = value line would at least be consistent with the behavior in SoundPoolPlayer.audioContext setter, which sets the field after the setting logic.

That said, I'm not too bothered by which solution is used. Feel free to push the alternative fix if you feel strongly.

[Gustl22]

@jasharpe thank you for your assessment. I don't mind moving the value initialization to the bottom of the setter. But I also think using an alterable value of a circular-referenced class isn't good coding style (here: holding WrappedPlayer inside SoundPoolPlayer).

It adds extra state that's used for only one very specific thing.

Yes it does, but it's also kind of done for the MediaPlayer part. It's better to keep consistent through the whole abstraction of the WrappedPlayer. There are far better places to look for performance improvements 😁

[jasharpe]

I think your version looks good. I was imagining something a little different (that I was against), but what you have makes sense.

My only comment is that soundPoolPlayer.release() is no longer guarded by a check of whether soundId is null. This means it calls stop() before returning. I think it's okay, but just wanted to point it out in case it's not intentional.

Also thank you so much for adding the integration test. I didn't see where to do that.

[Gustl22]

My only comment is that soundPoolPlayer.release() is no longer guarded by a check of whether soundId is null. This means it calls stop() before returning. I think it's okay, but just wanted to point it out in case it's not intentional.

Yes, I think it's even better to always stop the previous sound before replacing with the new source.

Also thank you so much for adding the integration test. I didn't see where to do that.

Yes, we didn't had platform specific tests yet. Also this test is only auditory, we can't really evaluate that (only by introducing some test variables or with some fancy machine learning audio evaluation...). But it's good to have an accessible audible test for this to try it locally.