Closed Bug 744303 Opened 8 years ago Closed 1 year ago

WebGL texture uploads should use BGRA when possible and desired

Categories

(Core :: Canvas: WebGL, enhancement)

enhancement
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: jgilbert, Assigned: jgilbert)

Details

(Whiteboard: webgl-perf)

Attachments

(1 file, 1 obsolete file)

When we call texImage2d on a DOM element, what we get back is (always?) BGRA data. When supported, we should upload this as BGRA without converting through RGBA.
Attachment #613860 - Flags: review?(bjacob)
Comment on attachment 613860 [details] [diff] [review]
Support for both texImage2d and texSubImage2d

Review of attachment 613860 [details] [diff] [review]:
-----------------------------------------------------------------

Enough questions / nits in here to warrant a second round.

::: content/canvas/src/WebGLContextGL.cpp
@@ +5283,5 @@
>  
> +    if (gl->CanUploadBGRA() &&
> +        actualSrcFormat == WebGLTexelFormat::BGRA8 &&
> +        dstFormat == WebGLTexelFormat::RGBA8)
> +    {

texSubImage2D can't really change the dstFormat, right? Here it seems we have a bug if texSubImage2D tries to change the dstFormat to BGRA but the texture was really RGBA. correct?

if i'm getting it right, since texSubImage2D can't really change the dstFormat, all we want to do is correctly track the (actual, after any tweaks) dstFormat in texture->mImageInfos and if that is BGRA, then tweak dstFormat from RGBA to BGRA.

::: gfx/gl/GLContext.h
@@ +1535,5 @@
>              NS_ASSERTION(index < setlen, "out of range");
>              return values[index];
>          }
>  
> +        bool operator[](size_t index) const {

Return a const bool& here. Otherwise it's too confusing that taking references to array elements works or doesn't work depending on constness (though compilers do warn when taking references to temporaries).

Also, it's up to you but you could implement one of these two variants on top of the other with a const_cast.

Also, these NS_ASSERTIONS should really be crashy assertions like NS_ABORT_IF_FALSE
Attachment #613860 - Flags: review?(bjacob) → review-
(In reply to Benoit Jacob [:bjacob] from comment #2)
> Comment on attachment 613860 [details] [diff] [review]
> Support for both texImage2d and texSubImage2d
> 
> Review of attachment 613860 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Enough questions / nits in here to warrant a second round.
> 
> ::: content/canvas/src/WebGLContextGL.cpp
> @@ +5283,5 @@
> >  
> > +    if (gl->CanUploadBGRA() &&
> > +        actualSrcFormat == WebGLTexelFormat::BGRA8 &&
> > +        dstFormat == WebGLTexelFormat::RGBA8)
> > +    {
> 
> texSubImage2D can't really change the dstFormat, right? Here it seems we
> have a bug if texSubImage2D tries to change the dstFormat to BGRA but the
> texture was really RGBA. correct?
> 
> if i'm getting it right, since texSubImage2D can't really change the
> dstFormat, all we want to do is correctly track the (actual, after any
> tweaks) dstFormat in texture->mImageInfos and if that is BGRA, then tweak
> dstFormat from RGBA to BGRA.

In texSubImage2d, it's only getting format/type, not internalFormat (which governs the internal storage, and could cause the conflict you mention). We are only saying that we're uploading BGRA data to the texture, and letting the GPU take care of the (hopefully no-op) conversion.


> ::: gfx/gl/GLContext.h
> @@ +1535,5 @@
> >              NS_ASSERTION(index < setlen, "out of range");
> >              return values[index];
> >          }
> >  
> > +        bool operator[](size_t index) const {
> 
> Return a const bool& here. Otherwise it's too confusing that taking
> references to array elements works or doesn't work depending on constness
> (though compilers do warn when taking references to temporaries).
> 
> Also, it's up to you but you could implement one of these two variants on
> top of the other with a const_cast.
> 
> Also, these NS_ASSERTIONS should really be crashy assertions like
> NS_ABORT_IF_FALSE

All three of these are probably good. I will add them.
Actually, I don't like the const_cast idea, much as I hate duplicate code.
Attached patch PatchSplinter Review
Attachment #613860 - Attachment is obsolete: true
Attachment #615574 - Flags: review?(bjacob)
Attachment #615574 - Flags: review?(bjacob) → review+
Attachment #615574 - Flags: checkin?(jgilbert)
Attachment #615574 - Flags: checkin?(jgilbert) → checkin+
Sorry, backed out for M1 orange:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=cc37cd8dac59
https://tbpl.mozilla.org/php/getParsedLog.php?id=11196088&tree=Mozilla-Inbound

{
57421 INFO TEST-PASS | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | [conformance/textures/tex-sub-image-2d-bad-args.html] Test passed - successfullyParsed is true
57422 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | [conformance/textures/tex-sub-image-2d-bad-args.html] (WebGL test error) 1 failure(s) and 0 timeout(s)
}

https://hg.mozilla.org/integration/mozilla-inbound/rev/049d28694d26
Target Milestone: mozilla15 → ---
Comment on attachment 615574 [details] [diff] [review]
Patch

M1 orange on inbound?
Attachment #615574 - Flags: checkin+ → feedback-
Mochitest failures are from us recording that we're uploading with |format = BGRA|, and later in TexSubImage2D throwing because it's trying to ostensibly upload RGBA. (INVALID_OPERATION is generated when Tex(Sub)Image2D's |format| doesn't match the TexImage2D's |internalFormat| in GLES, which we inherit down into WebGL)

We need to be careful on GLES: TexSubImage2D's |format| must match the original TexImage2D's |internalFormat|. We can still do BGRA uploads on GLES if we convert any later TexSubImage2D's format and data to BGRA. This means we need to keep track of what the upload format of a texture is, separate from the format we pretend it is for the purpose of WebGL.

Also, CopyTexSubImage2D only requires that the read framebuffer is bound to a source containing the superset of the channels specified by the format of the target texture.

On the plus side, WebGL can't query what internalFormat a texture has, so we're safe from having to shadow and catch that.
Whiteboard: webgl-perf

I think we should hold out for the future external texture extension for webgl.

Status: ASSIGNED → RESOLVED
Type: defect → enhancement
Closed: 1 year ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.