720467 - Use a FBO instead of a PBuffer in GLContextProviderEGL

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

This should fix lots of issues, especially on mobile GPUs that don't support NPOT PBuffers, like what we're seeing in bug 716770.

Comment 1

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

Attachment: use FBOs en EGL

Comment 2

[Kelsey Gilbert [:jgilbert]]

We shouldn't need to create a 16x16 dummy PBuffer, unless the extant GLContextProviderEGL::CreateForWindow() doesn't work. This is how we do FBOs vs PBuffers in WGL, at least.

Comment 3

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

Attachment: switch from PBuffers to FBOs

This should work, but b2g crashes on startup for me.

Comment 4

[Andreas Gal :gal]

mwu, can you give this a spin?

Comment 5

[Michael Wu [:mwu]]

(In reply to Andreas Gal :gal from comment #4)
> mwu, can you give this a spin?

I applied this patch but it seems to make things worse - now the apps don't show up at all.

Comment 6

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

I would like to see the logcat output with MOZ_GL_DEBUG_VERBOSE=1 MOZ_GL_DEBUG_ABORT_ON_ERROR=1 in a debug build (and bt if it hits a GL error).

Comment 7

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

This affecting all EGL-using platforms, could also be tested on Windows, where this patch breaks WebGL rendering.

Jeff G has found out why this doesn't work: GLContextProviderEGL is hardcoding use of PBuffers in several other places.

This code is pretty much un-owned which has hurt us several times in the past, especially for GL layers on Android, but Jeff is much more knowledgeable than me about that, so, assigning to him.

Comment 8

[Kelsey Gilbert [:jgilbert]]

Attachment: Use dummy PBuffer context to use for FBO backing in EGL

Seems to work in EGL with both accel and non-accel layers on windows.

Comment 9

[Ali Juma [:ajuma]]

(In reply to Jeff Gilbert [:jgilbert] from comment #8)
> Created attachment 591529 [details] [diff] [review]
> Use dummy PBuffer context to use for FBO backing in EGL

This doesn't work on Android -- nothing gets rendered.

Comment 10

[Michael Wu [:mwu]]

(In reply to Jeff Gilbert [:jgilbert] from comment #8)
> Created attachment 591529 [details] [diff] [review]
> Use dummy PBuffer context to use for FBO backing in EGL
> 
> Seems to work in EGL with both accel and non-accel layers on windows.

Tested on B2G and doesn't seem to help. The app icons don't show up like before.

Comment 11

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

(In reply to Ali Juma [:ajuma] from comment #9)
> (In reply to Jeff Gilbert [:jgilbert] from comment #8)
> > Created attachment 591529 [details] [diff] [review]
> > Use dummy PBuffer context to use for FBO backing in EGL
> 
> This doesn't work on Android -- nothing gets rendered.

This is actually good news, as it means that one can debug this on Android as well as on B2G. Please have a look!

Comment 12

[Ali Juma [:ajuma]]

(In reply to Benoit Jacob [:bjacob] from comment #11)
> This is actually good news, as it means that one can debug this on Android
> as well as on B2G. Please have a look!

The problem on Android seems unrelated to the problem on B2G, so I've filed Bug 721205 for the Android issue.

Comment 13

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

Jeff G just landed the fix for bug 721205 on inbound. Please retry with that.

Comment 14

[Ali Juma [:ajuma]]

(In reply to Benoit Jacob [:bjacob] from comment #13)
> Jeff G just landed the fix for bug 721205 on inbound. Please retry with that.

As before, nothing gets rendered. However, when I modify that fix so that mSupport_ES_ReadPixels_BGRA_Ubyte is false unconditionally, this works.

Comment 15

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

@ Michael Wu: we think that comment 14 is a driver bug on Ali's phone and still hope that Jeff's patch here, on top of the one in bug 721205, may make things work on the Maguro. Could you try?

Comment 16

[Michael Wu [:mwu]]

(In reply to Benoit Jacob [:bjacob] from comment #15)
> @ Michael Wu: we think that comment 14 is a driver bug on Ali's phone and
> still hope that Jeff's patch here, on top of the one in bug 721205, may make
> things work on the Maguro. Could you try?

I applied both patches and no icons show up.

Comment 17

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

Michael, if you have a debug build of B2G, can you run with the env vars in comment 6? Do you get a crash?  (these env vars will in particular convert any GL error into an assert failure)

Comment 18

[Michael Wu [:mwu]]

(In reply to Benoit Jacob [:bjacob] from comment #17)
> Michael, if you have a debug build of B2G, can you run with the env vars in
> comment 6? Do you get a crash?  (these env vars will in particular convert
> any GL error into an assert failure)

I tried a debug build but

- It takes enough space that I had to delete other things in the image to get it to fit
- It crashes

So I replaced instances of #ifdef DEBUG in gfx/gl/* with #ifdef DEBUGG and defined DEBUGG in the makefile. I'm not getting any aborts.

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Michael Wu [:mwu] from comment #18)
> (In reply to Benoit Jacob [:bjacob] from comment #17)
> > Michael, if you have a debug build of B2G, can you run with the env vars in
> > comment 6? Do you get a crash?  (these env vars will in particular convert
> > any GL error into an assert failure)
> 
> I tried a debug build but
> 
> - It takes enough space that I had to delete other things in the image to
> get it to fit

Which things?  Can they be permanently deleted? :)

Comment 20

[Joe Drew (not getting mail)]

Attachment: Share FBO textures

This modifies bjacob/jgilbert's previous patch to use context sharing, so we can address the WebGL context's backing texture from the layer manager's context.

Comment 21

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

Comment on attachment 593589 [details] [diff] [review]
Share FBO textures

Just listen to Jeff.

Comment 22

[Mozilla RelEng Bot]

Try run for af67d5743098 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=af67d5743098
Results (out of 206 total builds):
    success: 177
    warnings: 29
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-af67d5743098
 Timed out after 06 hours without completing.

Comment 23

[Joe Drew (not getting mail)]

Attachment: Shutdown EGL explicitly on Windows

This fails mochitests on Windows because we only shut down the WGL ContextProvider, since that's the default context provider on Windows. Shutting down the EGL context provider explicitly (in addition to the default context provider) fixes this problem.

If we didn't have two context providers on Windows, we wouldn't need this. :(

Comment 24

[Kelsey Gilbert [:jgilbert]]

If we can get a real opengl context from EGL on windows, that would be ideal.

I would prefer using EGL everywhere, really.

Comment 25

[Mozilla RelEng Bot]

Try run for e7e39f6b03a9 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e7e39f6b03a9
Results (out of 30 total builds):
    success: 27
    warnings: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e7e39f6b03a9

Comment 26

[Joe Drew (not getting mail)]

I don't think that is a possibility. EGL, after all, is implemented by ANGLE, and it hands out GLES contexts implemented on top of Direct3D 9.

Comment 27

[Joe Drew (not getting mail)]

Comment on attachment 593953 [details] [diff] [review]
Shutdown EGL explicitly on Windows

Jeff gave me r+ on IRC for a version of this that also shuts down OSMesa. As he points out, that context provider is also available on all platforms.

Comment 28

[Joe Drew (not getting mail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f6db301b290
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa92500f4ad2

Comment 29

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

You rock!

By the way, OSMesa isn't terrificly useful anymore. Maybe we should remove it?

Comment 30

[Kelsey Gilbert [:jgilbert]]

I believe some of the fuzzing still goes on on OSMesa, but I could be wrong. I think we should keep it, anyways.

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7f6db301b290
https://hg.mozilla.org/mozilla-central/rev/fa92500f4ad2

Comment 32

[Shian-Yow Wu [:swu]]

Attachment: oprfile report with slow FPS (after patch)

Comment 33

[Shian-Yow Wu [:swu]]

Attachment: oprfile report with fast FPS (before patch)

Comment 34

[Shian-Yow Wu [:swu]]

Not much difference in profiling result before or after patch.

The profiling result shows heavy loading in memcpy(), it's not related to this patch.
But about one month ago, there were no such heavy loaded memcpy().
It's another issue need to notice.  Unfortunately, the callgraph doesn't show who is calling
memcpy(), maybe it's called by some library without symbols or been optimized.

Comment 35

[Shian-Yow Wu [:swu]]

Unlike high CPU usage of b2g process at "Bug 708524" which happens all the time.
The high memcpy() only happens in screen on situation.  This is a separate issue.

Below is the perf top result.

   PerfTop:     799 irqs/sec  kernel: 2.8%  exact:  0.0% [1000Hz cycles],  (target_pid: 4000)
-------------------------------------------------------------------------------

             samples  pcnt function                    DSO
             _______ _____ ___________________________ _________________

             1838.00 86.9% memcpy                      libc.so          
               99.00  4.7% _gles_fb_tex_sub_image_2d   libGLESv2_mali.so
               24.00  1.1% floor                       libm.so          
               14.00  0.7% _raw_spin_unlock_irqrestore [kernel.kallsyms]
               13.00  0.6% __divsi3                    libmozglue.so    
                9.00  0.4% __do_softirq                [kernel.kallsyms]
                8.00  0.4% getnstimeofday              [kernel.kallsyms]
                6.00  0.3% pthread_mutex_lock          libc.so          
                5.00  0.2% memset                      libc.so

Comment 36

[Shian-Yow Wu [:swu]]

Attachment: oprfile report with slow FPS and keep scrolling

If I keep scrolling the screen, the heavy loaded module becomes to vmlinux.

1. oprofile summary: Phone with this patch and tested by scrolling.

CPU: ARM Cortex-A9, speed 0 MHz (estimated)
Counted CPU_CYCLES events (Number of CPU cycles) with a unit mask of 0x00 (No unit mask) count 125000
CPU_CYCLES:125000|
  samples|      %|
------------------
    14556 77.1056 vmlinux
     1111  5.8852 libxul.so
     1107  5.8640 oprofiled
      872  4.6191 libMali.so
      594  3.1465 libc.so
      257  1.3614 libGLESv2_mali.so
      187  0.9906 libmozglue.so
       87  0.4609 b2g

2. oprofile summary: Phone without this patch and tested by scrolling.

CPU: ARM Cortex-A9, speed 0 MHz (estimated)
Counted CPU_CYCLES events (Number of CPU cycles) with a unit mask of 0x00 (No unit mask) count 125000
CPU_CYCLES:125000|
  samples|      %|
------------------
    38756 37.2550 libxul.so
    21172 20.3520 vmlinux
    13190 12.6792 libc.so
     9496  9.1282 oprofiled
     7166  6.8885 libGLESv2_mali.so
     5705  5.4840 libmozglue.so
     3708  3.5644 libMali.so
     2218  2.1321 b2g

Comment 37

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

>              1838.00 86.9% memcpy                      libc.so          
>                99.00  4.7% _gles_fb_tex_sub_image_2d   libGLESv2_mali.so

This suggests that glTexSubImage2D is broken on this phone and we should use glTexImage2D instead. It can then make sense that the present patch would regress performance, if we are using glTexSubImage2D for the texture uploads here.

See also bug 724094.

Comment 38

[Joe Drew (not getting mail)]

Why does this suggest that glTexSubImage2D is broken? The data has to get there somehow.

Comment 39

[Andreas Gal :gal]

I think bjacob is right. The driver on adreno devices (at least some versions on it) is known to implement TexSubImage2D by downloading the whole texture in CPU memory, then doing a memcpy, and then re-uploading the entire texture. Even my radeon driver on my mac does this. I don't know why driver writers hate TexSubImage2D so much.

Comment 40

[Joe Drew (not getting mail)]

We should probably use pixel buffer objects, in that case. Have to check whether that's supported on our devices.

Comment 41

[Joe Drew (not getting mail)]

When I disable the animation for the battery icon, glTexSubImage2D is not called at all during panning (only a couple of times after I lift my finger). Going to profile now.

Comment 42

[Joe Drew (not getting mail)]

I should note that panning is still plenty slow with the battery animation disabled.

Comment 43

[Joe Drew (not getting mail)]

I'm having trouble getting callgraph results with oprofile, unfortunately. How does one set up perf to work on a B2G Galaxy S 2?

Shian-Yu, it would be very helpful if you could attach a full perf report of panning after applying the following gaia patch:

diff --git a/apps/homescreen/style/homescreen.css b/apps/homescreen/style/homescreen.css
index 9143032..79f55e7 100644
--- a/apps/homescreen/style/homescreen.css
+++ b/apps/homescreen/style/homescreen.css
@@ -263,7 +263,7 @@ iframe.appWindow.noTransition {
 
 .battery > #fuel.charging {
   background-image: -moz-linear-gradient(top, #b9fd00, #8fc400);
-  -moz-animation-duration: 3s;
+/*  -moz-animation-duration: 3s; */
   -moz-animation-name: batteryCharging;
   -moz-animation-iteration-count: infinite;
   -moz-animation-timing-function: linear;

Comment 44

[Shian-Yow Wu [:swu]]

No more heavy memcpy() after removing CSS animation.

   PerfTop:     101 irqs/sec  kernel:72.3%  exact:  0.0% [1000Hz cycles],  (all, 1 CPU)
-------------------------------------------------------------------------------

             samples  pcnt function                     DSO
             _______ _____ ____________________________ _____________

              150.00 21.6% s5pv310_enter_idle           [kernel]     
               82.00 11.8% _raw_spin_unlock_irqrestore  [kernel]     
               68.00  9.8% _raw_spin_unlock_irq         [kernel]     
               49.00  7.1% tick_nohz_stop_sched_tick    [kernel]     
               33.00  4.7% tick_nohz_restart_sched_tick [kernel]     
               20.00  2.9% add_preempt_count            [kernel]     
               19.00  2.7% schedule                     [kernel]     
               19.00  2.7% getnstimeofday               [kernel]     
               17.00  2.4% __do_softirq                 [kernel]     
               16.00  2.3% sub_preempt_count            [kernel]     
               15.00  2.2% vector_swi                   [kernel]     
               12.00  1.7% __divsi3                     [kernel]     
               10.00  1.4% __udivsi3                    libmozglue.so
               10.00  1.4% __mutex_unlock_slowpath      [kernel]     
                9.00  1.3% __mutex_lock_slowpath        [kernel]     
                7.00  1.0% memcpy                       [kernel]     

I tried perf with callgraph for having some problem on my system at this moment.
It works if you don't add a -g (callgraph) flag.

Here's a prebuilt perf for android, FYI.

http://code.google.com/p/android-group-korea/downloads/detail?name=perf-binary-kwangwoo-for-android.zip

Comment 45

[Shian-Yow Wu [:swu]]

Attachment: perf callgraph with panning

This is the perf callgraph on b2g process with panning.

Comment 46

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Nice!  Did you have to modify b2g at all to get the callgraph from perf?

Comment 47

[Shian-Yow Wu [:swu]]

No, I don't have to modify anything on b2g to get callgraph from perf.

Here is the perf command I used.  (2599 is b2g's pid)

# ./perf record -g -p 2599
# ./perf report > perf_callgraph.txt

There are still some C++ demanging issue to be improved in the callgraph.

Comment 48

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

c++filt is your friend.

Comment 49

[Andreas Gal :gal]

Is perf installed by default now?

Comment 50

[Andreas Gal :gal]

Is perf installed by default now?

Comment 51

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://github.com/andreasgal/B2G/issues/167

Comment 52

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

(In reply to Shian-Yow Wu from comment #45)
> Created attachment 595622 [details]
> perf callgraph with panning
> 
> This is the perf callgraph on b2g process with panning.

Spending > 50% time around IRQs??

Comment 53

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

There is some 'mali200' stuff in the callers for these 2 IRQ-related symbols, so, does that mean that some GPU IO is very inefficient (blocking) ??

Comment 54

[Joe Drew (not getting mail)]

I've filed bug 725747 on the performance regression.

Comment 55

[Joe Drew (not getting mail)]

I forgot to commit some improved comments when originally pushing this patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/965a86374b2e

Comment 56

[Shian-Yow Wu [:swu]]

(In reply to Shian-Yow Wu from comment #47)
> No, I don't have to modify anything on b2g to get callgraph from perf.
> 
More accurately, I build b2g image as usual, but replaced libxul.so with symbols on sgs2.

Comment 57

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/965a86374b2e

Comment 58

[daniel-bzmz]

(In reply to Jeff Gilbert [:jgilbert] from comment #24)
> If we can get a real opengl context from EGL on windows, that would be ideal.
> 
> I would prefer using EGL everywhere, really.

This would be possibly.  You have to lobby the Desktop GL vendors to provide it though. Currently they claim no one is interested in it.