Bug 217166

Summary: [GPU Process][Resource caching 7/7] Draw a debugging border and badge for the 2D canvas if it's drawn through GPU process
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: CanvasAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED WONTFIX    
Severity: Normal CC: annulen, calvaris, cdumez, cgarcia, changseok, dino, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gustavo, gyuyoung.kim, japhet, jer.noble, jonlee, menard, mmaxfield, philipj, pnormand, ryuan.choi, sam, sergio, simon.fraser, thorton, vjaquez, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 217596    
Bug Blocks: 217342    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
ews-feeder: commit-queue-
Screenshot of the GPU badging border
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch for review
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch none

Description Said Abou-Hallawa 2020-10-01 00:52:51 PDT
If "show debug border" setting is enabled, a border and badge with the text "GPU" will be drawn around the 2D canvas when it's drawn through the GPU process.
Comment 1 Said Abou-Hallawa 2020-10-01 01:06:46 PDT
Created attachment 410204 [details]
Patch
Comment 2 Said Abou-Hallawa 2020-10-01 01:23:25 PDT
Created attachment 410206 [details]
Patch
Comment 3 Said Abou-Hallawa 2020-10-01 10:26:10 PDT
Created attachment 410243 [details]
Patch
Comment 4 Said Abou-Hallawa 2020-10-01 11:02:38 PDT
Created attachment 410244 [details]
Patch
Comment 5 Said Abou-Hallawa 2020-10-01 12:07:28 PDT
Created attachment 410256 [details]
Patch
Comment 6 Sam Weinig 2020-10-01 12:15:53 PDT
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.
Comment 7 Radar WebKit Bug Importer 2020-10-08 00:53:15 PDT
<rdar://problem/70083612>
Comment 8 Said Abou-Hallawa 2020-10-12 10:57:52 PDT
Created attachment 411131 [details]
Patch
Comment 9 Said Abou-Hallawa 2020-10-12 11:03:15 PDT
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.
Comment 10 Said Abou-Hallawa 2020-10-12 11:22:32 PDT
Created attachment 411135 [details]
Patch
Comment 11 Said Abou-Hallawa 2020-10-12 12:13:07 PDT
Created attachment 411140 [details]
Patch
Comment 12 Said Abou-Hallawa 2020-10-12 12:22:16 PDT
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.
Comment 13 Said Abou-Hallawa 2020-10-12 12:25:41 PDT
Created attachment 411145 [details]
Patch
Comment 14 Said Abou-Hallawa 2020-10-12 12:32:25 PDT
Created attachment 411146 [details]
Patch
Comment 15 Said Abou-Hallawa 2020-10-12 12:36:07 PDT
Created attachment 411147 [details]
Patch
Comment 16 Said Abou-Hallawa 2020-10-12 13:09:39 PDT
Created attachment 411148 [details]
Patch for review
Comment 17 Myles C. Maxfield 2020-11-02 23:30:03 PST
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?
Comment 18 Said Abou-Hallawa 2020-11-04 21:11:24 PST
Created attachment 413248 [details]
Patch
Comment 19 Said Abou-Hallawa 2020-11-04 21:24:40 PST
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.
Comment 20 Said Abou-Hallawa 2020-11-04 21:32:45 PST
Created attachment 413249 [details]
Patch
Comment 21 Wenson Hsieh 2020-11-04 21:40:43 PST
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 22 Tim Horton 2020-11-05 11:51:46 PST
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"
Comment 23 Said Abou-Hallawa 2020-11-05 12:05:53 PST
Created attachment 413338 [details]
Patch
Comment 24 Simon Fraser (smfr) 2020-11-05 17:43:05 PST
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.
Comment 25 Said Abou-Hallawa 2020-11-12 17:45:50 PST
Created attachment 413987 [details]
Patch
Comment 26 Said Abou-Hallawa 2020-11-12 17:50:04 PST
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.
Comment 27 Said Abou-Hallawa 2021-03-05 10:47:24 PST
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.