Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Use a FBO instead of a PBuffer in GLContextProviderEGL

RESOLVED FIXED in mozilla13

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bjacob, Assigned: Joe Drew (not getting mail))

Tracking

(Blocks: 1 bug)

unspecified
mozilla13
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 3 obsolete attachments)

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

Comment 1

6 years ago
Created attachment 590832 [details] [diff] [review]
use FBOs en EGL
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.
(Reporter)

Comment 3

6 years ago
Created attachment 591288 [details] [diff] [review]
switch from PBuffers to FBOs

This should work, but b2g crashes on startup for me.
Attachment #590832 - Attachment is obsolete: true

Comment 4

6 years ago
mwu, can you give this a spin?

Comment 5

6 years ago
(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.
(Reporter)

Comment 6

6 years ago
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).
(Reporter)

Comment 7

6 years ago
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.
Assignee: nobody → jgilbert
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.
Attachment #591288 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Blocks: 716770

Comment 9

6 years ago
(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

6 years ago
(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.
(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!

Updated

6 years ago
Depends on: 721205

Comment 12

6 years ago
(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.
Jeff G just landed the fix for bug 721205 on inbound. Please retry with that.

Comment 14

6 years ago
(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.
@ 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

6 years ago
(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.
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

6 years ago
(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.
(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? :)
(Assignee)

Comment 20

6 years ago
Created attachment 593589 [details] [diff] [review]
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.
Attachment #591529 - Attachment is obsolete: true
Attachment #593589 - Flags: review?(jgilbert)
Attachment #593589 - Flags: review?(bjacob)
Attachment #593589 - Flags: review?(jgilbert) → review+
Comment on attachment 593589 [details] [diff] [review]
Share FBO textures

Just listen to Jeff.
Attachment #593589 - Flags: review?(bjacob)

Comment 22

6 years ago
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/jdrew@mozilla.com-af67d5743098
 Timed out after 06 hours without completing.
(Assignee)

Comment 23

6 years ago
Created attachment 593953 [details] [diff] [review]
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. :(
Assignee: jgilbert → joe
Attachment #593953 - Flags: review?(jgilbert)
Attachment #593953 - Flags: review?(bjacob)
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

6 years ago
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/jdrew@mozilla.com-e7e39f6b03a9
(Assignee)

Comment 26

6 years ago
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.
(Assignee)

Updated

6 years ago
Blocks: 724143
(Assignee)

Comment 27

6 years ago
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.
Attachment #593953 - Flags: review?(jgilbert)
Attachment #593953 - Flags: review?(bjacob)
Attachment #593953 - Flags: review+
(Assignee)

Comment 28

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f6db301b290
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa92500f4ad2
You rock!

By the way, OSMesa isn't terrificly useful anymore. Maybe we should remove it?
I believe some of the fuzzing still goes on on OSMesa, but I could be wrong. I think we should keep it, anyways.
https://hg.mozilla.org/mozilla-central/rev/7f6db301b290
https://hg.mozilla.org/mozilla-central/rev/fa92500f4ad2
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13

Comment 32

6 years ago
Created attachment 595348 [details]
oprfile report with slow FPS (after patch)

Comment 33

6 years ago
Created attachment 595349 [details]
oprfile report with fast FPS (before patch)
Attachment #595349 - Attachment mime type: text/x-log → text/plain
Attachment #595348 - Attachment mime type: text/x-log → text/plain

Comment 34

6 years ago
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

6 years ago
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

6 years ago
Created attachment 595359 [details]
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
>              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.
(Assignee)

Comment 38

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

Comment 39

6 years ago
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.
(Assignee)

Comment 40

6 years ago
We should probably use pixel buffer objects, in that case. Have to check whether that's supported on our devices.
(Assignee)

Comment 41

6 years ago
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.
(Assignee)

Comment 42

6 years ago
I should note that panning is still plenty slow with the battery animation disabled.
(Assignee)

Comment 43

6 years ago
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;
Target Milestone: mozilla13 → ---

Comment 44

6 years ago
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

6 years ago
Created attachment 595622 [details]
perf callgraph with panning

This is the perf callgraph on b2g process with panning.
Nice!  Did you have to modify b2g at all to get the callgraph from perf?

Comment 47

6 years ago
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.
c++filt is your friend.

Comment 49

6 years ago
Is perf installed by default now?

Comment 50

6 years ago
Is perf installed by default now?
https://github.com/andreasgal/B2G/issues/167
(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??
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) ??
(Assignee)

Updated

6 years ago
Depends on: 725747
(Assignee)

Comment 54

6 years ago
I've filed bug 725747 on the performance regression.
(Assignee)

Comment 55

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

Comment 56

6 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/965a86374b2e
Target Milestone: --- → mozilla13

Updated

6 years ago
Depends on: 724476

Updated

6 years ago
Depends on: 726396

Updated

6 years ago
No longer depends on: 724476
(Reporter)

Updated

6 years ago
Depends on: 728361

Comment 58

6 years ago
(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.
Depends on: 732143
You need to log in before you can comment on or make changes to this bug.