Closed Bug 1003607 Opened 10 years ago Closed 9 years ago

Header animation at acko.net is broken in FF 29 and above.

Categories

(Core :: Graphics: CanvasWebGL, defect)

29 Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox29 --- wontfix
firefox30 + wontfix
firefox31 + fixed
firefox32 --- fixed

People

(Reporter: u480271, Assigned: u480271)

References

()

Details

(Keywords: dev-doc-complete, regression, site-compat)

Attachments

(7 files, 7 obsolete files)

9.92 KB, text/html
Details
3.68 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
48.74 KB, patch
u480271
: review+
Details | Diff | Splinter Review
3.31 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
53.80 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
31.39 KB, patch
jgilbert
: review-
Details | Diff | Splinter Review
6.94 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
WebGL based animation in header at acko.net doesn't appear in FF 29 or later.  Error console contains:

Error: WebGL: texSubImage2D: format or type doesn't match the existing texture combo.min.js:722
Error: WebGL: texSubImage2D: format or type doesn't match the existing texture combo.min.js:722
Error: WebGL: No further warnings will be reported for this WebGL context (already reported 32 warnings) combo.min.js:722
Attached file testcase.html
On MacOSX, the internal format is GL_RGBA32F which is being checked
against GL_RGBA. We need to do this better!
Attachment #8414950 - Flags: review?(jgilbert)
Blocks: 1000443
(In reply to Dan Glastonbury :djg :kamidphish from comment #3)
> https://tbpl.mozilla.org/?tree=Try&rev=c8db897a4149

Aww, S3TC is broken. Time to do this properly.
When I refactored TexImageInfo::Format to
TexImageInfo::InternalFormat, I never fully groked the
consequences. We've been patching around this for a while now.

This patch is a stab at undoing the mess. I've separated format and
type into two categories: WebGL and Driver.

TexImageInfo and all validation code works on WebGL format and
type. These are the valid values defined by WebGL/GL ES. Before
sending the format and type to the driver, via GLContext, the values
are translated to what the driver wants. This is mostly for Open GL,
which has different rules to GL ES. There were a number of places that
translation was taking place. I hope to collate all these into one
place, DriverFormatsForFormatAndType() and
DriverTypeForFormatAndType(). The result of these functions should be
passed to GLContext calls that take format, internalformat, or type.
Attachment #8415160 - Flags: review?(jgilbert)
Attachment #8415160 - Flags: feedback?(bjacob)
Attachment #8414950 - Attachment is obsolete: true
Attachment #8414950 - Flags: review?(jgilbert)
Attached patch Mochitest (obsolete) — Splinter Review
Mochitest to catch regression of texSubImage with float type.
Attachment #8415176 - Flags: review?(jgilbert)
Comment on attachment 8415160 [details] [diff] [review]
Straighten out usage of GL format and type.

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

A general comment that I've already made on a previous patch --- and one which we might just disagree, in which case :jgilbert shall be the judge --- is I prefer to talk of "OpenGL type/format" than of "driver type/format". We're implementing one spec, WebGL, on top of another spec, OpenGL.

Aside from that the idea seems good and nothing seems bad from a quick skim of the patch, so feedback+; I haven't done a real review, though.
Attachment #8415160 - Flags: feedback?(bjacob) → feedback+
Comment on attachment 8415160 [details] [diff] [review]
Straighten out usage of GL format and type.

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

This is probably r-, unless all my concerns are unfounded.

::: content/canvas/src/WebGLContextGL.cpp
@@ +452,5 @@
>  
>      // copyTexImage2D only generates textures with type = UNSIGNED_BYTE
>      const WebGLTexImageFunc func = WebGLTexImageFunc::CopyTexImage;
> +    const GLenum format = internalformat; // WebGL/ES Format
> +    const GLenum type = LOCAL_GL_UNSIGNED_BYTE; // WebGL/ES Format

This might not be true if the bound FB is a float FB.

@@ +478,5 @@
>      }
>  
> +    bool texFormatRequiresAlpha = format == LOCAL_GL_RGBA ||
> +                                  format == LOCAL_GL_ALPHA ||
> +                                  format == LOCAL_GL_LUMINANCE_ALPHA;

I think we want a helper function for this, because there's also the sRGBA type, right?

@@ +3526,5 @@
>      bool sizeMayChange = true;
>  
>      if (tex->HasImageInfoAt(target, level)) {
>          const WebGLTexture::ImageInfo& imageInfo = tex->ImageInfoAt(target, level);
> +        // Checking WebGLFormat against DriverFormat. Should be OK.

Isn't this wrong for all the Float types? RGBA32F, etc.?

@@ +3663,5 @@
>      }
>  
>      // in all of the code paths above, we should have either initialized data,
>      // or allocated data and left it uninitialized, but in any case we shouldn't
> +     // have NoImageData at this point.

Bad indentation here.

::: content/canvas/src/WebGLContextUtils.cpp
@@ +52,5 @@
> +    // indicated purely by the type that's loaded.  For desktop GL, we
> +    // have to specify a floating point internal format.
> +    if (gl->IsGLES()) {
> +        if (driverFormat)
> +            *driverFormat = webGLFormat;

We should just assert that these outvar pointers are non-null, even if the callsite might not always need them.

@@ +85,5 @@
> +
> +    case LOCAL_GL_FLOAT:
> +        switch (format) {
> +        case LOCAL_GL_RGBA:
> +            internalFormat = LOCAL_GL_RGBA32F_ARB;

Drop the _ARB if the non-_ARB version is available in GLConsts.h.

@@ +139,5 @@
> +    // requires that format == internalformat, but GL will fail in this case.
> +    // GL requires:
> +    //      format  ->  internalformat
> +    //      GL_RGB      GL_SRGB_EXT
> +    //      GL_RGBA     GL_SRGB_ALPHA_EXT

Weird. Do you know if this will mark the uploaded data as linear, and cause the driver to convert it to sRGB-space? I would like to leave a comment here if this is not the case.

@@ +163,5 @@
> +        *driverInternalFormat = internalFormat;
> +}
> +
> +GLenum
> +DriverTypeFromFormatAndType(GLContext* gl, GLenum /*webGLFormat*/, GLenum webGLType)

Are we likely to need to use `format` later? Otherwise, I'd rather just get rid of this arg, instead of just marking it unused.

::: content/canvas/src/WebGLContextUtils.h
@@ +15,5 @@
>  bool IsGLDepthFormat(GLenum internalFormat);
>  bool IsGLDepthStencilFormat(GLenum internalFormat);
>  
> +void DriverFormatsFromFormatAndType(gl::GLContext* gl, GLenum webGLFormat, GLenum webGLType,
> +                                    GLenum* driverFormat, GLenum* driverInternalFormat);

`internalFormat` should be ordered before `format`, IMO, to match how they're used in texImage calls. I also prefer output variable pointers to be prefixed with `out_`, as `out_driverFormat`, since sometimes we use pointers for nullable args. It also makes it more obvious when we write to 'output' in the function definition.

::: content/canvas/src/WebGLTexture.h
@@ +132,3 @@
>      protected:
> +        GLenum mWebGLFormat; //!< This is the WebGL/GLES format
> +        GLenum mWebGLType;   //!< This is the WebGL/GLES type

If these can't be changed, they should be const.
Attachment #8415160 - Flags: review?(jgilbert) → review-
(In reply to Benoit Jacob [:bjacob] from comment #7)
> Comment on attachment 8415160 [details] [diff] [review]
> Straighten out usage of GL format and type.
> 
> Review of attachment 8415160 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A general comment that I've already made on a previous patch --- and one
> which we might just disagree, in which case :jgilbert shall be the judge ---
> is I prefer to talk of "OpenGL type/format" than of "driver type/format".
> We're implementing one spec, WebGL, on top of another spec, OpenGL.
I understand what you mean, but sometimes it's more clear to lie a bit, I think. s/Driver/OpenGL/ is probably fine, but I think Driver is clearer, even if it's not quite true.
Comment on attachment 8415176 [details] [diff] [review]
Mochitest

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

::: content/canvas/test/webgl-mochitest/mochitest.ini
@@ +10,5 @@
>  [test_highp_fs.html]
>  [test_no_arr_points.html]
>  [test_noprog_draw.html]
>  [test_privileged_exts.html]
> +[test_texsubimage_float.html]

You forgot to `hg add` this file!
Attachment #8415176 - Flags: review?(jgilbert) → review-
Attached patch MochitestSplinter Review
Now with added mochitest.
Attachment #8415656 - Flags: review?(jgilbert)
Attachment #8415176 - Attachment is obsolete: true
(In reply to Jeff Gilbert [:jgilbert] from comment #8)
> Comment on attachment 8415160 [details] [diff] [review]
> Straighten out usage of GL format and type.
> 
> Review of attachment 8415160 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is probably r-, unless all my concerns are unfounded.
> 
> ::: content/canvas/src/WebGLContextGL.cpp
> @@ +452,5 @@
> >  
> >      // copyTexImage2D only generates textures with type = UNSIGNED_BYTE
> >      const WebGLTexImageFunc func = WebGLTexImageFunc::CopyTexImage;
> > +    const GLenum format = internalformat; // WebGL/ES Format
> > +    const GLenum type = LOCAL_GL_UNSIGNED_BYTE; // WebGL/ES Format
> 
> This might not be true if the bound FB is a float FB.

This rule was encoded before I made the patch. As discussed on IRC, OES_texture_float bans CopyTexImage with float types, but EXT_color_buffer_float says:

    3.8.5 Alternate Texture Image Specification Commands, p. 138

    In the first paragraph, change the sentence beginning "The error
    INVALID_OPERATION is generated ..." to

 *  The error INVALID_OPERATION is generated if signed integer RGBA
    data is required and the format of the current color buffer is not
    signed integer; if unsigned integer RGBA data is required and the
    format of the current color buffer is not unsigned integer; or if
 *  floating- or fixed-point RGBA data is required and the format of
 *  the current color buffer is signed or unsigned integer.

Will implement this in another patch.
    
> @@ +478,5 @@
> >      }
> >  
> > +    bool texFormatRequiresAlpha = format == LOCAL_GL_RGBA ||
> > +                                  format == LOCAL_GL_ALPHA ||
> > +                                  format == LOCAL_GL_LUMINANCE_ALPHA;
> 
> I think we want a helper function for this, because there's also the sRGBA
> type, right?

Yes, this should be a helper. I almost made it one. There's a simple helper in WebGLTexture.cpp. Should we extract that into WebGLContextUtils.h?

> @@ +3526,5 @@
> >      bool sizeMayChange = true;
> >  
> >      if (tex->HasImageInfoAt(target, level)) {
> >          const WebGLTexture::ImageInfo& imageInfo = tex->ImageInfoAt(target, level);
> > +        // Checking WebGLFormat against DriverFormat. Should be OK.
> 
> Isn't this wrong for all the Float types? RGBA32F, etc.?

I'll pass WebGL format through DriverFormatsXXX() so we can compare "apples to apples".

> ::: content/canvas/src/WebGLContextUtils.cpp
> @@ +52,5 @@
> > +    // indicated purely by the type that's loaded.  For desktop GL, we
> > +    // have to specify a floating point internal format.
> > +    if (gl->IsGLES()) {
> > +        if (driverFormat)
> > +            *driverFormat = webGLFormat;
> 
> We should just assert that these outvar pointers are non-null, even if the
> callsite might not always need them.

OK.

> @@ +85,5 @@
> > +
> > +    case LOCAL_GL_FLOAT:
> > +        switch (format) {
> > +        case LOCAL_GL_RGBA:
> > +            internalFormat = LOCAL_GL_RGBA32F_ARB;
> 
> Drop the _ARB if the non-_ARB version is available in GLConsts.h.

OK

> @@ +139,5 @@
> > +    // requires that format == internalformat, but GL will fail in this case.
> > +    // GL requires:
> > +    //      format  ->  internalformat
> > +    //      GL_RGB      GL_SRGB_EXT
> > +    //      GL_RGBA     GL_SRGB_ALPHA_EXT
> 
> Weird. Do you know if this will mark the uploaded data as linear, and cause
> the driver to convert it to sRGB-space? I would like to leave a comment here
> if this is not the case.

No. It marks the uploaded data as in sRGB space. (The format is RGB[A] but the internal format marks it as sRGB encoded.) GL_RGB[A]/GL_RBG[A] is linear, but nearly all 8-bit textures data is not linearly encoded.

Most cards will do the conversion from sRGB to linear in the texture cache, or the drivers will do conversion on older models. 

> @@ +163,5 @@
> > +        *driverInternalFormat = internalFormat;
> > +}
> > +
> > +GLenum
> > +DriverTypeFromFormatAndType(GLContext* gl, GLenum /*webGLFormat*/, GLenum webGLType)
> 
> Are we likely to need to use `format` later? Otherwise, I'd rather just get
> rid of this arg, instead of just marking it unused.

It was line ball decision. Will remove.

> ::: content/canvas/src/WebGLContextUtils.h
> @@ +15,5 @@
> >  bool IsGLDepthFormat(GLenum internalFormat);
> >  bool IsGLDepthStencilFormat(GLenum internalFormat);
> >  
> > +void DriverFormatsFromFormatAndType(gl::GLContext* gl, GLenum webGLFormat, GLenum webGLType,
> > +                                    GLenum* driverFormat, GLenum* driverInternalFormat);
> 
> `internalFormat` should be ordered before `format`, IMO, to match how
> they're used in texImage calls. I also prefer output variable pointers to be
> prefixed with `out_`, as `out_driverFormat`, since sometimes we use pointers
> for nullable args. It also makes it more obvious when we write to 'output'
> in the function definition.

OK.
Address :jgilbert r-.
Attachment #8415727 - Flags: review?(jgilbert)
Attachment #8415160 - Attachment is obsolete: true
Attachment #8415656 - Flags: review?(jgilbert) → review+
Comment on attachment 8415727 [details] [diff] [review]
Straighten out usage of GL format and type.

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

Nits.

::: content/canvas/src/WebGLContextGL.cpp
@@ +589,5 @@
>      }
>  
> +    bool texFormatRequiresAlpha = (webGLFormat == LOCAL_GL_RGBA ||
> +                                   webGLFormat == LOCAL_GL_ALPHA ||
> +                                   webGLFormat == LOCAL_GL_LUMINANCE_ALPHA);

I still think it's a better idea to just use out FormatHasAlpha (or whatever it's called) helper here, even if it checks more formats than can appear here. At least that way we'll never miss any. (or at least we'll be less likely to, since we only have to change one place.

::: content/canvas/src/WebGLTexture.h
@@ +116,5 @@
>          }
>          int64_t MemoryUsage() const;
> +        //! This is the format passed from JS to WebGL. It can be converted to a value to be passed to driver with XXX.
> +        GLenum WebGLFormat() const { return mWebGLFormat; }
> +        //! This is the type passed from JS to WebGL. It can be converted to a value to be passed to driver with XXX.

Are these 'XXX' placeholders supposed to be copy-pasted over?
Attachment #8415727 - Flags: review?(jgilbert) → review+
https://tbpl.mozilla.org/?tree=Try&rev=d50f88d0789a

Manually tested with: maps.google.com, Sanctuary Demo, acko.net.
Carry r=jgilbert
Attachment #8415727 - Attachment is obsolete: true
Attachment #8416246 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/4774bb540acc
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Dan, could you request uplifts to aurora & beta? thanks
Flags: needinfo?(dglastonbury)
Comment on attachment 8416246 [details] [diff] [review]
Straighten out usage of GL format and type.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 966624
User impact if declined: WebGL incorrectly refused to accept float texture types in gl.texSubImage2D calls.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): This patch has been on m-c for weeks with extra testing introduced with a new mochitest, so I believe the risk is low.
String or IDL/UUID changes made by this patch:
Attachment #8416246 - Flags: approval-mozilla-beta?
Attachment #8416246 - Flags: approval-mozilla-aurora?
Flags: needinfo?(dglastonbury)
Here's another live site you might want to test that's affected by this texSubImage2D regression:

http://onemillionreasons.audi.de/

(select 3d version)
Attachment #8416246 - Flags: approval-mozilla-beta?
Attachment #8416246 - Flags: approval-mozilla-beta+
Attachment #8416246 - Flags: approval-mozilla-aurora?
Attachment #8416246 - Flags: approval-mozilla-aurora+
This needs rebasing for Aurora/Beta uplift.
Flags: needinfo?(dglastonbury)
Attachment #8428501 - Attachment description: Sort out usage of GL format and type into WebGL and driver versions for texture checks. → Beta - Sort out usage of GL format and type into WebGL and driver versions for texture checks.
Flags: needinfo?(dglastonbury)
Comment on attachment 8428502 [details] [diff] [review]
Aurora - Sort out usage of GL format and type into WebGL and driver versions for texture checks.

Carry r=jgilbert.
Attachment #8428502 - Attachment description: Sort out usage of GL format and type into WebGL and driver versions for texture checks. → Aurora - Sort out usage of GL format and type into WebGL and driver versions for texture checks.
Attachment #8428502 - Flags: review+
Comment on attachment 8428501 [details] [diff] [review]
Beta - Sort out usage of GL format and type into WebGL and driver versions for texture checks.

Carry r=jgilbert.
Attachment #8428501 - Flags: review+
Comment on attachment 8428501 [details] [diff] [review]
Beta - Sort out usage of GL format and type into WebGL and driver versions for texture checks.

Don't uplift these! They introduce a bug that allows glTexSubImage2D to accept depth textures. *Mumble mumble*
Attachment #8428501 - Attachment is obsolete: true
Comment on attachment 8428502 [details] [diff] [review]
Aurora - Sort out usage of GL format and type into WebGL and driver versions for texture checks.

Don't uplift these! They introduce a bug that allows glTexSubImage2D to accept depth textures. *Mumble mumble*
Attachment #8428502 - Attachment is obsolete: true
This breaks depth texture checks around TexSubImage.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8428633 - Flags: review?(jgilbert)
Attachment #8428634 - Flags: review?(jgilbert)
Attachment #8428627 - Flags: review?(jgilbert) → review+
Attachment #8428633 - Flags: review?(jgilbert) → review+
Comment on attachment 8428634 [details] [diff] [review]
Rolled up patches rebased for Beta

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

Is there really nothing less invasive that we can uplift for Beta? This would be much nicer for uplift if we reverted the naming changes in ImageInfo.
Attachment #8428634 - Flags: review?(jgilbert) → review+
Jeff was, understandably, freaked out by all the changes for a patch to apply to Beta. I've removed the renaming of functions in WebGLTexture to improve the signal-to-noise ratio.

Tested with:
Khronos 1.0.3beta conformance tests
The Audi.de site
acko.net
Attachment #8428634 - Attachment is obsolete: true
Attachment #8429825 - Flags: review?(jgilbert)
Try run for the beta rollup: https://tbpl.mozilla.org/?tree=Try&rev=125da225d546
Comment on attachment 8429825 [details] [diff] [review]
Rolled up patches rebased for Beta

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

Ouch, it looks like this isn't really better than the other patch. Can we really not do a minimally invasive fix for beta?
Attachment #8429825 - Flags: review?(jgilbert) → review-
We're too close to shipping Firefox 30 now and without a solid branch patch for this, I'm worried we won't have time to catch any regressions if any should occur. Since this is already in FF29 and we're not hearing a lot of feedback about it from Sumo, let's just get this into 31 and have it bake on a full Beta cycle.
Jeff, how about this? I'm pushing to try and will add link to results.
Attachment #8431277 - Flags: review?(jgilbert)
Flags: needinfo?(jgilbert)
Comment on attachment 8431277 [details] [diff] [review]
Minimal Beta patch

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

::: content/canvas/src/WebGLContextValidate.cpp
@@ +1381,5 @@
> +                                          format, func);
> +            return false;
> +        }
> +
> +        if (func == WebGLTexImageFunc::TexImage &&

Can't we just assert that `func` is TexImage at this point?
Attachment #8431277 - Flags: review?(jgilbert) → review+
(In reply to Dan Glastonbury :djg :kamidphish from comment #42)
> Created attachment 8431277 [details] [diff] [review]
> Minimal Beta patch
> 
> Jeff, how about this? I'm pushing to try and will add link to results.

Use this if it passes try and your supplemental testing.
Flags: needinfo?(jgilbert)
Results of minimal patch to Beta on 1.0.3beta conformance tests:
Results: (24408 of 24823 passed, 1 timed out) --> Results: (24481 of 24823 passed, 1 timed out)

float related tests go from failing to passing:
  conformance/extensions/oes-texture-float-with-canvas.html (99 of 99 passed)
  conformance/extensions/oes-texture-float-with-image-data.html (19 of 19 passed)
  conformance/extensions/oes-texture-float-with-image.html (27 of 27 passed)
  conformance/extensions/oes-texture-float-with-video.html (20 of 20 passed)

Failures that disappear:
  failed: getError expected: NO_ERROR. Was INVALID_OPERATION : texSubImage2D should succeed if OES_texture_float is enabled
  failed: getError expected: NO_ERROR. Was INVALID_OPERATION : texSubImage2D should succeed if OES_texture_float is enabled
Try (just minimal beta): https://tbpl.mozilla.org/?tree=Try&rev=f53e6b2e7032
Landed on Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/a31a77cd7951
Leaving Beta at the request of :lsblakk
This was already wontfixed for beta. If you want it to be reconsidered, you should really re-nominate the new beta patch...
Flags: needinfo?(dglastonbury)
RyanVM: But "leaving beta" I mean wontfix and will leave the hacky patches. I'd to get float fixed in 30 if possible, as seen by all the test results I've being adding as comments.
Flags: needinfo?(dglastonbury)
Flags: in-testsuite+
Status: REOPENED → RESOLVED
Closed: 10 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.