Closed Bug 761849 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Graphics, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla16
Tracking Status
firefox13 --- affected
firefox14 --- affected
firefox15 --- fixed
firefox16 --- fixed
firefox-esr10 --- wontfix

People

(Reporter: nical, Assigned: nical)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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)
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)
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
Attachment #630590 - Attachment is obsolete: true
Attachment #635450 - Flags: review?(joe)
Attachment #635450 - Flags: review?(joe) → review+
Keywords: checkin-needed
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
Closed: 8 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+
You need to log in before you can comment on or make changes to this bug.