Closed Bug 1458020 Opened 2 years ago Closed 2 years ago

Make WebRTC work in GeckoView w/ E10s

Categories

(GeckoView :: General, enhancement, P1)

59 Branch
enhancement

Tracking

(firefox-esr52 wontfix, firefox-esr60 wontfix, firefox59 wontfix, firefox60 wontfix, firefox61 wontfix, firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: snorp, Assigned: jchen)

References

Details

(Whiteboard: [geckoview:e10s] [geckoview:klar] [geckoview:crow])

Attachments

(8 files)

Whiteboard: [geckoview:e10s] → [geckoview:e10s] [geckoview:klar]
Attached patch webrtc-e10s.diffSplinter Review
This patch makes it so we don't crash, but then we have the issue with using nsIPermissionManager in the child.
Assignee: nobody → nchen
Status: NEW → ASSIGNED
Flags: needinfo?(nchen)
Priority: -- → P1
Whiteboard: [geckoview:e10s] [geckoview:klar] → [geckoview:e10s] [geckoview:klar] [geckoview:crow]
Comment on attachment 8975598 [details]
Bug 1458020 - 2. Add PermissionDelegateTest;

https://reviewboard.mozilla.org/r/243848/#review249760

lgtm.
Attachment #8975598 - Flags: review?(jib) → review+
Comment on attachment 8975597 [details]
Bug 1458020 - 1b. Add forceGarbageCollection;

https://reviewboard.mozilla.org/r/243846/#review249788
Attachment #8975597 - Flags: review?(snorp) → review+
Comment on attachment 8975597 [details]
Bug 1458020 - 1b. Add forceGarbageCollection;

https://reviewboard.mozilla.org/r/243846/#review249790

I feel like it might be easier to understand a test page that does all of the stuff you're doing in the evaluateJs calls, but I guess there is no functional difference.
Comment on attachment 8975599 [details]
Bug 1458020 - 3. Set JavaVM when actually using video capture;

https://reviewboard.mozilla.org/r/243850/#review249792
Attachment #8975599 - Flags: review?(snorp) → review+
Depends on: 1461746
Comment on attachment 8975600 [details]
Bug 1458020 - 4. Add camera permission in the parent process;

https://reviewboard.mozilla.org/r/243852/#review250010

r+ with nits.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java:1855
(Diff revision 1)
>      }
>  
>      @WrapForJNI(calledFrom = "any")
>      public static int getAudioOutputFramesPerBuffer() {
>          if (SysInfo.getVersion() < 17) {
> -            return 0;
> +            return 512;

Make that a constant and log a warning.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java:1860
(Diff revision 1)
> -            return 0;
> +            return 512;
>          }
>          final AudioManager am = (AudioManager)getApplicationContext()
>                                  .getSystemService(Context.AUDIO_SERVICE);
>          if (am == null) {
> -            return 0;
> +            return 512;

Same.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java:1864
(Diff revision 1)
>          if (am == null) {
> -            return 0;
> +            return 512;
>          }
>          final String prop = am.getProperty(AudioManager.PROPERTY_OUTPUT_FRAMES_PER_BUFFER);
>          if (prop == null) {
> -            return 0;
> +            return 512;

Same.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java:1872
(Diff revision 1)
>      }
>  
>      @WrapForJNI(calledFrom = "any")
>      public static int getAudioOutputSampleRate() {
>          if (SysInfo.getVersion() < 17) {
> -            return 0;
> +            return 44100;

Same.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java:1877
(Diff revision 1)
> -            return 0;
> +            return 44100;
>          }
>          final AudioManager am = (AudioManager)getApplicationContext()
>                                  .getSystemService(Context.AUDIO_SERVICE);
>          if (am == null) {
> -            return 0;
> +            return 44100;

Same.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java:1881
(Diff revision 1)
>          if (am == null) {
> -            return 0;
> +            return 44100;
>          }
>          final String prop = am.getProperty(AudioManager.PROPERTY_OUTPUT_SAMPLE_RATE);
>          if (prop == null) {
> -            return 0;
> +            return 44100;

Same.
Attachment #8975600 - Flags: review?(esawin) → review+
Attachment #8975598 - Flags: review?(snorp)
Attachment #8975599 - Flags: review?(jib)
Attachment #8975600 - Flags: review?(snorp)
Attachment #8975940 - Flags: review?(esawin)
Comment on attachment 8975941 [details]
Bug 1458020 - 6. Use separate emulator config for geckoview-junit tests;

https://reviewboard.mozilla.org/r/244142/#review250106

Interesting.

'try -b do -p android-api-16 -u mochitests,geckoview-junit --rebuild 3' recommended.
Attachment #8975941 - Flags: review?(gbrown) → review+
u(In reply to Geoff Brown [:gbrown] from comment #27)
> Comment on attachment 8975941 [details]
> Bug 1458020 - 6. Use OSS audio backend for ARM 4.3 emulator;
> 
> https://reviewboard.mozilla.org/r/244142/#review250106
> 
> Interesting.
> 
> 'try -b do -p android-api-16 -u mochitests,geckoview-junit --rebuild 3'
> recommended.

Good idea. This caused mochitests to mass-fail. I'm pretty sure "-audio oss" is the right approach for geckoview-junit, but fixing mochitests would take a while. Would you be okay with using a different "androidarm_4_3.py" for geckoview-junit, so we only specify "-audio oss" for the geckoview-junit emulators?
Flags: needinfo?(gbrown)
Yes, I think that would be okay.
Flags: needinfo?(gbrown)
Attachment #8975941 - Flags: review+ → review?(gbrown)
Comment on attachment 8975941 [details]
Bug 1458020 - 6. Use separate emulator config for geckoview-junit tests;

https://reviewboard.mozilla.org/r/244142/#review250892

That looks fine. Thanks.
Attachment #8975941 - Flags: review?(gbrown) → review+
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9d8c5d68082
1a. Add waitForJS and waitForChromeJS; r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a7da1af75d9
1b. Add forceGarbageCollection; r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd57487eee34
2. Add PermissionDelegateTest; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/30cb8d1f3fd3
3. Set JavaVM when actually using video capture; r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3b61f6598d1
4. Add camera permission in the parent process; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb09eefb0606
5. Return default sample rate / frames per buffer; r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/056515942855
6. Use separate emulator config for geckoview-junit tests; r=gbrown
Product: Firefox for Android → GeckoView
Version: Firefox 59 → 59 Branch
Target Milestone: Firefox 62 → mozilla62
You need to log in before you can comment on or make changes to this bug.