Use AndroidDecoderModule to decode VP8/VP9

RESOLVED FIXED in Firefox 44

Status

()

defect
P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: j, Assigned: j)

Tracking

Trunk
mozilla44
Points:
---

Firefox Tracking Flags

(firefox42 affected, firefox44 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
There is hardware decoder support for VP8/9 available in some Android devices.
(Assignee)

Comment 1

4 years ago
Attachment #8642423 - Flags: review?(jyavenard)
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 ?
Attachment #8642423 - Flags: review?(jyavenard) → review+
doh !
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.
Flags: needinfo?(j)
(Assignee)

Comment 7

4 years ago
ok, lets try another way. can you push that to try.
Assignee: nobody → j
Attachment #8642423 - Attachment is obsolete: true
Flags: needinfo?(j)
Attachment #8647984 - Flags: review?(jyavenard)
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 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
Attachment #8647984 - Flags: review?(jyavenard)
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)
Flags: needinfo?(snorp)
Summary: AndroidDecoderModule can decode VP8/VP9 → Use AndroidDecoderModule to decode VP8/VP9
Eugen is the media guy :)
Flags: needinfo?(snorp)
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
(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 ?
Priority: -- → P2
Android fragmentation causes problems with this. Need to be able to fall back if it fails.
And by fail, we mean crash on actual decode on Sony.
This will fix the configureCodec error, we haven't been translating the MIME type in CreateVideoDecoder.
Attachment #8647984 - Attachment is obsolete: true
Attachment #8654880 - Flags: review?(jyavenard)
(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.
Attachment #8654880 - Flags: review?(jyavenard) → review+
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).
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
Flags: needinfo?(droeh)
I'm ok with with disabling the copy on emulators
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
Attachment #8666784 - Flags: review?(snorp) → review+
https://hg.mozilla.org/mozilla-central/rev/5f2cdf6fee39
https://hg.mozilla.org/mozilla-central/rev/4d3f66d8a73d
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Flags: needinfo?(droeh)
You need to log in before you can comment on or make changes to this bug.