737071 - nexus s image layers are not drawn on B2G

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Andreas Gal :gal]

STR:
- install B2G trunk on nexus s
- go to gallery, pan images around
- images don't show until the layer is inserted back into the backgroun

Comment 1

[Andreas Gal :gal]

This is a very urgent, top priority bug.

Comment 2

[Joe Drew (not getting mail)]

What do you mean by "Pan images around"? I just installed B2G trunk on a Nexus S, and I can pan the gallery images fine. Do I have to set things up differently?

Comment 3

[Andreas Gal :gal]

Go to gallery. Move images left and right. On my phone nothing moves (black screen) until the image snaps into place. Also, no video plays (webm video). Can you try these things? If they work for you we have different hardware.

Comment 4

[Joe Drew (not getting mail)]

I can definitely reproduce it on video, so I'll work on that. Seems more straightforward :)

Comment 5

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

I poked at this for a few hours tonight, and found some interesting things
 - video is usually b0rked.  The framerate is extremely low.
 - HOWEVER, if I pull down the notification bar, sometimes I effect a state change in which video starts to play smoothly and flicker-free.  (There are transient artifacts, but I think they're due to a known PowerVR SGX bug.)
 - when the video is b0rked / framerate is low, I see a disturbing trend for ImageLayerOGL::RenderLayer calls to take 0.1-300ms (that's right, 300ms).  Texture upload itself consistently takes ~3ms, about right for memcpys.
 - bisecting on RenderLayer, the culprit is the |gl->MakeCurrent()| below, annotated with an example timing info from a "bad" frame

502 void
503 ImageLayerOGL::AllocateTexturesYCbCr(PlanarYCbCrImage *aImage)
504 {

(start timer)

505   if (!aImage->mBufferSize)
506     return;
507 
508   nsAutoPtr<PlanarYCbCrOGLBackendData> backendData(
509     new PlanarYCbCrOGLBackendData);
510 

AllocateTexturesYCbCr - new pyccbackenddata: 0.0041 ms

511   PlanarYCbCrImage::Data &data = aImage->mData;
512 
513   GLContext *gl = mOGLManager->glForResources();
514 

AllocateTexturesYCbCr - glforresources: 0.0199 ms

515   gl->MakeCurrent();
 
AllocateTexturesYCbCr - makecurrent: 248.885 ms (!?!?!?!)


So the next things I would like to know are
 - wtf is taking so long in the MakeCurrent call
 - what changes when the notification bar is pulled down, sometimes, to effect the switch to smooth playback.  Better STR for switching to smooth would help a lot.

Comment 6

[Kelsey Gilbert [:jgilbert]]

MakeCurrent is Known Slow, but I've never seen it be *that* bad. Does the length of MakeCurrent change at all if you put a glFinish (on the previous context) before it? What about a glFlush?

Comment 7

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

AFAIK there's only one context involved here.

Comment 9

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

... except for the global shared context.  This fixes the video bug:

diff --git a/gfx/gl/GLContextProviderEGL.cpp b/gfx/gl/GLContextProviderEGL.cpp
index 446ca73..eedf0c5 100644
--- a/gfx/gl/GLContextProviderEGL.cpp
+++ b/gfx/gl/GLContextProviderEGL.cpp
@@ -1992,7 +1992,7 @@ static nsRefPtr<GLContext> gGlobalContext;
 GLContext *
 GLContextProviderEGL::GetGlobalContext()
 {
-    static bool triedToCreateContext = false;
+    static bool triedToCreateContext = true;//false;
     if (!triedToCreateContext && !gGlobalContext) {
         triedToCreateContext = true;
         // Don't assign directly to gGlobalContext here, because

Comment 10

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

Attachment: Remove LayerManagerOGL::glForResources() because it's not needed and causes performance degradtion sometimes

The only place this is really needed is in ImageLayerOGL for containers moving parent contexts, but
 - that'll be incredibly uncommon
 - the code already handles reparenting to another context

So this should be pure win.

Comment 11

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

Attachment: Remove LayerManagerOGL::glForResources() because it's not needed and causes performance degradtion sometimes, v2

Now with reupload that I mistakenly thought was there in v1.

Comment 12

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

Comment on attachment 608221 [details] [diff] [review]
Remove LayerManagerOGL::glForResources() because it's not needed and causes performance degradtion sometimes, v2

+      cairoImage->SetBackendData(LayerManager::LAYERS_OPENGL, nsnull);
+      cairoImage = nsnull;

That's wrong.  Fixed locally.

Comment 13

[Bas Schouten (:bas.schouten)]

Comment on attachment 608221 [details] [diff] [review]
Remove LayerManagerOGL::glForResources() because it's not needed and causes performance degradtion sometimes, v2

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

::: gfx/layers/opengl/ImageLayerOGL.cpp
@@ +307,5 @@
> +    if (data && data->mTexture.GetGLContext() != gl()) {
> +      // If this texture was allocated by another layer manager, clear
> +      // it out and re-allocate below.
> +      cairoImage->SetBackendData(LayerManager::LAYERS_OPENGL, nsnull);
> +      cairoImage = nsnull;

data = nsnull;

@@ +525,5 @@
>      new PlanarYCbCrOGLBackendData);
>  
>    PlanarYCbCrImage::Data &data = aImage->mData;
>  
> +  gl()->MakeCurrent();

Sometime we should have a good look at redundant make current calls in the layers system.

Comment 14

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a0a9c190dd9

Comment 15

[Joe Drew (not getting mail)]

I worry whether this will screw us on shutdown, since we rely on that extra GL context to allow us to delete textures when our original context has been deleted.

Comment 16

[Kelsey Gilbert [:jgilbert]]

(In reply to Joe Drew (:JOEDREW!) from comment #15)
> I worry whether this will screw us on shutdown, since we rely on that extra
> GL context to allow us to delete textures when our original context has been
> deleted.

We probably shouldn't bother deleting GL stuff on shutdown, really.

If contexts clean up after themselves when they are destroyed, we shouldn't bother formally cleaning up at all. However, since everything's shared, that might persist objects even though the original creating context has been destroyed. (But it's worth checking)

Comment 17

[Kelsey Gilbert [:jgilbert]]

I didn't check the spec and didn't test it, but I found indications that if you do:

foo = new Context()
tex = foo.createTexture()
bar = new Context(share with foo)
foo.destroy

'tex' is still accessible from context bar, which is sensible.

However, if you then destroy bar, it'll clean everything up.

Comment 18

[Joe Drew (not getting mail)]

Er, when I said shutdown, I meant context shutdown. And, it turns out, I actually meant BasicTextureImage::~BasicTextureImage().

As long as that isn't affected, I'm ok with this.

Comment 19

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/1a0a9c190dd9