Bug 217166

Created attachment 410204 [details] Patch

Created attachment 410206 [details] Patch

Created attachment 410243 [details] Patch

Created attachment 410244 [details] Patch

Created attachment 410256 [details] Patch

Comment on attachment 410256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410256&action=review > Source/WebCore/platform/graphics/GraphicsContextExtras.h:28 > +#include "FloatRect.h" This can be forward declared instead of included. > Source/WebCore/platform/graphics/GraphicsContextExtras.h:35 > +WEBCORE_EXPORT void drawBorderAndBadge(GraphicsContext&, const FloatRect& containerRect, const FloatPoint& badgePosition, const Color&, const String& text); Do you intend to use this in more places in the future? Seems like an oddly specific function for platform. Also not a huge fan of the use of "Extras" in the name here.

<rdar://problem/70083612>

Created attachment 411131 [details] Patch

Comment on attachment 410256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410256&action=review >> Source/WebCore/platform/graphics/GraphicsContextExtras.h:35 >> +WEBCORE_EXPORT void drawBorderAndBadge(GraphicsContext&, const FloatRect& containerRect, const FloatPoint& badgePosition, const Color&, const String& text); > > Do you intend to use this in more places in the future? Seems like an oddly specific function for platform. Also not a huge fan of the use of "Extras" in the name here. Yes. I copied most of the code from PlatformCALayer::drawRepaintIndicator(). I am planning to remove the repetition. This function will be used to badge the GPU cached resources as well.

Created attachment 411135 [details] Patch

Created attachment 411140 [details] Patch

Created attachment 411142 [details] Screenshot of the GPU badging border In this screenshot: 1. Green "GPU" badge is drawn when the drawing of a 2D canvas is replayed back in the GPU Process. 2. Blue "CANVAS" badge is drawn when a 2D canvas is drawn into another 2D canvas and both canvases are backed by RemoteImageBuffers. 2. Red "IMAGE" badge is drawn when a BitmapImage frame is drawn into a 2D canvas and the canvases backed by a RemoteImageBuffer and the image frame is cached in the GPU Process.

Created attachment 411145 [details] Patch

Created attachment 411146 [details] Patch

Created attachment 411147 [details] Patch

Created attachment 411148 [details] Patch for review

Comment on attachment 411148 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=411148&action=review > Source/WebCore/platform/graphics/GraphicsContextExtras.cpp:87 > + context.fillPath(path); Why not fillRect, to simplify the above code? > Source/WebCore/platform/graphics/GraphicsContextExtras.h:37 > +enum RectCorner { I think we usually use enum classes now? > Source/WebCore/platform/graphics/GraphicsContextExtras.h:41 > + RectTopLeft = 0x00, > + RectTopRight = 0x01, > + RectBottomLeft = 0x02, > + RectBottomRight = 0x03, Why give these explicit values? > Source/WebCore/platform/graphics/GraphicsContextExtras.h:50 > +WEBCORE_EXPORT void drawBadgingBorder(GraphicsContext&, const FloatRect& borderRect, const Color&, const Vector<Badge>&); None of the callers seem to pass more than a single badge, so it doesn't seem like it needs to accept a vector. > Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackend.cpp:136 > + send(Messages::RemoteRenderingBackendProxy::FlushImageBufferDrawingContext(displayList, m_remoteResourceCache.shouldDrawBadgingBorder(), remoteResourceIdentifier), m_renderingBackendIdentifier); I think a single context can be flushed multiple times (e.g. if the page is doing getImageData/putImageData) so it seems like the badge could be drawn multiple times. Is "FlushDrawingContext" really the right level to be doing this in? Why not do it at creation instead, so it will never be drawn more than once? > Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCache.h:95 > + WebPage& m_page; This seems like a layering violation. Can't we just pull out the boolean setting ahead of time, and store a bool here instead?

Created attachment 413248 [details] Patch

Comment on attachment 411148 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=411148&action=review >> Source/WebCore/platform/graphics/GraphicsContextExtras.cpp:87 >> + context.fillPath(path); > > Why not fillRect, to simplify the above code? Fixed. >> Source/WebCore/platform/graphics/GraphicsContextExtras.h:37 >> +enum RectCorner { > > I think we usually use enum classes now? Fixed. >> Source/WebCore/platform/graphics/GraphicsContextExtras.h:41 >> + RectBottomRight = 0x03, > > Why give these explicit values? Explicit values were removed. >> Source/WebCore/platform/graphics/GraphicsContextExtras.h:50 >> +WEBCORE_EXPORT void drawBadgingBorder(GraphicsContext&, const FloatRect& borderRect, const Color&, const Vector<Badge>&); > > None of the callers seem to pass more than a single badge, so it doesn't seem like it needs to accept a vector. I replaced the code in PlatformCALayer::drawRepaintIndicator() by calling this function with three badges. Previously none indicative ways were used like changing the text color or the text drawing mode. >> Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackend.cpp:136 >> + send(Messages::RemoteRenderingBackendProxy::FlushImageBufferDrawingContext(displayList, m_remoteResourceCache.shouldDrawBadgingBorder(), remoteResourceIdentifier), m_renderingBackendIdentifier); > > I think a single context can be flushed multiple times (e.g. if the page is doing getImageData/putImageData) so it seems like the badge could be drawn multiple times. Is "FlushDrawingContext" really the right level to be doing this in? Why not do it at creation instead, so it will never be drawn more than once? The badge has to be on top of all the drawing so it has to be drawn every time we flush the DrawingContext. And yes it can be drawn multiple times but I think this is okay since it is just for debugging. We may use it only to confirm a certain rectangle is drawn by GPUP. >> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCache.h:95 >> + WebPage& m_page; > > This seems like a layering violation. Can't we just pull out the boolean setting ahead of time, and store a bool here instead? The reason for having the WebPage instead of the flag is the setting can change at run time. And if I keep the flag then I have to sync it with the setting every time it changes. I am not sure about the layering violation in WebKit, but in RemoteRenderingBackendProxy::RemoteRenderingBackendProxy() we call the WebProcess to ensureGPUProcessConnection(). And WebProcess is the owner of all the WebPages. But I will ask Tim Horton what the right approach should be.

Created attachment 413249 [details] Patch

Comment on attachment 413249 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413249&action=review > Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:161 > + SetForScope<bool> change(m_showDebugBorders, showDebugBorders); (Note to self — it looks like I might need to create a new "show debug borders" display list item for my upcoming concurrent display list rendering changes)

Comment on attachment 413249 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413249&action=review > Source/WebKit/ChangeLog:18 > + Pass the 'showDebugBorders' from WebPorcess to GPUP. "Porcess"

Created attachment 413338 [details] Patch

Comment on attachment 413338 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413338&action=review > Source/WebCore/platform/graphics/ca/PlatformCALayer.cpp:-122 > - const float cornerChunk = 8; > - boundsPath.addLineTo(FloatPoint(indicatorBox.x(), indicatorBox.y() + cornerChunk)); > - boundsPath.addLineTo(FloatPoint(indicatorBox.x() + cornerChunk, indicatorBox.y())); > - boundsPath.closeSubpath(); Looks like you dropped this part in the new code? I found that useful. > Source/WebCore/platform/graphics/ca/PlatformCALayer.cpp:116 > + if (!platformCALayer->isOpaque() && platformCALayer->supportsSubpixelAntialiasedText()) { > + badges.append({ > + DebugBadge::Corner::BottomRight, > + Color::green, > + platformCALayer->acceleratesDrawing() ? acceleratedTextColor : unacceleratedTextColor, > + smallFontSize, > + "sub-pixel"_s > + }); Just remove this. It's no longer interesting. > Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:152 > + m_remoteRenderingBackendProxy->drawDebugBorderAndBadge(context(), { { }, m_backend->logicalSize() }, "GPU"_s); how do we know what the CTM is in the display list at this point? Maybe there's some transform that will affect the badge.

Created attachment 413987 [details] Patch

Comment on attachment 413338 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413338&action=review >> Source/WebCore/platform/graphics/ca/PlatformCALayer.cpp:-122 >> - boundsPath.closeSubpath(); > > Looks like you dropped this part in the new code? I found that useful. I remove all the changes in this function till we have an agreement on the badging for GPUP. >> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:152 >> + m_remoteRenderingBackendProxy->drawDebugBorderAndBadge(context(), { { }, m_backend->logicalSize() }, "GPU"_s); > > how do we know what the CTM is in the display list at this point? Maybe there's some transform that will affect the badge. Fixed. The CTM will be set to the ImageBuffer::baseTransform() before drawing the border and badge.

Drawing borders for compositing and for the GPUP 2D canvas will be turned on by the same flag. It is a customer facing feature because this flag can be turned on from the Web inspector. I think there is no real need for this new debugging tool now and it may cause confusion when borders for compositing and for the GPUP 2D canvas are drawn at the same time.