1281098 - Fix UNPACK_ params

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1281098 - Fix UNPACK_ handling. -

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

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1281098 - Disallow querying texture swizzle. -

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

Comment 3

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1281098 - Cache LOCAL_GL_TEXTURE_COMPARE_MODE since we need it for validation. -

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

Comment 4

[Kelsey Gilbert [:jgilbert]]

This one has issues still:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9957c03a5e77&selectedJob=23334278

Comment 5

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8767793 [details]
Bug 1281098 - Fix UNPACK_ handling. -

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

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8767794 [details]
Bug 1281098 - Disallow querying texture swizzle. -

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

Comment 7

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8767795 [details]
Bug 1281098 - Cache LOCAL_GL_TEXTURE_COMPARE_MODE since we need it for validation. -

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

Comment 8

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1281098 - Fix failure case. -

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

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8767793 [details]
Bug 1281098 - Fix UNPACK_ handling. -

https://reviewboard.mozilla.org/r/62208/#review59436

::: dom/canvas/WebGLTextureUpload.cpp:61
(Diff revision 2)
>   */
>  
> +static bool
> +ValidateExtents(WebGLContext* webgl, const char* funcName, GLsizei width, GLsizei height,
> +                GLsizei depth, GLint border, uint32_t* const out_width,
> +                uint32_t* const out_height, uint32_t* const out_depth)

As a general comment a lot of the function in this file have a lot of parameters. It would be better if these sets of parameters were packaged up into appropriate aggregates.

This would make the method structures cleaner and would make returning Maybe<>s a better option than bags of out params. This makes it clearer which out params are set when.

::: dom/canvas/WebGLTextureUpload.cpp:267
(Diff revision 2)
> +                         &width, &height, &depth))
> +    {
> +        return;
> +    }
> +
> +    webgl::PackingInfo pi;

It'd be nice if we used a longer variable name than pi. Especially because when people see pi they usually think of 3.1415.

::: dom/canvas/WebGLTextureUpload.cpp:321
(Diff revision 2)
> +WebGLContext::GetUnpackValuesForImage(const char* funcName, uint32_t srcImageWidth,
> +                                      uint32_t srcImageHeight,
> +                                      uint32_t* const out_rowLength,
> +                                      uint32_t* const out_imageHeight)
> +{
> +    uint32_t rowLength = mPixelStore_UnpackRowLength;

How about the following instead:

if (mPixelStore_UnpackRowLength && mPixelStore_UnpackRowLength != srcImageWidth) {
   ErrorInvalidOperation("...");
   return false;
}

if (mPixelStore_UnpackImageHeight && mPixelStore_UnpackImageHeight != srcImageHeight) {
   ErrorInvalidOperation("...");
   return false;
}

*out_rowLength = srcImageWidth;
*out_imageHieght = srcImageHeight;

I think that makes it more clear what's going on.

Comment 10

[Ethan Lin[:ethlin]]

Comment on attachment 8767794 [details]
Bug 1281098 - Disallow querying texture swizzle. -

https://reviewboard.mozilla.org/r/62210/#review59594

Comment 11

[Ethan Lin[:ethlin]]

Comment on attachment 8768210 [details]
Bug 1281098 - Fix failure case. -

https://reviewboard.mozilla.org/r/62512/#review59604

Comment 12

[Ethan Lin[:ethlin]]

Comment on attachment 8767795 [details]
Bug 1281098 - Cache LOCAL_GL_TEXTURE_COMPARE_MODE since we need it for validation. -

https://reviewboard.mozilla.org/r/62212/#review59614

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8bdde3cb347
Fix UNPACK_ handling. - r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/67394a97b3cf
Disallow querying texture swizzle. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/27174fcdff4c
Cache LOCAL_GL_TEXTURE_COMPARE_MODE since we need it for validation. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/57118eba003d
Fix failure case.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a8bdde3cb347
https://hg.mozilla.org/mozilla-central/rev/67394a97b3cf
https://hg.mozilla.org/mozilla-central/rev/27174fcdff4c
https://hg.mozilla.org/mozilla-central/rev/57118eba003d