1178069 - [WebRTC] Check devices capability before enable use of vp8 hardware acceleration using android.media.MediaCodecList and android.media.MediaCodecInfo

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[qiang lu]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/41.0.2272.101 Safari/537.36

Steps to reproduce:

This bug implement an new mechanism for helping user enable/disable VP8 hardware acceleration according to their devices's capability.

In some devices (latest samsung, intel, qualcomm based devices), even the VP8 hardware acceleration is supported, we couldn't enable it by media.navigator.hardware.vp8_encode.acceleration_enabled/media.navigator.hardware.vp8_decode.acceleration_enabled. (since according to B973761, only the nexus5 equipt with the VP8 hardware codec)



Actual results:

user can enable the media.navigator.hardware.vp8_encode.acceleration_enabled/media.navigator.hardware.vp8_decode.acceleration_enabled in about:config, but VP8 hardware acceleration can  not be evaluated on those devices except on Nexus 5.


Expected results:

user can enable the media.navigator.hardware.vp8_encode.acceleration_enabled/media.navigator.hardware.vp8_decode.acceleration_enabled in about:config, then VP8 hardware acceleration can be evaluted on those devices. (see /etc/media_codec.xml)

Comment 1

[qiang lu]

Attachment: B1178069_v0.1.patch

Please help to review it.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8627078 [details] [diff] [review]
B1178069_v0.1.patch

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

See remarks. Deferring blacklist changes to kats, I'm unfamiliar with that code.

::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp
@@ +915,5 @@
>               if (status != nsIGfxInfo::FEATURE_STATUS_OK) {
>                 NS_WARNING("VP8 decoder hardware is not whitelisted: disabling.\n");
>               } else {
> +               NS_WARNING("VP8 decoder hardware is whitelisted: enabling.\n");
> +               MOZ_MTLOG(ML_ERROR, "VP8 decoder hardware us whitelisted. ");

nit: typo, extra space at end

::: mobile/android/base/GeckoAppShell.java
@@ +127,4 @@
>  {
>      private static final String LOGTAG = "GeckoAppShell";
>      private static final boolean LOGGING = false;
> +    // List of supported HW VP8 encoders.

Can we move this to a separate .java file outside GeckoAppShell?

@@ +984,5 @@
>      }
>  
> +    @WrapElementForJNI(stubName = "GetHWEncoderCapability")
> +    static boolean getHWEncoderCapability() {
> +      if (Build.VERSION.SDK_INT < Build.VERSION_CODES.KITKAT) {

You need to use AppConstants.java, see e.g. Bug 1042383

@@ +985,5 @@
>  
> +    @WrapElementForJNI(stubName = "GetHWEncoderCapability")
> +    static boolean getHWEncoderCapability() {
> +      if (Build.VERSION.SDK_INT < Build.VERSION_CODES.KITKAT) {
> +      return false; // MediaCodec.setParameters is missing.

nit: indentation

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8627078 [details] [diff] [review]
B1178069_v0.1.patch

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

nsIGfxInfo.idl will need a uuid bump, and I'm not sure if it's OK to renumber the features like this. Bouncing review to Jeff since I'm not confident in my review of this code.

Comment 4

[qiang lu]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #2)
> Comment on attachment 8627078 [details] [diff] [review]
> B1178069_v0.1.patch
> 
> Review of attachment 8627078 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> See remarks. Deferring blacklist changes to kats, I'm unfamiliar with that
> code.
> 

> ::: mobile/android/base/GeckoAppShell.java
> @@ +127,4 @@
> >  {
> >      private static final String LOGTAG = "GeckoAppShell";
> >      private static final boolean LOGGING = false;
> > +    // List of supported HW VP8 encoders.
> 
> Can we move this to a separate .java file outside GeckoAppShell?
> 
If i understand right, you suggest that extract those check logic to a separate class(in a new .java file), and get it can be used by GeckoAppShell directly?

Comment 5

[Gian-Carlo Pascutto [:gcp]]

(In reply to qiang lu from comment #4)

> > Can we move this to a separate .java file outside GeckoAppShell?
> > 
> If i understand right, you suggest that extract those check logic to a
> separate class(in a new .java file), and get it can be used by GeckoAppShell
> directly?

Yes, GeckoAppShell has so much stuff thrown into it...let's try to not make it worse and split off as much as possible.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8627078 [details] [diff] [review]
B1178069_v0.1.patch

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

I think it will be ok to renumber. Like, kats said, you'll need to re-uuid the class.

Comment 7

[qiang lu]

Attachment: B1178069_v0.2.patch

Addressing all the comments in this patch, please help to review it.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8629960 [details] [diff] [review]
B1178069_v0.2.patch

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

Looks good to me. Just a few tiny cosmetic issues.

::: mobile/android/base/util/HardwareCodecCapabilityUtils.java
@@ +88,5 @@
> +    return false;  // No HW encoder.
> +  }
> +
> +  public static boolean getHWDecoderCapability() {
> +    if (Versions.feature20Plus) { 

nit: stray whitespace

::: toolkit/mozapps/extensions/test/xpcshell/data/test_gfxBlacklist_AllOS.xml
@@ +144,5 @@
> +      <feature> WEBRTC_HW_ACCELERATION_DECODE </feature>
> +      <featureStatus> BLOCKED_DRIVER_VERSION </featureStatus>
> +    </gfxBlacklistEntry>
> +
> +

remove one empty line

::: widget/android/GfxInfo.cpp
@@ +597,1 @@
> +        // Blocklist all other devices except those VP8 hardware acceleration is supported

...except those where...

@@ +605,5 @@
> +      if (value) {
> +        *aStatus = nsIGfxInfo::FEATURE_STATUS_OK;
> +        return NS_OK;
> +      } else {
> +        // Blocklist all other devices except those VP8 hardware acceleration is supported

same

::: widget/nsIGfxInfo.idl
@@ +98,2 @@
> +  const long FEATURE_WEBRTC_HW_ACCELERATION_ENCODE = 10;
> +  /* Whether Webrtc Hardware acceleration is supported, starting in 31. */

What does "starting in 31" refer to?

Comment 9

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8629960 [details] [diff] [review]
B1178069_v0.2.patch

Making sure jrmuizel looks at the test_gfxBlacklist changes...from the discussion on IRC this hasn't actually been reviewed.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8629960 [details] [diff] [review]
B1178069_v0.2.patch

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

::: toolkit/mozapps/extensions/test/xpcshell/data/test_gfxBlacklist_AllOS.xml
@@ +138,1 @@
> +      <versionRange minVersion="42.0" maxVersion="13.0b2"/>

I don't see any reason that this tests should have these weird version range. You should probably fix it. You should also probably test that blocking works instead of testing that the feature is ok.

Comment 11

[qiang lu]

Could you please help look at Comment 10, i don't know how to address jrmuizel's comment， since i just changed this file follow your changeset(changeset 245153 b5fc35747dd4, B1164036 )：（

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8629960 [details] [diff] [review]
B1178069_v0.2.patch

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

From the blocklisting point of view, I would not remove the original feature, but just add the two new ones and change the logic in the code for the old one to mean both.  It is unnecessary risk to change the value of the existing blocklist constants.

::: toolkit/mozapps/extensions/test/xpcshell/data/test_gfxBlacklist_AllOS.xml
@@ +138,1 @@
> +      <versionRange minVersion="42.0" maxVersion="13.0b2"/>

Testing with these kinds of ranges is only really useful for the testing of the underlying system - since you're just adding an extra feature to test, I would stick with a valid range and test if the feature is getting properly disabled, and not worry too much about the "invalid" ranges.

::: widget/nsIGfxInfo.idl
@@ +98,2 @@
> +  const long FEATURE_WEBRTC_HW_ACCELERATION_ENCODE = 10;
> +  /* Whether Webrtc Hardware acceleration is supported, starting in 31. */

I would not do this.  Perhaps the values of the constants are safe to change, but there is no particular reason to chance it.  I would leave the old FEATURE_WEBRTC_HW_ACCELERATION constant in place, and just add the new _ENCODE and _DECODE as 14 and 15.

"Starting in..." is a comment telling us when the feature blocklist capability got added to Gecko.  These new _ENCODE and _DECODE values should thus be marked as "starting in 42", assuming this lands in 42.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8629960 [details] [diff] [review]
B1178069_v0.2.patch

What milan says.

Comment 14

[qiang lu]

Attachment: B1178069_v0.3.patch

Could you please help to review those part you mentioned in the comment 12.

Comment 15

[qiang lu]

Attachment: B1178069_v0.3.patch

Minor updates

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8635881 [details] [diff] [review]
B1178069_v0.3.patch

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

It's getting a bit complicated, but let's leave that for a separate bug - can you open one to figure out what we want to do with the old blocklisting "webrtc hw acceleration" now that we have two separate ones for decode and encode?  We can use that same bug to maybe clean the ordering of values in features[] array in GfxInfoBase::EvaluateDownloadedBlacklist.

We can also add the actual blocklisting tests for the two (three?) webrtc, perhaps in the same "cleanup" bug mentioned above, but for those tests to actually make sense, we first need to fix bug 1163038.

::: widget/android/AndroidBridge.cpp
@@ +489,5 @@
> +
> +  bool value = GeckoAppShell::GetHWDecoderCapability();
> +
> +  return value;
> +}

This is so much nicer than the previous "check the version, device, model" code.

::: widget/android/GfxInfo.cpp
@@ +590,2 @@
> +    if (aFeature == FEATURE_WEBRTC_HW_ACCELERATION_ENCODE) {
> +      bool value = mozilla::AndroidBridge::Bridge()->GetHWEncoderCapability();

Is AndroidBridge::Bridge() guaranteed to exist?  Can it return nullptr, in which case, we should check before calling a method on it.

@@ +593,1 @@
> +      if (value) {

I understand wanting to minimize the diff, but I wouldn't bother now that you have a simple function that lets you decide if encoder is supported or not; I'd just write:

*aStatus = mozilla::AndroidBridge::Bridge()->GetHWEncoderCapability() ? nsIGfxInfo::FEATURE_STATUS_OK : nsIGfxInfo::FEATURE_BLOCKED_DEVICE;
return NS_OK;

@@ +597,1 @@
> +        // Blocklist all other devices except those where VP8 hardware acceleration is supported

This comment doesn't really belong anymore, now that we've removed the explicit version/device check.

@@ +607,5 @@
> +        return NS_OK;
> +      } else {
> +        // Blocklist all other devices except those where VP8 hardware acceleration is supported
> +        *aStatus = nsIGfxInfo::FEATURE_BLOCKED_DEVICE;
> +        return NS_OK;

Same comment as above; this whole thing inside of the outside if, (modulo checking for the null pointer Bridge()) can be replaced with two, still readable lines.

::: widget/nsIGfxInfo.idl
@@ +11,1 @@
> +[scriptable, uuid(c8783499-4694-4d3f-b571-211f01f1e6d7)]

Pretty sure you don't have to change UUID, we're not changing the interface, but I see Kats' comment 3.

Comment 17

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> ...
> nsIGfxInfo.idl will need a uuid bump...

Kats, are you sure about this?

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Not 100% sure, but based on [1] which says "If you change an interface used in a method in such a way that it might break a user, update the uuid." - I think renaming constants in the IDL could break addons that use it, so a UUID update is warranted. I could be wrong though. AFAIK there's no downside to updating the UUID except it might make uplifting harder.

[1] https://blog.mozilla.org/sfink/2011/03/02/updating-uuids/

Comment 19

[qiang lu]

Attachment: B1178069_v1.0.patch

I addressed all comments and push my patch to try-server. but i got build failure message since i changed GeneratedJNIWrappers.h .cpp in this patch.

Build message told me (http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-4dfa520737ed/try-android-x86/try-android-x86-bm75-try1-build567.txt.gz)
-----------------
***   Error: The generated JNI code has changed   ***
00:48:29     INFO -  * To update generated code in the tree, please run  *
00:48:29     INFO -    make -C /builds/slave/try-and-x86-000000000000000000/build/src/obj-firefox/mobile/android/base update-generated-wrappers
00:48:29     INFO -  * Repeat the build, and check in any changes.
-----------------

Actually, In local test environment, i can address this issue by running 
"make -C xxxxx update-generated-wrappers" before "mach build".

Could you please tell me how can i avoid those errors in Try-server build system?

Comment 20

[Gian-Carlo Pascutto [:gcp]]

After you run the local make update-generated-wrappers, does hg status show any modified files? Add those to your patch.

Comment 21

[qiang lu]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #20)
> After you run the local make update-generated-wrappers, does hg status show
> any modified files? Add those to your patch.

Actually, i had include those generated files in my patch. see GeneratedJNIWrappers.h GeneratedJNIWrappers.cpp

I am afraid that there is some trick steps to tell the Try-Server build system how to use those new generated files.

Comment 22

[(inactive) Jim Chen [:jchen] [:darchons]]

There has been some changes to the generated code. You should update your local source tree (i.e. hg qpop -a && hg pull -u), then build again.

Also, your patch doesn't seem to include contextual lines. You should add "unified = 8" to your .hgrc file, under the [diff] section.

Comment 23

[qiang lu]

Attachment: B1178069_v1.1.patch

All comments are addressed and review+ by all reviewer.

Try-Server result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a426acbb8888

please help get it to checkin.

Comment 24

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8635881 [details] [diff] [review]
B1178069_v0.3.patch

If I understand correctly this version is obsolete.

Comment 25

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

https://hg.mozilla.org/integration/fx-team/rev/cf1dbe9a71cf

Comment 26

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

Please make sure you put a usable commit message in the patch next time when you request checkin-needed.

Comment 27

[qiang lu]

Sorry for this misunderstand about the check-in progress. 

I assume the latest patch would be used....

Please help to get it to merged again.

Try-Server result for the latest patch(B1178069_v1.1.patch)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a426acbb8888

Comment 28

[Carsten Book [:Tomcat]]

hi, this failed to apply:

applying B1178069_v1.1.patch
patching file media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp
Hunk #1 FAILED at 880
Hunk #2 FAILED at 905
2 out of 2 hunks FAILED -- saving rejects to file media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp.rej
patching file mobile/android/base/GeckoAppShell.java
Hunk #1 FAILED at 41
Hunk #2 FAILED at 86
Hunk #3 FAILED at 955
3 out of 3 hunks FAILED -- saving rejects to file mobile/android/base/GeckoAppShell.java.rej
patching file mobile/android/base/moz.build
Hunk #1 FAILED at 77
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/moz.build.rej
file mobile/android/base/util/HardwareCodecCapabilityUtils.java already exists
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/util/HardwareCodecCapabilityUtils.java.rej
patching file toolkit/mozapps/extensions/test/xpcshell/data/test_gfxBlacklist_AllOS.xml
Hunk #1 succeeded at 158 with fuzz 2 (offset 26 lines).
patching file toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_Version.js
Hunk #1 FAILED at 101
1 out of 1 hunks FAILED -- saving rejects to file toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_Version.js.rej
patching file widget/GfxInfoBase.cpp
Hunk #1 FAILED at 145
Hunk #2 FAILED at 334
Hunk #3 FAILED at 966
3 out of 3 hunks FAILED -- saving rejects to file widget/GfxInfoBase.cpp.rej
patching file widget/android/AndroidBridge.cpp
Hunk #1 FAILED at 466
1 out of 1 hunks FAILED -- saving rejects to file widget/android/AndroidBridge.cpp.rej
patching file widget/android/AndroidBridge.h
Hunk #1 FAILED at 201
1 out of 1 hunks FAILED -- saving rejects to file widget/android/AndroidBridge.h.rej
patching file widget/android/GeneratedJNIWrappers.cpp
Hunk #1 FAILED at 272
1 out of 1 hunks FAILED -- saving rejects to file widget/android/GeneratedJNIWrappers.cpp.rej
patching file widget/android/GeneratedJNIWrappers.h
Hunk #1 FAILED at 639
1 out of 1 hunks FAILED -- saving rejects to file widget/android/GeneratedJNIWrappers.h.rej
patching file widget/android/GfxInfo.cpp
Hunk #1 FAILED at 581
1 out of 1 hunks FAILED -- saving rejects to file widget/android/GfxInfo.cpp.rej
patching file widget/nsIGfxInfo.idl
Hunk #1 FAILED at 96
1 out of 1 hunks FAILED -- saving rejects to file widget/nsIGfxInfo.idl.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh B1178069_v1.1.patch

could you take a look, thanks!

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cf1dbe9a71cf

Comment 30

[qiang lu]

This patch is merged successfully.