Bug 76593

Summary: Merge WebGraphicsContext3D creation and initialization
Product: WebKit Reporter: Antoine Labour <piman>
Component: New BugsAssignee: Antoine Labour <piman>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, fishd, jamesr, kbr, rniwa, senorblanco, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Antoine Labour 2012-01-18 17:48:48 PST
Merge WebGraphicsContext3D creation and initialization
Comment 1 Antoine Labour 2012-01-18 17:51:00 PST
Created attachment 123045 [details]
Patch
Comment 2 Antoine Labour 2012-01-18 17:53:22 PST
webkit side of https://chromiumcodereview.appspot.com/9249048/
(chrome side needs to land first).
Comment 3 WebKit Review Bot 2012-01-18 17:54:10 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 4 Kenneth Russell 2012-01-18 18:08:26 PST
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.
Comment 5 Darin Fisher (:fishd, Google) 2012-01-18 21:20:06 PST
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.
Comment 6 Antoine Labour 2012-01-19 12:03:18 PST
Created attachment 123167 [details]
Patch
Comment 7 Antoine Labour 2012-01-19 12:05:11 PST
PTAL, chrome side is there: https://chromiumcodereview.appspot.com/9254035/
Comment 8 Kenneth Russell 2012-01-19 12:38:54 PST
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.
Comment 9 Darin Fisher (:fishd, Google) 2012-01-19 14:09:18 PST
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
Comment 10 Antoine Labour 2012-01-20 12:37:08 PST
Created attachment 123361 [details]
Patch
Comment 11 Antoine Labour 2012-01-20 12:39:36 PST
(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.
Comment 12 Kenneth Russell 2012-01-20 12:53:14 PST
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.
Comment 13 WebKit Review Bot 2012-01-20 14:43:26 PST
Comment on attachment 123361 [details]
Patch

Attachment 123361 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11173709
Comment 14 WebKit Review Bot 2012-01-24 18:20:59 PST
Comment on attachment 123361 [details]
Patch

Rejecting attachment 123361 [details] from commit-queue.

piman@chromium.org 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.
Comment 15 WebKit Review Bot 2012-01-25 02:55:18 PST
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
Comment 16 Kenneth Russell 2012-01-25 10:53:25 PST
Comment on attachment 123361 [details]
Patch

I doubt that CQ failure is related to this patch. Let's try again.
Comment 17 WebKit Review Bot 2012-01-25 12:12:09 PST
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
Comment 18 Antoine Labour 2012-01-25 15:51:33 PST
I definitely repro the failure without the patch (accelerated compositing doesn't turn on, when the expectation is that it does).
Comment 19 Antoine Labour 2012-01-25 15:53:58 PST
(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.
Comment 20 Antoine Labour 2012-01-25 16:32:18 PST
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?
Comment 21 Kenneth Russell 2012-01-26 13:24:50 PST
jamesr or senorblanco should know that code path.
Comment 22 Stephen White 2012-01-26 13:51:12 PST
(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.
Comment 23 Antoine Labour 2012-01-26 17:41:34 PST
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?
Comment 24 Darin Fisher (:fishd, Google) 2012-01-27 15:07:26 PST
(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.
Comment 25 Antoine Labour 2012-01-27 18:20:48 PST
Created attachment 124416 [details]
Patch
Comment 26 Antoine Labour 2012-01-27 18:21:26 PST
I implemented that. Chrome side at https://chromiumcodereview.appspot.com/9297046
Comment 27 WebKit Review Bot 2012-01-27 18:56:02 PST
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
Comment 28 James Robinson 2012-01-29 17:44:31 PST
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...() ?
Comment 29 Antoine Labour 2012-01-30 12:51:09 PST
(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.
Comment 30 James Robinson 2012-01-30 13:29:53 PST
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.
Comment 31 Antoine Labour 2012-01-30 14:39:56 PST
Created attachment 124601 [details]
Patch
Comment 32 Antoine Labour 2012-01-30 14:41:59 PST
New patch up that removes the "renderDirectlyToWebView parameter". offscreen contexts for webgl go through the WebKitPlatformSupport path.
Comment 33 Darin Fisher (:fishd, Google) 2012-01-30 15:09:19 PST
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
Comment 34 WebKit Review Bot 2012-01-30 15:14:57 PST
Comment on attachment 124601 [details]
Patch

Attachment 124601 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11373026
Comment 35 Antoine Labour 2012-01-30 15:50:06 PST
Created attachment 124617 [details]
Patch
Comment 36 Antoine Labour 2012-01-30 15:55:50 PST
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.
Comment 37 WebKit Review Bot 2012-01-30 16:54:10 PST
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
Comment 38 James Robinson 2012-01-30 17:21:24 PST
LGTM
Comment 39 Kenneth Russell 2012-01-30 17:47:55 PST
LGTM too. Per offline discussion, sounds like another patch is coming which will roll Chromium DEPS too to fix the build failure.
Comment 40 WebKit Review Bot 2012-01-31 11:12:09 PST
Comment on attachment 124617 [details]
Patch

Clearing flags on attachment: 124617

Committed r106371: <http://trac.webkit.org/changeset/106371>
Comment 41 WebKit Review Bot 2012-01-31 11:12:15 PST
All reviewed patches have been landed.  Closing bug.
Comment 43 James Robinson 2012-01-31 13:16:40 PST
This needs a roll of Source/WebKit/chromium/DEPS
Comment 44 Antoine Labour 2012-01-31 13:25:04 PST
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.
Comment 45 James Robinson 2012-01-31 13:37:53 PST
(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.