Refactor Canvas noise injection logic and improve support for gradients in post-processed ImageData #13199

sysrqb · 2023-04-26T17:38:23Z

`709720d`

Refactor Canvas noise injection logic and improve support for gradients in post-processed ImageData
https://bugs.webkit.org/show_bug.cgi?id=255993
rdar://107371244

Reviewed by NOBODY (OOPS!).

Gradients present an interesting problem when post-processing canvas ImageData
because tweaking colors result in significant visual anomalies. This change
introduces some naive gradient detection by looking at all of the immediate
neighbors of a pixel. We decide that a pixel is within a gradient if the
current color is between its opposing adjacent colors (e.g., above and below,
left and right, etc). If a color is decided to be within a gradient, then we
set those adjacent colors as bounds within which we can tweak the current
color.

In addition, within this change, we move the noise injection logic into its own
class, it removes the dependency on supplied colors for post-processing, and it
reduces the magnitude of noise from ~5% to ~1%.

* Source/WebCore/Headers.cmake:
* Source/WebCore/Sources.txt:
* Source/WebCore/WebCore.xcodeproj/project.pbxproj:
* Source/WebCore/html/CanvasBase.cpp:
(WebCore::CanvasBase::makeRenderingResultsAvailable):
(WebCore::CanvasBase::didDraw): We clip the rect before passing it into
CanvasNoiseInjection, and if the rect is std::nullopt then we pass in the
entire canvas.

(WebCore::CanvasBase::postProcessPixelBufferResults const):
(WebCore::CanvasBase::postProcessDirtyCanvasBuffer const): Deleted.
* Source/WebCore/html/CanvasBase.h:
* Source/WebCore/html/CanvasNoiseInjection.cpp: Added.
(WebCore::CanvasNoiseInjection::updateDirtyRect):
(WebCore::isIndexInBounds):
(WebCore::setTightnessBounds):
(WebCore::getGradientNeighbors):
(WebCore::CanvasNoiseInjection::postProcessDirtyCanvasBuffer):
(WebCore::CanvasNoiseInjection::postProcessPixelBufferResults const):
* Source/WebCore/html/CanvasNoiseInjection.h: Added.
* Source/WebCore/html/HTMLCanvasElement.cpp:
(WebCore::HTMLCanvasElement::getImageData):

webkit-early-warning-system · 2023-04-26T17:41:08Z

EWS run on previous version of this PR (hash c424770)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	❌ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	❌ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	❌ 🛠 gtk
	❌ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	❌ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	❌ 🧪 api-gtk
	✅ 🛠 tv	❌ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim
	❌ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-05-03T04:10:37Z

EWS run on previous version of this PR (hash 0d9701f)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	❌ 🛠 ios	❌ 🛠 mac	❌ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	❌ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	❌ 🧪 api-mac	❌ 🛠 gtk
	❌ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	❌ 🧪 gtk-wk2
	❌ 🧪 api-ios	❌ 🧪 mac-wk2	❌ 🧪 api-gtk
	❌ 🛠 tv	✅ 🧪 mac-AS-debug-wk2
	❌ 🛠 tv-sim	❌ 🧪 mac-wk2-stress
	❌ 🛠 watch
	❌ 🛠 watch-sim

webkit-early-warning-system · 2023-05-09T22:47:37Z

EWS run on previous version of this PR (hash 32396d9)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	❌ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	❌ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	❌ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	❌ 🧪 api-gtk
	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 tv-sim
	❌ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-05-10T00:12:39Z

EWS run on previous version of this PR (hash 9747080)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

kkinnunen-apple · 2023-05-10T08:42:18Z

Source/WebCore/html/CanvasBase.cpp

@@ -373,6 +374,203 @@ bool CanvasBase::shouldInjectNoiseBeforeReadback() const
    return scriptExecutionContext() && scriptExecutionContext()->noiseInjectionHashSalt();
 }

+struct RGBAColor {


I'd be hesitant to add yet-another color type. Mostly this seems to be usable as uint32_t?
E.g ordering seems to be uint32_t ordering.

Alternatively you could perhaps try to use existing ColorComponents<> or something?

kkinnunen-apple · 2023-05-10T08:43:51Z

Source/WebCore/html/CanvasBase.cpp

+    }
+    return tweak;
+}
+


This noise algorithm now adds significant noise to already complex CanvasBase, doubling its size, and still it's a bit peripheral to the Canvas functionality.

Could we encapsulate the noise injection into a new file, such as CanvasNoiseInjection.h/cpp?

kkinnunen-apple · 2023-05-10T08:47:01Z

Source/WebCore/html/CanvasBase.h

@@ -90,6 +90,8 @@ class CanvasBase {
    void setOriginTainted() { m_originClean = false; }
    bool originClean() const { return m_originClean; }

+    void setHadGradientFill() { m_hadGradientFill = true; }


Adding custom ad-hoc state to the CanvasBase will result in hard to maintain Canvas implementation down the line.
Adding ad-hoc state to the algorithm creates a problem where you need to run security analysis on each possible state.
Would it be possible to implement one, stateless algorithm that would be good?

If the gradient algorithm is good enough for injecting noise for the gradient case, could it be used also in non-gradient case? This would fix the corner-cases where a gradient would be drawn to a canvas a, canvas a to canvas b, and canvas b would be read back, injecting the noise in b read.

kkinnunen-apple · 2023-05-10T08:49:02Z

Source/WebCore/html/CanvasBase.cpp

+    bool isInLeftColumn = !(pixelIndex % size.width());
+    bool isInRightColumn = (pixelIndex % size.width()) == static_cast<unsigned>(size.width()) - 1;
+    bool isInTopLeftCorner = isInTopRow && isInLeftColumn;
+    bool isInBotomLeftCorner = isInBottomRow && isInLeftColumn;


kkinnunen-apple · 2023-05-10T08:52:39Z

Source/WebCore/html/CanvasBase.cpp

+    return { componentAt(offset), componentAt(offset + greenOffsetFromIndex), componentAt(offset + blueOffsetFromIndex), componentAt(offset + alphaOffsetFromIndex) };
+}
+
+static std::optional<std::pair<size_t, size_t>> getGradientNeighbors(size_t index, const Ref<PixelBuffer>&& pixelBuffer)


const Ref&& doesn't make much sense.
I think this means PixelBuffer&

kkinnunen-apple · 2023-05-10T08:56:19Z

Source/WebCore/html/CanvasBase.cpp

@@ -411,6 +609,8 @@ bool CanvasBase::postProcessPixelBufferResults(Ref<PixelBuffer>&& pixelBuffer, c
    HashMap<uint32_t, uint32_t> pixelColorMap;
    bool wasPixelBufferModified { false };

+    HashMap<size_t, std::pair<size_t, size_t>> gradientIndiceMap;


I don't quite understand this map.
If I have 2000x2000 canvas with a full-screen gradient, does this map get 4 million entries? Sounds that it doesn't work in general case.

I'm sure a fast algorithm to add visually non-distracting noise to a picture has been a solved research question for a while now. Maybe we could just look up a proper scientific noise kernel from some research paper and implement that?

webkit-early-warning-system · 2023-05-11T06:10:41Z

Starting EWS tests for 6c7d857. Live statuses available at the PR page, #13199

webkit-early-warning-system · 2023-05-11T06:10:42Z

EWS run on previous version of this PR (hash 6c7d857)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	❌ 🛠 ios	❌ 🛠 mac	❌ 🛠 wpe	❌ 🛠 wincairo
❌ 🧪 bindings	❌ 🛠 ios-sim	❌ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
❌ 🧪 webkitperl	❌ 🧪 ios-wk2	❌ 🧪 api-mac	❌ 🛠 gtk
	❌ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	❌ 🧪 gtk-wk2
	❌ 🧪 api-ios	❌ 🧪 mac-wk2	❌ 🧪 api-gtk
	❌ 🛠 tv	❌ 🧪 mac-AS-debug-wk2
	❌ 🛠 tv-sim	❌ 🧪 mac-wk2-stress
	❌ 🛠 watch
	❌ 🛠 watch-sim

webkit-early-warning-system · 2023-05-11T06:22:11Z

EWS run on previous version of this PR (hash a3b1808)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

kkinnunen-apple · 2023-05-11T07:37:14Z

Source/WebCore/html/CanvasNoiseInjection.cpp

+    }
+}
+
+bool CanvasNoiseInjection::postProcessPixelBufferResults(Ref<PixelBuffer>&& pixelBuffer, NoiseInjectionHashSalt salt) const


Here we're saying "this method may take ownership of the reference to the pixel buffer."

The method actually never takes the ownership of the reference.
So the signature should just be PixelBuffer& which means "this method uses the pixel buffer".

kkinnunen-apple · 2023-05-11T07:40:29Z

Source/WebCore/html/CanvasBase.h

    mutable Lock m_imageBufferAssignmentLock;
    mutable RefPtr<ImageBuffer> m_imageBuffer;
    mutable size_t m_imageBufferCost { 0 };
    mutable std::unique_ptr<GraphicsContextStateSaver> m_contextStateSaver;
+    mutable CanvasNoiseInjection m_canvasNoiseInjection;


It seems the CanvasNoiseInjection doesn't need to be mutable, as postprocessdirty is only used in mutable scope.

webkit-early-warning-system · 2023-05-11T08:13:14Z

EWS run on current version of this PR (hash 60e7230)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
🧪 style	🛠 ios	🛠 mac	🛠 wpe	🛠 wincairo
🧪 bindings	🛠 ios-sim	🛠 mac-AS-debug	🧪 wpe-wk2
🧪 webkitperl	🧪 ios-wk2	🧪 api-mac	🛠 gtk
	🧪 ios-wk2-wpt	🧪 mac-wk1	🧪 gtk-wk2
⏳ 🛠 🧪 jsc	🧪 api-ios	🧪 mac-wk2	🧪 api-gtk
	🛠 tv	🧪 mac-AS-debug-wk2
	🛠 tv-sim	🧪 mac-wk2-stress
	🛠 watch
	🛠 watch-sim

webkit-early-warning-system · 2023-05-11T08:13:17Z

EWS run on previous version of this PR (hash 60e7230)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-05-11T16:44:23Z

EWS run on current version of this PR (hash 59fa2ec)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	~~🧪 gtk-wk2~~
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	❌ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

sysrqb · 2023-05-11T18:55:56Z

Now I'm confident enough that the gtk/wpe failures are flakey.

…ts in post-processed ImageData https://bugs.webkit.org/show_bug.cgi?id=255993 rdar://107371244 Reviewed by Kimmo Kinnunen. Gradients present an interesting problem when post-processing canvas ImageData because tweaking colors result in significant visual anomalies. This change introduces some naive gradient detection by looking at all of the immediate neighbors of a pixel. We decide that a pixel is within a gradient if the current color is between its opposing adjacent colors (e.g., above and below, left and right, etc). If a color is decided to be within a gradient, then we set those adjacent colors as bounds within which we can tweak the current color. In addition, within this change, we move the noise injection logic into its own class, it removes the dependency on supplied colors for post-processing, and it reduces the magnitude of noise from ~5% to ~1%. * Source/WebCore/Headers.cmake: * Source/WebCore/Sources.txt: * Source/WebCore/WebCore.xcodeproj/project.pbxproj: * Source/WebCore/html/CanvasBase.cpp: (WebCore::CanvasBase::makeRenderingResultsAvailable): (WebCore::CanvasBase::didDraw): We clip the rect before passing it into CanvasNoiseInjection, and if the rect is std::nullopt then we pass in the entire canvas. (WebCore::CanvasBase::postProcessPixelBufferResults const): (WebCore::CanvasBase::postProcessDirtyCanvasBuffer const): Deleted. * Source/WebCore/html/CanvasBase.h: * Source/WebCore/html/CanvasNoiseInjection.cpp: Added. (WebCore::CanvasNoiseInjection::updateDirtyRect): (WebCore::isIndexInBounds): (WebCore::setTightnessBounds): (WebCore::getGradientNeighbors): (WebCore::CanvasNoiseInjection::postProcessDirtyCanvasBuffer): (WebCore::CanvasNoiseInjection::postProcessPixelBufferResults const): * Source/WebCore/html/CanvasNoiseInjection.h: Added. * Source/WebCore/html/HTMLCanvasElement.cpp: (WebCore::HTMLCanvasElement::getImageData): Canonical link: https://commits.webkit.org/263979@main

webkit-commit-queue · 2023-05-11T19:13:12Z

Committed 263979@main (eabcb3e): https://commits.webkit.org/263979@main

Reviewed commits have been landed. Closing PR #13199 and removing active labels.

sysrqb self-assigned this Apr 26, 2023

sysrqb added the New Bugs Unclassified bugs are placed in this component until the correct component can be determined. label Apr 26, 2023

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 26, 2023

sysrqb removed the merging-blocked Applied to prevent a change from being merged label May 3, 2023

sysrqb force-pushed the eng/255993 branch from c424770 to 0d9701f Compare May 3, 2023 04:04

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 3, 2023

sysrqb removed the merging-blocked Applied to prevent a change from being merged label May 9, 2023

sysrqb force-pushed the eng/255993 branch from 0d9701f to 32396d9 Compare May 9, 2023 22:45

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 9, 2023

sysrqb removed the merging-blocked Applied to prevent a change from being merged label May 10, 2023

sysrqb force-pushed the eng/255993 branch from 32396d9 to 9747080 Compare May 10, 2023 00:12

sysrqb requested review from grorg and kkinnunen-apple May 10, 2023 00:14

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 10, 2023

kkinnunen-apple reviewed May 10, 2023

View reviewed changes

sysrqb removed the merging-blocked Applied to prevent a change from being merged label May 11, 2023

sysrqb changed the title ~~Improve support for gradient in post-processed ImageData~~ Refactor Canvas noise injection logic and improve support for gradients in post-processed ImageData May 11, 2023

sysrqb force-pushed the eng/255993 branch from 9747080 to 6c7d857 Compare May 11, 2023 06:10

sysrqb force-pushed the eng/255993 branch from 6c7d857 to a3b1808 Compare May 11, 2023 06:21

sysrqb marked this pull request as ready for review May 11, 2023 06:35

sysrqb requested review from cdumez and rniwa as code owners May 11, 2023 06:35

kkinnunen-apple approved these changes May 11, 2023

View reviewed changes

sysrqb force-pushed the eng/255993 branch from a3b1808 to 60e7230 Compare May 11, 2023 08:12

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 11, 2023

sysrqb removed the merging-blocked Applied to prevent a change from being merged label May 11, 2023

sysrqb force-pushed the eng/255993 branch from 60e7230 to 59fa2ec Compare May 11, 2023 16:44

sysrqb added the merge-queue Applied to send a pull request to merge-queue label May 11, 2023

webkit-ews-buildbot added merging-blocked Applied to prevent a change from being merged and removed merge-queue Applied to send a pull request to merge-queue labels May 11, 2023

sysrqb added merge-queue Applied to send a pull request to merge-queue merging-blocked Applied to prevent a change from being merged and removed merging-blocked Applied to prevent a change from being merged merge-queue Applied to send a pull request to merge-queue labels May 11, 2023

webkit-commit-queue force-pushed the eng/255993 branch from 59fa2ec to eabcb3e Compare May 11, 2023 19:13

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 11, 2023

webkit-commit-queue merged commit eabcb3e into WebKit:main May 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Canvas noise injection logic and improve support for gradients in post-processed ImageData #13199

Refactor Canvas noise injection logic and improve support for gradients in post-processed ImageData #13199

sysrqb commented Apr 26, 2023 •

edited

webkit-early-warning-system commented Apr 26, 2023 •

edited

webkit-early-warning-system commented May 3, 2023 •

edited

webkit-early-warning-system commented May 9, 2023 •

edited

webkit-early-warning-system commented May 10, 2023 •

edited

kkinnunen-apple May 10, 2023

kkinnunen-apple May 10, 2023

kkinnunen-apple May 10, 2023

kkinnunen-apple May 10, 2023

kkinnunen-apple May 10, 2023

kkinnunen-apple May 10, 2023 •

edited

kkinnunen-apple May 10, 2023

webkit-early-warning-system commented May 11, 2023

webkit-early-warning-system commented May 11, 2023 •

edited

webkit-early-warning-system commented May 11, 2023 •

edited

kkinnunen-apple May 11, 2023

kkinnunen-apple May 11, 2023

webkit-early-warning-system commented May 11, 2023 •

edited

webkit-early-warning-system commented May 11, 2023 •

edited

webkit-early-warning-system commented May 11, 2023 •

edited

sysrqb commented May 11, 2023

webkit-commit-queue commented May 11, 2023

Refactor Canvas noise injection logic and improve support for gradients in post-processed ImageData #13199

Refactor Canvas noise injection logic and improve support for gradients in post-processed ImageData #13199

Conversation

sysrqb commented Apr 26, 2023 • edited

webkit-early-warning-system commented Apr 26, 2023 • edited

webkit-early-warning-system commented May 3, 2023 • edited

webkit-early-warning-system commented May 9, 2023 • edited

webkit-early-warning-system commented May 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkinnunen-apple May 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented May 11, 2023

webkit-early-warning-system commented May 11, 2023 • edited

webkit-early-warning-system commented May 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented May 11, 2023 • edited

webkit-early-warning-system commented May 11, 2023 • edited

webkit-early-warning-system commented May 11, 2023 • edited

sysrqb commented May 11, 2023

webkit-commit-queue commented May 11, 2023

sysrqb commented Apr 26, 2023 •

edited

webkit-early-warning-system commented Apr 26, 2023 •

edited

webkit-early-warning-system commented May 3, 2023 •

edited

webkit-early-warning-system commented May 9, 2023 •

edited

webkit-early-warning-system commented May 10, 2023 •

edited

kkinnunen-apple May 10, 2023 •

edited

webkit-early-warning-system commented May 11, 2023 •

edited

webkit-early-warning-system commented May 11, 2023 •

edited

webkit-early-warning-system commented May 11, 2023 •

edited

webkit-early-warning-system commented May 11, 2023 •

edited

webkit-early-warning-system commented May 11, 2023 •

edited