Closed Bug 1318820 Opened 3 years ago Closed 3 years ago

Fennec does not work in some x86 Android emulators

Categories

(Core :: Graphics, defect, P3)

x86
Android
defect

Tracking

()

RESOLVED WONTFIX

People

(Reporter: nalexander, Assigned: snorp)

References

()

Details

Attachments

(1 file)

Running Fennec (or GeckoView) in an x86 emulator (config.ini to follow), I see the following failure initializing the compositor:

11-18 18:54:56.139  3219  3289 I Gecko   : === SHADER COMPILATION FAILED ===
11-18 18:54:56.139  3219  3289 I Gecko   : === Source:
11-18 18:54:56.139  3219  3289 I Gecko   : #ifdef GL_ES
11-18 18:54:56.139  3219  3289 I Gecko   : #define EDGE_PRECISION mediump
11-18 18:54:56.139  3219  3289 I Gecko   : #else
11-18 18:54:56.139  3219  3289 I Gecko   : #define EDGE_PRECISION
11-18 18:54:56.139  3219  3289 I Gecko   : #endif
11-18 18:54:56.139  3219  3289 I Gecko   : uniform mat4 uMatrixProj;
11-18 18:54:56.139  3219  3289 I Gecko   : uniform vec4 uLayerRects[4];
11-18 18:54:56.139  3219  3289 I Gecko   : uniform mat4 uLayerTransform;
11-18 18:54:56.139  3219  3289 I Gecko   : uniform vec2 uRenderTargetOffset;
11-18 18:54:56.139  3219  3289 I Gecko   : attribute vec4 aCoord;
11-18 18:54:56.139  3219  3289 I Gecko   : void main() {
11-18 18:54:56.139  3219  3289 I Gecko   :   int vertexID = int(aCoord.w);
11-18 18:54:56.139  3219  3289 I Gecko   :   vec4 layerRect = uLayerRects[vertexID];
11-18 18:54:56.139  3219  3289 I Gecko   :   vec4 finalPosition = vec4(aCoord.xy * layerRect.zw + layerRect.xy, 0.0, 1.0);
11-18 18:54:56.139  3219  3289 I Gecko   :   finalPosition = uLayerTransform * finalPosition;
11-18 18:54:56.139  3219  3289 I Gecko   :   finalPosition.xy -= uRenderTargetOffset * finalPosition.w;
11-18 18:54:56.139  3219  3289 I Gecko   :   finalPosition = uMatrixProj * finalPosition;
11-18 18:54:56.139  3219  3289 I Gecko   :   gl_Position = finalPosition;
11-18 18:54:56.139  3219  3289 I Gecko   : }
11-18 18:54:56.139  3219  3289 I Gecko   :
11-18 18:54:56.139  3219  3289 I Gecko   :
11-18 18:54:56.139  3219  3289 I Gecko   : === Log:
11-18 18:54:56.139  3219  3289 I Gecko   : ERROR: Valid GLSL but not GLSL ES
11-18 18:54:56.139  3219  3289 I Gecko   : ============
11-18 18:54:56.141  3219  3289 I Gecko   : === SHADER COMPILATION FAILED ===
11-18 18:54:56.141  3219  3289 I Gecko   : === Source:
11-18 18:54:56.141  3219  3289 I Gecko   : #ifdef GL_ES
11-18 18:54:56.141  3219  3289 I Gecko   : precision mediump float;
11-18 18:54:56.141  3219  3289 I Gecko   : #define COLOR_PRECISION lowp
11-18 18:54:56.141  3219  3289 I Gecko   : #define EDGE_PRECISION mediump
11-18 18:54:56.141  3219  3289 I Gecko   : #else
11-18 18:54:56.141  3219  3289 I Gecko   : #define COLOR_PRECISION
11-18 18:54:56.141  3219  3289 I Gecko   : #define EDGE_PRECISION
11-18 18:54:56.141  3219  3289 I Gecko   : #endif
11-18 18:54:56.141  3219  3289 I Gecko   : uniform COLOR_PRECISION vec4 uRenderColor;
11-18 18:54:56.141  3219  3289 I Gecko   : uniform sampler2D uTexture;
11-18 18:54:56.141  3219  3289 I Gecko   : void main() {
11-18 18:54:56.141  3219  3289 I Gecko   :   vec4 color = uRenderColor;
11-18 18:54:56.141  3219  3289 I Gecko   :   COLOR_PRECISION float mask = 1.0;
11-18 18:54:56.141  3219  3289 I Gecko   :   color *= mask;
11-18 18:54:56.141  3219  3289 I Gecko   :   gl_FragColor = color;
11-18 18:54:56.141  3219  3289 I Gecko   : }
11-18 18:54:56.141  3219  3289 I Gecko   :
11-18 18:54:56.141  3219  3289 I Gecko   :
11-18 18:54:56.141  3219  3289 I Gecko   : === Log:
11-18 18:54:56.141  3219  3289 I Gecko   : ERROR: Valid GLSL but not GLSL ES
11-18 18:54:56.141  3219  3289 I Gecko   : ============
11-18 18:54:56.142  3219  3289 I Gecko   : [GFX1-]: [OPENGL] Failed to init compositor with reason: FEATURE_FAILURE_OPENGL_COMPILE_SHADER

This appears to be an issue with newer emulators, requiring crufty workarounds: see https://github.com/mapbox/mapbox-gl-native/issues/5878 and their fix at https://github.com/mapbox/mapbox-gl-native/pull/7044/commits/4dca4061a162cd4e3f803b05ee2c27a656355f36.

It's really helpful to develop against an x86 emulator, so could we try to do the same for Fennec?

config.ini:

avd.ini.encoding=UTF-8
AvdId=Nexus_5_API_23
abi.type=x86
avd.ini.displayname=Nexus 5 API 23
disk.dataPartition.size=200M
hw.accelerometer=yes
hw.audioInput=yes
hw.battery=yes
hw.camera.back=none
hw.camera.front=none
hw.cpu.arch=x86
hw.dPad=no
hw.device.hash2=MD5:2fa0e16c8cceb7d385183284107c0c88
hw.device.manufacturer=Google
hw.device.name=Nexus 5
hw.gps=yes
hw.gpu.enabled=yes
hw.keyboard=yes
hw.lcd.density=480
hw.mainKeys=no
hw.ramSize=1536
hw.sdCard=yes
hw.sensors.orientation=yes
hw.sensors.proximity=yes
hw.trackBall=no
image.sysdir.1=system-images/android-23/google_apis/x86/
runtime.network.latency=none
runtime.network.speed=full
runtime.scalefactor=auto
sdcard.size=100M
skin.dynamic=yes
skin.name=nexus_5
skin.path=/Applications/Android Studio.app/Contents/plugins/android/lib/device-art-resources/nexus_5
snapshot.present=no
tag.display=Google APIs
tag.id=google_apis
vm.heapSize=64
Attachment #8812646 - Flags: review?(bugmail) → review?(jgilbert)
OS: Unspecified → Android
Priority: -- → P3
Hardware: Unspecified → x86
I think I should detail the issue in the commit message, and not just link to SO.  I'll land with some expanded version of "x86 emulators define GL_ES but have some non-GL_ES incompatabilities".
I really don't like this fix. Please explain more why it's useful and how it was indicated. The links you posted don't seem to me to suggest this change.
Flags: needinfo?(nalexander)
(In reply to Jeff Gilbert [:jgilbert] from comment #4)
> I really don't like this fix. Please explain more why it's useful

This part is easy.  Android x86 emulators are a weird hybrid: they support a "host GPU" option which is very fast because it uses the host hardware for OpenGL, as well as a software emulated OpenGL layer.  This makes it possible to run Fennec in the fast x86 emulators, which is really the best emulator option. 

 and how it
> was indicated. The links you posted don't seem to me to suggest this change.

Sure.  I found the link I gave to be the most concise description (http://stackoverflow.com/a/40276036) but it's not exactly technically detailed.  Basically, it looks like a few GL_ES things aren't quite correct, given the host GPU mode is really a desktop mode, not a mobile mode.  The links I gave in #c0 point to the fix, namely: "This appears to be an issue with newer emulators, requiring crufty workarounds: see https://github.com/mapbox/mapbox-gl-native/issues/5878 and their fix at https://github.com/mapbox/mapbox-gl-native/pull/7044/commits/4dca4061a162cd4e3f803b05ee2c27a656355f36."  I transcribed what they did into Fennec's context and it Just Works.

I agree that this is crufty, but I don't see it being worse than guarding with MOZ_WIDGET_ANDROID or whatever.  We could do that if you prefer.
Flags: needinfo?(nalexander) → needinfo?(jgilbert)
Maybe you have the link wrong? It does look like that "their fix" link shows the shader uuid(?) changed, but doesn't show the shader changes. The fixes in that cset are not similar to your proposed solution here.

What it sounds like is that the translator for ESSL->GLSL isn't functioning properly, specifically that it adds a #define GL_ES but doesn't actually support the features used in ES.

If we're adding something this hacky, we really need to be good about documenting both the problem and how this works around it.
Flags: needinfo?(jgilbert) → needinfo?(nalexander)
(In reply to Jeff Gilbert [:jgilbert] from comment #6)
> Maybe you have the link wrong? It does look like that "their fix" link shows
> the shader uuid(?) changed, but doesn't show the shader changes. The fixes
> in that cset are not similar to your proposed solution here.

Sorry, I forgot how much chasing I had to do.  Here's the real fix: https://github.com/mapbox/mapbox-gl-shaders/commit/ec891ce5360e488d81f60991f95d2038b83c4e3c

> What it sounds like is that the translator for ESSL->GLSL isn't functioning
> properly, specifically that it adds a #define GL_ES but doesn't actually
> support the features used in ES.

It's a mixed bag, I gather.  It mostly supports GL_ES, but not entirely?  This changed recently; older emulators didn't have this bug.  It's a Google mistake and likely to be fixed eventually, so I view this as just a temporary work-around.

> If we're adding something this hacky, we really need to be good about
> documenting both the problem and how this works around it.

I agree, but I'm really not the right person to do this in full.  I know that patches that create work aren't great, but this at least shows how to work around the issue; it would be great if somebody with actual gfx experience (including in how to document ridiculous cruft!) could roll with it.

You could phrase this patch differently.  One way would be to ```
#ifdef GL_ES
#if !defined(lowp)
#define lowp
#endif
#if !defined(mediump)
#define mediump
#endif
#endif
```
That's not really hacky, 'cuz it always "makes sense", even if it's unexpected.
Flags: needinfo?(nalexander) → needinfo?(jgilbert)
Ok, great.
Flags: needinfo?(jgilbert)
Comment on attachment 8812646 [details]
Bug 1318820 - Work around Android x86 GL_ES issues.

https://reviewboard.mozilla.org/r/94296/#review96678

I don't think this is the correct solution. It doesn't seem to match the approach used in other solutions, either.

Feel free to keep this patch floating for local use, but this isn't the patch we should take into trunk.
Attachment #8812646 - Flags: review?(jgilbert) → review-
kats, snorp, rbarker: can one of you pick this up and get this into the tree?  Running a hardware accelerated x86 emulator is a HUGE win for my local development and should pay dividends for many other developers too.  I don't have enough knowledge of the gfx code to know how to solve this "for real".
Flags: needinfo?(snorp)
Flags: needinfo?(rbarker)
Flags: needinfo?(bugmail)
(In reply to Nick Alexander :nalexander (leave until January 2017) from comment #11)
> kats, snorp, rbarker: can one of you pick this up ...

> ... but this isn't the patch we should take into trunk.

To be clear -- can somebody rewrite this in some gfx approved manner, not "drive this exact patch across the line".
jgilbert: for some additional context, I use almost exclusively artifact builds.  Expeditions like this into the compiled code, and especially a "local patch", are a huge loss for me, since I'm almost always working on the build system, JavaScript, and Java.
(In reply to Jeff Gilbert [:jgilbert] from comment #10)

Can you elaborate on what you'd like to see? At first glance, I think a generic "insert this string blob into every shader" approach is better than modifying each one individually. There we would just #define any missing precision specifiers, which should be a noop on real drivers. If something like that sounds alright with you I can write that patch.
Flags: needinfo?(snorp) → needinfo?(jgilbert)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #14)
> (In reply to Jeff Gilbert [:jgilbert] from comment #10)
> 
> Can you elaborate on what you'd like to see? At first glance, I think a
> generic "insert this string blob into every shader" approach is better than
> modifying each one individually. There we would just #define any missing
> precision specifiers, which should be a noop on real drivers. If something
> like that sounds alright with you I can write that patch.

I'm really looking for a cohesive strategy at this point. The patches here don't seem to match what's done in the referenced project's solution, and I haven't seen a clear description of:
A) What is wrong with our current code on the emulator.
B) How we should fix it.

Let's identify the exact problem first, please!
Flags: needinfo?(jgilbert)
(In reply to Jeff Gilbert [:jgilbert] from comment #15)
> (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #14)
> > (In reply to Jeff Gilbert [:jgilbert] from comment #10)
> > 
> > Can you elaborate on what you'd like to see? At first glance, I think a
> > generic "insert this string blob into every shader" approach is better than
> > modifying each one individually. There we would just #define any missing
> > precision specifiers, which should be a noop on real drivers. If something
> > like that sounds alright with you I can write that patch.
> 
> I'm really looking for a cohesive strategy at this point. The patches here
> don't seem to match what's done in the referenced project's solution, and I
> haven't seen a clear description of:
> A) What is wrong with our current code on the emulator.
> B) How we should fix it.
> 
> Let's identify the exact problem first, please!

We don't know the exact problem.  I can't even find an Android ticket to link to, and I've pissed into that wind enough times to not bother filing one myself.

To your point A):

As I've tried to describe, and linked to other people describing, it appears that Android x86 hardware accelerated emulators define GL_ES but are really a desktop GL providing some cross-compatibility.  It appears that there's a bug in that cross-compatibility and things that one would expect to be a defined aren't.  I don't know what will make this discussion clearer for you, but perhaps snorp will view this or explain this in a different way.

B)

The patches I've linked to, and the patch I wrote, work around the apparent reality above by accepting that some defines won't be there and handling that situation.  But I'm clearly not the person to craft the correct fix, so I'll bow out, with a desperate plea to get something landed before all of Mozilla decamps for Hawaii at the end of the week.
It's very worrying to me that the fix you've proposed is materially different from the fix used here:
https://github.com/mapbox/mapbox-gl-shaders/commit/ec891ce5360e488d81f60991f95d2038b83c4e3c

Specifically, the proposed fix here is in the #ifdef GL_ES branch, whereas the mapbox fix is #ifndef GL_ES.
This indicates to me that one of the two is wrong. As such, this needs more root-cause investigation.

For someone knowledgeable in GLSL, it shouldn't take much poking around to figure out the problem with their translator.
I may be able to take a look if someone can get me in front of the emulator in Hawaii.
Component: Graphics, Panning and Zooming → Graphics
Product: Firefox for Android → Core
Sorry, I'm not really that familiar with GLSL so it would be pretty hard for me to pick this up.
Flags: needinfo?(bugmail)
Flags: needinfo?(rbarker)
I can take a look at what's going on here and coordinate with Jeff on a fix.
Assignee: nobody → snorp
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #19)
> I can take a look at what's going on here and coordinate with Jeff on a fix.

snorp: per our private email conversation, I've found that Fennec is working just fine in Android 7.1.1 images.  The bin/sdkmanager tag seems to be "system-images;android-25;google_apis;x86", possibly revision 3.

So, WONTFIX since this can be trivially worked around by using newer (or older) x86 images \o/
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
Summary: Make Fennec work in newer x86 Android emulators → Fennec does not work in some x86 Android emulators
You need to log in before you can comment on or make changes to this bug.