Last Comment Bug 737071 - nexus s image layers are not drawn on B2G
: nexus s image layers are not drawn on B2G
Status: RESOLVED FIXED
[whiteboard]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla14
Assigned To: Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
:
Mentors:
: 738082 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-19 09:57 PDT by Andreas Gal :gal
Modified: 2012-03-22 18:08 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove LayerManagerOGL::glForResources() because it's not needed and causes performance degradtion sometimes (6.91 KB, patch)
2012-03-21 21:04 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bas: review+
Details | Diff | Splinter Review
Remove LayerManagerOGL::glForResources() because it's not needed and causes performance degradtion sometimes, v2 (8.17 KB, patch)
2012-03-21 21:38 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bas: review+
Details | Diff | Splinter Review

Description Andreas Gal :gal 2012-03-19 09:57:40 PDT
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 2012-03-19 09:57:52 PDT
This is a very urgent, top priority bug.
Comment 2 Joe Drew (not getting mail) 2012-03-19 13:58:38 PDT
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 2012-03-19 14:06:39 PDT
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) 2012-03-19 14:15:50 PDT
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 2012-03-21 05:22:53 PDT
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 Jeff Gilbert [:jgilbert] 2012-03-21 07:50:05 PDT
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 2012-03-21 11:42:58 PDT
AFAIK there's only one context involved here.
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-03-21 17:11:57 PDT
*** Bug 738082 has been marked as a duplicate of this bug. ***
Comment 9 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-03-21 18:10:42 PDT
... 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 2012-03-21 21:04:27 PDT
Created attachment 608216 [details] [diff] [review]
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 2012-03-21 21:38:11 PDT
Created attachment 608221 [details] [diff] [review]
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 2012-03-21 21:42:33 PDT
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) 2012-03-21 22:26:14 PDT
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 2012-03-21 23:05:02 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a0a9c190dd9
Comment 15 Joe Drew (not getting mail) 2012-03-22 10:30:57 PDT
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 Jeff Gilbert [:jgilbert] 2012-03-22 10:44:02 PDT
(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 Jeff Gilbert [:jgilbert] 2012-03-22 10:56:11 PDT
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) 2012-03-22 11:25:27 PDT
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] 2012-03-22 18:08:38 PDT
https://hg.mozilla.org/mozilla-central/rev/1a0a9c190dd9

Note You need to log in before you can comment on or make changes to this bug.