Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

defaultAudioLink.flush causes OutOfMemory #10057

Closed
halizton opened this issue Mar 11, 2022 · 5 comments
Closed

defaultAudioLink.flush causes OutOfMemory #10057

halizton opened this issue Mar 11, 2022 · 5 comments
Assignees
Labels

Comments

@halizton
Copy link

This is a follow up of the issue 8728

We encounter the same issue in our application. The context is that we are a feed application where we may play several videos at the same time in the feed. We can also play full screen video from time to time. We try to keep reusing the players that have already been created.

There are several circumstances where we will release a player:

  • To treat the system memory nicely, we also release all players if the application goes to the background.
  • We will release a player if the cache pool already has enough players, for example, 4. We do this check after the video is scrolled off the screen.

Since we have quite a lot videos in our feed, we try to keep using the players and at the same time, we would release players periodically. There are some pages with lots of videos and we could suddenly release lots of players. I think the current implementation of the defaultAudioLink.flush() would lead to too many threads created in some cases in our application.

This is an example of the crash:
image

So it would be great to offer something like this
image

  • ExoPlayer version number: 2.14.2
  • Android version: OS 10 (this seems only happen on OS 10)
  • Android device: many
@tonihei
Copy link
Collaborator

tonihei commented Jun 9, 2022

Sorry for the delay in addressing this.

What do you think about using a shared single threaded ExecutorThread to release all the audio tracks? This was already suggested in #8728 as a potential solution. If we think this makes sense, we could even handle this inside DefaultAudioSink via a shared static ExecutorThread that can be kept alive as long as there are active audio sinks.

In #8728, @andrewlewis is concerned that

the main thread might be blocked for too long if doing that (the last player will wait until all the other AudioTrack.release calls have returned before it can do its call).

This particular concern only applies when we try to release a player with an AudioTrack that is currently being initialized and waiting for a previous AudioTrack in the same player to be released. Regular release calls are not blocked because AudioTrack.flush() returns without waiting. We could try to avoid this edge case by not blocking the playback thread when initializing a new AudioTrack and instead retry the initialiation later (=return false from AudioTrack.handleBuffer)

@halizton
Copy link
Author

Yeah, a single threaded ExecutorThread approach sounds good! Thanks for addressing.

rohitjoins pushed a commit to androidx/media that referenced this issue Jul 7, 2022
We wait until a previous AudioTrack has been released before
creating a new one. This is currently done with a thread
block operation, which may cause ANRs in the extreme case
when someone attempts to release the player while this is
still blocked.

The problem can be avoided by just returning false from
DefaultAudioSink.handleBuffer to try again until the previous
AudioTrack is released.

Reproduction steps to force the issue:
1. Add Thread.sleep(10000); to the AudioTrack release thread.
2. Add this to the demo app:
    private int positionMs = 0;

    Handler handler = new Handler();
    handler.post(new Runnable() {
      @OverRide
      public void run() {
        player.seekTo(positionMs++);
        if (positionMs == 10) {
          player.release();
        } else {
          handler.postDelayed(this, 1000);
        }
      }
3. Observe Player release timeout exception.

These steps can't be easily captured in a unit test as we can't
artifically delay the AudioTrack release from the test.

Issue: google/ExoPlayer#10057
PiperOrigin-RevId: 459468912
rohitjoins pushed a commit that referenced this issue Jul 7, 2022
We wait until a previous AudioTrack has been released before
creating a new one. This is currently done with a thread
block operation, which may cause ANRs in the extreme case
when someone attempts to release the player while this is
still blocked.

The problem can be avoided by just returning false from
DefaultAudioSink.handleBuffer to try again until the previous
AudioTrack is released.

Reproduction steps to force the issue:
1. Add Thread.sleep(10000); to the AudioTrack release thread.
2. Add this to the demo app:
    private int positionMs = 0;

    Handler handler = new Handler();
    handler.post(new Runnable() {
      @OverRide
      public void run() {
        player.seekTo(positionMs++);
        if (positionMs == 10) {
          player.release();
        } else {
          handler.postDelayed(this, 1000);
        }
      }
3. Observe Player release timeout exception.

These steps can't be easily captured in a unit test as we can't
artifically delay the AudioTrack release from the test.

Issue: #10057
PiperOrigin-RevId: 459468912
rohitjoins pushed a commit to androidx/media that referenced this issue Jul 13, 2022
We currently start a simple Thread to release AudioTracks
asynchronously. If many AudioTracks are released at the same
time, this may lead to OOM situations because we attempt to
create multiple new threads.

This can be improved by using a shared SingleThreadExecutor.
In the simple case of one simmultaneous release, it's exactly
the same behavior as before: create a thread and release it
as soon as it's done. For multiple simultanous releases we
get the advantage of sharing a single thread to avoid creating
more than one at the same time.

Issue: google/ExoPlayer#10057
PiperOrigin-RevId: 460698942
rohitjoins pushed a commit that referenced this issue Jul 13, 2022
We currently start a simple Thread to release AudioTracks
asynchronously. If many AudioTracks are released at the same
time, this may lead to OOM situations because we attempt to
create multiple new threads.

This can be improved by using a shared SingleThreadExecutor.
In the simple case of one simmultaneous release, it's exactly
the same behavior as before: create a thread and release it
as soon as it's done. For multiple simultanous releases we
get the advantage of sharing a single thread to avoid creating
more than one at the same time.

Issue: #10057
PiperOrigin-RevId: 460698942
@tonihei
Copy link
Collaborator

tonihei commented Jul 15, 2022

This should be fixed now by automatically using a single thread executor for releases that happen at the same time.

@tonihei tonihei closed this as completed Jul 15, 2022
rohitjoins pushed a commit that referenced this issue Jul 15, 2022
We wait until a previous AudioTrack has been released before
creating a new one. This is currently done with a thread
block operation, which may cause ANRs in the extreme case
when someone attempts to release the player while this is
still blocked.

The problem can be avoided by just returning false from
DefaultAudioSink.handleBuffer to try again until the previous
AudioTrack is released.

Reproduction steps to force the issue:
1. Add Thread.sleep(10000); to the AudioTrack release thread.
2. Add this to the demo app:
    private int positionMs = 0;

    Handler handler = new Handler();
    handler.post(new Runnable() {
      @OverRide
      public void run() {
        player.seekTo(positionMs++);
        if (positionMs == 10) {
          player.release();
        } else {
          handler.postDelayed(this, 1000);
        }
      }
3. Observe Player release timeout exception.

These steps can't be easily captured in a unit test as we can't
artifically delay the AudioTrack release from the test.

Issue: #10057
PiperOrigin-RevId: 459468912
(cherry picked from commit a80dd60)
rohitjoins pushed a commit to androidx/media that referenced this issue Jul 15, 2022
We wait until a previous AudioTrack has been released before
creating a new one. This is currently done with a thread
block operation, which may cause ANRs in the extreme case
when someone attempts to release the player while this is
still blocked.

The problem can be avoided by just returning false from
DefaultAudioSink.handleBuffer to try again until the previous
AudioTrack is released.

Reproduction steps to force the issue:
1. Add Thread.sleep(10000); to the AudioTrack release thread.
2. Add this to the demo app:
    private int positionMs = 0;

    Handler handler = new Handler();
    handler.post(new Runnable() {
      @OverRide
      public void run() {
        player.seekTo(positionMs++);
        if (positionMs == 10) {
          player.release();
        } else {
          handler.postDelayed(this, 1000);
        }
      }
3. Observe Player release timeout exception.

These steps can't be easily captured in a unit test as we can't
artifically delay the AudioTrack release from the test.

Issue: google/ExoPlayer#10057
PiperOrigin-RevId: 459468912
(cherry picked from commit a83ab05)
@halizton
Copy link
Author

This should be fixed now by automatically using a single thread executor for releases that happen at the same time.

Which release contains this fix, @tonihei ?

@andrewlewis
Copy link
Collaborator

andrewlewis commented Aug 26, 2022

[Edited to correct my previous reply as only part of this made it into 2.18.1, thanks to @icbaker for the heads up]

It's unreleased and will most likely go into 2.19.0 (as not all the referenced commits are marked for inclusion in a minor release).

@google google locked and limited conversation to collaborators Sep 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants