1280499 - Implement TexImage for PBOs (PIXEL_UNPACK_BUFFER)

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Comment 1

[Kelsey Gilbert [:jgilbert]]

So step one is to write the IDL, actually.

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1280499 - Allow SKIP_ROWS+height>IMAGE_HEIGHT and fix paranoid uploading. -

Review commit: https://reviewboard.mozilla.org/r/62976/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62976/

Comment 3

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1280499 - Add stubs and forwards. -

Review commit: https://reviewboard.mozilla.org/r/62978/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62978/

Comment 4

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1280499 - Implement PBOs for textures. -

Review commit: https://reviewboard.mozilla.org/r/62980/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62980/

Comment 5

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1280499 - Unlock PACK_BUFFER. -

Review commit: https://reviewboard.mozilla.org/r/62982/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62982/

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1280499 - Support paranoid uploading for nVidia. -

Review commit: https://reviewboard.mozilla.org/r/62984/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62984/

Comment 7

[Kelsey Gilbert [:jgilbert]]

https://reviewboard.mozilla.org/r/62980/#review59920

Comment 8

[Kelsey Gilbert [:jgilbert]]

Green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8805910808d6

Comment 9

[Ethan Lin[:ethlin]]

Comment on attachment 8768999 [details]
Bug 1280499 - Unlock PACK_BUFFER. -

https://reviewboard.mozilla.org/r/62982/#review60552

Comment 10

[Ethan Lin[:ethlin]]

Comment on attachment 8768997 [details]
Bug 1280499 - Add stubs and forwards. -

https://reviewboard.mozilla.org/r/62978/#review60600

Comment 11

[Ethan Lin[:ethlin]]

Comment on attachment 8768998 [details]
Bug 1280499 - Implement PBOs for textures. -

https://reviewboard.mozilla.org/r/62980/#review60966

::: dom/canvas/WebGLTextureUpload.cpp:93
(Diff revision 1)
> +{
> +    const auto usedPixelsPerRow = CheckedUint32(blob->mSkipPixels) + blob->mWidth;
> +    const auto usedRowsPerImage = CheckedUint32(blob->mSkipRows) + blob->mHeight;
> +    const auto usedImages = CheckedUint32(blob->mSkipImages) + blob->mDepth;
> +
> +    bool unpackVarsValid = false;

'unpackVarsValid' is unused.

Comment 12

[Ethan Lin[:ethlin]]

Comment on attachment 8769000 [details]
Bug 1280499 - Support paranoid uploading for nVidia. -

https://reviewboard.mozilla.org/r/62984/#review60970

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://reviewboard.mozilla.org/r/62976/#review61830

::: dom/webidl/WebGL2RenderingContext.webidl:393
(Diff revision 1)
>                                   ArrayBufferView data);
>  
> +    ////////////////
> +    // Texture from PBO
> +
> +    [Throws] // Can't actually throw.

....?

Comment 14

[Kelsey Gilbert [:jgilbert]]

https://reviewboard.mozilla.org/r/62976/#review61830

> ....?

A couple of the overloads can't actually throw, but the ones taking HTML elements such as HTMLImageElement can throw, and throws-or-not marking must match for our generator.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ah, ok.  "This overload can't actually throw" would be a better comment then.

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8768996 [details]
Bug 1280499 - Allow SKIP_ROWS+height>IMAGE_HEIGHT and fix paranoid uploading. -

https://reviewboard.mozilla.org/r/62976/#review61834

Comment 17

[Jeff Muizelaar [:jrmuizel]]

https://reviewboard.mozilla.org/r/62980/#review62374

::: dom/canvas/WebGLTextureUpload.cpp:410
(Diff revision 1)
>                              GLint yOffset, GLint zOffset, GLenum unpackFormat,
>                              GLenum unpackType, dom::ImageData* imageData)
>  {
> +    const bool usePBOs = false;
>      webgl::PackingInfo pi;
> -    if (mContext->ValidateUnpackInfo(funcName, unpackFormat, unpackType, &pi))
> +    if (!mContext->ValidateUnpackInfo(funcName, usePBOs, unpackFormat, unpackType, &pi))

Was the missing '!' a bug that was introduced earlier?

::: dom/canvas/WebGLTextureUpload.cpp:1329
(Diff revision 1)
>      auto dstFormat = dstUsage->format;
>  
>      if (!ValidateTargetForFormat(funcName, mContext, target, dstFormat))
>          return;
>  
> +    const bool hasData = blob->HasData();

This change seems unrelated. The larger the patch the higher the cost of including these unrelated changes is on the reviewer.

Comment 18

[Kelsey Gilbert [:jgilbert]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #17)
> https://reviewboard.mozilla.org/r/62980/#review62374
> 
> ::: dom/canvas/WebGLTextureUpload.cpp:410
> (Diff revision 1)
> >                              GLint yOffset, GLint zOffset, GLenum unpackFormat,
> >                              GLenum unpackType, dom::ImageData* imageData)
> >  {
> > +    const bool usePBOs = false;
> >      webgl::PackingInfo pi;
> > -    if (mContext->ValidateUnpackInfo(funcName, unpackFormat, unpackType, &pi))
> > +    if (!mContext->ValidateUnpackInfo(funcName, usePBOs, unpackFormat, unpackType, &pi))
> 
> Was the missing '!' a bug that was introduced earlier?
Yes.

Comment 19

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8768998 [details]
Bug 1280499 - Implement PBOs for textures. -

https://reviewboard.mozilla.org/r/62980/#review62992

Comment 20

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8769000 [details]
Bug 1280499 - Support paranoid uploading for nVidia. -

https://reviewboard.mozilla.org/r/62984/#review62996

Comment 21

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8768996 [details]
Bug 1280499 - Allow SKIP_ROWS+height>IMAGE_HEIGHT and fix paranoid uploading. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62976/diff/1-2/

Comment 22

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8768996 [details]
Bug 1280499 - Allow SKIP_ROWS+height>IMAGE_HEIGHT and fix paranoid uploading. -

https://reviewboard.mozilla.org/r/62976/#review63722

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/76ebe845bed9
Add webidl. - r=khuey
https://hg.mozilla.org/integration/mozilla-inbound/rev/4be2f9c41dd3
Add stubs and forwards. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/1326de2b7d1f
Implement PBOs for textures. - r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/07259f1b5eb7
Unlock UNPACK_BUFFER. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaf7778fef47
Support paranoid uploading for nVidia. - r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e90e83343b6
Allow SKIP_ROWS+height>IMAGE_HEIGHT and fix paranoid uploading. - r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/19b827d6c489
Mark tests.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/76ebe845bed9
https://hg.mozilla.org/mozilla-central/rev/4be2f9c41dd3
https://hg.mozilla.org/mozilla-central/rev/1326de2b7d1f
https://hg.mozilla.org/mozilla-central/rev/07259f1b5eb7
https://hg.mozilla.org/mozilla-central/rev/eaf7778fef47
https://hg.mozilla.org/mozilla-central/rev/9e90e83343b6
https://hg.mozilla.org/mozilla-central/rev/19b827d6c489

Comment 25

[Florian Scholz (Open Web Docs)]

Mentioned on:
https://developer.mozilla.org/en-US/Firefox/Releases/50#WebGL

The method signatures in the reference also mention the WebGL2-only gl.PIXEL_UNPACK_BUFFER:
https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/bindBuffer
https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/texImage2D
https://developer.mozilla.org/en-US/docs/Web/API/WebGL2RenderingContext/texImage3D
https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/texSubImage2D
https://developer.mozilla.org/en-US/docs/Web/API/WebGL2RenderingContext/texSubImage3D