973761 - [WebRTC] Check devices capability(HW codec and Android ver) before enable use of vp8 hardware acceleration on Fennec

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/31.0.1650.63 Safari/537.36

Steps to reproduce:

This bug is used for tracking the implementation of check logic of device capability in webrtc on fennec.

Basic idea:
create a new fennec preference which give user an option to enable/disable vp8 hardware support for webrtc


Test steps:
    1. enable VP8 HW support in fennec preference
    2. surf http://mozilla.github.io/webrtc-landing/gum_test.html


Actual results:

1. no popup dialog shown

2. SW encode/decode (libvpx in webrtc.org stack) is used for handling concrete codec process



Expected results:

1. show a popup dialog to user, let him know that he is using a experimental feature which is not stabled.

2. 
For device which support VP8 hardware encode/decode
    HW encode/decode is enabled and get smooth video stream in local and remote view area

For device which not support VP8 hardware encode/decode
     SW encode/decode (libvpx in webrtc.org stack) is used for handling concrete codec process

Comment 1

[Gian-Carlo Pascutto [:gcp]]

FWIW there's no need to show a popup. Just hide the entire feature behind a pref at first, and when it's starting to get reasonably stable, we'll flip the pref for Nightly only, until it's stable enough to be enabled everywhere. Users flipping a pref or using Nightly should understand things are experimental and unstable.

Comment 2

[qiang lu]

Attachment: adding device blacklist and feature preference for webrtc vp8 hardware acceleration support

Comment 3

[qiang lu]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #1)
> FWIW there's no need to show a popup. Just hide the entire feature behind a
> pref at first, and when it's starting to get reasonably stable, we'll flip
> the pref for Nightly only, until it's stable enough to be enabled
> everywhere. Users flipping a pref or using Nightly should understand things
> are experimental and unstable.

1. add one feature preference to enable/disable webrtc hardware acceleration on android which default value is false.

2. add blacklist to gfxinfo, only Nexus 5 is in the whitelist, others are in the blacklist.

Comment 4

[qiang lu]

(In reply to qiang lu from comment #3)
> (In reply to Gian-Carlo Pascutto (:gcp) from comment #1)
> > FWIW there's no need to show a popup. Just hide the entire feature behind a
> > pref at first, and when it's starting to get reasonably stable, we'll flip
> > the pref for Nightly only, until it's stable enough to be enabled
> > everywhere. Users flipping a pref or using Nightly should understand things
> > are experimental and unstable.
> 
> 1. add one feature preference to enable/disable webrtc hardware acceleration
> on android which default value is false.
> 
> 2. add blacklist to gfxinfo, only Nexus 5 is in the whitelist, others are in
> the blacklist.

Typical use case would be in the ConfigureSendMediaCodec/ConfigureRecvMediaCodec function of VideoCondiut.cpp like this:

#ifdef MOZILLA_INTERNAL_API
  CSFLogError(logTag, "%s Start to check hardware acceleration 1", __FUNCTION__);
  bool enabled = Preferences::GetBool("media.navigator.hardware.acceleration_enabled", false);
#else
  bool enabled = false;
#endif

  if (enabled) {
    CSFLogError(logTag, "%s Start to check hardware acceleration", __FUNCTION__);

    nsCOMPtr<nsIGfxInfo> gfxInfo = do_GetService("@mozilla.org/gfx/info;1");
    if (gfxInfo) {
      int32_t status;
      if (NS_SUCCEEDED(gfxInfo->GetFeatureStatus(nsIGfxInfo::FEATURE_WEBRTC_HW_ACCELERATION, &status))) {
        if (status != nsIGfxInfo::FEATURE_NO_INFO) {
          NS_WARNING("XXX webrtc hardware acceleration blacklisted\n");
          CSFLogError(logTag, "%s XXX webrtc hardware acceleration blacklisted", __FUNCTION__);
        } else {
          // TODO
          CSFLogError(logTag, "%s Start to registr hardware acceleration", __FUNCTION__);
        }
      }
    }
  }

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8388379 [details] [diff] [review]
adding device blacklist and feature preference for webrtc vp8 hardware acceleration support

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

::: modules/libpref/src/init/all.js
@@ +55,5 @@
>  pref("browser.cache.disk.max_entry_size",    51200);  // 50 MB
>  pref("browser.cache.memory.enable",         true);
>  // -1 = determine dynamically, 0 = none, n = memory capacity in kilobytes
>  //pref("browser.cache.memory.capacity",     -1);
> +// Max-size (in KB) for entries in memory cache. Set to -1 for no limit.

I presume you modified your editor settings after the remarks about spurious whitespace in previous patches? That's fine, but here you're modifying code that's not relevant to this patch, which you probably want to avoid (makes hg blame and so on harder to use, needless merge conflicts, etc).

It's also making this patch way bigger than it needs to be.

@@ +272,5 @@
>  pref("media.peerconnection.capture_delay", 50);
>  #elif defined(ANDROID)
>  pref("media.peerconnection.capture_delay", 100);
> +// Whether to enable Webrtc Hardware acceleration support
> +pref("media.navigator.hardware.acceleration_enabled", false);

I'd include VP8 in the name as we may want H264 too. And maybe it's worthwhile to split encode and decode to deal with potential device bugs? 

For example:
media.navigator.hardware.vp8_encode_acceleration
media.navigator.hardware.vp8_decode_acceleration

::: widget/nsIGfxInfo.idl
@@ +79,5 @@
>    const long FEATURE_WEBGL_MSAA = 8;
>    /* Whether Stagefright is supported */
>    const long FEATURE_STAGEFRIGHT = 9;
> +  /* Whether Webrtc Hardware acceleration is supported */
> +  const long FEATURE_WEBRTC_HW_ACCELERATION = 10;

I'm not sure this GfxInfo stuff is the place where this detection should happen, but I see why you put it here, i.e. the STAGEFRIGHT detection for H264 is here as well. 

I'm going to defer to bjacob for the review, based on bug 806396 and him having authored this.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

That should've been bug 806369.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

I don't want to be considered the 'author' of this blacklisting code, since it is one of the worst things we have in the tree and I've merely been one of the maintainers, but I'll do the review :-)

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8388379 [details] [diff] [review]
adding device blacklist and feature preference for webrtc vp8 hardware acceleration support

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

R+ on the GfxInfo parts, not commenting on the preference changes which are outside of my scope.

But know one very important thing:

There are two kinds of blacklist rules:
 1) blacklist rules hardcoded in C++ code, like here;
 2) blacklist rules in downloadable XML blacklists.

Here you are doing 1), which is good. It is very important that no-one adds a XML blacklist entry (case 2 above) based on this new FEATURE value, because our XML blacklists are not versioned and the same XML file is used by all existing users, including users still running older Firefox versions, and our stupid blacklisting code interpretes any unknown FEATURE value as "all features" i.e. WebGL, Stagefright, etc.

For that reason we've put GfxInfo in minimal maintainance mode and are very conservative about updating it, but here you should be safe as long as no one makes a XML blacklist entry based on this new FEATURE.

Comment 9

[qiang lu]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #5)
> Comment on attachment 8388379 [details] [diff] [review]
> adding device blacklist and feature preference for webrtc vp8 hardware
> acceleration support
> 
> Review of attachment 8388379 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: modules/libpref/src/init/all.js
> @@ +55,5 @@
> >  pref("browser.cache.disk.max_entry_size",    51200);  // 50 MB
> >  pref("browser.cache.memory.enable",         true);
> >  // -1 = determine dynamically, 0 = none, n = memory capacity in kilobytes
> >  //pref("browser.cache.memory.capacity",     -1);
> > +// Max-size (in KB) for entries in memory cache. Set to -1 for no limit.
> 
> I presume you modified your editor settings after the remarks about spurious
> whitespace in previous patches? That's fine, but here you're modifying code
> that's not relevant to this patch, which you probably want to avoid (makes
> hg blame and so on harder to use, needless merge conflicts, etc).
> 
> It's also making this patch way bigger than it needs to be.
Done. only change the part relevant to this patch.

> 
> @@ +272,5 @@
> >  pref("media.peerconnection.capture_delay", 50);
> >  #elif defined(ANDROID)
> >  pref("media.peerconnection.capture_delay", 100);
> > +// Whether to enable Webrtc Hardware acceleration support
> > +pref("media.navigator.hardware.acceleration_enabled", false);
> 
> I'd include VP8 in the name as we may want H264 too. And maybe it's
> worthwhile to split encode and decode to deal with potential device bugs? 
> 
> For example:
> media.navigator.hardware.vp8_encode_acceleration
> media.navigator.hardware.vp8_decode_acceleration
> 
Done.

> ::: widget/nsIGfxInfo.idl
> @@ +79,5 @@
> >    const long FEATURE_WEBGL_MSAA = 8;
> >    /* Whether Stagefright is supported */
> >    const long FEATURE_STAGEFRIGHT = 9;
> > +  /* Whether Webrtc Hardware acceleration is supported */
> > +  const long FEATURE_WEBRTC_HW_ACCELERATION = 10;
> 
> I'm not sure this GfxInfo stuff is the place where this detection should
> happen, but I see why you put it here, i.e. the STAGEFRIGHT detection for
> H264 is here as well. 
> 
> I'm going to defer to bjacob for the review, based on bug 806396 and him
> having authored this.

Comment 10

[qiang lu]

Attachment: B973761_v02.patch

adding device blacklist and feature preference for webrtc vp8 hardware acceleration support

Comment 11

[qiang lu]

Hi, gcp

Could you please have a look at it? 

I want to add this check to B969395.

Comment 12

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8394128 [details] [diff] [review]
B973761_v02.patch

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

::: widget/android/GfxInfo.cpp
@@ +479,5 @@
>            CompareVersions(mOSVersion.get(), "4.0.0") < 0)
>        {
>          // Honeycomb Samsung devices are whitelisted.
>          // All other Honeycomb devices are blacklisted.
> +        bool isWhitelisted =

nit: this line wasn't actually changed?

@@ +561,5 @@
> +          cModel.Equals("nexus 5", nsCaseInsensitiveCStringComparator())) {
> +        *aStatus = nsIGfxInfo::FEATURE_NO_INFO;
> +        return NS_OK;
> +      } else {
> +        // Blocklist all other devices except Nexus 5 which VP8 hardware acceleration is supported

which -> for which

Comment 13

[qiang lu]

Hi, gcp

It seems that all comments are addressed.

Could you please tell me who is available to review this patch as superreviewer?

Comment 14

[qiang lu]

Hi, gcp
If this patch not need to reviewed by superreviewer, could you please guide me how can i make this patch merged?

Comment 15

[Gian-Carlo Pascutto [:gcp]]

Put "checkin-needed" in the keywords flag. I just did it for you. 

FWIW, you can use the "need more information from" thingie on the bottom if you require a response. It'll show up for me as a task to do. Sometimes a bugzilla question gets overlooked - as was the case here. Sorry about that.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f5998994067

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/1f5998994067