With the patch for bug 1001687 I'm encountering a Linux/OS X only failure in content/canvas/test/webgl-conformance/test_webgl_conformance_test_suite.html (mochi.test:8888/tests/content/canvas/test/webgl-conformance/conformance/textures/gl-pixelstorei.html, more specifically). In debug builds we fail hitting the assertion under: mozilla::gl::GuessAlignment mozilla::gl::ReadPixelsIntoDataSurface mozilla::WebGLContext::GetSurfaceSnapshot mozilla::dom::HTMLCanvasElement::GetSurfaceSnapshot http://mxr.mozilla.org/mozilla-central/source/gfx/gl/GLReadTexImageHelper.cpp#318 because GuessAlignment assumes the maximum alignment is 8 bytes since that's the maximum that GLES allows, but in fact the stride is 208 bytes, which is 8 bytes longer than the bytes required for each row of pixels. Specifically in GuessAlignment we have: width == 50 pixelSize == 4 stride == 208 This is consistent with 16-byte alignment, and according to Matt was chosen for Moz2D DataSourceSurface because Bas wanted 16-byte alignment to get better vectorization. Since ReadPixelsIntoDataSurface requires 8-byte alignment Matt suggested adding API to allow Moz2D consumers to request a DataSourceSurface with that alignment for these sorts of cases.
I skimmed what Matt wrote a bit quickly. We have a mechanism for getting a DataSourceSurface with a specific alignment via Factory::CreateDataSourceSurfaceWithStride. What we need is a way to determine whether an existing DataSourceSurface has the alignment we need or not. We can sort of do that using Map() to get access to the stride and calculate the alignment from there, but Bas has explicitly said in the past that Map() is not guaranteed to produce the same stride from call to call on the same DataSourceSurface. That said, it seems like it probably always will. Bas, what are your thoughts on this?
The ReadPixelsIntoImageSurface function already manages to test the alignment of a mapped data source surface. Maybe it should be able to create a new temporary surface if that stride isn't correct and readback into that. Then it can produce lots of noisy assertions while copying from the temporary into the real data source surface. We should then make sure the callers do their absolute best to allocate an 8byte aligned surface so that we never actually hit said assertions.
(In reply to Matt Woodrow (:mattwoodrow) from comment #2) > The ReadPixelsIntoImageSurface function already manages to test the > alignment of a mapped data source surface. Maybe it should be able to create > a new temporary surface if that stride isn't correct and readback into that. > Then it can produce lots of noisy assertions while copying from the > temporary into the real data source surface. That doesn't sound unreasonable. > We should then make sure the callers do their absolute best to allocate an > 8byte aligned surface so that we never actually hit said assertions. Actually, that depends, a 16-byte aligned surface will be significantly faster, so if the callers want to do a lot with it, it's probably better to have 16-byte aligned. Note that on OS X and Linux, OpenGL should be able to upload with different strides as well! And we should use that. Map() should not be assumed to return the same stride repeatedly, it's a fundamentally bad assumption to make.
Assignee: nobody → jgilbert
Attachment #8414906 - Flags: review?(matt.woodrow)
Comment on attachment 8414906 [details] [diff] [review] row-stride Review of attachment 8414906 [details] [diff] [review]: ----------------------------------------------------------------- I assume the changes to all files except GLReadTexImageHelper weren't meant to be part of this patch, they don't seem related. I'd quite like if we could make it an NS_WARNING when we GuessAlignment returns 0. As Bas said, a 16byte aligned surface might make more sense for some callers, but a lot of the callers will be creating a surface explicitly for a single readback and silently incurring an extra copy would be sad. r=me with those changes.
Attachment #8414906 - Flags: review?(matt.woodrow) → review+
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.