960378 - PROCESS-CRASH | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | application crashed [@ mozilla::gl::ReadPixelsIntoImageSurface(mozilla::gl::GLContext*, gfxImageSurface*)] on Android 4.0 Debug

Status: RESOLVED FIXED

Description

[Geoff Brown [:gbrown]]

Android 4.0 *Debug* tests currently only run on Cedar. We are trying to get these tests to pass so that they can be turned on on trunk.

On Cedar, on Android 4.0 Debug, mochitest-gl tests consistently fail with a crash.

https://tbpl.mozilla.org/php/getParsedLog.php?id=33063997&tree=Cedar&full=1#error0

16:34:25     INFO -  6640 INFO TEST-PASS | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | [conformance/textures/texture-size-cube-maps.html] Test passed - 0, 0, 2, 1 should be red
16:34:25     INFO -  6641 INFO TEST-PASS | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | [conformance/textures/texture-size-cube-maps.html] Test passed - 0, 1, 2, 1 should be yellow
16:34:25     INFO -  6642 INFO TEST-PASS | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | [conformance/textures/texture-size-cube-maps.html] Test passed - 0, 0, 2, 1 should be yellow
16:34:25     INFO -  6643 INFO TEST-PASS | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | [conformance/textures/texture-size-cube-maps.html] Test passed - 0, 1, 2, 1 should be magenta
16:34:25     INFO -  INFO | automation.py | Application ran for: 0:08:04.362758
16:34:25     INFO -  INFO | zombiecheck | Reading PID log: /tmp/tmphv3Ni5pidlog
16:34:25     INFO -  /data/anr/traces.txt not found
16:34:26     INFO -  mozcrash INFO | Downloading symbols from: http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/cedar-android-debug/1389761500/fennec-29.0a1.en-US.android-arm.crashreporter-symbols.zip
16:34:41  WARNING -  PROCESS-CRASH | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | application crashed [@ mozilla::gl::ReadPixelsIntoImageSurface(mozilla::gl::GLContext*, gfxImageSurface*)]
16:34:41     INFO -  Crash dump filename: /tmp/tmpkgwgqj/1ea9ac52-abf1-ac60-3a8303db-410e6859.dmp
16:34:41     INFO -  Operating system: Android
16:34:41     INFO -                    0.0.0 Linux 3.2.0+ #2 SMP PREEMPT Thu Nov 29 08:06:57 EST 2012 armv7l pandaboard/pandaboard/pandaboard:4.0.4/IMM76I/5:eng/test-keys
16:34:41     INFO -  CPU: arm
16:34:41     INFO -       2 CPUs
16:34:41     INFO -  Crash reason:  SIGSEGV
16:34:41     INFO -  Crash address: 0x0
16:34:41     INFO -  Thread 13 (crashed)
16:34:41     INFO -   0  libxul.so!mozilla::gl::ReadPixelsIntoImageSurface(mozilla::gl::GLContext*, gfxImageSurface*) [GLReadTexImageHelper.cpp:9ea06b86b739 : 270 + 0x14]
16:34:41     INFO -       r4 = 0x6ac1dd40    r5 = 0x6ccde800    r6 = 0x00000000    r7 = 0x639bf4b4
16:34:41     INFO -       r8 = 0x00000010    r9 = 0x00008367   r10 = 0x000080e1    fp = 0x00000004
16:34:41     INFO -       sp = 0x5d401c98    lr = 0x61fbc89f    pc = 0x61fbce8e
16:34:41     INFO -      Found by: given as instruction pointer in context
16:34:41     INFO -   1  libxul.so!mozilla::gl::ReadScreenIntoImageSurface(mozilla::gl::GLContext*, gfxImageSurface*) [GLReadTexImageHelper.cpp:9ea06b86b739 : 479 + 0x7]
16:34:41     INFO -       r4 = 0x6ccde800    r5 = 0x6ac1dd40    r6 = 0x00000000    r7 = 0x6866be50
16:34:41     INFO -       r8 = 0x00000010    r9 = 0x63c29d0c   r10 = 0x636f9742    fp = 0x00088b88
16:34:41     INFO -       sp = 0x5d401d70    pc = 0x61fbd173
16:34:41     INFO -      Found by: call frame info
16:34:41     INFO -   2  libxul.so!mozilla::gl::SharedSurface_EGLImage::Fence() [SharedSurfaceEGL.cpp:9ea06b86b739 : 184 + 0x7]
16:34:41     INFO -       r4 = 0x6a76a390    r5 = 0x6ac1dd40    r6 = 0x00000000    r7 = 0x6866be50
16:34:41     INFO -       r8 = 0x00000010    r9 = 0x63c29d0c   r10 = 0x636f9742    fp = 0x00088b88
16:34:41     INFO -       sp = 0x5d401d90    pc = 0x61fc2a9f
16:34:41     INFO -      Found by: call frame info
16:34:41     INFO -   3  libxul.so!mozilla::gfx::SurfaceStream_TripleBuffer::SwapProducer(mozilla::gfx::SurfaceFactory*, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&) [SurfaceStream.cpp:9ea06b86b739 : 425 + 0x7]
16:34:41     INFO -       r4 = 0x69732d80    r5 = 0x68868a00    r6 = 0x6a76a398    r7 = 0x6b373218
16:34:41     INFO -       r8 = 0x6b373000    r9 = 0x620051d9   r10 = 0x697c0060    fp = 0x00000001
16:34:41     INFO -       sp = 0x5d401de8    pc = 0x61fc4427
16:34:41     INFO -      Found by: call frame info
16:34:41     INFO -   4  libxul.so!mozilla::gl::GLScreenBuffer::Swap(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&) [GLScreenBuffer.cpp:9ea06b86b739 : 425 + 0x9]
16:34:41     INFO -       r4 = 0x6a13db80    r5 = 0x00000000    r6 = 0x6a76a398    r7 = 0x6b373218
16:34:41     INFO -       r8 = 0x6b373000    r9 = 0x620051d9   r10 = 0x697c0060    fp = 0x00000001
16:34:41     INFO -       sp = 0x5d401e00    pc = 0x61fbeb99
16:34:41     INFO -      Found by: call frame info
16:34:41     INFO -   5  libxul.so!mozilla::WebGLContext::PresentScreenBuffer() [WebGLContext.cpp:9ea06b86b739 : 1189 + 0x9]
16:34:41     INFO -       r4 = 0x6dbefc00    r5 = 0x00000000    r6 = 0x6dbefc50    r7 = 0x6b373218
16:34:41     INFO -       r8 = 0x6b373000    r9 = 0x620051d9   r10 = 0x697c0060    fp = 0x00000001
16:34:41     INFO -       sp = 0x5d401e90    pc = 0x625c7e87
16:34:41     INFO -      Found by: call frame info




16:34:45     INFO -  01-15 16:33:58.226 I/GeckoDump( 2227): 6641 INFO TEST-PASS | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | [conformance/textures/texture-size-cube-maps.html] Test passed - 0, 1, 2, 1 should be yellow
16:34:45     INFO -  01-15 16:33:58.234 I/GeckoDump( 2227): 6642 INFO TEST-PASS | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | [conformance/textures/texture-size-cube-maps.html] Test passed - 0, 0, 2, 1 should be yellow
16:34:45     INFO -  01-15 16:33:58.234 I/GeckoDump( 2227): 6643 INFO TEST-PASS | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | [conformance/textures/texture-size-cube-maps.html] Test passed - 0, 1, 2, 1 should be magenta
16:34:45     INFO -  01-15 16:33:58.304 F/MOZ_Assert( 2227): Assertion failure: dest->Stride() == dest->Width() * destPixelSize, at ../../../gfx/gl/GLReadTexImageHelper.cpp:272
16:34:45     INFO -  01-15 16:33:59.382 F/libc    ( 2227): Fatal signal 11 (SIGSEGV) at 0x00000000 (code=1)
16:34:45     INFO -  01-15 16:33:59.812 I/DEBUG   ( 1290): *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
16:34:45     INFO -  01-15 16:33:59.812 I/DEBUG   ( 1290): Build fingerprint: 'pandaboard/pandaboard/pandaboard:4.0.4/IMM76I/5:eng/test-keys'
16:34:45     INFO -  01-15 16:33:59.812 I/DEBUG   ( 1290): pid: 2227, tid: 2247  >>> org.mozilla.fennec <<<
16:34:45     INFO -  01-15 16:33:59.812 I/DEBUG   ( 1290): signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 00000000
16:34:45     INFO -  01-15 16:33:59.812 I/DEBUG   ( 1290):  r0 00000081  r1 63422360  r2 0000007b  r3 00000000
16:34:45     INFO -  01-15 16:33:59.812 I/DEBUG   ( 1290):  r4 6ac1dd40  r5 6ccde800  r6 00000000  r7 639bf4b4
16:34:45     INFO -  01-15 16:33:59.812 I/DEBUG   ( 1290):  r8 00000010  r9 00008367  10 000080e1  fp 00000004
16:34:45     INFO -  01-15 16:33:59.812 I/DEBUG   ( 1290):  ip 400b4130  sp 5d401c98  lr 61fbc89f  pc 61fbce8e  cpsr 28000130

Comment 1

[Geoff Brown [:gbrown]]

More examples:

https://tbpl.mozilla.org/php/getParsedLog.php?id=33063997&tree=Cedar&full=1
https://tbpl.mozilla.org/php/getParsedLog.php?id=33064019&tree=Cedar&full=1
https://tbpl.mozilla.org/php/getParsedLog.php?id=32828669&tree=Cedar&full=1
https://tbpl.mozilla.org/php/getParsedLog.php?id=32789284&tree=Cedar&full=1

Comment 2

[Milan Sreckovic [:milan]]

CJ, we deemed this high priority to look at soon - can you get somebody to assign to this?

Comment 3

[C.J. Ku[:cjku](UTC+8)]

(In reply to Milan Sreckovic [:milan] from comment #2)
> CJ, we deemed this high priority to look at soon - can you get somebody to
> assign to this?

Milan, we will have a Chinese New Year vacation between Jan 30th and Feb 4th. Which means we can start working on this fix after Feb 4th. If you think it's ok, please assign to me, depend on internal resource, I will find correct one to work on this.

Comment 4

[Milan Sreckovic [:milan]]

That timing should be OK.

Comment 5

[Morris Tseng [:mtseng] [:Morris]]

Created attachment 8375254 [details] [diff] [review]
remove-assert

Sometimes we'll create SourceSurface with stride 16 ( http://dxr.mozilla.org/mozilla-central/source/gfx/2d/SourceSurfaceRawData.cpp#32 ), and this kind of SourceSurface will hit assertion that causes crash. But I think although stride doesn't match to width multiply destPixelSize, We still can read it through tempSurf.

Comment 6

[Jeff Gilbert [:jgilbert]]

Comment on attachment 8375254 [details] [diff] [review]
remove-assert

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

The code below relies on this assert.

Comment 7

[Jeff Gilbert [:jgilbert]]

Created attachment 8375282 [details] [diff] [review]
read-overstrided-surfs

This should allow for reading into non-tightly-packed surfaces.

Morris: Does this fix the issue?

Comment 8

[Jeff Gilbert [:jgilbert]]

Comment on attachment 8375282 [details] [diff] [review]
read-overstrided-surfs

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

::: gfx/gl/GLReadTexImageHelper.cpp
@@ +235,5 @@
> +static int
> +CalcStride(int width, int pixelSize, int alignment)
> +{
> +    MOZ_ASSERT(alignment);
> +    

Oops, more WS.

Comment 9

[Morris Tseng [:mtseng] [:Morris]]

Comment on attachment 8375282 [details] [diff] [review]
read-overstrided-surfs

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

This patch works for me.

Comment 10

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

Comment on attachment 8375282 [details] [diff] [review]
read-overstrided-surfs

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

::: gfx/gl/GLReadTexImageHelper.cpp
@@ +240,5 @@
> +    int stride = width * pixelSize;
> +    if (stride % alignment) { // Extra at the end of the line?
> +        int alignmentCount = stride / alignment;
> +        stride = (alignmentCount+1) * alignment;
> +    }

This function is not needed, if the next comment is correct.

If you decide to keep it, you could at least making it much faster by asserting that alignment is a POT and then replacing expensive / and % by bit ops.

@@ +248,5 @@
> +static int
> +GuessAlignment(int width, int pixelSize, int stride)
> +{
> +    int alignment = 8; // Max GLES allows.
> +    while (CalcStride(width, pixelSize, alignment) != stride) {

This function could be written more simply as:

int padding = stride - width * pixelSize;
return 1
       + 1 * ((padding % 2) == 0)
       + 2 * ((padding % 4) == 0)
       + 4 * ((padding % 8) == 0);

The % here are cheap because the RHS is POT.

Right?

(you'd probably want to assert that padding >= 0, too).

Comment 11

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

... or if you prefer a straightforward implementation because this is not so performance sensitive:

int padding = stride - width * pixelSize;
return ((padding % 8) == 0) ? 8
     : ((padding % 4) == 0) ? 4
     : ((padding % 2) == 0) ? 2
     : 1;

Comment 12

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

... in fact the latter is probably uniformly better, because compilers are smart enough, and this is definitely not that performance sensitive anyway.

Comment 13

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

Oh... nevermind my above comments, I was totally wrong :-)

Go with what you have and maybe just consider the above-mentioned optimization of assuming that alignment is POT (which it is) to avoid expensive arithmetic.

Comment 14

[Jeff Gilbert [:jgilbert]]

I would rather keep this obvious and 'expensive', because any time here will be dwarfed by the time the ReadPixels takes. This call should only happen about once per frame, so we'd only incur this cost on the order of a hundred times a second.

Comment 15

[Jeff Gilbert [:jgilbert]]

This is an issue caused by bug 892505.

Comment 16

[Jeff Gilbert [:jgilbert]]

Created attachment 8375870 [details] [diff] [review]
read-overstrided-surfs

r=bjacob
Fixed whitespace.

Comment 17

[Jeff Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d57e7e2dfe4

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5d57e7e2dfe4