Last Comment Bug 782785 - Compositing of WebGL canvases on B2G is broken unless canvas width is a multiple of 32
: Compositing of WebGL canvases on B2G is broken unless canvas width is a multi...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla17
Assigned To: Jeff Gilbert [:jgilbert]
:
Mentors:
Depends on: 784085
Blocks: 781000
  Show dependency treegraph
 
Reported: 2012-08-14 14:21 PDT by Benoit Jacob [:bjacob] (mostly away)
Modified: 2012-08-22 02:45 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
allow for readback onto too-large surfaces (14.23 KB, patch)
2012-08-14 17:05 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
Details | Diff | Splinter Review
Use temp surfaces with correct stride for reads. (14.85 KB, patch)
2012-08-20 16:07 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
Details | Diff | Splinter Review
de-bitrotted (11.50 KB, patch)
2012-08-21 16:11 PDT, Jeff Gilbert [:jgilbert]
jgilbert: review+
Details | Diff | Splinter Review

Description Benoit Jacob [:bjacob] (mostly away) 2012-08-14 14:21:14 PDT
Does not work (500x500 canvas):
http://people.mozilla.org/~bjacob/webgl-spotlight-js-2012/webgltriangle.html

Works (480x500 canvas):
http://people.mozilla.org/~bjacob/webgl-spotlight-js-2012/webgltriangle480.html

This issue is B2G-specific.
Comment 1 Benoit Jacob [:bjacob] (mostly away) 2012-08-14 14:24:16 PDT
Note that bug 782786 can also explain WebGL canvas compositing failing in B2G.
Comment 2 Benoit Jacob [:bjacob] (mostly away) 2012-08-14 15:56:10 PDT
Hoho,

[Child 1069] ###!!! ASSERTION: gfxImageSurface stride isn't what we expect!: 'isurf->Stride() == mBounds.width * 4', file /hack/b2g/B2G/gecko/gfx/layers/basic/BasicCanvasLayer.cpp, line 175
[Child 1069] WARNING: ReadPixelsIntoImageSurface called with wrong size or stride surface: file /hack/b2g/B2G/gecko/gfx/gl/GLContext.cpp, line 2019
[Parent 381] WARNING: NS_ENSURE_TRUE(AttrToDataProp(attr, prop)) failed: file /hack/b2g/B2G/gecko/content/html/content/src/nsDOMStringMap.cpp, line 166

Maybe the real number is not exactly 160 but some divisor of it.
Comment 3 Benoit Jacob [:bjacob] (mostly away) 2012-08-14 15:59:18 PDT
Indeed, the real criterion is: width must be a multiple of 32. So 480, 512, 544, 576 work, but not 496 or 528.
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-14 16:13:26 PDT
I believe we'll end up using 32-bit surfaces when reading back, so this may mean that the surfaces need to be a multiple of 128 bytes.  That sounds suspiciously familiar ....
Comment 5 Jeff Gilbert [:jgilbert] 2012-08-14 17:05:41 PDT
Created attachment 651940 [details] [diff] [review]
allow for readback onto too-large surfaces

It looks like the destSurface we're passed in UpdateSurface is a different size than mBounds. This patch assumes (and asserts) that the destSurface is always larger than the area we're reading. We reuse the temp-surface code to create a correctly-sized buffer to read into, before copying into the destination surface.
Comment 6 Benoit Jacob [:bjacob] (mostly away) 2012-08-14 17:24:02 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> I believe we'll end up using 32-bit surfaces when reading back

We are -- and Jeff's patch MOZ_ASSERTs that.
Comment 7 Benoit Jacob [:bjacob] (mostly away) 2012-08-15 08:15:07 PDT
Works!
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2012-08-15 08:16:32 PDT
Comment on attachment 651940 [details] [diff] [review]
allow for readback onto too-large surfaces

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

::: gfx/layers/basic/BasicCanvasLayer.cpp
@@ +166,4 @@
>      if (aDestSurface) {
> +      resultSurf = static_cast<gfxImageSurface*>(aDestSurface);
> +
> +      if (resultSurf->mSize != readSize) {

mSize is not public; changing this to GetSize() worked.
Comment 9 Benoit Jacob [:bjacob] (mostly away) 2012-08-15 08:19:08 PDT
https://tbpl.mozilla.org/?tree=Try&rev=ea30d062ac54
Comment 10 Benoit Jacob [:bjacob] (mostly away) 2012-08-15 08:42:24 PDT
Hm, I replied too fast -- I had tried a build without the patch, and inadvertently used the modified page that used a size multiple of 32.

With the patch, I currently get a crash, debugging.
Comment 11 Benoit Jacob [:bjacob] (mostly away) 2012-08-20 08:30:04 PDT
FYI: Last week I tried debugging the crash, but I wouldn't get any symbols when debugging a child process. Debugging the main process does find all the symbols. The GDB variables such as solib-search-path are all identical between the two cases. I followed these instructions:
https://developer.mozilla.org/en-US/docs/Mozilla/Boot_to_Gecko/Debugging_on_Boot_to_Gecko
Comment 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-20 10:35:13 PDT
I have not seen that problem.  You should drop by #b2g.
Comment 13 Benoit Jacob [:bjacob] (mostly away) 2012-08-20 10:39:47 PDT
Filed bug 784085 with a patch by jrmuizel that fixes it.
Comment 14 Benoit Jacob [:bjacob] (mostly away) 2012-08-20 10:42:06 PDT
Here is the crash:

Program received signal SIGSEGV, Segmentation fault.
0x4199465a in mozilla::layers::BasicCanvasLayer::UpdateSurface (this=0x21b9c28, aDestSurface=0x226aa78, aMaskLayer=0x0)
    at /hack/b2g/B2G/gecko/gfx/layers/basic/BasicCanvasLayer.cpp:185
185         MOZ_ASSERT(readSurf->Stride() == mBounds.width * 4, "gfxImageSurface stride isn't what we expect!");
(gdb) bt
#0  0x4199465a in mozilla::layers::BasicCanvasLayer::UpdateSurface (this=0x21b9c28, aDestSurface=0x226aa78, aMaskLayer=0x0)
    at /hack/b2g/B2G/gecko/gfx/layers/basic/BasicCanvasLayer.cpp:185
#1  0x4199532e in mozilla::layers::BasicShadowableCanvasLayer::Paint (this=0x21b9c28, aContext=0x22328b8, aMaskLayer=0x0)
    at /hack/b2g/B2G/gecko/gfx/layers/basic/BasicCanvasLayer.cpp:442
#2  0x41991d96 in mozilla::layers::BasicLayerManager::PaintLayer (this=0x2146f48, aTarget=0x22328b8, aLayer=0x21b9c28, 
    aCallback=0x4065bde1 <mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*)>, 
    aCallbackData=0xbee5e55c, aReadback=0xbee5dfbc) at /hack/b2g/B2G/gecko/gfx/layers/basic/BasicLayerManager.cpp:830
#3  0x41991e28 in mozilla::layers::BasicLayerManager::PaintLayer (this=0x2146f48, aTarget=0x22328b8, aLayer=0x22326f8, 
    aCallback=0x4065bde1 <mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*)>, 
    aCallbackData=0xbee5e55c, aReadback=0x0) at /hack/b2g/B2G/gecko/gfx/layers/basic/BasicLayerManager.cpp:843
#4  0x419908c0 in mozilla::layers::BasicLayerManager::EndTransactionInternal (this=0x2146f48, 
    aCallback=0x4065bde1 <mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*)>, 
    aCallbackData=0xbee5e55c, aFlags=mozilla::layers::LayerManager::END_NO_COMPOSITE) at /hack/b2g/B2G/gecko/gfx/layers/basic/BasicLayerManager.cpp:464
#5  0x41990342 in mozilla::layers::BasicLayerManager::EndTransaction (this=0x2146f48, 
    aCallback=0x4065bde1 <mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*)>, 
    aCallbackData=0xbee5e55c, aFlags=mozilla::layers::LayerManager::END_NO_COMPOSITE) at /hack/b2g/B2G/gecko/gfx/layers/basic/BasicLayerManager.cpp:390
#6  0x419928c0 in mozilla::layers::BasicShadowLayerManager::EndTransaction (this=0x2146f48, 
    aCallback=0x4065bde1 <mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*)>, 
    aCallbackData=0xbee5e55c, aFlags=mozilla::layers::LayerManager::END_NO_COMPOSITE) at /hack/b2g/B2G/gecko/gfx/layers/basic/BasicLayerManager.cpp:1043
#7  0x406a270c in nsDisplayList::PaintForFrame (this=0xbee5e940, aBuilder=0xbee5e55c, aCtx=0x0, aForFrame=0x229de38, aFlags=13)
    at /hack/b2g/B2G/gecko/layout/base/nsDisplayList.cpp:1040
#8  0x406a21ec in nsDisplayList::PaintRoot (this=0xbee5e940, aBuilder=0xbee5e55c, aCtx=0x0, aFlags=13) at /hack/b2g/B2G/gecko/layout/base/nsDisplayList.cpp:939
#9  0x406cac6e in nsLayoutUtils::PaintFrame (aRenderingContext=0x0, aFrame=0x229de38, aDirtyRegion=..., aBackstop=4294967295, aFlags=772)
    at /hack/b2g/B2G/gecko/layout/base/nsLayoutUtils.cpp:1868
#10 0x406edbd0 in PresShell::Paint (this=0x2291cc8, aViewToPaint=0x21a5010, aDirtyRegion=..., aType=nsIPresShell::PaintType_NoComposite, aWillSendDidPaint=true)
    at /hack/b2g/B2G/gecko/layout/base/nsPresShell.cpp:5337
#11 0x40c8320e in nsViewManager::ProcessPendingUpdatesForView (this=0x21b7128, aView=0x21a5010, aFlushDirtyRegion=true)
    at /hack/b2g/B2G/gecko/view/src/nsViewManager.cpp:428
#12 0x40c85596 in nsViewManager::ProcessPendingUpdates (this=0x21b7128) at /hack/b2g/B2G/gecko/view/src/nsViewManager.cpp:1369
#13 0x406fc034 in nsRefreshDriver::Notify (this=0x21b4c90, aTimer=0x2327b48) at /hack/b2g/B2G/gecko/layout/base/nsRefreshDriver.cpp:421
#14 0x418aa8f2 in nsTimerImpl::Fire (this=0x2327b48) at /hack/b2g/B2G/gecko/xpcom/threads/nsTimerImpl.cpp:476
#15 0x418aacae in nsTimerEvent::Run (this=0x1f500b0) at /hack/b2g/B2G/gecko/xpcom/threads/nsTimerImpl.cpp:556
#16 0x418a4c1a in nsThread::ProcessNextEvent (this=0x1f49c20, mayWait=false, result=0xbee5ee07) at /hack/b2g/B2G/gecko/xpcom/threads/nsThread.cpp:624
#17 0x4184a1ca in NS_ProcessNextEvent_P (thread=0x1f49c20, mayWait=false) at /hack/b2g/B2G/objdir-gecko/xpcom/build/nsThreadUtils.cpp:220
#18 0x416949ee in mozilla::ipc::MessagePump::Run (this=0x1f3e370, aDelegate=0xbee5f90c) at /hack/b2g/B2G/gecko/ipc/glue/MessagePump.cpp:82
#19 0x41694e6a in mozilla::ipc::MessagePumpForChildProcess::Run (this=0x1f3e370, aDelegate=0xbee5f90c) at /hack/b2g/B2G/gecko/ipc/glue/MessagePump.cpp:211
#20 0x418f2e60 in MessageLoop::RunInternal (this=0xbee5f90c) at /hack/b2g/B2G/gecko/ipc/chromium/src/base/message_loop.cc:208
#21 0x418f2dfa in MessageLoop::RunHandler (this=0xbee5f90c) at /hack/b2g/B2G/gecko/ipc/chromium/src/base/message_loop.cc:201
#22 0x418f2da2 in MessageLoop::Run (this=0xbee5f90c) at /hack/b2g/B2G/gecko/ipc/chromium/src/base/message_loop.cc:175
#23 0x4156581e in nsBaseAppShell::Run (this=0x2129150) at /hack/b2g/B2G/gecko/widget/xpwidgets/nsBaseAppShell.cpp:163
#24 0x404133c6 in XRE_RunAppShell () at /hack/b2g/B2G/gecko/toolkit/xre/nsEmbedFunctions.cpp:646
---Type <return> to continue, or q <return> to quit---
#25 0x41694dc4 in mozilla::ipc::MessagePumpForChildProcess::Run (this=0x1f3e370, aDelegate=0xbee5f90c) at /hack/b2g/B2G/gecko/ipc/glue/MessagePump.cpp:197
#26 0x418f2e60 in MessageLoop::RunInternal (this=0xbee5f90c) at /hack/b2g/B2G/gecko/ipc/chromium/src/base/message_loop.cc:208
#27 0x418f2dfa in MessageLoop::RunHandler (this=0xbee5f90c) at /hack/b2g/B2G/gecko/ipc/chromium/src/base/message_loop.cc:201
#28 0x418f2da2 in MessageLoop::Run (this=0xbee5f90c) at /hack/b2g/B2G/gecko/ipc/chromium/src/base/message_loop.cc:175
#29 0x40412e7c in XRE_InitChildProcess (aArgc=3, aArgv=0xbee5fa94, aProcess=GeckoProcessType_Content) at /hack/b2g/B2G/gecko/toolkit/xre/nsEmbedFunctions.cpp:485
#30 0x000085be in main (argc=5, argv=0xbee5fa94) at /hack/b2g/B2G/gecko/ipc/app/MozillaRuntimeMain.cpp:48
Comment 15 Benoit Jacob [:bjacob] (mostly away) 2012-08-20 10:45:32 PDT
(gdb) p readSurf.get()->Stride()
$4 = 2048
(gdb) p mBounds.width
$5 = 500
Comment 16 Benoit Jacob [:bjacob] (mostly away) 2012-08-20 12:12:45 PDT
(gdb) p readSurf.get()->Width()
$7 = 500

Jeff Muizelaar tells me that there is absolutely no guarantee that Stride == 4 * Width so that assertion is wrong. The only guarantee is Stride >= 4 & Width.
Comment 17 Benoit Jacob [:bjacob] (mostly away) 2012-08-20 12:12:56 PDT
*
Comment 18 Benoit Jacob [:bjacob] (mostly away) 2012-08-20 15:34:44 PDT
Normally we would want to force an explicit stride when creating this surface. But I don't know how to do that. The surface here is a AutoOpenSurface (it is the aDestSurface passed to UpdateSurface). I don't know anything about that stuff, or why it would use such a large stride (2048 bytes for width=500 means 48 extra bytes or 12 extra pixels).
Comment 19 Jeff Gilbert [:jgilbert] 2012-08-20 15:35:46 PDT
(In reply to Benoit Jacob [:bjacob] from comment #18)
> Normally we would want to force an explicit stride when creating this
> surface. But I don't know how to do that. The surface here is a
> AutoOpenSurface (it is the aDestSurface passed to UpdateSurface). I don't
> know anything about that stuff, or why it would use such a large stride
> (2048 bytes for width=500 means 48 extra bytes or 12 extra pixels).

Maybe it's reusing a surface that's >= the requested width,height?
Comment 20 Jeff Gilbert [:jgilbert] 2012-08-20 16:07:41 PDT
Created attachment 653567 [details] [diff] [review]
Use temp surfaces with correct stride for reads.

Try this?
Comment 21 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-20 18:28:36 PDT
This seems to be severely bitrotted.
Comment 22 Jeff Gilbert [:jgilbert] 2012-08-20 18:43:27 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #21)
> This seems to be severely bitrotted.

It applies cleanly for me. What problem are you seeing?
Comment 23 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-20 18:46:08 PDT
$ hg tip
changeset:   102864:0900ffbccd14
tag:         tip
user:        Wes Kocher <wkocher@mozilla.com>
date:        Mon Aug 20 16:13:46 2012 -0500
summary:     Bug 784113 - Update Jetpack code being tested against mozilla-central.

$ hg qimport -n webgl https://bugzilla.mozilla.org/attachment.cgi?id=653567
adding webgl to series file
$ hg qpush
applying webgl
patching file gfx/gl/GLContext.cpp
Hunk #2 FAILED at 1992
1 out of 2 hunks FAILED -- saving rejects to file gfx/gl/GLContext.cpp.rej
patching file gfx/gl/GLContext.h
Hunk #1 FAILED at 1379
1 out of 1 hunks FAILED -- saving rejects to file gfx/gl/GLContext.h.rej
patching file gfx/layers/basic/BasicCanvasLayer.cpp
Hunk #2 FAILED at 149
1 out of 2 hunks FAILED -- saving rejects to file gfx/layers/basic/BasicCanvasLayer.cpp.rej
patching file gfx/layers/d3d10/CanvasLayerD3D10.cpp
Hunk #1 FAILED at 152
1 out of 1 hunks FAILED -- saving rejects to file gfx/layers/d3d10/CanvasLayerD3D10.cpp.rej
patching file gfx/layers/d3d9/CanvasLayerD3D9.cpp
Hunk #1 FAILED at 102
1 out of 1 hunks FAILED -- saving rejects to file gfx/layers/d3d9/CanvasLayerD3D9.cpp.rej
patching file gfx/layers/opengl/CanvasLayerOGL.cpp
Hunk #1 FAILED at 201
1 out of 1 hunks FAILED -- saving rejects to file gfx/layers/opengl/CanvasLayerOGL.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh webgl
Comment 24 Jeff Gilbert [:jgilbert] 2012-08-20 20:49:16 PDT
You're applying against inbound, not m-c. Inbound has at least one patch from me that breaks this.
Comment 25 Jeff Gilbert [:jgilbert] 2012-08-20 20:51:53 PDT
Specifically, bug 783663 conflicts with this. I'll de-bitrot it tomorrow assuming bug 783663 survives the merge.
Comment 26 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-20 20:52:01 PDT
Does that make it more or less bitrotted? ;)
Comment 27 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-20 20:52:12 PDT
(mid-aired, oops.)
Comment 28 Benoit Jacob [:bjacob] (mostly away) 2012-08-21 12:54:33 PDT
The new patch works!
Comment 29 Benoit Jacob [:bjacob] (mostly away) 2012-08-21 13:21:56 PDT
Comment on attachment 653567 [details] [diff] [review]
Use temp surfaces with correct stride for reads.

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

::: gfx/layers/basic/BasicCanvasLayer.cpp
@@ +200,5 @@
>      if (currentFramebuffer != mCanvasFramebuffer)
>        mGLContext->fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, mCanvasFramebuffer);
>  
>      // We need to Flush() the surface before modifying it outside of cairo.
> +    readSurf->Flush();

Can you explain the flushes there? What was the conclusion from the discussion of whether flushes have any usefulness? The world will end in finite time, too.
Comment 30 Benoit Jacob [:bjacob] (mostly away) 2012-08-21 13:22:17 PDT
Oh oops, that's a surface flush, not a glFlush. Sorry.
Comment 31 Jeff Gilbert [:jgilbert] 2012-08-21 16:11:23 PDT
Created attachment 653997 [details] [diff] [review]
de-bitrotted

Patch to push.
Comment 33 Ed Morley [:emorley] 2012-08-22 02:45:24 PDT
https://hg.mozilla.org/mozilla-central/rev/854c1028d147

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