Closed Bug 1316539 Opened 3 years ago Closed 3 years ago

[WebGL2 conformance test]Pass 2/conformance/textures/misc/tex-sub-image-2d-bad-args.html

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox51 --- fixed
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: cleu, Assigned: cleu)

References

Details

Attachments

(2 files, 8 obsolete files)

No description provided.
Assignee: nobody → cleu
Attachment #8809311 - Attachment is obsolete: true
Attachment #8809314 - Attachment description: WIP Fix texSubImage2D bad arg conformance test0001-fix-texSubImage2D-bad-arg-conformance-test.patch → WIP Fix texSubImage2D bad arg conformance test
Attachment #8809314 - Attachment description: WIP Fix texSubImage2D bad arg conformance test → Fix texSubImage2D bad arg conformance test
Attachment #8809314 - Flags: review?(ethlin)
Michael, can you point out the related spec descriptions?
Flags: needinfo?(cleu)
Attached image Spec of texSubImage2D
https://www.khronos.org/registry/webgl/specs/latest/1.0/

It is in the WebGL1.0 spec.
Flags: needinfo?(cleu)
Comment on attachment 8809314 [details] [diff] [review]
Fix texSubImage2D bad arg conformance test

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

The check is for making sure if any WebGLTexture is bound and the return value(INVALID_OPERATION) should be correct.
Attachment #8809314 - Flags: review?(ethlin) → review-
We check the ArrayBufferView value here[1]. But that function is for both TexImage2D and TexSubImage2D. For TexImage2D, pixels is null-able, so you may need to check the pixels value earlier.  

[1] https://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLTextureUpload.cpp#135
This patch checks if src is generated from null pixels or not in TexSubImage.

Ethan, how do you think about this one?
Attachment #8809314 - Attachment is obsolete: true
Attachment #8810741 - Flags: feedback?(ethlin)
Attachment #8810741 - Attachment is obsolete: true
Attachment #8810741 - Flags: feedback?(ethlin)
Attachment #8810781 - Flags: review?(jgilbert)
Comment on attachment 8810781 [details] [diff] [review]
Add null pixel check in TexSubImage

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

I don't think this is the right place for this.

Generally, null checks should be very early during validation.
Attachment #8810781 - Flags: review?(jgilbert) → review-
Move null check to make it earlier.
Attachment #8810781 - Attachment is obsolete: true
Attachment #8812082 - Flags: review?(jgilbert)
Comment on attachment 8812994 [details]
Bug 1316539 - Do null TexSubImage check with blob->HasData(). -

I was hoping to cover both texSubImage and compressedTexSubImage here, but it turns out compressedTexSubImage is non-nullable. (see https://github.com/KhronosGroup/WebGL/issues/2169)

I don't love the approach of using an 'IsNull' check, but I suppose it might be the least bad solution.
Attachment #8812994 - Flags: review?(cleu) → review-
Attachment #8812994 - Flags: review-
Attachment #8812994 - Attachment is obsolete: true
Comment on attachment 8812082 [details] [diff] [review]
Add null pixel check in TexSubImage

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

::: dom/canvas/WebGLContext.h
@@ +1160,4 @@
>                         GLenum unpackType, const T& anySrc, ErrorResult& out_error)
>      {
>          const TexImageSourceAdapter src(anySrc, &out_error);
> +        if (src.IsNull()) {

This should go in WebGLTexture::TexSubImage.
Attachment #8812082 - Flags: review?(jgilbert) → review-
Move null check to WebGLTexture::TexSubImage
Attachment #8812082 - Attachment is obsolete: true
Attachment #8813004 - Attachment is obsolete: true
Attachment #8813005 - Flags: review?(jgilbert)
Comment on attachment 8813005 [details] [diff] [review]
Add null pixel check in TexSubImage

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

::: dom/canvas/WebGLTextureUpload.cpp
@@ +445,5 @@
>                            GLsizei height, GLsizei depth,
>                            const webgl::PackingInfo& pi, const TexImageSource& src)
>  {
> +    if (src.IsNull()) {
> +        mContext->ErrorInvalidValue("TexSubImage2D: Pixels must be non-null.");

Use %s and funcName instead of hardcoding TexSubImage2D here.
Attachment #8813005 - Flags: review?(jgilbert) → review+
Using funcName and  %s instead of hard-coding
Attachment #8813005 - Attachment is obsolete: true
Attachment #8813017 - Flags: review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d906f5908d1
Add null pixel check in TexSubImage. r=jgilbert
Keywords: checkin-needed
I'll update the tests and reland.
Flags: needinfo?(cleu)
Let's land this after bug 1310247, since that has a fix which should handle this test better.
Depends on: 1310247
Comment on attachment 8812994 [details]
Bug 1316539 - Do null TexSubImage check with blob->HasData(). -

https://reviewboard.mozilla.org/r/94508/#review100032
Attachment #8812994 - Flags: review?(cleu) → review+
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f86e0ceb12a4
Do null TexSubImage check with blob->HasData(). - r=lenzak800
https://hg.mozilla.org/mozilla-central/rev/f86e0ceb12a4
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Attachment #8813017 - Attachment is obsolete: true
Comment on attachment 8812994 [details]
Bug 1316539 - Do null TexSubImage check with blob->HasData(). -

Approval Request Comment
[Feature/Bug causing the regression]: webgl2
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:
Attachment #8812994 - Flags: approval-mozilla-beta?
Attachment #8812994 - Flags: approval-mozilla-aurora?
Comment on attachment 8812994 [details]
Bug 1316539 - Do null TexSubImage check with blob->HasData(). -

Fix WebGL2 related issue. Beta51+ & Aurora52+. Should be in 51 beta 10.
Attachment #8812994 - Flags: approval-mozilla-beta?
Attachment #8812994 - Flags: approval-mozilla-beta+
Attachment #8812994 - Flags: approval-mozilla-aurora?
Attachment #8812994 - Flags: approval-mozilla-aurora+
Check-in: https://hg.mozilla.org/releases/mozilla-aurora/rev/62eb5ff8893e - Jeff Gilbert - Bug 1316539 - Do null TexSubImage check with blob->HasData(). - r=lenzak800. a=gchang
You need to log in before you can comment on or make changes to this bug.