Closed Bug 1281098 Opened 3 years ago Closed 3 years ago

Fix UNPACK_ params

Categories

(Core :: Canvas: WebGL, defect, critical)

49 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

References

()

Details

Attachments

(4 files)

No description provided.
Severity: normal → critical
Summary: Crash in conformance2/textures/misc/tex-unpack-params.html → Fix UNPACK_ params
Review commit: https://reviewboard.mozilla.org/r/62208/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62208/
Attachment #8767793 - Flags: review?(jmuizelaar)
Attachment #8767794 - Flags: review?(ethlin)
Attachment #8767795 - Flags: review?(ethlin)
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 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 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 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.
Attachment #8767793 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8767794 [details]
Bug 1281098 - Disallow querying texture swizzle. -

https://reviewboard.mozilla.org/r/62210/#review59594
Attachment #8767794 - Flags: review?(ethlin) → review+
Attachment #8768210 - Flags: review?(ethlin) → review+
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
Attachment #8767795 - Flags: review?(ethlin) → review+
Pushed by jgilbert@mozilla.com:
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.
Blocks: 1280499
Blocks: 1286348
You need to log in before you can comment on or make changes to this bug.