950050 - Black screen on PowerVR SGX GPU (Nexus S, Galaxy Nexus, Geeksphone Revolution)

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[:gerard-majax]

Since bug 892285 landed, it seems like at least those devices are broken. The symptom is blackscreen, nothing visible. Logcat shows nothing obvious, and the device in fact correctly boots.

We are even able to unlock the device, see the small white locker appearing for a short period of time, and then everything is still black, a part from the orange bar of Gaia homescreen.

Comment 1

[Sotaro Ikeda [:sotaro]]

Changed to a correct component.

Comment 2

[Sotaro Ikeda [:sotaro]]

attachment 831862 [details] [diff] [review] is dependent on an assumption that mDescriptor is opened only when mSurface is valid. But there seems a path to open SurfaceDescriptor without assigning it. It might affect to it. 

one example is the AutoLockShmemClient::Update()
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/TextureClient.cpp#829

Comment 3

[Lorenzo Stanco]

I revived the black screen on a clean build flashed on a Galaxy Nexus. 
The same fix used for Nexus S worked also on my device:
https://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp#366

After line 377 I added:

     if (strcmp("maguro",propValue) == 0) {
       NS_WARNING("Galaxy Nexus has issues with gralloc, falling back to shmem");
       disableGralloc = true;
     }

Comment 4

[Sotaro Ikeda [:sotaro]]

The problem might be fixed when all layers are move to new gfx textures. The new texture does not use SurfaceDescriptor out side of IPC except GonkNativeWindow related code on gonk. And the new texture does not use ShadowLayerForwarder::OpenDescriptor() at all on gonk. Right now, only image layer are using "gralloc + new gfx textures" combination. Thebes layer(ContentClient/ContentHost) is going to be enabled by Bug 946720.

Comment 5

[:gerard-majax]

(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> The problem might be fixed when all layers are move to new gfx textures. The
> new texture does not use SurfaceDescriptor out side of IPC except
> GonkNativeWindow related code on gonk. And the new texture does not use
> ShadowLayerForwarder::OpenDescriptor() at all on gonk. Right now, only image
> layer are using "gralloc + new gfx textures" combination. Thebes
> layer(ContentClient/ContentHost) is going to be enabled by Bug 946720.

How does it relates to your previous patch posted above ?

Comment 6

[:gerard-majax]

(In reply to Sotaro Ikeda [:sotaro] (PTO Dec. 21 ~ Jan. 5) from comment #2)
> attachment 831862 [details] [diff] [review] is dependent on an assumption
> that mDescriptor is opened only when mSurface is valid. But there seems a
> path to open SurfaceDescriptor without assigning it. It might affect to it. 
> 
> one example is the AutoLockShmemClient::Update()
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/
> TextureClient.cpp#829

Playing with flatfish, I see this patch is already there. I suppose it's not a coincindence :). Anyway, on Flatfish, I still see those -EINVAL, and I confirm that it totally relates to thhe laggish behavior, since we see that layers are skipped:

E/IMGSRV  ( 2319): :0: gralloc_unregister_buffer: Cannot unregister unregistered buffer (ID=554)
W/GraphicBufferMapper( 2319): unregisterBuffer(0x456661a0) failed -22 (Invalid argument)
D/HwcUtils( 2319): Skip layer
D/HwcUtils( 2319): Skip layer
D/HwcUtils( 2319): Skip layer
D/HwcUtils( 2319): Skip layer
D/HwcUtils( 2319): Skip layer
D/HwcUtils( 2319): Skip layer
D/HwcUtils( 2319): Skip layer
D/HwcUtils( 2319): Skip layer
D/HwcUtils( 2319): Skip layer
D/HwcUtils( 2319): Skip layer
D/HwcUtils( 2319): Skip layer
D/HwcUtils( 2319): Skip layer
D/HwcUtils( 2319): Skip layer
D/HwcUtils( 2319): Skip layer
D/HwcUtils( 2319): Skip layer

I tried the WIP patch available in bug 946720, but I suspect it's not finished, since I see no change :)

Comment 7

[Greg Weng [:snowmantw][:gweng][:λ]]

We should claim that these devices can't be flashed anymore...at least before a formal patch got landed.

https://developer.mozilla.org/en-US/Firefox_OS/Firefox_OS_build_prerequisites#Tier_3

The page says:

"We are not currently aware of any variations that are not compatible."

Comment 8

[:gerard-majax]

(In reply to Greg Weng [:snowmantw] from comment #7)
> We should claim that these devices can't be flashed anymore...at least
> before a formal patch got landed.
> 
> https://developer.mozilla.org/en-US/Firefox_OS/
> Firefox_OS_build_prerequisites#Tier_3
> 
> The page says:
> 
> "We are not currently aware of any variations that are not compatible."

For Nexus S, I document it clearly on the related page: https://developer.mozilla.org/en-US/Firefox_OS/Samsung_Nexus_S

Comment 9

[stunts513]

This bug exists also on the Kindle Fire HD 7", running b2g 1.4 (android version 4.3), and also has an sgx540. I believe this will effect the Kindle Fire 1 and Kindle Fire 2 as well as they have an sgx540 if i remember correctly, but i don't belive the Kindle Fire HD 8.9" model does. Simplest fix was to pretty much do what was mentioned above by lorenzo except that snippet of code is already part of the ShadowLayerUtilsGralloc.cpp already so just take off the "#if ANDROID_VERSION <= 15", change crespo to tate, and delete the #endif

Comment 10

[:gerard-majax]

(In reply to stunts513 from comment #9)
> This bug exists also on the Kindle Fire HD 7", running b2g 1.4 (android
> version 4.3), and also has an sgx540. I believe this will effect the Kindle
> Fire 1 and Kindle Fire 2 as well as they have an sgx540 if i remember
> correctly, but i don't belive the Kindle Fire HD 8.9" model does. Simplest
> fix was to pretty much do what was mentioned above by lorenzo except that
> snippet of code is already part of the ShadowLayerUtilsGralloc.cpp already
> so just take off the "#if ANDROID_VERSION <= 15", change crespo to tate, and
> delete the #endif

Thanks for this.

So obviously we would need to make the change not limited to ICS.

Michael, what's your opinion on this?

Comment 11

[:gerard-majax]

FYI, I just tried some google search ("sgx540 black screen") and found similar issues reported by game developpers on tablets.

One of them lead me to a patch: https://github.com/hrydgard/native/commit/1b46a9ce6bc57aa4aa83d3670fc3c8d46964a0eb#commitcomment-4472794. The patch is:

     // Bind it all together
     glBindFramebuffer(GL_DRAW_FRAMEBUFFER, fbo->handle);
     glFramebufferTexture2D(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, fbo->color_texture, 0);
-    glFramebufferRenderbuffer(GL_DRAW_FRAMEBUFFER, GL_DEPTH_ATTACHMENT, GL_RENDERBUFFER, fbo->z_stencil_buffer);
-    glFramebufferRenderbuffer(GL_DRAW_FRAMEBUFFER, GL_STENCIL_ATTACHMENT, GL_RENDERBUFFER, fbo->z_stencil_buffer);
+    glFramebufferRenderbuffer(GL_DRAW_FRAMEBUFFER, GL_DEPTH_STENCIL_ATTACHMENT, GL_RENDERBUFFER, fbo->z_stencil_buffer);
   } else {
     ILOG("Creating %i x %i FBO using separate stencil", width, height);
     // TEGRA

Some comments from this patch:
« By the way, I did some testing:
GL_DEPTH_ATTACHMENT -> black screen
GL_STENCIL_ATTACHMENT -> perfect
GL_DEPTH_ATTACHMENT and GL_STENCIL_ATTACHMENT (what we are using right now) -> perfect
GL_DEPTH_STENCIL_ATTACHMENT -> depth issues

So it looks like the depth is unnecessary? »

Comment 12

[:gerard-majax]

There are also reports with suggestions that have been marked as solving on StackOverflow [http://stackoverflow.com/a/7560967]: 

> Removing EGL10.EGL_RED_SIZE, EGL10.EGL_GREEN_SIZE, and EGL10.EGL_BLUE_SIZE but leaving
> EGL10.EGL_DEPTH_SIZE, EGL10.EGL_NONE in the eglChooseConfig worked. I assume that the
> PowerVR chip processes RGB in a way that makes defining them a problem.

Comment 13

[:gerard-majax]

There is also a discussion on the vendor forum, which leads to nothing: http://forum.imgtec.com/discussion/1666/when-to-delete-render-buffer-in-fbo-on-android

Comment 14

[:gerard-majax]

It seems that we are also reproducing this on Canvas. I get the issue with HERE Maps and Poppit! apps.

Comment 15

[:gerard-majax]

While working on another bug with a debug build, I noticed quite a couple of messages like this one:
I/Gecko   ( 1363): [Child 1363] WARNING: We don't support transparent content with displayports, force it to be opqaue: file /home/alex/codaz/Mozilla/b2g/devices/Inari/B2G/gecko/layout/base/nsDisplayList.cpp, line 1211

Not sure if it's related at all, but talking about opaque I'd prefer mentioning it.

Comment 16

[:gerard-majax]

So Canvas seems to trigger this issue, as I reported previously. We also have this triggered when picking an image from the gallery, at the resize/crop step.

logcat shows:
I/Gecko   ( 2666): Attempting load of libEGL.so
D/libEGL  ( 2666): loaded /system/lib/egl/libGLES_android.so
D/libEGL  ( 2666): loaded /vendor/lib/egl/libEGL_POWERVR_SGX540_120.so
D/libEGL  ( 2666): loaded /vendor/lib/egl/libGLESv1_CM_POWERVR_SGX540_120.so
D/libEGL  ( 2666): loaded /vendor/lib/egl/libGLESv2_POWERVR_SGX540_120.so
I/Gecko   ( 2666): SharedSurface_Gralloc::Create -------
I/Gecko   ( 2666): SharedSurface_Gralloc::Create: success -- surface 0x4479ef70, GraphicBuffer 0x44756180.
I/Gecko   ( 2666): SharedSurface_Gralloc::Create -------
I/Gecko   ( 2666): SharedSurface_Gralloc::Create: success -- surface 0x44dff580, GraphicBuffer 0x44756880.
I/Gecko   ( 2666): SharedSurface_Gralloc::Create -------
I/Gecko   ( 2666): SharedSurface_Gralloc::Create: success -- surface 0x437d1190, GraphicBuffer 0x44757880.
I/Gecko   ( 2666): SharedSurface_Gralloc::Create -------
I/Gecko   ( 2666): SharedSurface_Gralloc::Create: success -- surface 0x44dffc70, GraphicBuffer 0x44757900.

I also see those SharedSurface_Gralloc at other places where display is okay. So there is obviously something that we are doing wrong at some point.

Comment 17

[:gerard-majax]

Dumping the DrawTarget as PNG to sdcard, I get correct images.

FYI, the reported surfaceformat is GGL_PIXEL_FORMAT_BGRA_8888 (5).

Comment 18

[:gerard-majax]

I had a look at the DrawTarget's SurfaceFormat. Two values: B8G8R8X8 and B8G8R8A8.

As far as I can say, taping on the status bar makes the date and network name visibles. When I do see the network name displayed in the status bar, then the SurfaceFormat is B8G8R8A8. The rest of the time when the screen is black, I see that SurfaceFormat is B8G8R8X8.

Comment 19

[:gerard-majax]

Could it be related to bug 727311 by any chance ?

Comment 20

[:gerard-majax]

I can't find either the code of bug 728724 in the current master.

Comment 21

[:gerard-majax]

Attachment: DrawTarget: lockscreen part1

Comment 22

[:gerard-majax]

Attachment: DrawTarget: lockscreen part0

Comment 23

[:gerard-majax]

Attachment: DrawTarget: lockscreen part2

Comment 24

[:gerard-majax]

Attachment: DrawTarget: lockscreen part3

Comment 25

[:gerard-majax]

Attachment: DrawTarget: lockscreen part4

Comment 26

[:gerard-majax]

As suggested by :nical, I dumped frames that were in DrawTarget inside this if branch: https://hg.mozilla.org/mozilla-central/file/3a264db9f9e7/gfx/layers/client/ClientThebesLayer.cpp#l57. The basic motion of the frams dumped can be reduced to those 4. It is interesting to notice that none of the dump frames contains the unlocker part that we see, i.e., the elipse that surrounds the lock and camera icons at the bottom of the lockscreen.

Comment 27

[Chris Lord [:cwiiis]]

I don't know if it's useful or not, but I see gralloc-related weirdness on the Peak too (though it doesn't manifest in black screens, like it does here, it manifests in frames not updating/flickering oddly instead). The Peak has a different chipset (Adreno 205), so I'd be a bit suspect of either the gralloc code, the drivers, or both.

I think bjacob did a lot of gralloc-related work?

Comment 28

[:gerard-majax]

On http://forum.imgtec.com/discussion/2031/is-fbo-with-eglimage-working-on-sgx540, one suggestion is:

> I've discussed this issue with our driver team. The driver is designed in such a way that drawing commands
> issued for FBOs will not be executed until that FBO is referenced by the main framebuffer's rendering. The
> idea behind this is to optimize out FBO rendering that doesn't affect the final image to avoid redundant
> rendering.
> 
> I would suggest drawing an off-screen primitive that references the FBO (e.g. a triangle) into the main
> framebuffer to force the FBO to render, if you are not doing so already.

Do you know if we do this or not?

Comment 29

[:gerard-majax]

Nicola, I checked in lockscreen code and I suspect that the parts we see are actually ... canvas ?.

lockscreen-canvas is defined at: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/index.html#L1088
and it is being filled by: https://github.com/mozilla-b2g/gaia/blob/master/shared/js/lockscreen_slide.js#L249

Comment 30

[Greg Weng [:snowmantw][:gweng][:λ]]

LockScreen now is drawing in Canvas: Bug 919410

Comment 31

[:gerard-majax]

(In reply to Greg Weng [:snowmantw] from comment #30)
> LockScreen now is drawing in Canvas: Bug 919410

Yes, I remember working on a regression following this bug :). That's strange then, since it means:
 - the elipse and the arrows are drawn as canvas, and they are the only visible components on the lockscreen when we use gralloc on SGX540
 - when we disable gralloc, canvas elements are drawn as black

Is there anyone with some OpenGL or gfx skills who has an idea how this can be possible ?

Comment 32

[Sotaro Ikeda [:sotaro]]

canvas's SkiaGL might be affected. SkiaGL's rendering request gralloc buffer as same as WebGL. You can disable SkiaGL by commenting out the following.

http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js

#ifdef MOZ_WIDGET_GONK
pref("gfx.canvas.azure.backends", "skia");
pref("gfx.canvas.azure.accelerated", true);
#endif

Comment 33

[:gerard-majax]

(In reply to Sotaro Ikeda [:sotaro] from comment #32)
> canvas's SkiaGL might be affected. SkiaGL's rendering request gralloc buffer
> as same as WebGL. You can disable SkiaGL by commenting out the following.
> 
> http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js
> 
> #ifdef MOZ_WIDGET_GONK
> pref("gfx.canvas.azure.backends", "skia");
> pref("gfx.canvas.azure.accelerated", true);
> #endif

I tried, no impact so far.

While searching, I just came accross this code in Kitkat: https://android.googlesource.com/platform/frameworks/native/+/android-4.4_r0.7%5E/opengl/libs/EGL/eglApi.cpp

And this patch: https://gist.github.com/steven676/7287471 which enables the workaround. I don't understand precisely what the workaround does, but it has an impact on the colorformat.

Comment 34

[:gerard-majax]

(In reply to Sotaro Ikeda [:sotaro] from comment #32)
> canvas's SkiaGL might be affected. SkiaGL's rendering request gralloc buffer
> as same as WebGL. You can disable SkiaGL by commenting out the following.
> 
> http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js
> 
> #ifdef MOZ_WIDGET_GONK
> pref("gfx.canvas.azure.backends", "skia");
> pref("gfx.canvas.azure.accelerated", true);
> #endif

Sorry, I tried again after and it has an impact: when gralloc is disabled and those two prefs are commented, Canvas are working again (verified in HERE Maps).

Comment 35

[Sotaro Ikeda [:sotaro]]

Can you try again a patch in Bug 946720 if gralloc works in your hardware with recent master? It is going to fix how gralloc buffer is locked.

Comment 36

[:gerard-majax]

(In reply to Sotaro Ikeda [:sotaro] from comment #35)
> Can you try again a patch in Bug 946720 if gralloc works in your hardware
> with recent master? It is going to fix how gralloc buffer is locked.

Sure, I'm re-testing right now with a master freshly updated :)

Comment 37

[:gerard-majax]

Sotaro, Nical was right, you are my hero. With the patches from bug 946720, I see things !

Right now, I have black background, I'm retesting without commenting the SkiaGL prefs.

Comment 38

[:gerard-majax]

With patches from bug 946720 and default b2g.js:
 - lockscreen is black, apart from the slider
 - homescreen displays icons, black background
 - settings app is completely black
 - statusbar black, network activity icon working and date/operator visible when taping the status bar
 - camera app works okay
 - email app header visible, content black
 - dialer works, dialer's contact turns to black, call log works
 - gallery is black
 - messages app is black, switching to a thread shows the mobile number header only when scrolling

Etc.

Comment 39

[:gerard-majax]

I checked when disabling hwcomposer (renaming the lib, restarting b2g): no impact.

However, swiping on the homescreen, I do not see anymore unregisterBuffer() returning -EINVAL. Thus, this is still happening when loading Settings app for example, which is black.

So the patch fixes things but there is probably other code path that are invalid :)

Comment 40

[:gerard-majax]

Lorenzo, could you try the patches proposed by Sotaro on your Galaxy Nexus ?

Comment 41

[Lorenzo Stanco]

(In reply to Alexandre LISSY :gerard-majax from comment #40)
> Lorenzo, could you try the patches proposed by Sotaro on your Galaxy Nexus ?

Ok I'm going to. Should I remove my own fix (disableGralloc = true) and update all sources (git pull; ./repo sync -d) before?

Comment 42

[Lorenzo Stanco]

I removed my own fix, updated the sources and applied the path. I had no luck, and I also tried flashing at various points:

$ cd gecko
$ git checkout -- gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp
$ cd ..
$ ./build gecko; ./flash.sh gecko # All black, only unlock slider was visible
$ git pull
$ ./repo sync -d
$ ./build gecko; ./flash.sh gecko # All black
$ cd gecko
$ wget -O sotaro-patch https://bug946720.bugzilla.mozilla.org/attachment.cgi?id=8361447
$ patch -p1 < sotaro-patch
$ cd ..
$ ./build gecko; ./flash.sh gecko # All black :(

Comment 43

[:gerard-majax]

(In reply to Lorenzo Stanco from comment #42)
> I removed my own fix, updated the sources and applied the path. I had no
> luck, and I also tried flashing at various points:
> 
> $ cd gecko
> $ git checkout -- gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp
> $ cd ..
> $ ./build gecko; ./flash.sh gecko # All black, only unlock slider was visible
> $ git pull
> $ ./repo sync -d
> $ ./build gecko; ./flash.sh gecko # All black
> $ cd gecko
> $ wget -O sotaro-patch
> https://bug946720.bugzilla.mozilla.org/attachment.cgi?id=8361447
> $ patch -p1 < sotaro-patch
> $ cd ..
> $ ./build gecko; ./flash.sh gecko # All black :(

Did you unlocked the device ? As I documented lockscreen is still completely black (apart from the slider), but the homescreen is partly working.

Comment 44

[:gerard-majax]

Sotaro, looks like bug 946720 needs to be rebased after bug 952507 landed :)

Comment 45

[Sotaro Ikeda [:sotaro]]

(In reply to Alexandre LISSY :gerard-majax from comment #44)
> Sotaro, looks like bug 946720 needs to be rebased after bug 952507 landed :)

Updated a patch.

Comment 46

[:gerard-majax]

(In reply to Sotaro Ikeda [:sotaro] from comment #45)
> (In reply to Alexandre LISSY :gerard-majax from comment #44)
> > Sotaro, looks like bug 946720 needs to be rebased after bug 952507 landed :)
> 
> Updated a patch.

Woo :(. Applying this new version without even turning on gralloc, I reproduce the black screen everywhere :(

Comment 47

[Sotaro Ikeda [:sotaro]]

gerard-majax, I tried to get b2g master nexus s source code. But it failed at icu4c. Can you get the all source code without error?

> error: revision refs/tags/android-4.0.4_r1.2 in platform/external/icu4c not found

Comment 48

[:gerard-majax]

(In reply to Sotaro Ikeda [:sotaro] from comment #47)
> gerard-majax, I tried to get b2g master nexus s source code. But it failed
> at icu4c. Can you get the all source code without error?
> 
> > error: revision refs/tags/android-4.0.4_r1.2 in platform/external/icu4c not found

You may need to switch to caf instead of linaro.

Comment 49

[Sotaro Ikeda [:sotaro]]

Thanks for the advice. After that by modifying the manifest, I confirmed the nexus-s build.

Comment 50

[:gerard-majax]

Looks like none of those dependencies have an impact on this bug, sadly :(. We're back at square one.

Comment 51

[:gerard-majax]

Attachment: crespo_gralloc_egl.trace

Please find attached the EGL trace produced after enabling gralloc and following this doc: https://wiki.mozilla.org/B2G/Debugging_OpenGL

I hope one of you can find some time to have a look at this and check if our OpenGL is okay

Comment 52

[:gerard-majax]

With Gecko from today, now, with gralloc enabled on boot I do not see anymore the Firefox logo image (or the based on mozilla technology).

I also noticed that I if I launch some apps and go to the task switcher, I see the gray background of Rocketbar. On top of this, I see the thumbnails of apps, but their content is black and a bit transparent.

Comment 53

[:gerard-majax]

Comparing the result of running apitrace in both case, I see that when I don't see the firefox logo (but rather a plain and white screen), uLayerOpacity is -1 and there is only one glGetUniformLocation() call while in the case I see the logo, there is two glGetUniformLocation() calls, and the first one has uLayerOpacity equal to 7. The second call has it to -1.

Comment 54

[Chris Lord [:cwiiis]]

Attachment: Use BGRA instead of RGBA for 32-bit gralloc textures

This patch partially fixes the issue for me on a Galaxy Nexus - I assume it doesn't completely due to the continued use of RGBX (rather than BGRX).

I'll look at this tomorrow, but I'm marking this for feedback so we can decide on how to go about fixing this nicely.

Comment 55

[Chris Lord [:cwiiis]]

In fact, it would appear any non BGRA_8888 gralloc allocation fails on PVR. I suggest we just check for PVR quirk inside AllocateGralloc and switch the format to BGRA_8888 for any 32-bit format we need (and fiddle with the texture flags if necessary).

Comment 56

[Chris Lord [:cwiiis]]

Attachment: Use BGRA instead of RGB[AX] for 32-bit gralloc textures

Sorry for churn - here's a patch that fixes everything for me on PVR (likely at the expense of other platforms) in case it's handy for others.

Comment 57

[:gerard-majax]

(In reply to Chris Lord [:cwiiis] from comment #56)
> Created attachment 8388255 [details] [diff] [review]
> Use BGRA instead of RGB[AX] for 32-bit gralloc textures
> 
> Sorry for churn - here's a patch that fixes everything for me on PVR (likely
> at the expense of other platforms) in case it's handy for others.

Excellent. This is some code path I already tried hacking back then, but the bit I was missing was the texture swap one. Applying on my Nexus S, I do confirm it works also.

For those testing with Nexus S, remember to switch the 'disableGralloc' from true to false in gfx/layers/client/TextureClient.cpp and gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp

Comment 58

[Nicolas Silva [:nical]]

Comment on attachment 8388255 [details] [diff] [review]
Use BGRA instead of RGB[AX] for 32-bit gralloc textures

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

How do we differentiate between opaque and semi-transparent surfaces on the Host side? Perhaps we need an extra flags like TEXTURE_FORCE_OPAQUE to make sure we select the right shader.

Comment 59

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8388255 [details] [diff] [review]
Use BGRA instead of RGB[AX] for 32-bit gralloc textures

Seem to work.

Comment 60

[Chris Lord [:cwiiis]]

(In reply to Nicolas Silva [:nical] from comment #58)
> Comment on attachment 8388255 [details] [diff] [review]
> Use BGRA instead of RGB[AX] for 32-bit gralloc textures
> 
> Review of attachment 8388255 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> How do we differentiate between opaque and semi-transparent surfaces on the
> Host side? Perhaps we need an extra flags like TEXTURE_FORCE_OPAQUE to make
> sure we select the right shader.

Yes, I think we'll need to do this.

Also, I don't know if we always want to prefer BGR or not? In my experience, GPUs either don't care, or they do and BGR is faster, but I wouldn't want to put money on that. I'll add it as a PVR quirk for now.

Comment 61

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #59)
> Comment on attachment 8388255 [details] [diff] [review]
> Use BGRA instead of RGB[AX] for 32-bit gralloc textures
> 
> Seem to work.

But effect to HwComposer is not clear yet. HwComposer do rendering just using gralloc buffer.

Comment 62

[Chris Lord [:cwiiis]]

Attachment: Prefer BGR pixel formats for gralloc on certain devices

Here's a nicer patch. I'd really much rather be matching on driver vendor string, as this will affect all PVR devices (including flatfish and GP Revolution), so I'm open to suggestions on how to do that nicely without a GLContext (or the best way to create one and store this information, if need be).

Comment 63

[Nicolas Silva [:nical]]

Comment on attachment 8388604 [details] [diff] [review]
Prefer BGR pixel formats for gralloc on certain devices

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

I would prefer to store directly gfx::SurfaceFormat in GrallocTextureClient/GrallocTextureHost/NewSurfaceDescriptorGralloc that represents the surface format from the point of view of gecko (while the GraphicBuffer's format is the actual format in the driver). The code would look much simpler and would be more resistant to future driver weirdness (whatever driver only accepting that other format). I find IsRGBX a little confusing and specific to powervr's quirks.

Comment 64

[Chris Lord [:cwiiis]]

Attachment: Prefer BGR pixel formats for gralloc

As discussed in the meeting, this just prefers BGR for all devices. I also just ship the format along with the gralloc handle instead of the isBGRX flag, which lets us remove a nice chunk of code host-side.

Comment 65

[Nicolas Silva [:nical]]

Comment on attachment 8389244 [details] [diff] [review]
Prefer BGR pixel formats for gralloc

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

r=me with at least the if statement removed and the comment about PrfersBgrtoRGB -> false forcing us to not use BGRX either fixed or explained.

::: gfx/layers/opengl/GrallocTextureClient.cpp
@@ +273,5 @@
>    return gfx::SurfaceFormat::R8G8B8A8;
>  }
>  
> +static bool
> +PreferBGRtoRGB()

Perhaps this should be called ForceBGRA? You decide between BGRX and BGRA dependening on that in some cases.

@@ +415,5 @@
> +    if (PreferBGRtoRGB()) {
> +      // There is no android BGRX format.
> +      format = android::PIXEL_FORMAT_BGRA_8888;
> +    } else {
> +      format = android::PIXEL_FORMAT_RGBX_8888;

Strange, in the hypothetical case where we don't force BGRA, if we ask BGRX we'll get RGBX.
PreferBGRtoRGB() (or ForceBGRA if you rename it) should force BGRA when it returns true but not prevent us from using it when false, I think.

::: gfx/layers/opengl/GrallocTextureHost.cpp
@@ +250,5 @@
>      graphicBuffer = mGrallocActor->GetGraphicBuffer();
>    }
>  
>    if (graphicBuffer) {
> +    format = aDescriptor.format();

Don't need to put this inside the if statement anymore, you should just remove the if.

Comment 66

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Don't forget to test this with HWComposer on b2g devices, especially if we switch the format unconditionally.

If we can, let's hold off on landing this until the 1.4 train moves on... a little nervous about a big-whack change.

Comment 67

[Chris Lord [:cwiiis]]

(In reply to Nicolas Silva [:nical] from comment #65)
> Comment on attachment 8389244 [details] [diff] [review]
> Prefer BGR pixel formats for gralloc
> 
> Review of attachment 8389244 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with at least the if statement removed and the comment about
> PrfersBgrtoRGB -> false forcing us to not use BGRX either fixed or explained.
> 
> ::: gfx/layers/opengl/GrallocTextureClient.cpp
> @@ +273,5 @@
> >    return gfx::SurfaceFormat::R8G8B8A8;
> >  }
> >  
> > +static bool
> > +PreferBGRtoRGB()
> 
> Perhaps this should be called ForceBGRA? You decide between BGRX and BGRA
> dependening on that in some cases.
> 
> @@ +415,5 @@
> > +    if (PreferBGRtoRGB()) {
> > +      // There is no android BGRX format.
> > +      format = android::PIXEL_FORMAT_BGRA_8888;
> > +    } else {
> > +      format = android::PIXEL_FORMAT_RGBX_8888;
> 
> Strange, in the hypothetical case where we don't force BGRA, if we ask BGRX
> we'll get RGBX.
> PreferBGRtoRGB() (or ForceBGRA if you rename it) should force BGRA when it
> returns true but not prevent us from using it when false, I think.

Android has no BGRX, it's not that I'm deciding to not use it, just that you can't use it at all. I don't mind calling it ForceBGRA though, that's probably more accurate.

> ::: gfx/layers/opengl/GrallocTextureHost.cpp
> @@ +250,5 @@
> >      graphicBuffer = mGrallocActor->GetGraphicBuffer();
> >    }
> >  
> >    if (graphicBuffer) {
> > +    format = aDescriptor.format();
> 
> Don't need to put this inside the if statement anymore, you should just
> remove the if.

Ah yes, cheers.

Comment 68

[Chris Lord [:cwiiis]]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #66)
> Don't forget to test this with HWComposer on b2g devices, especially if we
> switch the format unconditionally.
> 
> If we can, let's hold off on landing this until the 1.4 train moves on... a
> little nervous about a big-whack change.

I've tested this on buri with HWC on and everything seems fine, but I'm ok with holding off on this.

Reminder for self: Land this on Friday 21st March.

Comment 69

[Nicolas Silva [:nical]]

(In reply to Chris Lord [:cwiiis] from comment #67)
> (In reply to Nicolas Silva [:nical] from comment #65)
> > Strange, in the hypothetical case where we don't force BGRA, if we ask BGRX
> > we'll get RGBX.
> > PreferBGRtoRGB() (or ForceBGRA if you rename it) should force BGRA when it
> > returns true but not prevent us from using it when false, I think.
> 
> Android has no BGRX, it's not that I'm deciding to not use it, just that you
> can't use it at all. I don't mind calling it ForceBGRA though, that's
> probably more accurate.

Ah! Right, the comment was pretty explicit about it :) my bad

Comment 70

[stunts513]

Thought i would add this patch is working great on my kindle fire hd 7" port, can't test hwc out though since my hwc from the cm device tree seems to be incompatible and causes the video to never get as far as the boot animation.

Comment 71

[aniellob]

The patch works perfectly on my crespo device. I'll investigate for more bugs ..

Comment 72

[Chris Lord [:cwiiis]]

Attachment: Prefer BGR pixel formats for gralloc v2

Carrying r+, this is just the same patch with the if removed as suggested and a fix in GrallocTextureHostOGL where it was missing the BGRA format.

Comment 73

[Chris Lord [:cwiiis]]

Attachment: Allow gralloc surfaces on crespo (Nexus S)

I'm told this patch fixes Gralloc on the Nexus S, so let's remove these checks.

Comment 74

[Chris Lord [:cwiiis]]

Attachment: Prefer BGR pixel formats for gralloc v3

Oh interesting, things break when using LOCAL_GL_TEXTURE_2D, but are fine with LOCAL_GL_TEXTURE_EXTERNAL... I wonder if that was the problem all along, rather than RGB vs. BGR?

Comment 75

[:gerard-majax]

(In reply to Chris Lord [:cwiiis] from comment #74)
> Created attachment 8390398 [details] [diff] [review]
> Prefer BGR pixel formats for gralloc v3
> 
> Oh interesting, things break when using LOCAL_GL_TEXTURE_2D, but are fine
> with LOCAL_GL_TEXTURE_EXTERNAL... I wonder if that was the problem all
> along, rather than RGB vs. BGR?

Enabling gralloc and only changing from TEXTURE_2D to TEXTURE_EXTERNAL does work, but we get color swap.

Comment 76

[:gerard-majax]

(In reply to Chris Lord [:cwiiis] from comment #74)
> Created attachment 8390398 [details] [diff] [review]
> Prefer BGR pixel formats for gralloc v3
> 
> Oh interesting, things break when using LOCAL_GL_TEXTURE_2D, but are fine
> with LOCAL_GL_TEXTURE_EXTERNAL... I wonder if that was the problem all
> along, rather than RGB vs. BGR?

This v3 patch is working but there is something broken regarding IPDL, that makes the Camera app crashing:

I/Gecko   ( 2500): IPDL protocol error: Error deserializing 'format' (SurfaceFormat) member of 'NewSurfaceDescriptorGralloc'
I/Gecko   ( 2500): IPDL protocol error: Error deserializing 'SurfaceDescriptor'
I/Gecko   ( 2500): 
I/Gecko   ( 2500): ###!!! [Parent][DispatchAsyncMessage] Error: Value error: message was deserialized, but contained an illegal value
I/Gecko   ( 2500): 
I/Gecko   ( 2500): IPDL protocol error: could not look up PTexture
I/Gecko   ( 2500): IPDL protocol error: Error deserializing 'textureParent' (PTexture) member of 'OpUpdateTexture'
I/Gecko   ( 2500): IPDL protocol error: Error deserializing 'CompositableOperation[i]'
I/Gecko   ( 2500): IPDL protocol error: Error deserializing 'InfallibleTArray'
I/Gecko   ( 2500): 
I/Gecko   ( 2500): ###!!! [Parent][DispatchAsyncMessage] Error: Value error: message was deserialized, but contained an illegal value
I/Gecko   ( 2500): 
I/Gecko   ( 2500): 
I/Gecko   ( 2500): ###!!! [Parent][MessageChannel] Error: Channel error: cannot send/recv
I/Gecko   ( 2500): 
E/CameraHardwareSec( 2216): Could not dequeue gralloc buffer!
I/CameraHardwareSec( 2216): int android::CameraHardwareSec::previewThreadWrapper(): calling mSecCamera->stopPreview() and waiting

Comment 77

[:gerard-majax]

(gdb) print *__v
$2 = {bufferParent_ = 0x456a7b9c, bufferChild_ = 0x0, size_ = {<mozilla::gfx::BaseSize<int, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> >> = {width = 352, 
      height = 288}, <mozilla::gfx::UnknownUnits> = {<No data fields>}, <No data fields>}, format_ = {mEnum = mozilla::gfx::SurfaceFormat::B8G8R8A8}}

Comment 78

[:gerard-majax]

Comment on attachment 8390369 [details] [diff] [review]
Allow gralloc surfaces on crespo (Nexus S)

This part is obviously correct, but the v3 patch is wrong regarding IPDL, making it breaking Video and Camera.

Comment 79

[Chris Lord [:cwiiis]]

Attachment: Use TEXTURE_EXTERNAL for gralloc texture targets on SGX

So, this was the real issue - SGX doesn't like using TEXTURE_2D for EGLImage texture targets. This is *not* in line with the Khronos documentation, which says that you should *always* use TEXTURE_2D: https://www.khronos.org/registry/gles/extensions/OES/OES_EGL_image.txt

I could've sworn that when using glBindTexture, you're always meant to use TEXTURE_EXTERNAL with EGLimage and platform-specific handles (like gralloc), but I can't find the docs to back that up and just changing that isn't enough to fix this.

Comment 80

[:gerard-majax]

(In reply to Chris Lord [:cwiiis] from comment #79)
> Created attachment 8392277 [details] [diff] [review]
> Use TEXTURE_EXTERNAL for gralloc texture targets on SGX
> 
> So, this was the real issue - SGX doesn't like using TEXTURE_2D for EGLImage
> texture targets. This is *not* in line with the Khronos documentation, which
> says that you should *always* use TEXTURE_2D:
> https://www.khronos.org/registry/gles/extensions/OES/OES_EGL_image.txt
> 
> I could've sworn that when using glBindTexture, you're always meant to use
> TEXTURE_EXTERNAL with EGLimage and platform-specific handles (like gralloc),
> but I can't find the docs to back that up and just changing that isn't
> enough to fix this.

The fix works nice, and it does not crashes the Camera. However, Camera preview is R/B swapped. Pictures taken are correct, however. This may be a resurgence of an old bug I filed a long time ago about R/B swapped on <video> tag when used on Nexus S.

Comment 81

[Chris Lord [:cwiiis]]

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/6649c8d91e91

re comment #80, if that's a new issue, let's fix that in follow-up - if you could file a bug and cc, that'd be great :) (though note, I don't have a Nexus S, so it might gate somewhat on fixing bug 984106).

Comment 82

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

https://hg.mozilla.org/mozilla-central/rev/6649c8d91e91

Comment 83

[:gerard-majax]

(In reply to Chris Lord [:cwiiis] from comment #81)
> Pushed to inbound:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6649c8d91e91
> 
> re comment #80, if that's a new issue, let's fix that in follow-up - if you
> could file a bug and cc, that'd be great :) (though note, I don't have a
> Nexus S, so it might gate somewhat on fixing bug 984106).

Now that your code has been merged, I'm also seeing this on my Desire Z, which has an Adreno GPU.

Comment 84

[:gerard-majax]

needinfo for the red/blue swap in Camera preview also happening on Adreno (Desire Z)

Comment 85

[Chris Lord [:cwiiis]]

Forgot to push the Nexus S part: https://hg.mozilla.org/integration/mozilla-inbound/rev/1775f8b66913

(In reply to Alexandre LISSY :gerard-majax from comment #84)
> needinfo for the red/blue swap in Camera preview also happening on Adreno
> (Desire Z)

Filed bug 985322, fix awaiting review.

Comment 86

[:gerard-majax]

It seems like you forgot to also land attachement 8390369 that removes the gralloc disabling on Nexus S devices :)

Comment 87

[Chris Lord [:cwiiis]]

(In reply to Alexandre LISSY :gerard-majax from comment #86)
> It seems like you forgot to also land attachement 8390369 that removes the
> gralloc disabling on Nexus S devices :)

See comment #85 :)

Comment 88

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1775f8b66913