Closed
Bug 987845
Opened 11 years ago
Closed 11 years ago
WebGL context MakeCurrent always unconditionally calls into eglGetError().
Categories
(Core :: Graphics: CanvasWebGL, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: jujjyl, Assigned: jujjyl)
Details
(Whiteboard: [games])
Attachments
(1 file)
(deleted),
patch
|
vlad
:
review+
bjacob
:
review+
jgilbert
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
With the help of ARM Mali guys at GDC and bjacob, we found out that the function GLContextEGL::MakeCurrentImpl in GLContextProviderEGL.cpp always calls eglGetError() after each operation, which shows up in api trace tools as a eglGetError() function call after each GL function. Looking at the code, this seems to be accidental and not always necessary.
Assignee | ||
Updated•11 years ago
|
Whiteboard: [games]
Assignee | ||
Comment 1•11 years ago
|
||
Move the check to inside scope to avoid always calling to eglGetError().
Attachment #8396534 -
Flags: review?(bjacob)
Attachment #8396534 -
Flags: review?(bjacob) → review+
Comment 2•11 years ago
|
||
Comment on attachment 8396534 [details] [diff] [review]
987845-avoid-eglgeterror.patch
Review of attachment 8396534 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks for catching this!
You will want to push this to try before landing, because calling getError has the side effect of clearing the error flag, so if you're sufficiently unlucky you might get a test failure.
Once this is landed, we'll probably want to backport this at least to aurora and B2G 1.4.
Attachment #8396534 -
Flags: review+
Assignee | ||
Comment 3•11 years ago
|
||
Ok, I'm blocked on this for a while, since this is actually my first Firefox-side patch and I'm now waiting to get a SSH public key added via servicedesk. I'll do a try once I've got the key set up.
Comment 4•11 years ago
|
||
Comment 5•11 years ago
|
||
Comment on attachment 8396534 [details] [diff] [review]
987845-avoid-eglgeterror.patch
Review of attachment 8396534 [details] [diff] [review]:
-----------------------------------------------------------------
I want to r+ this too, because that was really silly of us.
Attachment #8396534 -
Flags: review+
Comment 6•11 years ago
|
||
Anyone else wants to r+ before we land?
Try is green, and, what is more remarkable, inbound is currently open...
Updated•11 years ago
|
Assignee: nobody → jujjyl
Comment 7•11 years ago
|
||
Comment 8•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment 9•11 years ago
|
||
Comment on attachment 8396534 [details] [diff] [review]
987845-avoid-eglgeterror.patch
[Approval Request Comment]
Bug caused by (feature/regressing bug #): Not sure; may not be a regression.
User impact if declined: Unnecessary graphics inefficiency (performance problems, depending on driver)
Testing completed (on m-c, etc.): m-c for a couple of days.
Risk to taking this patch (and alternatives if risky): Zero risk, trivial patch.
String or IDL/UUID changes made by this patch: none
Attachment #8396534 -
Flags: approval-mozilla-aurora?
Updated•11 years ago
|
Attachment #8396534 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 10•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/27349a797425
Lukas: I think that this should be taken in B2G 1.4. How do I achieve that? (I don't see a tracking-b2g-v1.4 flag).
status-b2g-v1.4:
--- → affected
status-firefox30:
--- → fixed
status-firefox31:
--- → fixed
Flags: needinfo?(lsblakk)
Comment 11•11 years ago
|
||
Aurora = v1.4
You need to log in
before you can comment on or make changes to this bug.
Description
•