1980264 - Canvas randomization is too slow

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, enhancement)

Description

[Tom Ritter [:tjr]]

In Bug 1980261 we profiled some private canvas rendering examples that were causing bad performance due to randomization. It's been a long-standing problem that we calculate the canvaskey over the entire image data, causing us to read all the image data multiple times - we can make this more efficient moving it later in the pipeline. We should to implement it under a separate RFPTarget (call it 'Efficient' or something) so we can pull the escape cord if we misjudged things.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:lsalzman, could you have a look please?

For more information, please visit BugBot documentation.

Comment 2

[Tom Ritter [:tjr]]

We are targeting next cycle for this.

Comment 3

[Tom Ritter [:tjr]]

Attachment: Bug 1980264: Clang Formatting and a missing Permission Check r?timhuang

The other place we return ImageExtraction::Randomize we have a
permission check - if you've granted the canvas permission
the result should not be randomized. It was missing in this
location though.

Comment 4

[Tom Ritter [:tjr]]

Attachment: Bug 1980264: Rename a variable to be more generic r?timhuang

ImageExtractionResult can be one of three values, soon to be four,
so do not call its result 'spoofing' as that's too specific.

Comment 5

[Tom Ritter [:tjr]]

Attachment: Bug 1980264: Plumb metatadata down to the Image Encoders

Image Encoders will be able to make use of a metadata
(aka randomization) key that will influence the resulting output
image.

This patch just adds that variable into all the API surfaces it
will need to be in to be accessible at the correct layer where
we will use it.

Comment 6

[Tom Ritter [:tjr]]

Attachment: Bug 1980264: Wire up an alternate efficient randomization layer

In the RFPService we expose functions that let us get the
Randomization Key for a context, and then later to take that
key and mix it with the existing image data hash.

At the hottest paths for image randomization, switch from using
the old randomization to the new, more efficient version.

Comment 7

[Tom Ritter [:tjr]]

Attachment: Bug 1980264: Implement a streaming randomization approach for PNG images

As we read the resulting image, hash the bytes (eliminating the
need to read them twice), then modify the resulting image based
on the randomization key and the image hash.

Comment 8

[Tom Ritter [:tjr]]

Attachment: Bug 1980264: Test Infra Fixes r?timhuang

These weren't breaking things before, but they were incorrect and now they broke things.

Comment 9

[Tom Ritter [:tjr]]

Attachment: Bug 1980264: Add a test r?timhuang

Comment 10

[Tom Ritter [:tjr]]

Attachment: Bug 1980264: Clang Formatting and a missing Permission Check r?timhuang

The other place we return ImageExtraction::Randomize we have a
permission check - if you've granted the canvas permission
the result should not be randomized. It was missing in this
location though.

Comment 11

[Tom Ritter [:tjr]]

Attachment: Bug 1980264: Rename a variable to be more generic r?timhuang

ImageExtractionResult can be one of three values, soon to be four,
so do not call its result 'spoofing' as that's too specific.

Comment 12

[Tom Ritter [:tjr]]

Attachment: Bug 1980264: Plumb metatadata down to the Image Encoders

Image Encoders will be able to make use of a metadata
(aka randomization) key that will influence the resulting output
image.

This patch just adds that variable into all the API surfaces it
will need to be in to be accessible at the correct layer where
we will use it.

Comment 13

[Tom Ritter [:tjr]]

Attachment: Bug 1980264: Wire up an alternate efficient randomization layer

In the RFPService we expose functions that let us get the
Randomization Key for a context, and then later to take that
key and mix it with the existing image data hash.

At the hottest paths for image randomization, switch from using
the old randomization to the new, more efficient version.

Comment 14

[Tom Ritter [:tjr]]

Attachment: Bug 1980264: Implement a streaming randomization approach for PNG images

As we read the resulting image, hash the bytes (eliminating the
need to read them twice), then modify the resulting image based
on the randomization key and the image hash.

Comment 15

[Tom Ritter [:tjr]]

Attachment: Bug 1980264: Test Infra Fixes r?timhuang

These weren't breaking things before, but they were incorrect and now they broke things.

Comment 16

[Tom Ritter [:tjr]]

Attachment: Bug 1980264: Add a test r?timhuang

Comment 17

[Phabricator Automation]

Comment on attachment 9517827 [details]
Bug 1980264: Clang Formatting and a missing Permission Check r?timhuang

Revision D265364 was moved to bug 1980261. Setting attachment 9517827 [details] to obsolete.

Comment 18

[Phabricator Automation]

Comment on attachment 9517828 [details]
Bug 1980264: Rename a variable to be more generic r?timhuang

Revision D265365 was moved to bug 1980261. Setting attachment 9517828 [details] to obsolete.

Comment 19

[Phabricator Automation]

Comment on attachment 9517829 [details]
Bug 1980264: Plumb metatadata down to the Image Encoders

Revision D265366 was moved to bug 1980261. Setting attachment 9517829 [details] to obsolete.

Comment 20

[Phabricator Automation]

Comment on attachment 9517830 [details]
Bug 1980264: Wire up an alternate efficient randomization layer

Revision D265367 was moved to bug 1980261. Setting attachment 9517830 [details] to obsolete.

Comment 21

[Phabricator Automation]

Comment on attachment 9517831 [details]
Bug 1980264: Implement a streaming randomization approach for PNG images

Revision D265368 was moved to bug 1980261. Setting attachment 9517831 [details] to obsolete.

Comment 22

[Phabricator Automation]

Comment on attachment 9517832 [details]
Bug 1980264: Test Infra Fixes r?timhuang

Revision D267088 was moved to bug 1980261. Setting attachment 9517832 [details] to obsolete.

Comment 23

[Phabricator Automation]

Comment on attachment 9517833 [details]
Bug 1980264: Add a test r?timhuang

Revision D267271 was moved to bug 1980261. Setting attachment 9517833 [details] to obsolete.

Comment 24

[Tom Ritter [:tjr]]

Going to try and stick this - there were intermittent failures in try; but all of them worked on at least one run, and were completely unrelated tests so I think they're unrelated.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/f3c68965b8f7
https://hg.mozilla.org/integration/autoland/rev/bce006e0b2f0
Clang Formatting and a missing Permission Check r=timhuang
https://github.com/mozilla-firefox/firefox/commit/b6b02cefea71
https://hg.mozilla.org/integration/autoland/rev/07e97c6ccb5c
Rename a variable to be more generic r=timhuang
https://github.com/mozilla-firefox/firefox/commit/348790428488
https://hg.mozilla.org/integration/autoland/rev/070da823790b
Plumb metatadata down to the Image Encoders r=tnikkel
https://github.com/mozilla-firefox/firefox/commit/a8a220b58adf
https://hg.mozilla.org/integration/autoland/rev/2fe4d5f71162
Wire up an alternate efficient randomization layer r=timhuang
https://github.com/mozilla-firefox/firefox/commit/5910c78f6181
https://hg.mozilla.org/integration/autoland/rev/7bb24ab2bbb2
Implement a streaming randomization approach for PNG images r=tnikkel
https://github.com/mozilla-firefox/firefox/commit/992c24e4cf66
https://hg.mozilla.org/integration/autoland/rev/3e45d968419a
Test Infra Fixes r=timhuang
https://github.com/mozilla-firefox/firefox/commit/d4cf73ac9411
https://hg.mozilla.org/integration/autoland/rev/b579e0274961
Add a test r=timhuang

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/d749d6e28edf
https://hg.mozilla.org/integration/autoland/rev/9dc00f59a927
Revert "Bug 1980264: Add a test r=timhuang" for causing xpcshell failures on test_encoder_png.js

Comment 27

[Cristina Horotan [:chorotan]]

Backed out for causing xpcshell failures on test_encoder_png.js

Backout link

Push with failures

Failure log

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/8d934a4e7d1d
https://hg.mozilla.org/integration/autoland/rev/22bb89149d6e
Clang Formatting and a missing Permission Check r=timhuang
https://github.com/mozilla-firefox/firefox/commit/585dbdc65e01
https://hg.mozilla.org/integration/autoland/rev/fbe5677a6f5e
Rename a variable to be more generic r=timhuang
https://github.com/mozilla-firefox/firefox/commit/7e5eaaabc7b4
https://hg.mozilla.org/integration/autoland/rev/895a17d730c5
Plumb metatadata down to the Image Encoders r=tnikkel
https://github.com/mozilla-firefox/firefox/commit/27fdfda522bb
https://hg.mozilla.org/integration/autoland/rev/ab828196a118
Wire up an alternate efficient randomization layer r=timhuang
https://github.com/mozilla-firefox/firefox/commit/f31c5d7aa74d
https://hg.mozilla.org/integration/autoland/rev/f11416295204
Implement a streaming randomization approach for PNG images r=tnikkel
https://github.com/mozilla-firefox/firefox/commit/10ef6a8971cf
https://hg.mozilla.org/integration/autoland/rev/23a4f669a204
Test Infra Fixes r=timhuang
https://github.com/mozilla-firefox/firefox/commit/546cf0627eb7
https://hg.mozilla.org/integration/autoland/rev/321202a9c030
Add a test r=timhuang

Comment 29

[amarc]

https://hg.mozilla.org/mozilla-central/rev/22bb89149d6e
https://hg.mozilla.org/mozilla-central/rev/fbe5677a6f5e
https://hg.mozilla.org/mozilla-central/rev/895a17d730c5
https://hg.mozilla.org/mozilla-central/rev/ab828196a118
https://hg.mozilla.org/mozilla-central/rev/f11416295204
https://hg.mozilla.org/mozilla-central/rev/23a4f669a204
https://hg.mozilla.org/mozilla-central/rev/321202a9c030