Implement TexImage for PBOs (PIXEL_UNPACK_BUFFER)

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

({dev-doc-complete})

49 Branch
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Assignee

Description

3 years ago
No description provided.
Assignee

Comment 1

3 years ago
So step one is to write the IDL, actually.
Assignee

Comment 2

3 years ago
Review commit: https://reviewboard.mozilla.org/r/62976/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62976/
Attachment #8768996 - Flags: review?(khuey)
Attachment #8768997 - Flags: review?(ethlin)
Attachment #8768998 - Flags: review?(jmuizelaar)
Attachment #8768999 - Flags: review?(ethlin)
Attachment #8769000 - Flags: review?(jmuizelaar)
Assignee

Updated

3 years ago
Attachment #8768998 - Flags: review?(ethlin)
Attachment #8769000 - Flags: review?(ethlin)
Assignee

Updated

3 years ago
Depends on: 1281098
Assignee

Updated

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

https://reviewboard.mozilla.org/r/62982/#review60552
Attachment #8768999 - Flags: review?(ethlin) → review+
Comment on attachment 8768997 [details]
Bug 1280499 - Add stubs and forwards. -

https://reviewboard.mozilla.org/r/62978/#review60600
Attachment #8768997 - Flags: review?(ethlin) → review+
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.
Attachment #8768998 - Flags: review?(ethlin) → review+
Comment on attachment 8769000 [details]
Bug 1280499 - Support paranoid uploading for nVidia. -

https://reviewboard.mozilla.org/r/62984/#review60970
Attachment #8769000 - Flags: review?(ethlin) → review+
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.

....?
Assignee

Comment 14

3 years ago
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.
Ah, ok.  "This overload can't actually throw" would be a better comment then.
Comment on attachment 8768996 [details]
Bug 1280499 - Allow SKIP_ROWS+height>IMAGE_HEIGHT and fix paranoid uploading. -

https://reviewboard.mozilla.org/r/62976/#review61834
Attachment #8768996 - Flags: review?(khuey) → review+
Assignee

Updated

3 years ago
Flags: needinfo?(jgilbert)
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.
Assignee

Comment 18

3 years ago
(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 on attachment 8768998 [details]
Bug 1280499 - Implement PBOs for textures. -

https://reviewboard.mozilla.org/r/62980/#review62992
Attachment #8768998 - Flags: review?(jmuizelaar) → review+
Attachment #8769000 - Flags: review?(jmuizelaar) → review+
Assignee

Comment 21

3 years ago
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/
Attachment #8768996 - Attachment description: Bug 1280499 - Add webidl. - → Bug 1280499 - Allow SKIP_ROWS+height>IMAGE_HEIGHT and fix paranoid uploading. -
Attachment #8768996 - Flags: review+ → review?(jmuizelaar)
Assignee

Updated

3 years ago
Attachment #8768997 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8768998 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8768999 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8769000 - Attachment is obsolete: true
Attachment #8768996 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8768996 [details]
Bug 1280499 - Allow SKIP_ROWS+height>IMAGE_HEIGHT and fix paranoid uploading. -

https://reviewboard.mozilla.org/r/62976/#review63722
You need to log in before you can comment on or make changes to this bug.