Refactor Canvas Post-Processing logic in CanvasBase #13003
Conversation
|
EWS run on previous version of this PR (hash ca1f3a0) |
sysrqb
added
the
New Bugs
sysrqb
changed the title
|
EWS run on previous version of this PR (hash 10ab969)
|
| if (!buffer) | ||
| return; | ||
|
|
||
| auto canvasRect = texImageSourceSize(canvasBase()); |
There was a problem hiding this comment.
The function 'tezImageSoruceSize' is incorrectly used. E.g. if you have a canvasBase, you know how to get its size without any additional function.
texImageSourceSize function is intended to be used for TexImage sources for arbitrary source T. The canvas here isn't used as texImageSource, it's just this canvas.
As you can observe, there's nothing webgl about this function. The logic of this function is:
Post process each pixel that changed after previous post process.
This applies to any canvas.
So one option would be that you would make following changes:
- Move CanvasRenderingContext::postProcessPixelBuffer() to CanvasBase::postProcessPixelBuffer()
- Rename CanvasBase::postProcessPixelBuffer() to CanvasBase::postProcessRenderingResults()
- Move
CanvasRenderingContext::postProcessPixelBuffer(ImageBuffer&, IntRect, const HashSet<uint32_t>&) to CanvasBase::postProcessPixelBuffer(ImageBuffer&, IntRect, const HashSet<uint32_t>&) - Introduce CanvasBase::didDraw(const std::optional& rect, SomeOptions) where you record m_postProcessDirtyRect
- Remove m_wasLastDrawPutImageData, this is superseeded by m_postProcessDirtyRect. Current m_wasLastDrawPutImageData doesn't seem to handle putImageData correctly, it will allow attacker to circumvent the post processing by calling with for example 1,1 region as per my reading of my checkout, I may be wrong though
- Make SomeOption to be able to get "SomeOption::TheDrawWasActuallyPutImageDataSoDoNotAccumulateThisToPostProcessingDirtyRect"
So in other words:
- canvas drawing contexts signal CanvasBase that "something changed, maintain the post processing state"
- CanvasBase already has the image buffer for the display buffer of the context. CanvasBase is already the sole entity giving the pixels out, with the exception of WebGL non-premul alpha PixelBuffer case. Make CanvasBase apply the post processing.
There was a problem hiding this comment.
Thanks for the review! I agree that your suggestion is better.
sysrqb
changed the title
|
EWS run on previous version of this PR (hash 70ae388)
|
|
EWS run on previous version of this PR (hash 4ab6b5f)
|
webkit-ews-buildbot
added
the
merging-blocked
|
EWS run on previous version of this PR (hash 32d2f84)
|
webkit-ews-buildbot
added
the
merging-blocked
Source/WebCore/html/CanvasBase.h
Outdated
| @@ -127,7 +130,10 @@ class CanvasBase { | |||
| virtual void dispatchEvent(Event&) = 0; | |||
|
|
|||
| bool shouldInjectNoiseBeforeReadback() const; | |||
| bool postProcessPixelBuffer(Ref<PixelBuffer>&&, bool, const HashSet<uint32_t>&) const; | |||
| void postProcessDirtyCanvasBuffer() const; | |||
There was a problem hiding this comment.
You could check which of these functions can be moved as private now.
Source/WebCore/html/CanvasBase.h
Outdated
| @@ -109,7 +112,7 @@ class CanvasBase { | |||
|
|
|||
| GraphicsClient* graphicsClient() const; | |||
|
|
|||
| virtual void didDraw(const std::optional<FloatRect>&) = 0; | |||
| virtual void didDraw(const std::optional<FloatRect>&, const ShouldApplyPostProcessingToDirtyRect = ShouldApplyPostProcessingToDirtyRect::Yes); | |||
There was a problem hiding this comment.
In normal C++, the default arguments for virtual functions are frowned upon, as they're hard to keep in sync.
typical suggestion is to add inline overload for the default call, in your case
void didDraw(const std::optional<FloatRect> rect) ( didDraw(rect, ...DirtyRect::Yes };
virtual void didDraw(const std::optional<FloatRect>&, ShouldApplyPostProcessingToDirtyRect);
This way you don't need the default argument to match in all the overrides.
(Note, WebKit C++ does have default parameters in the virtual functions in many places. Still, I'd argue adding more of these wouldn't be such a good idea)
| @@ -558,6 +558,8 @@ void HTMLCanvasElement::didDraw(const std::optional<FloatRect>& rect) | |||
| return; | |||
| } | |||
|
|
|||
| CanvasBase::didDraw(rect, shouldApplyPostProcessingToDirtyRect); | |||
There was a problem hiding this comment.
I think it would be consistent to forward the base class call first, and then do whatever logic the subclass needs.
| @@ -420,7 +420,7 @@ void OffscreenCanvas::convertToBlob(ImageEncodeOptions&& options, Ref<DeferredPr | |||
| promise->resolveWithNewlyCreated<IDLInterface<Blob>>(WTFMove(blob)); | |||
| } | |||
|
|
|||
| void OffscreenCanvas::didDraw(const std::optional<FloatRect>& rect) | |||
| void OffscreenCanvas::didDraw(const std::optional<FloatRect>& rect, const ShouldApplyPostProcessingToDirtyRect) | |||
| { | |||
| clearCopiedImage(); | |||
There was a problem hiding this comment.
I think here you would still forward the call to the base class.
Source/WebCore/html/CanvasBase.cpp
Outdated
| @@ -199,6 +200,12 @@ void CanvasBase::notifyObserversCanvasChanged(const std::optional<FloatRect>& re | |||
| observer.canvasChanged(*this, rect); | |||
| } | |||
|
|
|||
| void CanvasBase::didDraw(const std::optional<FloatRect>& rect, const ShouldApplyPostProcessingToDirtyRect shouldApplyPostProcessingToDirtyRect) | |||
| { | |||
| if (shouldInjectNoiseBeforeReadback() && shouldApplyPostProcessingToDirtyRect == ShouldApplyPostProcessingToDirtyRect::Yes && rect && !m_postProcessDirtyRect.contains(*rect)) | |||
There was a problem hiding this comment.
if (!a.contains(b)) a.unite(b); == a.unite(b);
I.e. no need to check contains, at that is equal to just doing the unite in terms of complexity
I think it would be better to categorize the data in rect as "something needing postprocess" and "something does not need post process".
So we have a goal to give out prestine putImageData back in some cases.
With your patch, consider:
empty canvas + full image putImageData -> you can get prestine putImageData back
empty canvas + draw line + full image putimageData -> you cannot get prestine putImageData back
It is not logical. If putImageData is "not need postprocess", you should always get prestine putImageData back in relevant, same cases.
There was a problem hiding this comment.
if (!a.contains(b)) a.unite(b); == a.unite(b);
Good point.
So we have a goal to give out prestine putImageData back in some cases.
I was approaching this as best-effort because I made a wrong assumption that alpha != 1 would cause compositing. That is obviously wrong in retrospect.
If putImageData is "not need postprocess", you should always get prestine putImageData back in relevant, same cases.
I agree this is the ideal and expected behavior, but I'll add this in a follow-up patch. Supporting this makes the implementation more complicated.
Source/WebCore/html/CanvasBase.cpp
Outdated
|
|
||
| if (postProcessPixelBufferResults(*pixelBuffer, { }, imageRect)) { | ||
| IntSize destOffset { 0, 0 }; | ||
| IntRect sourceRect = computeImageDataRect(*imageBuffer, imageRect.width(), imageRect.height(), imageRect, destOffset); |
There was a problem hiding this comment.
So here it's pointless to use an helper function to compute the source rect.
Source rect is the dirty postprocess rect.
Destination rect is the dirty postprocess rect.
Source/WebCore/html/CanvasBase.cpp
Outdated
| if (!imageBuffer) | ||
| return; | ||
|
|
||
| auto imageRect = !m_postProcessDirtyRect.isEmpty() ? enclosingIntRect(m_postProcessDirtyRect) : IntRect { { }, size() }; |
There was a problem hiding this comment.
if the post process dirty rect is empty, it should be logical that there's nothing to postprocess.
There was a problem hiding this comment.
This was another wrong assumption that WebGL didn't emit didDraw - but it does, so indeed this was wrong. This is now changed.
Source/WebCore/html/CanvasBase.cpp
Outdated
| return; | ||
|
|
||
| auto imageRect = !m_postProcessDirtyRect.isEmpty() ? enclosingIntRect(m_postProcessDirtyRect) : IntRect { { }, size() }; | ||
| auto colorSpace = renderingContext() ? renderingContext()->colorSpace() : DestinationColorSpace::SRGB(); |
There was a problem hiding this comment.
Here we are postprocessing the image buffer attached to the canvas element.
as such, the image buffer itself has a color space, and we shouldn't need to consult the possible context2d for the colorspace.
There was a problem hiding this comment.
Corrected, thanks for noticing that.
Source/WebCore/html/CanvasBase.cpp
Outdated
| { | ||
| if (!shouldInjectNoiseBeforeReadback() || wasLastDrawByBitMap) | ||
| if (!shouldInjectNoiseBeforeReadback() || (!m_postProcessDirtyRect.isEmpty() && !m_postProcessDirtyRect.intersects(imageRect))) |
There was a problem hiding this comment.
the caller resets the postprocess dirty rect. This means it should be logical that the caller also consults the post process dirty rect.
Suppose caller calls toData(), inducing a post process for the image buffer.
Now you have a semi-bug where the postProcessPixelBufferResults is called by the context2d in getImageData case, and it subtly happens to not do anything, as expected, because the toData did reset the dirty rect. However, the same thing doesn't work for webgl non-alpha case, where the toData would reset the dirty rect, and then the postProcessPixelBufferResults would early return incorrectly and unexpectedly.
instead, postProcessPixelBufferResults should mostly be internal business of the CanvasBase, to the degree it is able to be.
There was a problem hiding this comment.
That's a good suggestion.
| IntRect sourceRect = computeImageDataRect(*buffer, imageDataRect.width(), imageDataRect.height(), imageDataRect, destOffset); | ||
| buffer->putPixelBuffer(*pixelBuffer, sourceRect, IntPoint { destOffset }); | ||
| } | ||
| canvasBase().postProcessPixelBufferResults(*pixelBuffer, suppliedColors(), imageDataRect); |
There was a problem hiding this comment.
This call seems like a layering violation, still.
I'd consider it a better approach if above, you had:
canvasBase().makeRenderingResultsAvailable();
ImageBuffer* buffer = canvasBase().buffer();
In other words: the post processing would be always maintained before an escaping read.
There was a problem hiding this comment.
That's a good suggestion. The one reason I am using postProcessPixelBufferResults here is because this is a case where the "post processing" is not written back to the ImageBuffer and it does not result in repainting the canvas with the changes. Ideally, post-processing should only affect the script-readable extracted data. Currently, this isn't true because we rely on makeRenderingResultsAvailable for handling the other extraction methods. I see your point that this is a layering violation, and maybe this isn't sufficiently valuable.
There was a problem hiding this comment.
Ideally, post-processing should only affect the script-readable extracted data.
I think this is a really good idea, but the code has to realize this, then, ideally for all cases. I think mix-and-match sometimes mutating sometimes not is a bad combination.
|
EWS run on previous version of this PR (hash 8b296a7)
|
webkit-ews-buildbot
added
the
merging-blocked
Source/WebCore/html/CanvasBase.h
Outdated
| @@ -109,7 +112,8 @@ class CanvasBase { | |||
|
|
|||
| GraphicsClient* graphicsClient() const; | |||
|
|
|||
| virtual void didDraw(const std::optional<FloatRect>&) = 0; | |||
| void didDraw(const std::optional<FloatRect>& rect) { return didDraw(rect, ShouldApplyPostProcessingToDirtyRect::Yes); } | |||
| virtual void didDraw(const std::optional<FloatRect>&, const ShouldApplyPostProcessingToDirtyRect); | |||
There was a problem hiding this comment.
Forgot to mention this before: const of by-value function parameters are not useful, so you could remove the const from ShouldApplyPostProcessingToDirtyRect.
|
EWS run on current version of this PR (hash f95ae97)
|
https://bugs.webkit.org/show_bug.cgi?id=255756 rdar://106872464 Reviewed by Kimmo Kinnunen. When the noise injection policy is active, we are currently post-processing Canvas2D pixel buffers, but that processing was not applied to WebGL image data when alpha was premultiplied. This change fixes that bug by refactoring the logic and moving it into CanvasBase. For Canvas2D, this patch tracks the dirty rect from each drawing operation and only that area is post-processed before Canvas image data is implicitly extracted (e.g., toDataURL). For WebGL, the entire Canvas' image data is post-processed. This patch leaves tracking CanvasRenderingContext2DBase::putImageData rects as future work because those rects do not need post-processing. * Source/WebCore/html/CanvasBase.cpp: (WebCore::CanvasBase::makeRenderingResultsAvailable): (WebCore::CanvasBase::didDraw): Here we now track the dirty area if we should post-process the data in the future. (WebCore::CanvasBase::shouldInjectNoiseBeforeReadback const): (WebCore::CanvasBase::postProcessDirtyCanvasBuffer const): (WebCore::CanvasBase::postProcessPixelBufferResults const): (WebCore::CanvasBase::postProcessPixelBuffer const): Deleted. These functions are moved and renamed from CanvasRenderingContext2DBase. * Source/WebCore/html/CanvasBase.h: (WebCore::CanvasBase::didDraw): * Source/WebCore/html/CustomPaintCanvas.h: * Source/WebCore/html/HTMLCanvasElement.cpp: (WebCore::HTMLCanvasElement::didDraw): (WebCore::HTMLCanvasElement::getImageData): (WebCore::HTMLCanvasElement::setImageBufferAndMarkDirty): * Source/WebCore/html/HTMLCanvasElement.h: * Source/WebCore/html/OffscreenCanvas.cpp: (WebCore::OffscreenCanvas::didDraw): (WebCore::OffscreenCanvas::setImageBufferAndMarkDirty): * Source/WebCore/html/OffscreenCanvas.h: * Source/WebCore/html/canvas/CanvasRenderingContext.h: (WebCore::CanvasRenderingContext::postProcessPixelBuffer const): Deleted. * Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp: (WebCore::computeImageDataRect): (WebCore::CanvasRenderingContext2DBase::didDrawEntireCanvas): (WebCore::CanvasRenderingContext2DBase::didDraw): (WebCore::CanvasRenderingContext2DBase::getImageData const): Now this function behaves in the same way as the other APIs that extract image data. (WebCore::CanvasRenderingContext2DBase::putImageData): (WebCore::CanvasRenderingContext2DBase::postProcessPixelBuffer const): Deleted. This function is moved into CanvasBase. * Source/WebCore/html/canvas/CanvasRenderingContext2DBase.h: (WebCore::CanvasRenderingContext2DBase::didDraw): Canonical link: https://commits.webkit.org/263694@main
webkit-commit-queue
force-pushed
the
eng/255756
branch
from
f95ae97
to
44522d1
Compare
|
Committed 263694@main (44522d1): https://commits.webkit.org/263694@main Reviewed commits have been landed. Closing PR #13003 and removing active labels. |
webkit-commit-queue
removed
the
merge-queue
44522d1
f95ae97
🧪 mac-AS-debug-wk2