Closed Bug 1280499 Opened 8 years ago Closed 8 years ago

Implement TexImage for PBOs (PIXEL_UNPACK_BUFFER)

Categories

(Core :: Graphics: CanvasWebGL, defect)

Product:
Core
Component:
Graphics: CanvasWebGL
Version:
49 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 4 obsolete files)

Bug 1280499 - Allow SKIP_ROWS+height>IMAGE_HEIGHT and fix paranoid uploading. -
58 bytes, text/x-review-board-request
jrmuizel
: review+
Details 
      No description provided.
So step one is to write the IDL, actually.
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)
Attachment #8768998 - Flags: review?(ethlin)
Attachment #8769000 - Flags: review?(ethlin)
Depends on: 1281098
Blocks: webgl2-blockers
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.

....?
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+
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.
(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+
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)
Attachment #8768997 - Attachment is obsolete: true
Attachment #8768998 - Attachment is obsolete: true
Attachment #8768999 - Attachment is obsolete: true
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.

Attachment

General

Creator:
Created:
Updated:
Size: