Closed qvk closed 5 years ago
Also, I'm not sure if this is feasible or already being done, but is it possible (or even useful) to avoid dispatching/handling MSG_SET_SURFACE
when ExoPlayer#release
has been called?
In the lifecycle event onStop()
, we call ExoPlayer#release
on the application main thread. surfaceDestroyed
should also be called sometime around this (although I believe it's timing is not tied to the application lifecycle methods). Since both release
and setSurface
are processed on the internal playback thread (correct me if I'm wrong about this), is it possible/safe/useful to skip setSurface if the release call was received earlier?
Thanks :)
Addressing your second comment first, since it's easy to tick off:
Also, I'm not sure if this is feasible or already being done, but is it possible (or even useful) to avoid dispatching/handling MSG_SET_SURFACE when ExoPlayer#release has been called?
I'm pretty sure this already happens. Which as an aside, implies that surfaceDestroyed
is being called before onStop
in cases where you're seeing this error.
Addressing your first comment:
We recently split the workaround you mention in two: codecNeedsSetOutputSurfaceWorkaround
and codecNeedsDummySurfaceSurfaceWorkaround
. It's better to use the latter if that's sufficient to fix the problem, since it still allows for seamless switching of playbacks from one valid surface to another.
Does your app ever switch the surface that it outputs too? If so, and if that case isn't failing, that would indicate that codecNeedsDummySurfaceSurfaceWorkaround
would be sufficient. On the other hand, if your app doesn't ever do this, there's no easy way to tell which workaround is needed for each device.
If you could answer the question above, that would be great. In the meantime we'll try and get hold of some of the devices listed and test them manually.
It sure doesn't look good adding all these devices to the workaround list, but there are only 4 or 5 unique decoder names in the list, so maybe there is some pattern here?
I don't think we can blacklist solely based on decoder name. SOC vendors tend to use the same decoder name across all of their products. So for example, any device that uses a MediaTek chipset will have a decoder named OMX.MTK.VIDEO.DECODER.AVC. However most likely only a subset of MediaTek chipsets need the workaround, and there's no public API on Android for querying what the chipset is.
Still, the data provided is really useful for helping us to dig for a pattern. What we'll do on our side is:
After all that is done, we'll try and close the loop with you (i.e. check that some version you release in the future is not still logging these failures).
Note to self: It was suggested in #3835 that all devices that use MT6737 and MT6753 chipsets may be affected by this issue.
@qvk - Is it possible for you to add a new column to this data that shows more specific Android build information? I'm not sure what data you have exactly. Build fingerprints (Build.FINGERPRINT
) would be ideal. They look something like:
motorola/taido_row/taido_row:6.0/MRA58K/XT1706_S149_180322_ROW:user/release-keys
If you don't have that just the build name (MRA58K
) would be helpful, albeit not to the same extent!
Some of the devices in your list appear to have passed the tests that are supposed to prevent this issue from occurring, however it's unclear whether the reports you're seeing are from devices that are running earlier builds than those which passed the tests. It would be good to check that as part of our cross-referencing.
@ojw28 I suggest tracking all the devices that need any type of ExoPlayer workaround in a wiki page, here on GitHub. Doesn't have to be a public-edit wiki.
@ojw28 Thanks for the help!
We recently split the workaround you mention in two: codecNeedsSetOutputSurfaceWorkaround and codecNeedsDummySurfaceSurfaceWorkaround.
I believe this is present in dev-v2
; yes indeed that seems like a better way to divide the generic workaround.
Does your app ever switch the surface that it outputs too?
No. I guess this means there isn't enough information to use the more targeted codecNeedsDummySurfaceSurfaceWorkaround
workaround for these devices.
Is it possible for you to add a new column to this data that shows more specific Android build information? ... Build fingerprints (Build.FINGERPRINT) would be ideal.
Sorry, we currently don't have Build.FINGERPRINT
or build name data. If you believe this information is critical to applying more targeted workarounds, we could additionally log this information on the affected devices in a future release, however this might take a couple of weeks to gather any significant data.
Is the build name available inside a Build
class property, or does it need to be parsed out of the build fingerprint?
Also, I was wondering that if an application never needs to switch surfaces, is it ok to have codecNeedsDummySurfaceSurfaceWorkaround
or codecNeedsSetOutputSurfaceWorkaround
always true? Only a single Surface
is involved and shouldInitCodec
returns false when invoked anytime before surfaceCreated
and invoked when surfaceDestroyed
-> setSurface
-> maybeInitCodec
.
Sorry, we currently don't have Build.FINGERPRINT or build name data. If you believe this information is critical to applying more targeted workarounds, we could additionally log this information on the affected devices in a future release, however this might take a couple of weeks to gather any significant data.
Assuming it's relatively straightforward for you to add this to the data that's being logged, it would be great if you could! Unsure about the build name (by itself), but providing the full fingerprints would be ideal.
Also, I was wondering that if an application never needs to switch surfaces, is it ok to have codecNeedsDummySurfaceSurfaceWorkaround or codecNeedsSetOutputSurfaceWorkaround always true?
Yes, I think doing that would be fine for that use case. Alternatively, it's probably significantly easier for you just to call release()
on the player from whatever happens first out of onStop
and surfaceDestroyed
(i.e. have both methods call a maybeReleasePlayer
method, and release the player there if it's not been released already). This should also prevent the surface switch that you're currently seeing.
Some initial findings:
codecNeedsDummySurfaceWorkaround
really is. It seems like devices either (a) work for all cases, or (b) work for cases where the type of texture remains the same, or (c) don't work at all. I think codecNeedsDummySurfaceWorkaround
is approximating case (b), because most often apps are using SurfaceView
, and SurfaceView
-> DummySurface
is a switch between texture types. However codecNeedsDummySurfaceWorkaround
is not targeted correctly for case (b) because it doesn't apply the workaround for SurfaceView
-> TextureView
switches (which need it) and does apply the workaround for TextureView
-> DummySurface
switches (which don't).@ojw28 we had the same issue on a Meizu Pro 7 phone (the device name is PRO7S). I added the device name inside the codecNeedsSetOutputSurfaceWorkaround
method and now everything works well.
Could you add this device name too, please?
This should have been largely addressed in 2.8.3. Please let us know if you see further devices experiencing this issue using 2.8.3 or later (a list with occurrence counts, similar to the one above, would be great). Thanks!
Thanks! It might take a while to gather data with the 2.8.3 version though.
We're seeing this issue since updating to v2.8.4, so it seems this issue is still present. We don't have a list of occurrence counts per device yet as we didn't have this problem with an older version of ExoPlayer, but we have quite a significant count of these Exceptions. On Fabric alone we can see around 10,000 to be exact. Following information is what I was able to extract from Fabric's logged Exceptions.
We're using an instance of SimpleExoPlayer
and we're not switching any surfaces. We are calling clearVideoSurface
however when our player instance does not need to render video. This is when this exception occurs and prohibits audio-only playback.
Devices and their API levels from last several Exceptions being thrown:
Example log:
Non-fatal Exception: com.google.android.exoplayer2.ExoPlaybackException
at com.google.android.exoplayer2.ExoPlayerImplInternal.handleMessage(ExoPlayerImplInternal.java:354)
at android.os.Handler.dispatchMessage(Handler.java:107)
at android.os.Looper.loop(Looper.java:210)
at android.os.HandlerThread.run(HandlerThread.java:61)
Caused by java.lang.IllegalArgumentException
at android.media.MediaCodec.native_setSurface(MediaCodec.java)
at android.media.MediaCodec.setOutputSurface(MediaCodec.java:1837)
at com.google.android.exoplayer2.video.MediaCodecVideoRenderer.setOutputSurfaceV23(MediaCodecVideoRenderer.java:918)
at com.google.android.exoplayer2.video.MediaCodecVideoRenderer.setSurface(MediaCodecVideoRenderer.java:408)
at com.google.android.exoplayer2.video.MediaCodecVideoRenderer.handleMessage(MediaCodecVideoRenderer.java:375)
at com.google.android.exoplayer2.ExoPlayerImplInternal.deliverMessage(ExoPlayerImplInternal.java:861)
at com.google.android.exoplayer2.ExoPlayerImplInternal.sendMessageToTarget(ExoPlayerImplInternal.java:829)
at com.google.android.exoplayer2.ExoPlayerImplInternal.sendMessageInternal(ExoPlayerImplInternal.java:811)
at com.google.android.exoplayer2.ExoPlayerImplInternal.handleMessage(ExoPlayerImplInternal.java:328)
at android.os.Handler.dispatchMessage(Handler.java:107)
at android.os.Looper.loop(Looper.java:210)
at android.os.HandlerThread.run(HandlerThread.java:61)
@gapl - I think we need better data (see e.g. the kind of data in the original post of this issue). In particular occurrence counts for each device (Util.DEVICE
/ Util.MODEL
) are important.
We've since implemented the workaround by always returning true
for codecNeedsSetOutputSurfaceWorkaround(:)
. So sadly I cannot provide additional data without pushing out a version that would not work for some of our users.
We're still adding the odd device to this list, but I think the bulk of the work here is done. We also made it easy for applications to override the method that decides whether the workaround is applied. I think it's fine to close at this point.
@ojw28 does Google plan to make ExoPlayer's OEM testing a mandatory part of Android certification? That should make sure this list doesn't get any longer...
A subset of the tests you refer to are already a mandatory part of Android certification, including the one covering this issue.
That's what I was hoping for, thanks.
Hi,
we got customer complaints which point to a similar problem when decoding DRM streams.
Caused by java.lang.IllegalArgumentException at android.media.MediaCodec.native_setSurface(MediaCodec.java) at android.media.MediaCodec.setOutputSurface(MediaCodec.java:1975) at com.google.android.exoplayer2.video.MediaCodecVideoRenderer.setOutputSurfaceV23(SourceFile:1014) at com.google.android.exoplayer2.video.MediaCodecVideoRenderer.setSurface(SourceFile:422) at com.google.android.exoplayer2.video.MediaCodecVideoRenderer.handleMessage(SourceFile:387) at com.google.android.exoplayer2.ExoPlayerImplInternal.deliverMessage(SourceFile:871) at com.google.android.exoplayer2.ExoPlayerImplInternal.sendMessageToTarget(SourceFile:842) at com.google.android.exoplayer2.ExoPlayerImplInternal.sendMessageInternal(SourceFile:824) at com.google.android.exoplayer2.ExoPlayerImplInternal.handleMessage(SourceFile:333) at android.os.Handler.dispatchMessage(Handler.java:105) at android.os.Looper.loop(Looper.java:166) at android.os.HandlerThread.run(HandlerThread.java:65)
Device info: Build.MANUFACTURER: HUAWEI Build.MODEL: EML-L29 Android version: 8.1.0
Since this ticket is closed, should we enable the workaround for this device ourselves or do you guys still add devices to the list?
We can enable the workaround for that device (end EML-L09, which looks identical).
Perfect, thanks!
Issue description
We're getting multiple records of
ExoPlaybackException
caused byIllegalArgumentException
(sometimesMediaCodec#CodecException
) whenMediaCodec#setOutputSurface
is called as seen in collected stacktraces. These happen when exiting the video activity, so I'm assumingSimpleExoPlayer
sends aMSG_SET_SURFACE
whensurfaceDestroyed
is invoked by the system, which eventually leads to the playback exception.We started logging decoder name, error stacktraces and device info inside
onPlayerError
callback only in recent releases, so the actual occurrences are likely much more than we could record.It sure doesn't look good adding all these devices to the workaround list, but there are only 4 or 5 unique decoder names in the list, so maybe there is some pattern here?
Reproduction steps
We don't have the affected devices on hand but according to our logs, the issue occurs upon exiting any activity which is playing media using exoplayer.
Version of ExoPlayer being used
2.7.0
Device(s) and version(s) of Android being used
These are data collected from last couple of weeks for cases where
onPlayerError
callback is received with the above mentioned exception.Many devices near the end of the list with very few occurrences might not warrant adding them to the exception list. Although they could be new devices just starting to grow in use.
A full bug report captured from the device
Unfortunately, we only noticed these errors from logs collected from remote user devices, so it's not possible to collect bug report using
adb
.However, the stacktrace collected in all cases (collected inside
onPlayerError
callback) is either of the following two: