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.

Sign up for GitHub

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

Jump to bottom

Video playback freezes after closed captions track change #8203

Closed
alexeyr15 opened this issue Nov 11, 2020 · 8 comments
Closed

Video playback freezes after closed captions track change #8203

alexeyr15 opened this issue Nov 11, 2020 · 8 comments
Assignees
@ojw28
Labels
bug

Comments

@alexeyr15
Copy link

[REQUIRED] Issue description

Video playback freezes after changing the closed captions track when using FFmpeg extension.

[REQUIRED] Reproduction steps

  1. Build ExoPlayer Demo app with FFmpeg extension enabled (use "withDecoderExtensions" build variant also)
  2. Play the provided test stream
  3. Open the track selection dialog and change the closed captions track
  4. Video playback freezes

[REQUIRED] Link to test content

Provided by email.

[REQUIRED] A full bug report captured from the device

N/A

[REQUIRED] Version of ExoPlayer being used

2.12.1

[REQUIRED] Device(s) and version(s) of Android being used

No device specific
@kim-vde
Copy link
Contributor

kim-vde commented Nov 11, 2020

I get a 404 when opening the link you sent by email. Could you please send a new link and update this issue when you have done it?

@kim-vde kim-vde self-assigned this Nov 11, 2020
@alexeyr15
Copy link
Author

I have updated the link.

@kim-vde kim-vde assigned tonihei and unassigned kim-vde Nov 11, 2020
@skogl
Copy link

skogl commented Nov 16, 2020

I can see this issue as well after upgrading from 2.11.5 to 2.12.1. The issue is only noticed when actually using a ffmpeg audio track and then switching text track. If I were to play a stream which is supported by my device (and ffmpeg is not needed) I do not see this issue.

@skogl
Copy link

skogl commented Nov 18, 2020

Is this issue being looked at? It really makes an upgrade impossible.

@tonihei tonihei assigned ojw28 and unassigned tonihei Nov 18, 2020
@ojw28
Copy link
Contributor

ojw28 commented Nov 18, 2020 •
edited
Loading

Yes. This is a fairly obscure issue where MediaCodecAudioRenderer (which is not being used) inadvertently resets the state of the AudioSink that's shared across all of the audio renderers.

As a workaround, you can implement an RenderersFactory to provide each audio renderer with its own AudioSink, rather than doing what DefaultRenderersFactory is doing and sharing a single instance.

@ojw28
Copy link
Contributor

ojw28 commented Nov 19, 2020

I got a few class names wrong in my response above, so it didn't make much sense. I've updated it to be correct now (if reading by email, please open the thread on GitHub to see the correct version). Thanks!

@skogl
Copy link

skogl commented Nov 19, 2020 •
edited
Loading

Thank you for your answer. I was just composing my answer with a question if you meant RenderersFactory and not ExtractorsFactory.
The workaround is working.

ojw28 added a commit that referenced this issue Nov 19, 2020
@ojw28
Minimal fix for playback freezes when enabling tracks #8203
8c81766 
Background:

1. When the player has multiple audio renderers, by default they share a
   single AudioSink.
2. When any new renderer is enabled, all disabled renderers are reset
   prior to the new renderer being enabled. This is to give them a chance
   to free up resources in case the renderer being enabled needs them. These
   reset calls are expected to be no-ops for renderers that have never been
   enabled.

The issue:

The problematic case arises when there are two audio renderers and a third
renderer (e.g., text) is being enabled. In this case, the disabled audio
renderer's reset call ends up resetting the AudioSink that's shared with the
enabled audio renderer. The enabled audio renderer is then unable to make
progress, causing playback to freeze.

This is a minimal fix that directly prevents the mentioned issue. There are
multiple follow-ups that would probably make sense:

1. Having ExoPlayerImplInternal track which renderers need to be reset, and
   only resetting those renderers rather than all that are disabled. This
   seems like a good thing to do regardless, rather than relying on those
   calls being no-ops.
2. If we want to continue sharing AudioSink, we need to formalize this much
   better and make sure we have good test coverage. Messages like
   MSG_SET_VOLUME are also delivered to the AudioSink multiple times via
   each of the renderers, which works currently because DefaultAudioSink
   no-ops all but the first call in each case. This is pretty fragile though!

Issue: #8203
#minor-release
PiperOrigin-RevId: 343296081
@ojw28
Copy link
Contributor

ojw28 commented Nov 19, 2020 •
edited
Loading

This is fixed by the change ref'd above. We'll include this in 2.12.2, which is due in the next couple of weeks. Note that:

  1. Once using 2.12.2, the workaround will be unnecessary.
  2. We didn't fix this by having each renderer use its own AudioSink. Instead, we fixed the problem that was causing sharing of the AudioSink to not work.

@ojw28 ojw28 closed this as completed Nov 19, 2020
@ojw28 ojw28 mentioned this issue Nov 26, 2020
Closed
icbaker pushed a commit that referenced this issue Nov 30, 2020
@ojw28 @icbaker
Minimal fix for playback freezes when enabling tracks #8203
305ec3b 
Background:

1. When the player has multiple audio renderers, by default they share a
   single AudioSink.
2. When any new renderer is enabled, all disabled renderers are reset
   prior to the new renderer being enabled. This is to give them a chance
   to free up resources in case the renderer being enabled needs them. These
   reset calls are expected to be no-ops for renderers that have never been
   enabled.

The issue:

The problematic case arises when there are two audio renderers and a third
renderer (e.g., text) is being enabled. In this case, the disabled audio
renderer's reset call ends up resetting the AudioSink that's shared with the
enabled audio renderer. The enabled audio renderer is then unable to make
progress, causing playback to freeze.

This is a minimal fix that directly prevents the mentioned issue. There are
multiple follow-ups that would probably make sense:

1. Having ExoPlayerImplInternal track which renderers need to be reset, and
   only resetting those renderers rather than all that are disabled. This
   seems like a good thing to do regardless, rather than relying on those
   calls being no-ops.
2. If we want to continue sharing AudioSink, we need to formalize this much
   better and make sure we have good test coverage. Messages like
   MSG_SET_VOLUME are also delivered to the AudioSink multiple times via
   each of the renderers, which works currently because DefaultAudioSink
   no-ops all but the first call in each case. This is pretty fragile though!

Issue: #8203
PiperOrigin-RevId: 343296081
@google google locked and limited conversation to collaborators Jan 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@ojw28 ojw28

Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@alexeyr15 @ojw28 @google-oss-bot @tonihei @skogl @kim-vde