782785 - Compositing of WebGL canvases on B2G is broken unless canvas width is a multiple of 32

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

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)]

Note that bug 782786 can also explain WebGL canvas compositing failing in B2G.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

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)]

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]

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

[Kelsey Gilbert [:jgilbert]]

Attachment: 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)]

(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)]

Works!

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

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)]

https://tbpl.mozilla.org/?tree=Try&rev=ea30d062ac54

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

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)]

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]

I have not seen that problem.  You should drop by #b2g.

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

Filed bug 784085 with a patch by jrmuizel that fixes it.

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

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)]

(gdb) p readSurf.get()->Stride()
$4 = 2048
(gdb) p mBounds.width
$5 = 500

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

(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)]

*

Comment 18

[Benoit Jacob [:bjacob] (mostly away)]

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

[Kelsey Gilbert [:jgilbert]]

(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

[Kelsey Gilbert [:jgilbert]]

Attachment: Use temp surfaces with correct stride for reads.

Try this?

Comment 21

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

This seems to be severely bitrotted.

Comment 22

[Kelsey Gilbert [:jgilbert]]

(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]

$ hg tip
changeset:   102864:0900ffbccd14
tag:         tip
user:        Wes Kocher <[e-mail]>
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

[Kelsey Gilbert [:jgilbert]]

You're applying against inbound, not m-c. Inbound has at least one patch from me that breaks this.

Comment 25

[Kelsey Gilbert [:jgilbert]]

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]

Does that make it more or less bitrotted? ;)

Comment 27

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

(mid-aired, oops.)

Comment 28

[Benoit Jacob [:bjacob] (mostly away)]

The new patch works!

Comment 29

[Benoit Jacob [:bjacob] (mostly away)]

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)]

Oh oops, that's a surface flush, not a glFlush. Sorry.

Comment 31

[Kelsey Gilbert [:jgilbert]]

Attachment: de-bitrotted

Patch to push.

Comment 32

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/854c1028d147

Comment 33

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/854c1028d147