Bug 76593 – Merge WebGraphicsContext3D creation and initialization

• Component: New Bugs
• Status: RESOLVED FIXED

[Antoine Labour]

Merge WebGraphicsContext3D creation and initialization

[Antoine Labour]

Created attachment 123045 [details]
Patch

[Antoine Labour]

webkit side of https://chromiumcodereview.appspot.com/9249048/
(chrome side needs to land first).

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Kenneth Russell]

Comment on attachment 123045 [details]
Patch

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

This looks fine overall. Please request review again when you're ready to land it. You might roll Source/WebKit/chromium/DEPS in the same patch.

> Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:326
>      virtual WebGraphicsContext3D* createGraphicsContext3D() { return 0; }

Add a comment indicating that this variant is obsolete and is being removed.

[Darin Fisher (:fishd, Google)]

Comment on attachment 123045 [details]
Patch

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

> Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:322
> +    virtual WebGraphicsContext3D* createGraphicsContext3D(

WebKitPlatformSupport is not supposed to know about WebView.  please don't add this parameter here.  for that matter, WebGraphicsContext3D should not know about WebView either.  we have a layering violation here :(

one way to get around this problem is to introduce a creation function on WebViewClient.  that way, you are creating a WebGraphicsContext3D at a higher level, and yet the WebGraphicsContext3D interface would not need to know about WebView directly.

[Antoine Labour]

Created attachment 123167 [details]
Patch

[Antoine Labour]

PTAL, chrome side is there: https://chromiumcodereview.appspot.com/9254035/

[Kenneth Russell]

Comment on attachment 123167 [details]
Patch

Looks fine to me, but as before, will need the Chromium side to land and Chromium DEPS rolled forward. Suggest rolling DEPS in the same patch.

Leaving r? for Darin's comments.

[Darin Fisher (:fishd, Google)]

Comment on attachment 123167 [details]
Patch

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

> Source/WebKit/chromium/public/WebViewClient.h:114
> +    virtual WebGraphicsContext3D* createGraphicsContext3D(WebGraphicsContext3D::Attributes, WebView*, bool renderDirectlyToWebView) { return 0; }

nit: no need for the WebView parameter since the WebViewClient knows what WebView it is associated with.  (I presume you don't need to ask a RenderWidget to create a WGC3D for some other RenderWidget.)

What does renderDirectlyToWebView mean?  Maybe you could document that a bit?

> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:159
> +    OwnPtr<WebKit::WebGraphicsContext3D> webContext = adoptPtr(webViewImpl->client()->createGraphicsContext3D(webAttributes, webViewImpl, renderDirectlyToHostWindow));

In the future, we'll probably want to bridge up through an interface like HostWindow, but that doesn't have to happen now.  You see, GraphicsContext3DChromium.cpp is technically part of WebCore/platform/, and as such it should not have direct access to the WebViewImpl either.  But, please don't try to fix this in this patch :-)

Otherwise, LGTM

[Antoine Labour]

Created attachment 123361 [details]
Patch

[Antoine Labour]

(In reply to comment #9)
> (From update of attachment 123167 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123167&action=review
> 
> > Source/WebKit/chromium/public/WebViewClient.h:114
> > +    virtual WebGraphicsContext3D* createGraphicsContext3D(WebGraphicsContext3D::Attributes, WebView*, bool renderDirectlyToWebView) { return 0; }
> 
> nit: no need for the WebView parameter since the WebViewClient knows what WebView it is associated with.  (I presume you don't need to ask a RenderWidget to create a WGC3D for some other RenderWidget.)

Done.

> 
> What does renderDirectlyToWebView mean?  Maybe you could document that a bit?

Done.

[Kenneth Russell]

Comment on attachment 123361 [details]
Patch

LGTM for what it's worth. Still suggest waiting for the Chromium side to land and rolling forward Chromium DEPS in this patch.

[WebKit Review Bot]

Comment on attachment 123361 [details]
Patch

Attachment 123361 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11173709

[WebKit Review Bot]

Comment on attachment 123361 [details]
Patch

Rejecting attachment 123361 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Comment on attachment 123361 [details]
Patch

Rejecting attachment 123361 [details] from commit-queue.

New failing tests:
compositing/layer-creation/spanOverlapsCanvas.html
Full output: http://queues.webkit.org/results/11177422

[Kenneth Russell]

Comment on attachment 123361 [details]
Patch

I doubt that CQ failure is related to this patch. Let's try again.

[WebKit Review Bot]

Comment on attachment 123361 [details]
Patch

Rejecting attachment 123361 [details] from commit-queue.

New failing tests:
compositing/layer-creation/spanOverlapsCanvas.html
Full output: http://queues.webkit.org/results/11314293

[Antoine Labour]

I definitely repro the failure without the patch (accelerated compositing doesn't turn on, when the expectation is that it does).

[Antoine Labour]

(In reply to comment #18)
> I definitely repro the failure without the patch (accelerated compositing doesn't turn on, when the expectation is that it does).

Oh, never mind, operator error. It does seem to be coming from this patch, I'll track it down.

[Antoine Labour]

Well, it turns out we're trying to create a graphics context without a HostWindow/WebView

#0  WebCore::(anonymous namespace)::createGraphicsContext (attrs=..., hostWindow=0x0, 
    renderStyle=WebCore::GraphicsContext3D::RenderOffscreen, 
    threadUsage=WebCore::GraphicsContext3DPrivate::ForUseOnThisThread)
    at ../../../third_party/WebKit/Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:158
#1  0x00000000004f9efe in WebCore::GraphicsContext3D::create (attrs=..., hostWindow=0x0, 
    renderStyle=WebCore::GraphicsContext3D::RenderOffscreen)
    at ../../../third_party/WebKit/Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:1044
#2  0x00000000007a68b1 in WebCore::SharedGraphicsContext3D::get ()
    at ../../../third_party/WebKit/Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:41
#3  0x0000000000723a6d in WebCore::createAcceleratedCanvas (size=..., data=0x7fffea361a80)
    at ../../../third_party/WebKit/Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:70
#4  0x0000000000724073 in WebCore::ImageBuffer::ImageBuffer (this=0x7fffea361a80, size=..., 
    renderingMode=WebCore::Accelerated, success=@0x7fffffffaf5f)
    at ../../../third_party/WebKit/Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:110
#5  0x00000000004cea58 in WebCore::ImageBuffer::create (size=..., 
    colorSpace=WebCore::ColorSpaceDeviceRGB, renderingMode=WebCore::Accelerated)
    at ../../../third_party/WebKit/Source/WebCore/platform/graphics/ImageBuffer.h:78
#6  0x0000000001811e4d in WebCore::HTMLCanvasElement::createImageBuffer (this=0x7fffea1ddb40)
    at ../../../third_party/WebKit/Source/WebCore/html/HTMLCanvasElement.cpp:474
#7  0x00000000018120b0 in WebCore::HTMLCanvasElement::buffer (this=0x7fffea1ddb40)
    at ../../../third_party/WebKit/Source/WebCore/html/HTMLCanvasElement.cpp:510
#8  0x000000000181202a in WebCore::HTMLCanvasElement::drawingContext (this=0x7fffea1ddb40)
    at ../../../third_party/WebKit/Source/WebCore/html/HTMLCanvasElement.cpp:496
#9  0x00000000018a33f8 in WebCore::CanvasRenderingContext2D::drawingContext (this=0x7fffea227a80)
    at ../../../third_party/WebKit/Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1828
#10 0x000000000189d26f in WebCore::CanvasRenderingContext2D::setFillStyle (this=0x7fffea227a80, 
    prpStyle=...)
    at ../../../third_party/WebKit/Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:378
#11 0x000000000189ea42 in WebCore::CanvasRenderingContext2D::setFillColor (this=0x7fffea227a80, 
    color=...)
    at ../../../third_party/WebKit/Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:727
#12 0x00000000011704e6 in WebCore::V8CanvasRenderingContext2D::fillStyleAccessorSetter (name=..., 
    value=..., info=...)
    at ../../../third_party/WebKit/Source/WebCore/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp:99
#13 0x0000000000eaa338 in v8::internal::JSObject::SetPropertyWithCallback (this=0x1c75fcf9daa1, 
    structure=0x1194d6b5221, name=0x2d2041e1f031, value=0x2d2041e1f059, holder=0x1c75fcf9daa1, 
    strict_mode=v8::internal::kNonStrictMode) at ../../../v8/src/objects.cc:2038
#14 0x0000000000eadcae in v8::internal::JSObject::SetPropertyForResult (this=0x1c75fcf9daa1, 
    result=0x7fffffffb4d0, name=0x2d2041e1f031, value=0x2d2041e1f059, attributes=NONE, 
    strict_mode=v8::internal::kNonStrictMode) at ../../../v8/src/objects.cc:3011
#15 0x0000000000eac279 in v8::internal::JSReceiver::SetProperty (this=0x1c75fcf9daa1, 
    result=0x7fffffffb4d0, key=0x2d2041e1f031, value=0x2d2041e1f059, attributes=NONE, 
    strict_mode=v8::internal::kNonStrictMode) at ../../../v8/src/objects.cc:2655
#16 0x0000000000eaa044 in v8::internal::JSReceiver::SetProperty (this=0x1c75fcf9daa1, 
    name=0x2d2041e1f031, value=0x2d2041e1f059, attributes=NONE, 
    strict_mode=v8::internal::kNonStrictMode) at ../../../v8/src/objects.cc:1993
#17 0x00000000010d271c in v8::internal::StoreIC::Store (this=0x7fffffffb640, 
    state=v8::internal::UNINITIALIZED, strict_mode=v8::internal::kNonStrictMode, object=..., 
    name=..., value=...) at ../../../v8/src/ic.cc:1329
#18 0x00000000010d4f9e in v8::internal::StoreIC_Miss (args=..., isolate=0x7ffff7e42000)
    at ../../../v8/src/ic.cc:1881


Any idea?

[Kenneth Russell]

jamesr or senorblanco should know that code path.

[Stephen White]

(In reply to comment #21)
> jamesr or senorblanco should know that code path.

Yeah, I made that work back in http://trac.webkit.org/changeset/93157.  This was necessary in order to unify our accelerated ImageBuffer creation with the CoreGraphics path, where it only needs to pass down an enum, rather than the platform-specific crud we were doing before.  In order to do that, I had to avoid passing down the Page-related stuff (WebViewImpl I think) so I allowed it to be NULL.  This works fine for offscreen contexts like <canvas>, just not for onscreen ones (where the WebViewImpl must not be NULL).

I haven't looked at this patch in detail, but It would be really unfortunate to lose this ability and to have to go back to platform-specific hacks.

[Antoine Labour]

Darin: what do you think? 2D Canvas needs a shared offscreen context that isn't tied to a given WebView, so we don't have a WebViewClient to use as a factory. We can add back a createGraphicsContext3D on the WebKitPlatformSupport, that would only be used to create/initialize an offscreen context, that we would go to when we don't have a WebView (hence, no need for it as a parameter). Would that solution be acceptable to you?

[Darin Fisher (:fishd, Google)]

(In reply to comment #23)
> Darin: what do you think? 2D Canvas needs a shared offscreen context that isn't tied to a given WebView, so we don't have a WebViewClient to use as a factory. We can add back a createGraphicsContext3D on the WebKitPlatformSupport, that would only be used to create/initialize an offscreen context, that we would go to when we don't have a WebView (hence, no need for it as a parameter). Would that solution be acceptable to you?

Yes.  I was going to only suggest that you name it createOffscreenGraphicsContext3D to emphasize the point.

[Antoine Labour]

Created attachment 124416 [details]
Patch

[Antoine Labour]

I implemented that. Chrome side at https://chromiumcodereview.appspot.com/9297046

[WebKit Review Bot]

Comment on attachment 124416 [details]
Patch

Attachment 124416 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11358387

New failing tests:
compositing/layer-creation/spanOverlapsCanvas.html

[James Robinson]

Comment on attachment 124416 [details]
Patch

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

> Source/WebKit/chromium/public/WebViewClient.h:117
> +    virtual WebGraphicsContext3D* createGraphicsContext3D(WebGraphicsContext3D::Attributes, bool renderDirectlyToWebView) { return 0; }

when would you pass false as the second parameter here instead of just calling createOffscreen...() ?

[Antoine Labour]

(In reply to comment #28)
> (From update of attachment 124416 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124416&action=review
> 
> > Source/WebKit/chromium/public/WebViewClient.h:117
> > +    virtual WebGraphicsContext3D* createGraphicsContext3D(WebGraphicsContext3D::Attributes, bool renderDirectlyToWebView) { return 0; }
> 
> when would you pass false as the second parameter here instead of just calling createOffscreen...() ?

Today, it's called for WebGL contexts, which do have a WebView. createOffscreenGraphicsContext3D is only called for the hackish "shared context", and relies on implicit (rather than explicit) context sharing, that actually shares cross-pages. I wish at some point we can have the shared context be page-specific and go through WebViewClient::createGraphicsContext3D instead, but that seems unlikely in the short term.
We could make WebGL contexts go through createOffscreenGraphicsContext3D and it'd probably work, but unless you feel strongly about this, I'd rather not go through yet another round of 2-sided changes to remove the bool: this API looks like what you'd want long term anyway.

[James Robinson]

But why do WebGL contexts have to be associated with a WebView anyway? A given WebGL context might render to any number of different WebViews over its lifetime (if its owning <canvas> is moved into a popup window by JS, for example). I don't think that conceptually any offscreen context should be associated with a given Page or WebView since that association can change at any time.

[Antoine Labour]

Created attachment 124601 [details]
Patch

[Antoine Labour]

New patch up that removes the "renderDirectlyToWebView parameter". offscreen contexts for webgl go through the WebKitPlatformSupport path.

[Darin Fisher (:fishd, Google)]

Comment on attachment 124601 [details]
Patch

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

> Source/WebKit/chromium/public/WebViewClient.h:117
> +    virtual WebGraphicsContext3D* createGraphicsContext3D(WebGraphicsContext3D::Attributes) { return 0; }

nit: "const ref" like the one on WebKitPlatformSupport?

> Source/WebKit/chromium/public/platform/WebGraphicsContext3D.h:132
> +    virtual bool initialize(Attributes, WebView*, bool renderDirectlyToWebView) { return false; }

nit: this takes Attributes too?  is it needed?  if needed, should it also be pass by "const ref"?

> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:158
> +      Chrome* chrome = static_cast<Chrome*>(hostWindow);

indentation

[WebKit Review Bot]

Comment on attachment 124601 [details]
Patch

Attachment 124601 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11373026

[Antoine Labour]

Created attachment 124617 [details]
Patch

[Antoine Labour]

Note: As discussed by chat, today some implementations rely on having the WebView even for offscreen contexts (e.g. WebGraphicsContext3DCommandBufferImpl to grab the URL to tell the GPU process, WebGraphicsContext3DInProcessImpl to properly share contexts), and this CL isn't intended to change semantics there, so I reverted to the previous version with the explicit "renderDirectlyToWebView" (aka !offscreen) parameter.
A follow-up CL can clean this up.

(In reply to comment #33)
> (From update of attachment 124601 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124601&action=review
> 
> > Source/WebKit/chromium/public/WebViewClient.h:117
> > +    virtual WebGraphicsContext3D* createGraphicsContext3D(WebGraphicsContext3D::Attributes) { return 0; }
> 
> nit: "const ref" like the one on WebKitPlatformSupport?

Done.

> 
> > Source/WebKit/chromium/public/platform/WebGraphicsContext3D.h:132
> > +    virtual bool initialize(Attributes, WebView*, bool renderDirectlyToWebView) { return false; }
> 
> nit: this takes Attributes too?  is it needed?  if needed, should it also be pass by "const ref"?

This function will go away once the chrome side refactoring is done. I noted in the doc as such. There's no point in fixing this now (it'd require extra 2-sided changes).

> 
> > Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:158
> > +      Chrome* chrome = static_cast<Chrome*>(hostWindow);
> 
> indentation

Because of the revert to previous version, the problem is gone.

[WebKit Review Bot]

Comment on attachment 124617 [details]
Patch

Attachment 124617 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11367741

New failing tests:
compositing/layer-creation/spanOverlapsCanvas.html

[James Robinson]

LGTM

[Kenneth Russell]

LGTM too. Per offline discussion, sounds like another patch is coming which will roll Chromium DEPS too to fix the build failure.

[WebKit Review Bot]

Comment on attachment 124617 [details]
Patch

Clearing flags on attachment: 124617

Committed r106371: <http://trac.webkit.org/changeset/106371>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

This patch broke Chromium builds: http://build.webkit.org/builders/Chromium%20Mac%20Release%20%28Perf%29/builds/132/steps/compile-webkit/logs/stdio

[James Robinson]

This needs a roll of Source/WebKit/chromium/DEPS

[Antoine Labour]

Not a DEPS roll issue, just I removed a method that is OVERRIDE chrome-side. I temporarily put it back in https://bugs.webkit.org/show_bug.cgi?id=77467 and it should have fixed the build.

[James Robinson]

(In reply to comment #44)
> Not a DEPS roll issue, just I removed a method that is OVERRIDE chrome-side. I temporarily put it back in https://bugs.webkit.org/show_bug.cgi?id=77467 and it should have fixed the build.

Gah, this is why we should never OVERRIDE implementations of WebKit APIs.  I'll look in to  having the style checker enforce that OVERRIDE is banned for these.