Closed Bug 1313541 Opened 7 years ago Closed 7 years ago

Update WebIDL, glue code, and add ArrayBufferView offset entrypoints

Categories

(Core :: Graphics: CanvasWebGL, defect, P1)

Product:
Core
Component:
Graphics: CanvasWebGL
Version:
51 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

(Keywords: dev-doc-complete, Whiteboard: gfx-noted)

Attachments

(23 files)

Bug 1313541 - Add ValidateArrayBufferView. -
58 bytes, text/x-review-board-request
ethlin
: review+
gchang
: approval-mozilla-aurora+
Details
Bug 1313541 - ReadPixels webidl. -
58 bytes, text/x-review-board-request
qdot
: review+
Details
Bug 1313541 - ReadPixels impl. -
58 bytes, text/x-review-board-request
ethlin
: review+
Details
Bug 1313541 - Buffer[Sub]Data webidl. -
58 bytes, text/x-review-board-request
qdot
: review+
Details
Bug 1313541 - Buffer[Sub]Data impl. -
58 bytes, text/x-review-board-request
ethlin
: review+
Details
Bug 1313541 - Add WebGLBuffer::ValidateRange. -
58 bytes, text/x-review-board-request
ethlin
: review+
Details
Bug 1313541 - GetBufferSubData webidl. -
58 bytes, text/x-review-board-request
qdot
: review+
Details
Bug 1313541 - GetBufferSubData impl. -
58 bytes, text/x-review-board-request
ethlin
: review+
Details
Bug 1313541 - Texture webidl. -
58 bytes, text/x-review-board-request
qdot
: review+
Details
Bug 1313541 - Texture impl. -
58 bytes, text/x-review-board-request
ethlin
: review+
Details
Bug 1313541 - Texture webidl. -
58 bytes, text/x-review-board-request
qdot
: review+
Details
Bug 1313541 - Texture impl. -
58 bytes, text/x-review-board-request
ethlin
: review+
Details
Bug 1313541 - Uniform webidl. -
58 bytes, text/x-review-board-request
qdot
: review+
Details
Bug 1313541 - Uniform impl. -
58 bytes, text/x-review-board-request
ethlin
: review+
Details
Bug 1313541 - ClearBuffer style fixes. -
58 bytes, text/x-review-board-request
ethlin
: review+
Details
Bug 1313541 - ClearBuffer webidl. -
58 bytes, text/x-review-board-request
qdot
: review+
Details
Bug 1313541 - ClearBuffer impl. -
58 bytes, text/x-review-board-request
ethlin
: review+
Details
Bug 1313541 - Minimize deviation from top of tree webidl. -
58 bytes, text/x-review-board-request
qdot
: review+
Details
Bug 1313541 - Reimplement glue in accordance to webidl deviation minimization. -
58 bytes, text/x-review-board-request
ethlin
: review+
Details
Bug 1313541 - Rewrite TexImageSource glue. -
58 bytes, text/x-review-board-request
ethlin
: review+
Details
Bug 1313541 - Fix test since TexSubImage is no longer nullable.
58 bytes, text/x-review-board-request
Details
Bug 1313541 - Fix CompressedTexImage entrypoints to add size. -
58 bytes, text/x-review-board-request
qdot
: review+
Details
Bug 1313541 - Fix compressedTexImage size validation. -
58 bytes, text/x-review-board-request
ethlin
: review+
Details 
      No description provided.
Comment on attachment 8805377 [details]
Bug 1313541 - Buffer[Sub]Data webidl. -

https://reviewboard.mozilla.org/r/89078/#review88546
Attachment #8805377 - Flags: review?(kyle) → review+
Comment on attachment 8805380 [details]
Bug 1313541 - GetBufferSubData webidl. -

https://reviewboard.mozilla.org/r/89084/#review88548
Attachment #8805380 - Flags: review?(kyle) → review+
Comment on attachment 8805384 [details]
Bug 1313541 - Texture webidl. -

https://reviewboard.mozilla.org/r/89092/#review88554

::: dom/webidl/WebGL2RenderingContext.webidl:416
(Diff revision 1)
>      void texSubImage3D(GLenum target, GLint level, GLint xoffset, GLint yoffset,
>                         GLint zoffset, GLenum format, GLenum type, HTMLVideoElement video);
>  
>      ////
>  
> +    //void compressedTexImage2D(GLenum target, GLint level, GLenum internalformat, GLsizei width,

Nit: Why are there a bunch of commented out APIs in this one? I see them in the spec, are they just not implemented by us yet? If that's the case, could you comment them as such?
Attachment #8805384 - Flags: review?(kyle) → review+
Comment on attachment 8805391 [details]
Bug 1313541 - Minimize deviation from top of tree webidl. -

https://reviewboard.mozilla.org/r/89106/#review88564
Attachment #8805391 - Flags: review?(kyle) → review+
Comment on attachment 8805395 [details]
Bug 1313541 - Fix CompressedTexImage entrypoints to add size. -

https://reviewboard.mozilla.org/r/89114/#review88566
Attachment #8805395 - Flags: review?(kyle) → review+
Comment on attachment 8805388 [details]
Bug 1313541 - ClearBuffer style fixes. -

https://reviewboard.mozilla.org/r/89100/#review88724
Attachment #8805388 - Flags: review?(ethlin) → review+
Comment on attachment 8805374 [details]
Bug 1313541 - Add ValidateArrayBufferView. -

https://reviewboard.mozilla.org/r/89072/#review88750
Attachment #8805374 - Flags: review?(ethlin) → review+
Comment on attachment 8805374 [details]
Bug 1313541 - Add ValidateArrayBufferView. -

https://reviewboard.mozilla.org/r/89072/#review88754

::: dom/canvas/WebGLContext.cpp:2520
(Diff revision 1)
> +                                      const dom::ArrayBufferView& view, GLuint elemOffset,
> +                                      GLuint elemCountOverride, uint8_t** const out_bytes,
> +                                      size_t* const out_byteLen)
> +{
> +    view.ComputeLengthAndData();
> +    uint8_t* const bytes = view.DataAllowShared();

Should you check if bytes is nullptr?
Comment on attachment 8805376 [details]
Bug 1313541 - ReadPixels impl. -

https://reviewboard.mozilla.org/r/89076/#review88756
Attachment #8805376 - Flags: review?(ethlin) → review+
Comment on attachment 8805379 [details]
Bug 1313541 - Add WebGLBuffer::ValidateRange. -

https://reviewboard.mozilla.org/r/89082/#review88764
Attachment #8805379 - Flags: review?(ethlin) → review+
Comment on attachment 8805378 [details]
Bug 1313541 - Buffer[Sub]Data impl. -

https://reviewboard.mozilla.org/r/89080/#review88766
Attachment #8805378 - Flags: review?(ethlin) → review+
Comment on attachment 8805381 [details]
Bug 1313541 - GetBufferSubData impl. -

https://reviewboard.mozilla.org/r/89086/#review88770
Attachment #8805381 - Flags: review?(ethlin) → review+
Comment on attachment 8805383 [details]
Bug 1313541 - Texture impl. -

https://reviewboard.mozilla.org/r/89090/#review88772

::: dom/canvas/WebGL2ContextTextures.cpp:67
(Diff revision 1)
>      const GLint xOffset = 0;
>      const GLint yOffset = 0;
>      const GLint zOffset = 0;
>      tex->TexOrSubImage(isSubImage, funcName, target, level, internalFormat, xOffset,
>                         yOffset, zOffset, width, height, depth, border, unpackFormat,
> -                       unpackType, view);
> +                       unpackType, view, srcElemOffset);

'view' should be renamed to 'srcView'.
Attachment #8805383 - Flags: review?(ethlin) → review+
Comment on attachment 8805385 [details]
Bug 1313541 - Texture impl. -

https://reviewboard.mozilla.org/r/89094/#review88776
Attachment #8805385 - Flags: review?(ethlin) → review+
Comment on attachment 8805383 [details]
Bug 1313541 - Texture impl. -

https://reviewboard.mozilla.org/r/89090/#review88772

> 'view' should be renamed to 'srcView'.

oh...The next patch do the rename. Never mind.
Comment on attachment 8805387 [details]
Bug 1313541 - Uniform impl. -

https://reviewboard.mozilla.org/r/89098/#review88820
Attachment #8805387 - Flags: review?(ethlin) → review+
Comment on attachment 8805390 [details]
Bug 1313541 - ClearBuffer impl. -

https://reviewboard.mozilla.org/r/89104/#review88840
Attachment #8805390 - Flags: review?(ethlin) → review+
Comment on attachment 8805392 [details]
Bug 1313541 - Reimplement glue in accordance to webidl deviation minimization. -

https://reviewboard.mozilla.org/r/89108/#review88846
Attachment #8805392 - Flags: review?(ethlin) → review+
Comment on attachment 8805396 [details]
Bug 1313541 - Fix compressedTexImage size validation. -

https://reviewboard.mozilla.org/r/89116/#review88858
Attachment #8805396 - Flags: review?(ethlin) → review+
Comment on attachment 8805393 [details]
Bug 1313541 - Rewrite TexImageSource glue. -

https://reviewboard.mozilla.org/r/89110/#review88852

::: dom/canvas/TexUnpackBlob.cpp:143
(Diff revision 1)
> +    if (!blob->mWidth || !blob->mHeight || !blob->mDepth)
> +        return true;
> +
> +    const auto usedPixelsPerRow = CheckedUint32(blob->mSkipPixels) + blob->mWidth;
> +    if (!usedPixelsPerRow.isValid() || usedPixelsPerRow.value() > blob->mRowLength) {
> +        webgl->ErrorInvalidOperation("%s: UNPACK_SKIP_PIXELS + height >"

Should be UNPACK_SKIP_PIXELS + "width".

::: dom/webidl/WebGL2RenderingContext.webidl:376
(Diff revision 1)
>                      GLenum type, ArrayBufferView? pixels);
> -    [Throws] // Another overhead throws.
> -    void texImage2D(GLenum target, GLint level, GLenum internalformat, GLenum format,
> -                    GLenum type, ImageData pixels);
>      [Throws]
> -    void texImage2D(GLenum target, GLint level, GLenum internalformat, GLenum format,
> +    void texImage2D(GLenum target, GLint level, GLenum internalformat,

This file should be reviewed by dom peer.

::: dom/webidl/WebGLRenderingContext.webidl:705
(Diff revision 1)
>      void texImage2D(GLenum target, GLint level, GLenum internalformat,
>                      GLsizei width, GLsizei height, GLint border, GLenum format,
>                      GLenum type, ArrayBufferView? pixels);
>      [Throws] // Can't actually throw.
>      void texImage2D(GLenum target, GLint level, GLenum internalformat,
> -                    GLenum format, GLenum type, ImageData? pixels);
> +                    GLenum format, GLenum type, ImageData pixels);

This file should be reviewed by dom peer too.
Attachment #8805393 - Flags: review?(ethlin) → review+
Comment on attachment 8805384 [details]
Bug 1313541 - Texture webidl. -

https://reviewboard.mozilla.org/r/89092/#review88554

> Nit: Why are there a bunch of commented out APIs in this one? I see them in the spec, are they just not implemented by us yet? If that's the case, could you comment them as such?

We implement them later in this branch but I can leave a comment there for completeness.
(In reply to Jeff Gilbert [:jgilbert] from comment #48)
> Comment on attachment 8805384 [details]
> Bug 1313541 - Texture webidl. -
> 
> https://reviewboard.mozilla.org/r/89092/#review88554
> 
> > Nit: Why are there a bunch of commented out APIs in this one? I see them in the spec, are they just not implemented by us yet? If that's the case, could you comment them as such?
> 
> We implement them later in this branch but I can leave a comment there for
> completeness.

Ah, the fun of reading linearly. :) Don't worry about it then, doesn't really matter in the space of the few commits that it happens.
Comment on attachment 8805393 [details]
Bug 1313541 - Rewrite TexImageSource glue. -

https://reviewboard.mozilla.org/r/89110/#review88852

> Should be UNPACK_SKIP_PIXELS + "width".

Oops, yes.
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/afc43605df19
Add ValidateArrayBufferView. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e495997ff2e
ReadPixels webidl. - r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cfad155dc87
ReadPixels impl. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/67c3fecf5cee
Buffer[Sub]Data webidl. - r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/199e25c045e4
Buffer[Sub]Data impl. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/e31eb2124b44
Add WebGLBuffer::ValidateRange. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1786e6a9995
GetBufferSubData webidl. - r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a73165c9408
GetBufferSubData impl. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/78d5e10a84e4
Texture webidl. - r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/159dd65b1045
Texture impl. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/603cc63e6369
Texture webidl. - r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/796ebd8abd19
Texture impl. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc526ef04ba3
Uniform webidl. - r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/26b31f8052ca
Uniform impl. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/bee38e8d9c9d
ClearBuffer style fixes. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f8edbf381fd
ClearBuffer webidl. - r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f7eada57765
ClearBuffer impl. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c4cee3d0cd4
Minimize deviation from top of tree webidl. - r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/46412c6e5560
Reimplement glue in accordance to webidl deviation minimization. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/5569ca0fe5a9
Rewrite TexImageSource glue. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5e52c3a3759
Fix test since TexSubImage is no longer nullable.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a31068014e1b
Fix CompressedTexImage entrypoints to add size. - r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b00f0d87c02
Fix compressedTexImage size validation. - r=ethlin
Jeff, I think you need to uplift this to fix some conformance failures.
Flags: needinfo?(jgilbert)
status-firefox51: --- → affected
Flags: needinfo?(jgilbert)
Comment on attachment 8805374 [details]
Bug 1313541 - Add ValidateArrayBufferView. -

Approval Request Comment
[Feature/regressing bug #]: webgl2
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

This and all other patches in the bug.
Attachment #8805374 - Flags: approval-mozilla-aurora?
Depends on: 1316829
Note that this patch causes crash bug 1316829.  We should probably fix that before aurora uplift.
Comment on attachment 8805374 [details]
Bug 1313541 - Add ValidateArrayBufferView. -

WebGL 2 is the feature we want to ship in 51. Aurora51+.
Attachment #8805374 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/6af58753df16
https://hg.mozilla.org/releases/mozilla-aurora/rev/b80438cb0825
https://hg.mozilla.org/releases/mozilla-aurora/rev/a99e419681e5
https://hg.mozilla.org/releases/mozilla-aurora/rev/4031945696c2
https://hg.mozilla.org/releases/mozilla-aurora/rev/9ee2e73a7312
https://hg.mozilla.org/releases/mozilla-aurora/rev/f83d9fa111a1
https://hg.mozilla.org/releases/mozilla-aurora/rev/9db4213007d9
https://hg.mozilla.org/releases/mozilla-aurora/rev/827e25dec6e1
https://hg.mozilla.org/releases/mozilla-aurora/rev/103e04e023df
https://hg.mozilla.org/releases/mozilla-aurora/rev/bbdad8221e41
https://hg.mozilla.org/releases/mozilla-aurora/rev/9d9f68fa1abc
https://hg.mozilla.org/releases/mozilla-aurora/rev/7c2dded3db02
https://hg.mozilla.org/releases/mozilla-aurora/rev/0c0aba7c3d2e
https://hg.mozilla.org/releases/mozilla-aurora/rev/a001d6e786d2
https://hg.mozilla.org/releases/mozilla-aurora/rev/4c980dc86a39
https://hg.mozilla.org/releases/mozilla-aurora/rev/1d2c29c98b70
https://hg.mozilla.org/releases/mozilla-aurora/rev/b8817659b5d8
https://hg.mozilla.org/releases/mozilla-aurora/rev/fe55eee60666
https://hg.mozilla.org/releases/mozilla-aurora/rev/4eb05beee849
https://hg.mozilla.org/releases/mozilla-aurora/rev/8b56b957a05b
https://hg.mozilla.org/releases/mozilla-aurora/rev/cb27254fc424
https://hg.mozilla.org/releases/mozilla-aurora/rev/ee066afe41e5
https://hg.mozilla.org/releases/mozilla-aurora/rev/0f867a884677
status-firefox51: affected → fixed
I've tried to make sense of the giant IDL diffs here and updated signatures for a few methods in the docs:

Buffers
https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/readPixels
https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/bufferData
https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/bufferSubData
https://developer.mozilla.org/en-US/docs/Web/API/WebGL2RenderingContext/getBufferSubData
https://developer.mozilla.org/en-US/docs/Web/API/WebGL2RenderingContext/clearBuffer

Textures
https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/texImage2D
https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/texSubImage2D
https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/compressedTexImage2D
https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/compressedTexSubImage2D
https://developer.mozilla.org/en-US/docs/Web/API/WebGL2RenderingContext/texImage3D
https://developer.mozilla.org/en-US/docs/Web/API/WebGL2RenderingContext/texSubImage3D
https://developer.mozilla.org/en-US/docs/Web/API/WebGL2RenderingContext/compressedTexImage3D
https://developer.mozilla.org/en-US/docs/Web/API/WebGL2RenderingContext/compressedTexSubImage3D

Uniforms
https://developer.mozilla.org/en-US/docs/Web/API/WebGL2RenderingContext/uniform
https://developer.mozilla.org/en-US/docs/Web/API/WebGL2RenderingContext/uniformMatrix
https://developer.mozilla.org/en-US/docs/Web/API/WebGL2RenderingContext/vertexAttribI
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 1333676
You need to log in before you can comment on or make changes to this bug.