Closed Bug 1078109 Opened 10 years ago Closed 10 years ago

Severe SVG performance regression from tiled thebes layers on OSX on the IE test drive helicopter dem

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox35 + fixed

People

(Reporter: bzbarsky, Assigned: mattwoodrow)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

[Tracking Requested - why for this release]:

See bug 608495 comment 16.
Blocks: 608495
This is a fun bug.

The compositor thread tile upload seems to be doing swizzling on the cpu before uploading the texture. That delays the unlock of the tile buffer, and the content thread has to allocate a new tile instead.
Attached patch Choose the correct RGBA format (obsolete) — Splinter Review
This matches the logic in UploadImageDataToTexture. If we don't do this then we end up trying to do glTexSubImage2D with a different format to what we used for glTexImage2D and sadness ensues.
Assignee: nobody → matt.woodrow
Attachment #8500216 - Flags: review?(jgilbert)
Might be worth either factoring out the code that needs to match into a helper function or at least having comments both places about it needing to match...
Comment on attachment 8500216 [details] [diff] [review]
Choose the correct RGBA format

I have wrangled with this code path in the driver before. This is righteous and recommended by Apple.
Attachment #8500216 - Flags: review?(jgilbert) → review+
Comment on attachment 8500216 [details] [diff] [review]
Choose the correct RGBA format

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

::: gfx/gl/GLTextureImage.cpp
@@ +235,5 @@
> +    GLenum format;
> +    GLenum type;
> +    if (mGLContext->GetPreferredARGB32Format() == LOCAL_GL_BGRA) {
> +        format = LOCAL_GL_BGRA;
> +        type = LOCAL_GL_UNSIGNED_INT_8_8_8_8_REV;

Assert that this is not GLES.

@@ +237,5 @@
> +    if (mGLContext->GetPreferredARGB32Format() == LOCAL_GL_BGRA) {
> +        format = LOCAL_GL_BGRA;
> +        type = LOCAL_GL_UNSIGNED_INT_8_8_8_8_REV;
> +    } else {
> +        format = LOCAL_GL_BGRA;

This should be RGBA.
Attachment #8500216 - Flags: review+ → review-
Attachment #8500216 - Attachment is obsolete: true
Attachment #8500862 - Flags: review?(jgilbert)
I still stand by comment 3...
(In reply to Boris Zbarsky [:bz] from comment #7)
> I still stand by comment 3...

Yeah, I'm just not sure how to do that in a useful way without changing the TextureImage API significantly.

I can add a comment when I land at least.
Attachment #8500862 - Flags: review?(jgilbert) → review+
https://hg.mozilla.org/mozilla-central/rev/43c963b56723
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1080845
Depends on: 1086642
Depends on: 1110229
You need to log in before you can comment on or make changes to this bug.