Closed Bug 978407 Opened 6 years ago Closed 6 years ago

Do deferred image init with glClear if we can

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: jgilbert, Assigned: jgilbert)

Details

(Whiteboard: webgl-correctness webgl-perf)

Attachments

(3 files)

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.
Attached patch patchSplinter Review
Attachment #8384068 - Flags: review?(dglastonbury)
Some cleanup of something that was annoying me.
Attachment #8384072 - Flags: review?(dglastonbury)
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)
(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+
(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!)
https://tbpl.mozilla.org/?tree=Try&rev=c2d2535a3b45

We appear to be crashing on Android.
Whiteboard: webgl-correctness webgl-perf
Forgot to mark it as cleared after clearing!
Attachment #8414832 - Flags: review?(dglastonbury)
Attachment #8414832 - Flags: review?(dglastonbury) → review+
You need to log in before you can comment on or make changes to this bug.