Provide API to create DataSourceSurfaces with 8-byte alignment

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jwatt, Assigned: jgilbert)

Tracking

Trunk
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

5 years ago
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.
Reporter

Comment 1

5 years ago
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?
Flags: needinfo?(bas)
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.
Flags: needinfo?(bas)
Assignee

Comment 4

5 years ago
Posted patch row-stride (obsolete) — Splinter Review
Like this?
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+
Assignee

Comment 6

5 years ago
Posted patch row-stride2Splinter Review
r=mattwoodrow
https://tbpl.mozilla.org/?tree=Try&rev=a0d48f47ccc3
Attachment #8414906 - Attachment is obsolete: true
Attachment #8416205 - Flags: review+
Assignee

Comment 7

5 years ago
Looks green.
https://hg.mozilla.org/mozilla-central/rev/b3a88dc52da7
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.