1323617 - WebGL conformance tests for canvas_sub_rectangle test fail

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect, P1)

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1323617 - Fix canvas_sub_rectangle tests. -

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

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1323617 - Don't assert for performance trap when srcPremult == dstPremult, even if the formats allow us to skip premult. -

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

Comment 3

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8818760 [details]
Bug 1323617 - Fix canvas_sub_rectangle tests. -

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

Comment 5

[Ethan Lin[:ethlin]]

Comment on attachment 8819114 [details]
Bug 1323617 - Don't assert for performance trap when srcPremult == dstPremult, even if the formats allow us to skip premult. -

https://reviewboard.mozilla.org/r/98956/#review99306

Comment 6

[Ethan Lin[:ethlin]]

Comment on attachment 8818760 [details]
Bug 1323617 - Fix canvas_sub_rectangle tests. -

https://reviewboard.mozilla.org/r/98702/#review99300

::: dom/canvas/TexUnpackBlob.cpp:248
(Diff revision 2)
> -                                funcName);
> -        return false;
> -    }
> -    const uint32_t skipBytes = offset.value();
>  
> -    auto const srcBegin = srcBytes + skipBytes;
> +    *out_begin = srcBegin;

Why not skipping bytes now?

::: dom/canvas/TexUnpackBlob.cpp:698
(Diff revision 2)
> +
> +    ////
>  
>      WebGLTexelFormat srcFormat;
>      uint8_t srcBPP;
> -    if (!GetFormatForSurf(mSurf, &srcFormat, &srcBPP)) {
> +    bool alphaPremultMatters;

Is 'alphaPremultMatters' an unnecessary variable?

Comment 7

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8818760 [details]
Bug 1323617 - Fix canvas_sub_rectangle tests. -

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

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8819114 [details]
Bug 1323617 - Don't assert for performance trap when srcPremult == dstPremult, even if the formats allow us to skip premult. -

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

Comment 9

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8818760 [details]
Bug 1323617 - Fix canvas_sub_rectangle tests. -

https://reviewboard.mozilla.org/r/98702/#review99300

> Why not skipping bytes now?

We just convert the whole source buffer now, instead of only what's used.

> Is 'alphaPremultMatters' an unnecessary variable?

Yes.

Comment 10

[Ethan Lin[:ethlin]]

Comment on attachment 8818760 [details]
Bug 1323617 - Fix canvas_sub_rectangle tests. -

https://reviewboard.mozilla.org/r/98702/#review99516

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49141c456780
Fix canvas_sub_rectangle tests. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e5513e34226
Don't assert for performance trap when srcPremult == dstPremult, even if the formats allow us to skip premult. - r=ethlin

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/49141c456780
https://hg.mozilla.org/mozilla-central/rev/6e5513e34226

Comment 13

[Vincent Liu[:vliu]]

I tried to send a try with the attached patch. The test result only covers the test cases in /canvas_sub_rectangle. From this link, I don't see any crash but still have fail items.


[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ad0cc87c00792c52548863f9408f890b2c304b4

Comment 14

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8818760 [details]
Bug 1323617 - Fix canvas_sub_rectangle tests. -

Approval Request Comment
[Feature/Bug causing the regression]: webgl2
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:

(this and other patches)

Comment 15

[Gerry Chang [:gchang]]

Comment on attachment 8818760 [details]
Bug 1323617 - Fix canvas_sub_rectangle tests. -

Fix a WebGL 2 related issue. Beta51+ and Aurora52+. Should be in 51 beta 10.

Comment 16

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/06637572b51f

Comment 17

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/releases/mozilla-beta/rev/904a33bad7c4