Closed Bug 1242347 Opened 8 years ago Closed 8 years ago

Pass WebGL2 conformance test texture-npot

Categories

(Core :: Graphics: CanvasWebGL, defect)

46 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 + wontfix
firefox47 --- fixed

People

(Reporter: mtseng, Assigned: mtseng)

References

()

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

The check added by bug 1239259 was not completed. The spec says

According to the spec[1], GL_INVALID_OPERATION is generated if the levelbaselevelbase array was not specified with an unsized internal format or a sized internal format that is both color-renderable and texture-filterable. So I add the check.

But we only check sized internal format that is both color-renderable and texture-filterable. We also need check the format is unsized internal format or not.
Comment on attachment 8711593 [details] [diff] [review]
Allow unsized internal format when generate mipmap.

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

This approach is workable, but I think it would be nicer to just do:

// GLES 3.0.4 p160: ...
bool canGenerateMipmap = (usage->isRenderable && usage->isFilterable);
switch (usage->format->effectiveFormat) {
case webgl::EffectiveFormat::Luminance8:
case webgl::EffectiveFormat::Alpha8:
case webgl::EffectiveFormat::Luminance8Alpha8:
  // Non-color-renderable formats from Table 3.3.
  canGenerateMipmap = true;
}

This way we don't have to add state to ImageInfo.

::: dom/canvas/WebGLTexture.cpp
@@ +748,5 @@
> +    // OpenGL ES 3.0.4 p160:
> +    // If the level base array was not specified with an unsized internal format from
> +    // table 3.3 or a sized internal format that is both color-renderable and
> +    // texture-filterable according to table 3.13, an INVALID_OPERATION error
> +    // is generated.

This is extremely unfortunate. :(
Specifically, LUM/LUM_ALPHA/ALPHA are filterable, but not color-renderable.

::: dom/canvas/WebGLTexture.h
@@ +116,5 @@
>  
>          const uint32_t mWidth;
>          const uint32_t mHeight;
>          const uint32_t mDepth;
> +        const bool mIsUnsizedFormat;

mCanGenerateMipmap

::: dom/canvas/WebGLTextureUpload.cpp
@@ +1038,5 @@
>              return;
>          }
>  
>          dstUsage = fua->GetUnsizedTexUsage(srcPacking);
> +        isUnsizedFormat = true;

It's not that it's any unsized formats, but rather that it's in Table 3.3, which is a list of unsized formats.

We don't want to allow unsized DEPTH_COMPONENT textures
Attachment #8711593 - Flags: review?(jgilbert) → review-
Assignee: nobody → mtseng
Status: NEW → ASSIGNED
Addressed jgilbert's comment.
Attachment #8712544 - Flags: review?(jgilbert)
Attachment #8711593 - Attachment is obsolete: true
Comment on attachment 8712544 [details] [diff] [review]
Allow unsized internal format when generate mipmap v2.

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

::: dom/canvas/WebGLTexture.cpp
@@ +748,5 @@
> +    // texture-filterable according to table 3.13, an INVALID_OPERATION error
> +    // is generated.
> +    bool canGenerateMipmap = (baseImageInfo.mFormat->isRenderable &&
> +                              baseImageInfo.mFormat->isFilterable);
> +    switch (baseImageInfo.mFormat->format->effectiveFormat) {

const auto usage = baseImageInfo.mFormat;
Attachment #8712544 - Flags: review?(jgilbert) → review+
https://hg.mozilla.org/mozilla-central/rev/adace6a3f916
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Toof bad this patch hasn't been backported to 46, because bug 1239259 regressed some WebGL apps (see bug 1268442 for an example) and this current patch fixed it. Now there is an unwanted regression in 46. :/
[Tracking Requested - why for this release]:

As there is another regression for WebGL in 46 (bug 1268096), I request to track this bug for 46 to fix this another regression (e.g bug 1268442).
Keywords: regression
Flags: needinfo?(lhenry)
How widespread is this issue? I'll track it for now for 46. Let's look at whether the fix may be safe to uplift if we end up releasing a 46.0.1.
Flags: needinfo?(lhenry)
+1 for fix 46.0.1
Many bugs on webgl application/game here in 46 with that regression.
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #11)
> How widespread is this issue? I'll track it for now for 46. Let's look at
> whether the fix may be safe to uplift if we end up releasing a 46.0.1.

I didn't see this earlier.
I imagine we're too late for 46, yes?
Flags: needinfo?(lhenry)
We already released 46.0.1 last week, yes. But this will be fixed in 47 (planned release date June 7)
Flags: needinfo?(lhenry)
Version: unspecified → 46 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: