Closed Bug 729538 Opened 13 years ago Closed 12 years ago

MAPLE: use gralloc to reduce upload costs

Categories

(Firefox for Android Graveyard :: General, defect, P1)

x86
macOS
defect

Tracking

(blocking-fennec1.0 -)

RESOLVED WONTFIX
Tracking Status
blocking-fennec1.0 --- -

People

(Reporter: joe, Unassigned)

References

Details

(Whiteboard: MAPLE mwc-demo [gfx])

Attachments

(1 obsolete file)

We pay a heavy penalty to upload textures on Android; we should use gralloc in a way that reduces that overhead to reduce checkerboarding and improve interactive performance.
Blocks: 729391
Assignee: snorp → pwalton
Current status: I have stuff on the screen with gralloc on my Droid RAZR. It seems quite fast. Unresolved issues so far: (1) Surface destruction isn't implemented, so we have bad leaks. (2) GL state is getting corrupted somehow; Java compositor decorations disappear. (3) Memory waste: We waste a buffer, leading to 2x memory consumption. This may be fixable with more investigation. (4) Graphic backing buffers are not used on the content side, so there's a memmove involved. I still usually get 60FPS on my Droid RAZR, but this may need to be improved for slower devices. (5) Untested on other phones.
Whiteboard: MAPLE → MAPLE mwc-demo
Priority: -- → P1
Attached patch WIP patch. (obsolete) — Splinter Review
This is an early WIP. The main problem is that there's a lot of graphical corruption (skew mostly -- it looks like the stride can get off) and black screens. The black screens seem to correlate well with this error: 02-22 23:14:17.180 11253 11298 W IMGSRV : eglimage.c:340: glEGLImageTargetTexture2DOES: Invalid name I suspect there's some sort of race. Hopefully that race is on our end and isn't in the driver.
Attachment #599902 - Flags: feedback?(snorp)
Comment on attachment 599902 [details] [diff] [review] WIP patch. There doesn't appear to be any double buffering here, which is definitely going to cause issues. Also, I really wish you would have just changed the implementation in AndroidDirectTexture (or AndroidGraphicBuffer I guess). This doesn't handle the shadow layer stuff, does it? It seems to me that this is what matters the most, especially where the demo is concerned.
Attachment #599902 - Flags: feedback?(snorp) → feedback-
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #3) > Comment on attachment 599902 [details] [diff] [review] > WIP patch. > > There doesn't appear to be any double buffering here, which is definitely > going to cause issues. It actually shouldn't. Since we aren't drawing directly into the buffer (we use a memmove to move the basic layer buffer), and we never composite while that memmove is happening, I think it's ok. > Also, I really wish you would have just changed the > implementation in AndroidDirectTexture (or AndroidGraphicBuffer I guess). I'll eventually merge the two, but I really wanted at first a direct copy of my test code, because this stuff is really finicky and I didn't want any confounding factors. > This doesn't handle the shadow layer stuff, does it? It seems to me that > this is what matters the most, especially where the demo is concerned. Actually, memmove from a basic layer to the image is just fine on my Droid RAZR -- it seems much faster than texture upload and allows us to hit 60fps -- but for slower phones you may be right.
(In reply to Patrick Walton (:pcwalton) from comment #4) > (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #3) > > Comment on attachment 599902 [details] [diff] [review] > > WIP patch. > > > > There doesn't appear to be any double buffering here, which is definitely > > going to cause issues. > > It actually shouldn't. Since we aren't drawing directly into the buffer (we > use a memmove to move the basic layer buffer), and we never composite while > that memmove is happening, I think it's ok. I don't see any memmove in your patch. Anyway, how would that be different from just drawing into the gralloc buffer like we do in m-c? Even though you can guarantee that you aren't compositing during that time, you cannot guarantee that the GPU isn't doing something with the buffer. > > > Also, I really wish you would have just changed the > > implementation in AndroidDirectTexture (or AndroidGraphicBuffer I guess). > > I'll eventually merge the two, but I really wanted at first a direct copy of > my test code, because this stuff is really finicky and I didn't want any > confounding factors. Fair enough, I guess. > > > This doesn't handle the shadow layer stuff, does it? It seems to me that > > this is what matters the most, especially where the demo is concerned. > > Actually, memmove from a basic layer to the image is just fine on my Droid > RAZR -- it seems much faster than texture upload and allows us to hit 60fps > -- but for slower phones you may be right. Like I said above, I don't see the memmove anywhere, but isn't this less efficient than what we do on m-c? If you just back the basic layer with the gralloc buffer directly and then pass the AndroidDirectTexture to the compositor you save whatever time the memmove takes. We could also half the memory usage by eliminating the traditional cairo buffer.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #5) > I don't see any memmove in your patch. Anyway, how would that be different > from just drawing into the gralloc buffer like we do in m-c? Even though you > can guarantee that you aren't compositing during that time, you cannot > guarantee that the GPU isn't doing something with the buffer. The memmove is here: http://mxr.mozilla.org/mozilla-central/source/gfx/gl/GLContextProviderEGL.cpp#1754 The GPU never draws to the framebuffer, it only draws to a back buffer in the GLSurfaceView, which is exchanged with eglSwapBuffers(). > Like I said above, I don't see the memmove anywhere, but isn't this less > efficient than what we do on m-c? If you just back the basic layer with the > gralloc buffer directly and then pass the AndroidDirectTexture to the > compositor you save whatever time the memmove takes. We could also half the > memory usage by eliminating the traditional cairo buffer. Yes, it's less efficient. Should definitely be changed in the future.
At this point, I don't believe this technique works. Despite the fact that the small testcase works, when using GLES 2.0 and the complex shaders that Gecko uses, glEGLImageTargetTexture2DOES just doesn't work on most devices (Galaxy S2 in particular, and the Galaxy Nexus) -- you see black textures. For some devices (Droid RAZR) it does work as expected. However, the classic gralloc technique works on the Droid RAZR, so this steal-a-surface technique isn't a win -- it wastes memory. Right now I feel that our time would be better spent on SurfaceTexture for ICS, and double-buffered incremental upload for earlier devices. The latter has a higher probability of working on problematic devices like the Droid RAZR.
(In reply to Patrick Walton (:pcwalton) from comment #7) > > Right now I feel that our time would be better spent on SurfaceTexture for > ICS, and double-buffered incremental upload for earlier devices. The latter > has a higher probability of working on problematic devices like the Droid > RAZR. I tend to agree. If we can get SurfaceTexture working well on ICS we can always come back and reinvestigate this later.
The docs for SurfaceTexture say you can use it with GLES 2.0 by adding a "#extension GL_OES_EGL_image_external : require" shader directive and using the samplerExternalOES tex sampler type. If you then look at the implementation for SurfaceTexture::updateTexImage() (https://github.com/android/platform_frameworks_base/blob/master/libs/gui/SurfaceTexture.cpp#L749) you can see it's just using the same EGLImage/glEGLImageTargetTexture2DOES stuff we are accustomed to. I think we can make this work by going that route. It's probably best to make it work with SurfaceTexture first, though, and then implement our own equivalent of updateTexImage() later.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #9) > The docs for SurfaceTexture say you can use it with GLES 2.0 by adding a > "#extension GL_OES_EGL_image_external : require" shader directive and using > the samplerExternalOES tex sampler type. > > If you then look at the implementation for SurfaceTexture::updateTexImage() > (https://github.com/android/platform_frameworks_base/blob/master/libs/gui/ > SurfaceTexture.cpp#L749) you can see it's just using the same > EGLImage/glEGLImageTargetTexture2DOES stuff we are accustomed to. I think we > can make this work by going that route. It's probably best to make it work > with SurfaceTexture first, though, and then implement our own equivalent of > updateTexImage() later. Yeah, that's probably the best route. I investigated the implementation of SurfaceTexture and there are three main differences: (1) EGL fences are (optionally) used. (I tried this and it didn't work, unfortunately.) (2) A new IPC API has been added to allow unprivileged applications to allocate graphics buffers. (3) GPU manufacturers actually presumably test it.
blocking-fennec1.0: --- → beta+
Status: NEW → ASSIGNED
AFAIK we're not doing this any more, so marking this as wontfix. Feel free to reopen if you think it shouldn't be closed.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
At some point we will want to do gralloc, I think, but we can leave this closed until that time.
Benoit is working on this.
Assignee: pwalton → bgirard
Blocks: 734164
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Whiteboard: MAPLE mwc-demo → MAPLE mwc-demo [gfx]
Depends on: 712716
Attachment #599902 - Attachment is obsolete: true
No longer blocks: checkerboarding
blocking-fennec1.0: beta+ → -
Still important, but glimage is the primary solution for beta.
Blocks: 749062
Gralloc is no longer a requirement for maple and a priority now that we have low res tile based priority drawing.
Assignee: bgirard → nobody
Status: REOPENED → RESOLVED
Closed: 13 years ago12 years ago
Resolution: --- → WONTFIX
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: