1190379 - Use AndroidDecoderModule to decode VP8/VP9

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[Jan Gerber]

There is hardware decoder support for VP8/9 available in some Android devices.

Comment 1

[Jan Gerber]

Attachment: Bug-1190379-Use-AndroidDecoderModule-for-VP8-9.patch

Comment 2

[Jean-Yves Avenard [:jya]]

Comment on attachment 8642423 [details] [diff] [review]
Bug-1190379-Use-AndroidDecoderModule-for-VP8-9.patch

Review of attachment 8642423 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/platforms/android/AndroidDecoderModule.cpp
@@ +37,5 @@
>  static MediaCodec::LocalRef CreateDecoder(const nsACString& aMimeType)
>  {
>    MediaCodec::LocalRef codec;
> +  nsACString type = *aMimeType;
> +  if (aMimeType.EqualsLiteral("video/webm; codecs=vp8")) {

all this shows me that the use of video/webm inside the codec mimetype was a bad choice.

it shouldn't be depending on the container.

Could you create a 2nd patch that removes from the codec mimetype any mentions of the container ?

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a7ec92071a5

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android bustage.
https://treeherder.mozilla.org/logviewer.html#?job_id=12623871&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/c95476e9b53b

Comment 5

[Jean-Yves Avenard [:jya]]

doh !

Comment 6

[Jean-Yves Avenard [:jya]]

From https://bugzilla.mozilla.org/show_bug.cgi?id=1188870#c16:

this doesn't appear to work anyway.

Now JanH had crashes with and without, so it may be something else.

Comment 7

[Jan Gerber]

Attachment: Bug-1190379-Use-AndroidDecoderModule-for-VP8-9.patch

ok, lets try another way. can you push that to try.

Comment 8

[Jean-Yves Avenard [:jya]]

Oh, if you check the actual commit in the try run in bug 1188870, I had fixed all the compilations issue.

The issue here is that we can't test webm regardless.

With software decoding it crashes on Android ; and with this patch it crashes even quicker.

Comment 9

[Jean-Yves Avenard [:jya]]

Comment on attachment 8647984 [details] [diff] [review]
Bug-1190379-Use-AndroidDecoderModule-for-VP8-9.patch

Review of attachment 8647984 [details] [diff] [review]:
-----------------------------------------------------------------

The new webm isn't used on android by the look of things.. Need to fix that first.

And I like the earlier version of the code (with my fixes) better

Comment 10

[Jean-Yves Avenard [:jya]]

Confirming that this doesn't work for some reason on Android.

Immediately get a decoding error. Maybe the demuxer doesn't provide the samples in a format stagefright decoder expect.

snorp is this something your team could look into ? (I'll post an update to bug 1195066 to test this code)

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

Eugen is the media guy :)

Comment 12

[Eugen Sawin [:esawin]]

There are two issues I've found when running with the patch: off-main-thread pref fetching (failed assertions in Preferences::InitStaticMembers and pref_HashTableLookup) and ACodec::configureCodec returns an error (MEDIA_ERROR_UNSUPPORTED [1]).

Tested on Nexus 5 Android 5.1.1 (so VP9 decoding should be supported according to [2]) on the VP9 example from http://me73.com/media.

[1] http://developer.android.com/reference/android/media/MediaPlayer.html#MEDIA_ERROR_UNSUPPORTED
[2] http://developer.android.com/guide/appendix/media-formats.html

Comment 13

[Jean-Yves Avenard [:jya]]

(In reply to Eugen Sawin [:esawin] from comment #12)
> There are two issues I've found when running with the patch: off-main-thread
> pref fetching (failed assertions in Preferences::InitStaticMembers and

would that be the same as bug 1183788?

> pref_HashTableLookup) and ACodec::configureCodec returns an error
> (MEDIA_ERROR_UNSUPPORTED [1]).

that's fine, it should then fall back to software as CreateDecoder would have returned nullptr.

So why is it crashing ?

Comment 14

[Ralph Giles (:rillian)]

Android fragmentation causes problems with this. Need to be able to fall back if it fails.

Comment 15

[Ralph Giles (:rillian)]

And by fail, we mean crash on actual decode on Sony.

Comment 16

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1190379-Use-AndroidDecoderModule-for-VP8-9.patch

This will fix the configureCodec error, we haven't been translating the MIME type in CreateVideoDecoder.

Comment 17

[Eugen Sawin [:esawin]]

(In reply to [PTO Until 02/Sep] Jean-Yves Avenard [:jya] from comment #13)
> (In reply to Eugen Sawin [:esawin] from comment #12)
> > There are two issues I've found when running with the patch: off-main-thread
> > pref fetching (failed assertions in Preferences::InitStaticMembers and
> 
> would that be the same as bug 1183788?

Yes, and it has been fixed in bug 1195401, so it's not related to this patch.

> > pref_HashTableLookup) and ACodec::configureCodec returns an error
> > (MEDIA_ERROR_UNSUPPORTED [1]).
> 
> that's fine, it should then fall back to software as CreateDecoder would
> have returned nullptr.

That's not OK since we first claim to support the format and then fail when creating the decoder, see latest patch.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/49a711e67d8b

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for various crashes on Android like:
 https://treeherder.mozilla.org/logviewer.html#?job_id=14432821&repo=mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/36d21c206b91

Comment 20

[James Willcox (:snorp) ([e-mail]) (he/him)]

Eugen, we have a pref to disable this blit, "gfx.SurfaceTexture.detach.enabled". I would just try setting that to false in remotereftest.py (we already set a bunch of other prefs).

Comment 21

[Eugen Sawin [:esawin]]

So disabling surface texture detaching didn't help [1], so I ran a sanity check with hard-coded disabling [2], which gave a different crash signature - disabling detaching without the VPx patch didn't cause the same issues [3], so it's definitely this patch.

The test file which fails in [1] runs without issues on a real device. I'll try to run these tests on a local emulator next.
Dylan, do any of those failures look like anything you've encountered before? 

Preventing the surface copying early on, seems to tame Try a little [4], maybe we should go with this instead?

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ae189460ac5
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=18bbc1a2e84b
[3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=d55a5ba6fbfc
[4] https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c1aacdaf83f

Comment 22

[James Willcox (:snorp) ([e-mail]) (he/him)]

I'm ok with with disabling the copy on emulators

Comment 23

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1190379-Disable-surface-copying-on-emulators.-r-.patch

Comment 24

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8666784 [details] [diff] [review]
0002-Bug-1190379-Disable-surface-copying-on-emulators.-r-.patch

Review of attachment 8666784 [details] [diff] [review]:
-----------------------------------------------------------------

A comment with an explanation or a link to this bug may be useful

Comment 25

[Eugen Sawin [:esawin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1e32a13e301

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f2cdf6fee39
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d3f66d8a73d

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5f2cdf6fee39
https://hg.mozilla.org/mozilla-central/rev/4d3f66d8a73d