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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Nicolas Silva [:nical]]

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]]

Attachment: Removes the line that enforces GL_RGBA format

Comment 2

[Jeff Muizelaar [:jrmuizel]]

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]

Comment on attachment 630365 [details] [diff] [review]
Removes the line that enforces GL_RGBA format

Seconding jrmuizel's question.

Comment 4

[Nicolas Silva [:nical]]

Attachment: (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

[Kelsey Gilbert [:jgilbert]]

(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

[Kelsey Gilbert [:jgilbert]]

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)]

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)]

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)]

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)]

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

[Kelsey Gilbert [:jgilbert]]

(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

[Kelsey Gilbert [:jgilbert]]

Oh, oops, got this confused with another bug. Disregard. :P

Comment 13

[Nicolas Silva [:nical]]

Attachment: Remove internalFormat

Comment 14

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/68a0e956ed24

I typo'd the commit message :(

Comment 15

[Benoit Girard (:BenWa)]

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]]

https://hg.mozilla.org/mozilla-central/rev/68a0e956ed24

Comment 17

[Alex Keybl [:akeybl]]

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.

Comment 18

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/23cdee264e3f