Closed Bug 960378 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Graphics: CanvasWebGL, defect)

Product:
Core
Component:
Graphics: CanvasWebGL
Platform:
x86
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla30

People

(Reporter: gbrown, Assigned: jgilbert)

References

Details

Attachments

(1 file, 2 obsolete files)

read-overstrided-surfs
7.20 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
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
Blocks: 940080
CJ, we deemed this high priority to look at soon - can you get somebody to assign to this?
Flags: needinfo?(cku)
OS: Mac OS X → Android
(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.
Flags: needinfo?(cku)
That timing should be OK.
Assignee: nobody → cku
Assignee: cku → mtseng
Attached patch remove-assert (obsolete) — — Splinter Review
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.
Attachment #8375254 - Flags: review?(jgilbert)
Comment on attachment 8375254 [details] [diff] [review] remove-assert Review of attachment 8375254 [details] [diff] [review]: ----------------------------------------------------------------- The code below relies on this assert.
Attachment #8375254 - Flags: review?(jgilbert) → review-
Attached patch read-overstrided-surfs (obsolete) — — Splinter Review
This should allow for reading into non-tightly-packed surfaces. Morris: Does this fix the issue?
Attachment #8375254 - Attachment is obsolete: true
Attachment #8375282 - Flags: review?(bjacob)
Attachment #8375282 - Flags: feedback?(mtseng)
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 on attachment 8375282 [details] [diff] [review] read-overstrided-surfs Review of attachment 8375282 [details] [diff] [review]: ----------------------------------------------------------------- This patch works for me.
Attachment #8375282 - Flags: feedback?(mtseng) → feedback+
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).
Attachment #8375282 - Flags: review?(bjacob) → review+
... 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;
... in fact the latter is probably uniformly better, because compilers are smart enough, and this is definitely not that performance sensitive anyway.
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.
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.
This is an issue caused by bug 892505.
Depends on: 892505
r=bjacob Fixed whitespace.
Assignee: mtseng → jgilbert
Attachment #8375282 - Attachment is obsolete: true
Attachment #8375870 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/5d57e7e2dfe4
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: