Closed
Bug 978407
Opened 10 years ago
Closed 10 years ago
Do deferred image init with glClear if we can
Categories
(Core :: Graphics: CanvasWebGL, defect)
Core
Graphics: CanvasWebGL
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: jgilbert, Assigned: jgilbert)
Details
(Whiteboard: webgl-correctness webgl-perf)
Attachments
(3 files)
9.58 KB,
patch
|
u480271
:
review+
|
Details | Diff | Splinter Review |
9.34 KB,
patch
|
u480271
:
review+
|
Details | Diff | Splinter Review |
1.76 KB,
patch
|
u480271
:
review+
|
Details | Diff | Splinter Review |
I swear we already have a bug for this somewhere, but I don't see it. Currently, when we need to upload blank data for a texture, we calloc an array of zeros and use TexImage2D to upload it. What I was hitting today was an issue where ANGLE doesn't allow uploads for depth textures via TexImage2D. We still need to init these textures, but we can't use TexImage2D to do it. That's fine though, since we know ANGLE will reliably let us attach depth (and depth-stencil, I think) textures to FBs, without even needing to attach a fake color buffer. So all we need to do is have a different path for ANGLE that attaches the texture to an FB and clear it with glClear. Since this is actually much much faster than callocing and uploading zeros, we should actually try to do this first every time, as our fast path.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8384068 -
Flags: review?(dglastonbury)
Assignee | ||
Comment 2•10 years ago
|
||
Some cleanup of something that was annoying me.
Attachment #8384072 -
Flags: review?(dglastonbury)
Assignee | ||
Comment 3•10 years ago
|
||
This patch seems to be causing a ton of conformance fails under conformance/textures. I'll look into this.
Jeff, do you want me to hold off reviewing 'til you can sort of the test failures?
Flags: needinfo?(jgilbert)
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Dan Glastonbury :djg :kamidphish from comment #4) > Jeff, do you want me to hold off reviewing 'til you can sort of the test > failures? No, I think it can be reviewed already. You might even find the bug by inspection. :P I'll take a look at the failures tomorrow and probably just submit a 'patch 2' if this one looks good on its own.
Flags: needinfo?(jgilbert)
Comment on attachment 8384068 [details] [diff] [review] patch Review of attachment 8384068 [details] [diff] [review]: ----------------------------------------------------------------- r+ conditional on receiving explanation for WebGLTexture.cpp:542. ::: content/canvas/src/WebGLTexture.cpp @@ +538,5 @@ > + bool cleared = ClearWithTempFB(mContext, GLName(), > + imageTarget, level, > + format, imageInfo.mHeight, imageInfo.mWidth); > + if (cleared) > + return; Does this need to change the mImageDataStatus? @@ +555,3 @@ > > + ScopedFreePtr<void> zeros; > + zeros = calloc(1, checked_byteLength.value()); nit: Can this be done in one line? ScopedFreePtr<void> zeros = ... ;
Attachment #8384068 -
Flags: review?(dglastonbury) → review+
Comment on attachment 8384072 [details] [diff] [review] patch 2: Static consts are k-prefixed. Review of attachment 8384072 [details] [diff] [review]: ----------------------------------------------------------------- kk
Attachment #8384072 -
Flags: review?(dglastonbury) → review+
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to Dan Glastonbury :djg :kamidphish from comment #6) > Comment on attachment 8384068 [details] [diff] [review] > patch > > Review of attachment 8384068 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ conditional on receiving explanation for WebGLTexture.cpp:542. > > ::: content/canvas/src/WebGLTexture.cpp > @@ +538,5 @@ > > + bool cleared = ClearWithTempFB(mContext, GLName(), > > + imageTarget, level, > > + format, imageInfo.mHeight, imageInfo.mWidth); > > + if (cleared) > > + return; > > Does this need to change the mImageDataStatus? Yep, patch coming tomorrow. > > @@ +555,3 @@ > > > > + ScopedFreePtr<void> zeros; > > + zeros = calloc(1, checked_byteLength.value()); > > nit: Can this be done in one line? > ScopedFreePtr<void> zeros = ... ; IIRC no, which is why I did this. I'll try again, and file a bug if it doesn't work. (It totally should!)
Assignee | ||
Comment 9•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=c2d2535a3b45 We appear to be crashing on Android.
Assignee | ||
Updated•10 years ago
|
Whiteboard: webgl-correctness webgl-perf
Assignee | ||
Comment 10•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=ab2aaceefd9e
Assignee | ||
Comment 11•10 years ago
|
||
Forgot to mark it as cleared after clearing!
Attachment #8414832 -
Flags: review?(dglastonbury)
Attachment #8414832 -
Flags: review?(dglastonbury) → review+
Assignee | ||
Comment 12•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e7576bf025af remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/af970f26f6ce remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/fd28060279e7
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e7576bf025af https://hg.mozilla.org/mozilla-central/rev/af970f26f6ce https://hg.mozilla.org/mozilla-central/rev/fd28060279e7
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•