GLTexture upload forces RGBA format on desktop, causing performance problems with video and accelerated layers.

RESOLVED FIXED in Firefox 15

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nical, Assigned: nical)

Tracking

unspecified
mozilla16
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox13 affected, firefox14 affected, firefox15 fixed, firefox16 fixed, firefox-esr10 wontfix)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
When we call GLContext::UploadSurfaceToTexture, the texture format is ignored when we are not using gl ES and GL_RGBA is enforced.

see: http://mxr.mozilla.org/mozilla-central/source/gfx/gl/GLContext.cpp#2301
internalformat = mIsGLES2 ? format : LOCAL_GL_RGBA;

As a result, when we upload a GL_LUMINANCE texture, the upload takes very long. It slows down video a lot, since for each frame we upload the Y, Cb and Cr in separate GL_LUMINANCE textures.

I don't know why we are enforcing GL_RGBA. I hope we can just fix this by removing the line shown above.
(Assignee)

Comment 1

5 years ago
Created attachment 630365 [details] [diff] [review]
Removes the line that enforces GL_RGBA format
Attachment #630365 - Flags: review?(jones.chris.g)
Comment on attachment 630365 [details] [diff] [review]
Removes the line that enforces GL_RGBA format

Why was this line added? (You should be able to check the blame.)
Comment on attachment 630365 [details] [diff] [review]
Removes the line that enforces GL_RGBA format

Seconding jrmuizel's question.
Attachment #630365 - Flags: review?(jones.chris.g)
(Assignee)

Comment 4

5 years ago
Created attachment 630590 [details] [diff] [review]
(temporary fix) Do not force GL_RGBA on non-GLES setups.

Sorry, I uploaded the patch as a quick'n dirty temporary fix because it affects async video a lot and I want to make available some kind of workaround in case someone wants to try the async video work in progress.

By the way removing the line was definitely not the right thing to do. What we (may) need is rather always use the passed format as internalFormat.

I know this is likely not to be the final solution (since I don't yet know why are forcing RGBA in the first place).

I cc'd Matt Woodrow who implemented the function (according to hg log).
Attachment #630365 - Attachment is obsolete: true
Attachment #630590 - Flags: review?
Attachment #630590 - Flags: feedback?(matt.woodrow)
(Assignee)

Updated

5 years ago
Attachment #630590 - Flags: review?
(In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> Comment on attachment 630365 [details] [diff] [review]
> Removes the line that enforces GL_RGBA format
> 
> Why was this line added? (You should be able to check the blame.)

It was actually added not to force desktop to only RGBA, but because GLES does not support doing texture conversion-on-upload with differing internalFormat and format. That is, in GLES, internalFormat must equal format.

We should be fine uploading with whatever format we want on desktop, though.

I will note that the case for |ImageFormatA8| looks wrong, since it uploads with LOCAL_GL_LUMINANCE instead of LOCAL_GL_ALPHA. It's also possible ImageFormatA8 is misnamed and should really be ImageFormatR8, though.
Or at least, that's why it seems to me it would have been added, when on desktop we may have been trying to take advantage of using RGBA everywhere, perhaps for performance reasons? (Though it seems like it would be worse to use RGBA for everything, even on GPUs)
Comment on attachment 630590 [details] [diff] [review]
(temporary fix) Do not force GL_RGBA on non-GLES setups.

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

I don't remember why this was done like this, nor can I think of a good reason to continue doing so.

We really don't want to be doing texture format conversion during upload (since we've measure it before to be much slower).

The ImageFormatA8 -> LOCAL_GL_LUMINANCE part is intentional, because we use ImageFormatA8 surfaces to upload YUV image frames (see UploadYUVToTexture in ImageLayerOGL.cpp). I don't think we can easily rename ImageFormatA8, or add a new enum option since these just mirror the cairo internal enums.
Attachment #630590 - Flags: feedback?(matt.woodrow) → feedback+
Comment on attachment 630590 [details] [diff] [review]
(temporary fix) Do not force GL_RGBA on non-GLES setups.

Patch looks good to me. I don't see a use in waiting to land this as RGBA is simply terrible for video. In fact we should see if the performance is better on OSX and uplift!
Attachment #630590 - Flags: review?(matt.woodrow)
Comment on attachment 630590 [details] [diff] [review]
(temporary fix) Do not force GL_RGBA on non-GLES setups.

Moving the review since matt is away.
Attachment #630590 - Flags: review?(matt.woodrow) → review?(joe)
Comment on attachment 630590 [details] [diff] [review]
(temporary fix) Do not force GL_RGBA on non-GLES setups.

Remove internalformat altogether; just use format where internalformat is used. :)
Attachment #630590 - Flags: review?(joe) → review-
(In reply to Joe Drew (:JOEDREW!) from comment #10)
> Comment on attachment 630590 [details] [diff] [review]
> (temporary fix) Do not force GL_RGBA on non-GLES setups.
> 
> Remove internalformat altogether; just use format where internalformat is
> used. :)

Desktop GL doesn't accept an internalFormat of BGRA, only RGBA. Unfortunately, GLES requires internalFormat and format to match, it has to look like:
Desktop GL: internalFormat = RGBA, format = BGRA.
GLES: internalFormat = format = BGRA.
Oh, oops, got this confused with another bug. Disregard. :P
(Assignee)

Comment 13

5 years ago
Created attachment 635450 [details] [diff] [review]
Remove internalFormat
Attachment #630590 - Attachment is obsolete: true
Attachment #635450 - Flags: review?(joe)
Attachment #635450 - Flags: review?(joe) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/68a0e956ed24

I typo'd the commit message :(

Updated

5 years ago
Keywords: checkin-needed
Target Milestone: --- → mozilla16
Comment on attachment 635450 [details] [diff] [review]
Remove internalFormat

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Not a regression
User impact if declined: Worse performance playing video on Mac
Testing completed (on m-c, etc.): m-c and extensive testing locally while building OMTC direct video.
Risk to taking this patch (and alternatives if risky): Very low
String or UUID changes made by this patch: none

This patch is simple, low risk and makes our video play back performance better. I vote that we ship it earlier!
Attachment #635450 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/68a0e956ed24
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Comment on attachment 635450 [details] [diff] [review]
Remove internalFormat

[Triage Comment]
Low risk performance win. If this causes any new issues, we'll just back out immediately.
Attachment #635450 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/23cdee264e3f
status-firefox-esr10: --- → affected
status-firefox13: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → fixed
status-firefox16: --- → fixed

Updated

5 years ago
status-firefox-esr10: affected → wontfix
You need to log in before you can comment on or make changes to this bug.