Skip to content

fix: crash when opening a new video on media tunneling devices (ex: FireTV) - #85

Open
lucarosellini wants to merge 1 commit into
InfinityLoop1308:devfrom
lucarosellini:fix/firetv-tunneled-surface-detach
Open

fix: crash when opening a new video on media tunneling devices (ex: FireTV)#85
lucarosellini wants to merge 1 commit into
InfinityLoop1308:devfrom
lucarosellini:fix/firetv-tunneled-surface-detach

Conversation

@lucarosellini

Copy link
Copy Markdown

Problem

On a device using media tunneling, playing a video and then opening another one
crashes the app. Reproduced 100% of the time on a Fire TV Stick 4K (AFTMM /
mantis, MediaTek MT8695, Fire OS 6 / API 25) playing tunneled VP9. Phones do
not reproduce it, because they rarely get a tunneled decoder for these streams.

Root cause

SurfaceHolderCallback.surfaceDestroyed() created a PlaceholderSurface of its
own and handed it to the player. That dates from an ExoPlayer 2.x workaround
(google/ExoPlayer#2703, 2017), written when the player did not manage placeholder
surfaces itself.

The player does that itself now, and it checks first whether swapping the
surface is even possible:

// MediaCodecVideoRenderer, media3 1.10.1
protected boolean shouldUsePlaceholderSurface(MediaCodecInfo codecInfo) {
  return !tunneling
      && !codecNeedsSetOutputSurfaceWorkaround(codecInfo.name)
      && (!codecInfo.secure || PlaceholderSurface.isSecureSupported(context));
}

The !tunneling term is the one that matters here. MediaCodec.setOutputSurface()
cannot change the surface of a tunneled codec, and ACodec rejects the attempt.
When setOutput() finds no usable surface it takes the other branch
and calls releaseCodec(); maybeInitCodecOrBypass(); instead, which works.

Passing our own placeholder made that check unreachable. displaySurface was
never null, so the renderer always took the swap path:

W ACodec  : cannot change tunneled surface
E ExoPlayerImplInternal: Playback error
  Caused by: java.lang.IllegalStateException
      at android.media.MediaCodec.native_setSurface(Native Method)
      at MediaCodecVideoRenderer.setOutputSurfaceV23(MediaCodecVideoRenderer.java:2534)
      at ExoPlayerImplInternal.setVideoOutputInternal(ExoPlayerImplInternal.java:1943)

The exception escapes handleMessage, so the PlayerMessage is never marked
delivered. ExoPlayerImpl.setVideoOutputInternal() blocks the main thread in
blockUntilDelivered() until the detach timeout expires (two seconds), then
raises the timeout as a playback error:

E Player  : ExoPlaybackException: Unexpected runtime error
  Caused by: ExoTimeoutException: Detaching surface timed out.
      at ExoPlayerImpl.setVideoOutputInternal(ExoPlayerImpl.java:2988)
      at SurfaceHolderCallback.surfaceDestroyed(SurfaceHolderCallback.java:53)
      at android.view.SurfaceView.updateWindow(SurfaceView.java:588)
      at android.view.ViewGroup.removeView(ViewGroup.java:4620)
      at PlayerService.removeViewFromParent(PlayerService.java:254)
      at VideoDetailFragment.hideMainPlayerOnLoadingNewStream(VideoDetailFragment.java:1469)

The error is delivered synchronously, which puts onPlayerError inside the
ViewGroup.removeView() whose detach pass called surfaceDestroyed in the first
place. VideoDetailFragment.onPlayerError() then calls
hideMainPlayerOnLoadingNewStream() again, re-entering removeViewFromParent()
while the outer removal is still running. ViewGroup.removeView() resolves the child index up front and only writes through it later in removeFromArray(), so the nested removal shrinks the array underneath it and
the outer call dereferences an emptied slot.

The crash comes from that nested removal. On API 25 it can also surface one frame
earlier, in SurfaceView.updateWindow(), which dereferences its
IWindowSession with no null check before API 26 rewrote the class.

Fix

Pass null so the renderer makes the decision itself. It substitutes its own
placeholder when that is safe, and releases the codec when it is not.

Behaviour is unchanged for non-tunneled codecs: the renderer allocates the same
PlaceholderSurface we used to allocate here. The rest of the diff is the now
dead plumbing coming out (the field, its lazy init, release() and its call
site, two imports), plus a Javadoc line that no longer described the code.

Testing

Fire TV Stick 4K, API 25, tunneled VP9. Four consecutive videos, four clean
codec cycles:

08:59:40  setting surface generation 22809601   Configuring TUNNELED video playback
09:00:09    MtkOmxVdec::ComponentDeInit                   codec released, not re-pointed
09:00:25  setting surface generation 22809602   Configuring TUNNELED video playback
09:01:14    MtkOmxVdec::ComponentDeInit
09:01:15  setting surface generation 22809603   Configuring TUNNELED video playback
09:01:43    MtkOmxVdec::ComponentDeInit
09:01:45  setting surface generation 22809604   Configuring TUNNELED video playback

Tunneling stays enabled, so no hardware A/V sync is lost. cannot change tunneled surface, Detaching surface timed out, onPlayerError,
NullPointerException, IllegalStateException and FATAL are all absent from
the log. The two second main thread stall at each transition is gone as well:
before the fix it landed squarely on the video switch, and now the only
Choreographer: Skipped frames events are at stream start.

Phone (arm64, non-tunneled) checked for regressions in the paths the original
workaround existed for: rotation during playback, background and return, and
fullscreen toggle. No change in behaviour, and no Unrecoverable player error occurred.

Notes

This is not specific to Fire TV. Any device that ends up with a tunneled decoder
is affected, which on Android TV is common. Users hitting it today can work
around it with the existing "Disable media tunneling" preference, at the cost of
tunneled playback.

SurfaceHolderCallback handed the player a PlaceholderSurface of its own
on surfaceDestroyed. MediaCodecVideoRenderer refuses to swap a surface
onto a tunneled codec and releases it instead, so ours made the swap
fail and the detach timed out. Pass null and let the renderer decide.
@InfinityLoop1308

Copy link
Copy Markdown
Owner

Thanks, LGTM. I'll merge this later since we'll release 5.2.5 tomorrow and it's better not introduce new modifications at this time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants