Closed
Bug 1359055
Opened 7 years ago
Closed 7 years ago
WebGL2: Upload of compressed textures from PBO fails
Categories
(Core :: Graphics: CanvasWebGL, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: pdaehne, Assigned: daoshengmu)
References
()
Details
(Whiteboard: [gfx-noted])
Attachments
(4 files, 1 obsolete file)
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0 Build ID: 20170424030211 Steps to reproduce: I try to upload compressed textures from pixel buffer objects. I.e. i try to do this: var pbo = gl.createBuffer(); gl.bindBuffer(gl.PIXEL_UNPACK_BUFFER, pbo); gl.bufferData(gl.PIXEL_UNPACK_BUFFER, img_8x8_rgb_dxt1, gl.STATIC_DRAW); gl.compressedTexImage2D(gl.TEXTURE_2D, 0, extS3tc.COMPRESSED_RGB_S3TC_DXT1_EXT, 8, 8, 0, 32, 0); (or similar for the other compressed texture functions compressedTexImage3D, compressedTexSubImage2D, and compressedTexSubImage3D.) See the attached test files. Actual results: I get the following error messages: TypeError: Argument 7 of WebGL2RenderingContext.compressedTexImage2D is not an object. TypeError: Argument 8 of WebGL2RenderingContext.compressedTexImage3D is not an object. TypeError: Argument 8 of WebGL2RenderingContext.compressedTexSubImage2D is not an object. TypeError: Argument 10 of WebGL2RenderingContext.compressedTexSubImage3D is not an object. Expected results: The upload of the compressed textures should succeed. The attached tests work as expected on Chrome (except compressedTexSubImage3D, but that seems to be yet another bug in ANGLE).
Updated•7 years ago
|
Component: Untriaged → Canvas: WebGL
Priority: -- → P3
Product: Firefox → Core
Comment 1•7 years ago
|
||
I can reproduce it on Firefox 53 as well. It seems that something going wrong in our webgl implementation.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•7 years ago
|
Flags: needinfo?(jgilbert)
Whiteboard: [gfx-noted]
Assignee | ||
Comment 3•7 years ago
|
||
First of all, We have difference from the document of compressedTexImage2D() between https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7 and our MDN, https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/compressedTexImage2D. void compressedTexImage2D(GLenum target, GLint level, GLenum internalformat, GLsizei width, GLsizei height, GLint border, GLsizei imageSize, GLintptr offset); But, this doesn't matter because our implementation in Gecko webidl is correct, Jump to, https://dxr.mozilla.org/mozilla-central/rev/b07db5d650b7056c78ba0dbc409d060ec4e922cd/dom/webidl/WebGL2RenderingContext.webidl#513 void compressedTexImage2D(GLenum target, GLint level, GLenum internalformat, GLsizei width, GLsizei height, GLint border, GLintptr offset); void compressedTexImage2D(GLenum target, GLint level, GLenum internalformat, GLsizei width, GLsizei height, GLint border, ArrayBufferView srcData, optional GLuint srcOffset = 0, optional GLuint srcLengthOverride = 0); We can find the 7th argument could be a GLintptr or ArrayBufferView, however, the binding code from https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-pc-linux-gnu/dom/bindings/WebGL2RenderingContextBinding.cpp#2869 only allows a JS object.
Assignee | ||
Comment 4•7 years ago
|
||
I find it is our wrong implementation at WebGL2RenderingContext.webidl. According to https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7, it should be: void compressedTexImage2D(GLenum target, GLint level, GLenum internalformat, GLsizei width, GLsizei height, GLint border, GLsizei imageSize, GLintptr offset); void compressedTexImage3D(GLenum target, GLint level, GLenum internalformat, GLsizei width, GLsizei height, GLsizei depth, GLint border, GLsizei imageSize, GLintptr offset); void compressedTexSubImage2D(GLenum target, GLint level, GLint xoffset, GLint yoffset, GLsizei width, GLsizei height, GLenum format, GLsizei imageSize, GLintptr offset); void compressedTexSubImage3D(GLenum target, GLint level, GLint xoffset, GLint yoffset, GLint zoffset, GLsizei width, GLsizei height, GLsizei depth, GLenum format, GLsizei imageSize, GLintptr offset);
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Daosheng Mu[:daoshengmu] from comment #5) > Created attachment 8887577 [details] > Bug 1359055 - (wip)Correct the implementation of compressedTexImage in WebGL > 2; > > Review commit: https://reviewboard.mozilla.org/r/158440/diff/#index_header > See other reviews: https://reviewboard.mozilla.org/r/158440/ I have done the API work and need to continue to do some validation.
Comment 7•7 years ago
|
||
(In reply to Daosheng Mu[:daoshengmu] from comment #4) > I find it is our wrong implementation at WebGL2RenderingContext.webidl. > > According to https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7, > it should be: > > void compressedTexImage2D(GLenum target, GLint level, GLenum internalformat, > GLsizei width, > GLsizei height, GLint border, GLsizei imageSize, > GLintptr offset); > > > void compressedTexImage3D(GLenum target, GLint level, GLenum internalformat, > GLsizei width, > GLsizei height, GLsizei depth, GLint border, > GLsizei imageSize, GLintptr offset); > > > void compressedTexSubImage2D(GLenum target, GLint level, GLint xoffset, > GLint yoffset, > GLsizei width, GLsizei height, GLenum format, > GLsizei imageSize, GLintptr offset); > > > void compressedTexSubImage3D(GLenum target, GLint level, GLint xoffset, > GLint yoffset, > GLint zoffset, GLsizei width, GLsizei height, > GLsizei depth, > GLenum format, GLsizei imageSize, GLintptr > offset); Yep, our webidl is out of date. Update the idl and glue everything back together.
Flags: needinfo?(jgilbert)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
(In reply to Daosheng Mu[:daoshengmu] from comment #8) > Comment on attachment 8887577 [details] > Bug 1359055 - (wip)Correct the implementation of compressedTexImage in WebGL > 2; > > Review request updated; see interdiff: > https://reviewboard.mozilla.org/r/158440/diff/1-2/ We need to access the buffer data of unpack WebGLBuffer for PBO offset.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
I have confirmed we can get the same result as Chrome after applying the paches, and try looks good https://treeherder.mozilla.org/#/jobs?repo=try&revision=a71734fed4b16cff82b76f5fb08642324a221229.
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8887577 [details] Bug 1359055 - Part 1: PBO offset for WebGL compressedTexImage; https://reviewboard.mozilla.org/r/158440/#review166004 ::: dom/canvas/WebGLContext.h:233 (Diff revision 3) > + GLsizei mImageSize; > + bool mHasImageSize; Maybe<GLsizei> mPboImageSize; ::: dom/canvas/WebGLContext.h:272 (Diff revision 3) > mView = view; > mViewElemOffset = viewElemOffset; > mViewElemLengthOverride = viewElemLengthOverride; > } > > - TexImageSourceAdapter(const WebGLsizeiptr* pboOffset, GLuint ignored1, GLuint ignored2 = 0) { > + TexImageSourceAdapter(const WebGLsizeiptr* pboOffset, GLuint imageSize, GLuint ignored2 = 0) { Do not modify this entrypoint! I believe it may be safe to do this for the moment, but it is conceptually the wrong place to do this. Just add a new entrypoint that takes `GLsizei imageSize, const WebGLintptr* offset`. ::: dom/canvas/WebGLTextureUpload.cpp:210 (Diff revision 3) > + if (size_t(imageSize) > availBufferBytes) { > + webgl->ErrorInvalidOperation("%s: ImageSize is larger than the rest of buffer.", funcName); > + return nullptr; > + } else { > + availBufferBytes = imageSize; > + } We must validate that imageSize matches the required upload byte size exactly. ::: dom/canvas/WebGLTextureUpload.cpp:1512 (Diff revision 3) > + if (mContext->mBoundPixelUnpackBuffer) { > + mContext->gl->fBindBuffer(LOCAL_GL_PIXEL_UNPACK_BUFFER, > + mContext->mBoundPixelUnpackBuffer->mGLName); > + } const ScopedLazyBind bindPBO(mContext->gl, LOCAL_GL_PIXEL_UNPACK_BUFFER, mContext->mBoundPixelUnpackBuffer); ::: dom/canvas/WebGLTextureUpload.cpp:1520 (Diff revision 3) > + } > > // Warning: Possibly shared memory. See bug 1225033. > GLenum error = DoCompressedTexImage(mContext->gl, target, level, internalFormat, > blob->mWidth, blob->mHeight, blob->mDepth, > blob->mAvailBytes, blob->mPtr); blob->mAvailBytes should be blob->mRequiredBytes.
Attachment #8887577 -
Flags: review?(jgilbert) → review-
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•7 years ago
|
||
(In reply to Jeff Gilbert [:jgilbert] from comment #13) > Comment on attachment 8887577 [details] > Bug 1359055 - Part 1: PBO offset for WebGL compressedTexImage; > > https://reviewboard.mozilla.org/r/158440/#review166004 > > > ::: dom/canvas/WebGLTextureUpload.cpp:210 > (Diff revision 3) > > + if (size_t(imageSize) > availBufferBytes) { > > + webgl->ErrorInvalidOperation("%s: ImageSize is larger than the rest of buffer.", funcName); > > + return nullptr; > > + } else { > > + availBufferBytes = imageSize; > > + } > > We must validate that imageSize matches the required upload byte size > exactly. > IIUC, the imageSize is the required upload byte size that is given the content side. (https://www.khronos.org/registry/OpenGL-Refpages/es3.0/html/glCompressedTexImage2D.xhtml) We have no chance to know the required upload byte size from WebGLBuffer. The possible way to validate it is to check whether it is oversize for the available size.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8887577 [details] Bug 1359055 - Part 1: PBO offset for WebGL compressedTexImage; https://reviewboard.mozilla.org/r/158440/#review171502 Change of plan. Pass it directly into CompressedTex[Sub]Image. Sorry to change direction so late! ::: dom/canvas/WebGL2Context.h:126 (Diff revision 5) > + { > + const char funcName[] = "compressedTexImage3D"; > + const uint8_t funcDims = 3; > + const TexImageSourceAdapter src(imageSize, &offset); > + CompressedTexImage(funcName, funcDims, target, level, internalFormat, width, > + height, depth, border, src); Change CompressedTex[Sub]Image to accept a Maybe<GLsizei> expectedImageSize, and pass Some(imageSize) here. ::: dom/canvas/WebGLContext.h:233 (Diff revision 5) > const dom::ArrayBufferView* mView; > GLuint mViewElemOffset; > GLuint mViewElemLengthOverride; > > const WebGLsizeiptr* mPboOffset; > + Maybe<GLsizei> mPboImageSize; This isn't needed. We'll just pass directly imageSize directly. ::: dom/canvas/WebGLTextureUpload.cpp:1527 (Diff revision 5) > + if (mContext->mBoundPixelUnpackBuffer) { > + mContext->gl->fBindBuffer(LOCAL_GL_PIXEL_UNPACK_BUFFER, 0); > + } This is handled by ScopedLazyBind. ::: dom/canvas/WebGLTextureUpload.cpp:1686 (Diff revision 5) > + if (mContext->mBoundPixelUnpackBuffer) { > + mContext->gl->fBindBuffer(LOCAL_GL_PIXEL_UNPACK_BUFFER, 0); > + } This is handled by ScopedLazyBind.
Attachment #8887577 -
Flags: review?(jgilbert) → review-
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8889369 [details] Bug 1359055 - Part 2: Enable compressed texture unpack buffer tests; https://reviewboard.mozilla.org/r/160428/#review174250
Attachment #8889369 -
Flags: review?(jgilbert) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8899224 -
Attachment is obsolete: true
Attachment #8899224 -
Flags: review?(jgilbert)
Assignee | ||
Comment 27•7 years ago
|
||
Try looks good, https://treeherder.mozilla.org/#/jobs?repo=try&revision=4637a6560de980ecb3c83da8d1db859baaf3b473
Comment 28•7 years ago
|
||
mozreview-review |
Comment on attachment 8887577 [details] Bug 1359055 - Part 1: PBO offset for WebGL compressedTexImage; https://reviewboard.mozilla.org/r/158440/#review177254 Please fix these issues, but this is safe to land, so r+. ::: dom/canvas/WebGLTextureUpload.cpp:209 (Diff revision 7) > webgl->ErrorInvalidOperation("%s: Offset is passed end of buffer.", funcName); > return nullptr; > } > availBufferBytes -= pboOffset; > - > + if (expectedImageSize.isSome()) { > + if (size_t(expectedImageSize.ref()) > availBufferBytes) { ==, not > ::: dom/canvas/WebGLTextureUpload.cpp:209 (Diff revision 7) > webgl->ErrorInvalidOperation("%s: Offset is passed end of buffer.", funcName); > return nullptr; > } > availBufferBytes -= pboOffset; > - > + if (expectedImageSize.isSome()) { > + if (size_t(expectedImageSize.ref()) > availBufferBytes) { First test if expectedImageSize < 0, which should be INVALID_VALUE.
Attachment #8887577 -
Flags: review?(jgilbert) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 31•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8887577 [details] Bug 1359055 - Part 1: PBO offset for WebGL compressedTexImage; https://reviewboard.mozilla.org/r/158440/#review177254 > ==, not > I think you mean '!='
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 34•7 years ago
|
||
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland searching for changes remote: adding changesets remote: adding manifests remote: adding file changes remote: added 2 changesets with 8 changes to 8 files remote: remote: WebIDL file dom/webidl/WebGL2RenderingContext.webidl altered in changeset ace35f608a2d without DOM peer review remote: remote: remote: remote: ************************** ERROR **************************** remote: remote: Changes to WebIDL files in this repo require review from a DOM peer in the form of r=... remote: This is to ensure that we behave responsibly with exposing new Web APIs. We appreciate your understanding.. remote: remote: ************************************************************* remote: remote: remote: transaction abort! remote: rollback completed remote: pretxnchangegroup.d_webidl hook failed abort: push failed on remote
Assignee | ||
Updated•7 years ago
|
Attachment #8887577 -
Flags: review?(amarchesini)
Assignee | ||
Comment 35•7 years ago
|
||
Baku, please help review the webidl part. Thanks.
Assignee | ||
Updated•7 years ago
|
Comment 36•7 years ago
|
||
mozreview-review |
Comment on attachment 8887577 [details] Bug 1359055 - Part 1: PBO offset for WebGL compressedTexImage; https://reviewboard.mozilla.org/r/158440/#review178960 r+ for the dom/webidl bit.
Attachment #8887577 -
Flags: review?(amarchesini) → review+
Comment 37•7 years ago
|
||
Pushed by dmu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7c2db4c459cc Part 1: PBO offset for WebGL compressedTexImage; r=baku,jgilbert https://hg.mozilla.org/integration/autoland/rev/41850a57de47 Part 2: Enable compressed texture unpack buffer tests; r=jgilbert
Comment 38•7 years ago
|
||
Backed out in https://hg.mozilla.org/integration/autoland/rev/76edd38bb942 for https://treeherder.mozilla.org/logviewer.html#?job_id=126966842&repo=autoland on Windows 7 (opt/pgo/debug, you don't need pgo to hit it).
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 41•7 years ago
|
||
(In reply to Daosheng Mu[:daoshengmu] from comment #39) > Created attachment 8902868 [details] > Bug 1359055 - Part 3: Fail-if webgl-compressed-texture-size-limit tests on > Win 7; > > Review commit: https://reviewboard.mozilla.org/r/174560/diff/#index_header > See other reviews: https://reviewboard.mozilla.org/r/174560/ Add some special cases for fail-if on Win 7 but it will get green on Win 8.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 43•7 years ago
|
||
Try looks good now, https://treeherder.mozilla.org/#/jobs?repo=try&revision=4517761b3e5dbd274ac073de178bd0fc1eb869cd&selectedJob=127485102
Comment 44•7 years ago
|
||
mozreview-review |
Comment on attachment 8902868 [details] Bug 1359055 - Part 3: Fail-if webgl-compressed-texture-size-limit tests on Win 7; https://reviewboard.mozilla.org/r/174560/#review182110 ::: dom/canvas/test/webgl-conf/mochitest-errata.ini:868 (Diff revision 2) > [generated/test_conformance__textures__misc__tex-image-and-sub-image-2d-with-array-buffer-view.html] > # time out crash > skip-if = (os == 'win' && debug) > +[generated/test_conformance__extensions__webgl-compressed-texture-size-limit.html] > +# time out on win7. > +fail-if = (os == 'win' && os_version == '6.1') Skip timeouts. (also "timeout" is one word) ::: dom/canvas/test/webgl-conf/mochitest-errata.ini:870 (Diff revision 2) > skip-if = (os == 'win' && debug) > +[generated/test_conformance__extensions__webgl-compressed-texture-size-limit.html] > +# time out on win7. > +fail-if = (os == 'win' && os_version == '6.1') > +# Frequent but intermittent timeout on win7 debug e10s. > +skip-if = (os == 'win' && os_version == '6.1' && (debug && e10s)) Drop the parens from around debug and e10s. There's no reason to nest them here.
Attachment #8902868 -
Flags: review?(jgilbert) → review+
Comment hidden (mozreview-request) |
Comment 46•7 years ago
|
||
Pushed by dmu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1c9fd18131fe Part 1: PBO offset for WebGL compressedTexImage; r=baku,jgilbert https://hg.mozilla.org/integration/autoland/rev/84528f05408b Part 2: Enable compressed texture unpack buffer tests; r=jgilbert https://hg.mozilla.org/integration/autoland/rev/d2315652e64f Part 3: Fail-if webgl-compressed-texture-size-limit tests on Win 7; r=jgilbert
Comment 47•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1c9fd18131fe https://hg.mozilla.org/mozilla-central/rev/84528f05408b https://hg.mozilla.org/mozilla-central/rev/d2315652e64f
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•