Closed Bug 1359055 Opened 2 years ago Closed 2 years ago

WebGL2: Upload of compressed textures from PBO fails

Categories

(Core :: Canvas: WebGL, defect, P3)

55 Branch
defect

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).
Component: Untriaged → Canvas: WebGL
Priority: -- → P3
Product: Firefox → Core
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
Flags: needinfo?(jgilbert)
Whiteboard: [gfx-noted]
I can work on this
Assignee: nobody → dmu
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.
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);
(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.
(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)
(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.
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 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-
(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 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 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+
Attachment #8899224 - Attachment is obsolete: true
Attachment #8899224 - Flags: review?(jgilbert)
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 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 '!='
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
Attachment #8887577 - Flags: review?(amarchesini)
Baku, please help review the webidl part. Thanks.
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+
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
(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 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+
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
You need to log in before you can comment on or make changes to this bug.