Last Comment Bug 761849 - GLTexture upload forces RGBA format on desktop, causing performance problems with video and accelerated layers.
: GLTexture upload forces RGBA format on desktop, causing performance problems ...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla16
Assigned To: Nicolas Silva [:nical]
:
:
Mentors:
Depends on:
Blocks: 598868
  Show dependency treegraph
 
Reported: 2012-06-05 16:33 PDT by Nicolas Silva [:nical]
Modified: 2012-09-13 16:34 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
affected
fixed
fixed
wontfix


Attachments
Removes the line that enforces GL_RGBA format (995 bytes, patch)
2012-06-05 16:45 PDT, Nicolas Silva [:nical]
no flags Details | Diff | Splinter Review
(temporary fix) Do not force GL_RGBA on non-GLES setups. (948 bytes, patch)
2012-06-06 09:19 PDT, Nicolas Silva [:nical]
joe: review-
matt.woodrow: feedback+
Details | Diff | Splinter Review
Remove internalFormat (2.23 KB, patch)
2012-06-21 14:01 PDT, Nicolas Silva [:nical]
joe: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Nicolas Silva [:nical] 2012-06-05 16:33:24 PDT
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.
Comment 1 Nicolas Silva [:nical] 2012-06-05 16:45:20 PDT
Created attachment 630365 [details] [diff] [review]
Removes the line that enforces GL_RGBA format
Comment 2 Jeff Muizelaar [:jrmuizel] 2012-06-05 18:21:29 PDT
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 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-06 01:46:44 PDT
Comment on attachment 630365 [details] [diff] [review]
Removes the line that enforces GL_RGBA format

Seconding jrmuizel's question.
Comment 4 Nicolas Silva [:nical] 2012-06-06 09:19:34 PDT
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).
Comment 5 Jeff Gilbert [:jgilbert] 2012-06-06 21:10:52 PDT
(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.
Comment 6 Jeff Gilbert [:jgilbert] 2012-06-06 21:12:51 PDT
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 7 Matt Woodrow (:mattwoodrow) 2012-06-06 21:29:56 PDT
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.
Comment 8 Benoit Girard (:BenWa) 2012-06-14 13:08:26 PDT
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!
Comment 9 Benoit Girard (:BenWa) 2012-06-14 13:11:34 PDT
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.
Comment 10 Joe Drew (not getting mail) 2012-06-14 13:15:43 PDT
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. :)
Comment 11 Jeff Gilbert [:jgilbert] 2012-06-14 15:10:15 PDT
(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.
Comment 12 Jeff Gilbert [:jgilbert] 2012-06-14 15:12:25 PDT
Oh, oops, got this confused with another bug. Disregard. :P
Comment 13 Nicolas Silva [:nical] 2012-06-21 14:01:58 PDT
Created attachment 635450 [details] [diff] [review]
Remove internalFormat
Comment 14 Benoit Girard (:BenWa) 2012-06-22 12:41:01 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/68a0e956ed24

I typo'd the commit message :(
Comment 15 Benoit Girard (:BenWa) 2012-06-22 12:52:03 PDT
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!
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-06-23 05:46:00 PDT
https://hg.mozilla.org/mozilla-central/rev/68a0e956ed24
Comment 17 Alex Keybl [:akeybl] 2012-06-24 13:28:04 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.