Bug 221811 – WebGL contexts do not work as source for Context2D drawImage calls in GPU process

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kimmo Kinnunen]

WebGL contexts do not work as source for Context2D drawImage calls in GPU process

[Kimmo Kinnunen]

Created attachment 420117 [details]
Patch

[Kimmo Kinnunen]

Created attachment 420312 [details]
rebase

[Darin Adler]

Comment on attachment 420312 [details]
rebase

View in context: https://bugs.webkit.org/attachment.cgi?id=420312&action=review

I’m not a deep subject matter expert, but this looks good.

> Source/WebCore/platform/graphics/cairo/GraphicsContextGLCairo.cpp:109
> +void GraphicsContextGLOpenGL::paintToCanvas(const GraphicsContextGLAttributes& attributes, Ref<ImageData>&& imageData, const IntSize& canvasSize, GraphicsContext& context)

Some ambiguity since both contextAttributes() and this attributes argument exist. Is there any way to make the distinction between the two clearer. I’m guessing that it’s about source attributes vs. destination attributes?

> Source/WebCore/platform/graphics/cg/GraphicsContextGLCG.cpp:518
> +    auto attrs = attributes;

No reason to copy these just to check two flags a couple lines later. I suggest getting rid of this local variable.

> Source/WebCore/platform/graphics/cocoa/GraphicsContextGLIOSurfaceSwapChain.cpp:55
> +    void* result = m_displayBuffer.handle;
> +    m_displayBuffer.handle = nullptr;
> +    return result;

Might be nice to write this with std::exchange:

    return std::exchange(m_displayBuffer.handle, nullptr);

> Source/WebCore/platform/graphics/cocoa/GraphicsContextGLIOSurfaceSwapChain.cpp:61
> +    m_spareBuffer = WTFMove(m_displayBuffer);
> +    m_displayBuffer = WTFMove(buffer);

Might be nice to write this with std::exchange:

    m_spareBuffer = std::exchange(m_displayBuffer, WTFMove(buffer));

> Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGLCocoa.cpp:55
> +    return std::unique_ptr<RemoteGraphicsContextGL>(new RemoteGraphicsContextGLCocoa(attributes, gpuConnectionToWebProcess, graphicsContextGLIdentifier, renderingBackend));

Can this be written using make_unique instead? It’s worth a little effort to avoid the bare "new" for cases like this.

> Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGLCocoa.cpp:68
> +    auto* surface = m_swapChain.displayBuffer().surface.get();
> +    if (surface)

Nicer to define inside the if.

    if (auto surface = ...

[Kimmo Kinnunen]

Created attachment 420661 [details]
Patch

[Kimmo Kinnunen]

Created attachment 420663 [details]
Patch

[Kimmo Kinnunen]

Thanks for the review!
I had to do a cosmetic simplification from
"send an async message with a makeshift fence and wait for the fence"
to
"send a synchronous message".

There's no functional difference, as both were synchronous.

This is due to the use of flush id as the makeshift fence. The RemoteImageBufferProxy did not survive changing the flush ids like that, yet. I'll work making this more asynchronous later, once other bugs in RemoteRenderingBackend are fixed.

[Darin Adler]

Comment on attachment 420663 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420663&action=review

Again a few style thoughts.

> Source/WebCore/platform/graphics/cocoa/WebGLLayer.mm:115
> +    WebCore::GraphicsContextGLIOSurfaceSwapChain::present(WTFMove(buffer));

Pretty sure that you don’t need the namespace prefix "WebCore" when using the name of a base class.

> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.cpp:226
>      // Reading premultiplied alpha would involve unpremultiplying, which is
>      // lossy.

Drives me crazy to have that orphaned word. Merging this into one line will make me sleep better at night. (If I coded for a few years with 80 column style like they use at Google maybe my tastes would change?)

> Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGL.cpp:154
> +    // TODO: We do not have functioning read/write fences in RemoteRenderingBackend. Thus this is synchronous,
> +    // as is the messages that call these.

Grammar mistake, "is" -> "are".

We use FIXME and not TODO on WebKit.

I am not at all familiar with how OK or not OK it is to use synchronous messages for this, nor if this is a long term thing or a short term expedient intermediate step.

> Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGL.cpp:160
> +            // Note: here we do not try to play back pending commands for imageBuffer. Currently this call is only made for empty

I suggest leaving out "Note:" here.

> Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGL.messages.in:40
> +    void PaintRenderingResultsToCanvas(WebCore::RenderingResourceIdentifier renderingResourceIdentifier) -> () Synchronous
> +    void PaintCompositedResultsToCanvas(WebCore::RenderingResourceIdentifier renderingResourceIdentifier) -> () Synchronous

Given the type name, I would be tempted to just use the word "identifier" as the argument name.

> Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGLCocoa.cpp:53
> +std::unique_ptr<RemoteGraphicsContextGL> RemoteGraphicsContextGL::create(const WebCore::GraphicsContextGLAttributes& attributes, GPUConnectionToWebProcess& gpuConnectionToWebProcess, GraphicsContextGLIdentifier graphicsContextGLIdentifier, RemoteRenderingBackend& renderingBackend)

I would name the arguments with shorter names: "connection", "identifier", "backEnd". The type names have enough of the rest of the context.

> Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGLCocoa.cpp:58
> +RemoteGraphicsContextGLCocoa::RemoteGraphicsContextGLCocoa(const WebCore::GraphicsContextGLAttributes& attributes, GPUConnectionToWebProcess& connection, GraphicsContextGLIdentifier identifier, RemoteRenderingBackend& renderingBackend)

Hey, you did two of the three of those here!

> Source/WebKit/WebProcess/GPU/graphics/RemoteGraphicsContextGLProxy.cpp:47
> +RefPtr<RemoteGraphicsContextGLProxy> RemoteGraphicsContextGLProxy::create(const GraphicsContextGLAttributes& attributes, RenderingBackendIdentifier renderingBackendIdentifier)

Shorter name idea again.

> Source/WebKit/WebProcess/GPU/graphics/RemoteGraphicsContextGLProxy.cpp:52
> +RemoteGraphicsContextGLProxy::RemoteGraphicsContextGLProxy(GPUProcessConnection& gpuProcessConnection, const GraphicsContextGLAttributes& attrs, RenderingBackendIdentifier renderingBackendIdentifier)

Shorter name idea again. Also would be nice to consistently use "attributes" instead of mixing "attrs" and "attributes".

> Tools/Scripts/generate-gpup-webgl:105
> +    void PaintRenderingResultsToCanvas(WebCore::RenderingResourceIdentifier renderingResourceIdentifier) -> () Synchronous
> +    void PaintCompositedResultsToCanvas(WebCore::RenderingResourceIdentifier renderingResourceIdentifier) -> () Synchronous

Shorter name idea again.

[Kimmo Kinnunen]

Created attachment 420808 [details]
Patch

[Kimmo Kinnunen]

Created attachment 420812 [details]
Patch

[Kimmo Kinnunen]

Thanks! Applied the naming changes where I could and where I understood to spot them.

Not shortening the name GPUConnectionToWebProcess& gpuConnectionToWebProcess at the sites the variable is used since it's ambiguous to the use of "connection", as that's used in related code for "Connection connection". Not using "gpuConnection" either, as that's a different thing. Similar to GPUProcessConnection, not using "connection", "gpuProcess" or similar shortening, to avoid confusion with different, exisiting concepts "Connection", "GPUProcess". Existing code uses the naming.

Not shortening the name "RemoteRenderingBackend& renderingBackend" to "backend", as that would imply that the backend was backend of RemoteGraphicsContextGL -- it's not. Similar to imaginary case "RemoteRenderingService& renderingService", it would not be shortened to "service". The concept is "RenderingBackend" (whether or not that's a good name). Not using "backEnd", since the type is "Backend", not "BackEnd".

[Darin Adler]

I’m OK with those decisions, but let me tell you why I wouldn’t have necessarily made the same ones:

There’s no English word "backend", and I wish we would not coin that word for WebKit. Even if it’s already done, I won’t go along with it in new names I come up with.

In short functions, I don’t think the argument names need to carry the entire semantic. I agree that if the function was longer you wouldn’t want a name that could be ambiguous, but I don’t think that each variable name needs to be unambiguous on its own. We prefer words to letters in most cases because we think it’s better for human understanding in most cases, but like other programmers who use letters, we understand that in a narrow scope a variable name is just a local handle for something and doesn’t carry the full context.

[EWS]

Committed r273080: <https://commits.webkit.org/r273080>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420812 [details].

[Radar WebKit Bug Importer]

<rdar://problem/74481898>

[Kimmo Kinnunen]

(In reply to Darin Adler from comment #11)
> In short functions, I don’t think the argument names need to carry the
> entire semantic. I agree that if the function was longer you wouldn’t want a
> name that could be ambiguous, but I don’t think that each variable name
> needs to be unambiguous on its own.

Sure, and I agree. I did follow that rule for the cases which were unambiguous enough. For the cases that I did not follow that rule: the variable names need to be unambiguous in the context of the code at hand.

In this part of the proxy side of the code, there's frequent use of concepts:
- gpuProcessConnection
- gpuProcessProxy
- gpuProcess
- connection
- gpuConnectionToWebProcess


Often times around this code, as was in the patch, multiple of these concepts are present even in the scope. As such, I'd imagine using ambiguous shorthand that clashes semantically as well as lexically is more confusing than the benefit of the short name.


> There’s no English word "backend", and I wish we would not coin that word for WebKit. Even if it’s already done, I won’t go along with it in new names I come up with.

Sure, I can understand this, especially with *new* new names, e.g. when the concept is created. In this patch, the parameter was new in the sense that it was added to a constructor, but not new in the sense that similar things were all over the code base already.

But you must see this being quite impossible for me. This is problematic as it creates reviewer-induced inconsistency: if the original author of RenderingBackend would have reviewed, he would call my 'backEnd' incorrect. If you review, you call my 'backend' incorrect. You say 'not a word', he says 'defacto word' or 'consistency'. 

This puts me into a situation that I need to guess who eventually is reviewing the patch and what are their personal preferences. Now I had other existing code calling backend 'backend', and then if I add another function, suddenly the backend is 'backEnd', since it was a different reviewer. I don't necessarily agree that this decision would've made the code base better, even though technically backend is not a word in non-technical dictionary.

One solution would be to list commonly misused English phrases like "back 
end", "wake up", "code base" in coding style and bring everybody up-up-to-date with the expectation of what is correct and what is incorrect. If one reviewer has a rule and other reviewer has an opposing rule, it is hard to work autonomously.

I went with the solution of being consistent with the existing codebase, which I interpreted as implicit rule in the coding style.

[Kimmo Kinnunen]

(In reply to Kimmo Kinnunen from comment #14)
> But you must see this being quite impossible for me. This is problematic as
> it creates reviewer-induced inconsistency: if the original author of
> RenderingBackend would have reviewed, he would call my 'backEnd' incorrect.
> If you review, you call my 'backend' incorrect. You say 'not a word', he
> says 'defacto word' or 'consistency'. 

OTOH, I don't know if this is a problem in practice. And even if it were, maybe it should be addressed at that point (suggesting incorrect use) than in this point (suggesting correct use of word).

I'll use 'backEnd' from now on.

[Kimmo Kinnunen]

*** Bug 219351 has been marked as a duplicate of this bug. ***