Benoit Girard (:BenWa) Reporter
	Description • 13 years ago

Currently we can only run WebGL from the main thread. This is bad for responsiveness.

My idea is to allow users to create a WebGL canvas meant to run inside a Web Worker. With off-main-thread-compositing we could allow WebGL to run when the main thread is blocked and prevent the main thread from blocking on WebGL. Note that even electrolysis doesn't solve this problem. This may also provide some JS GC benefit if we allow GC to run in web workers without using the main thread but I can't speak for this.

Here's a rough idea of how this could be done, perhaps we can think of something better:

== index.js ==

// Create a context that can be accessed directly. It wraps a gl context
// until it is bound by a web worker after which it can not be rebound.
glWorker = canvas.getContext("experimental-webworker-webgl");
try {
  // Will throw
  glWorker.clearColor(0.0, 0.0, 0.0, 1.0);
} catch (e) {
  // glWorker is not a gl context so clearColor isn't defined.
}

// Create a regular worker. This must use messages to access DOM
// and communicate with the main thread.
var worker = new Worker('async_webgl.js');
worker.postMessage( { "name": "start_async", "context": glWorker } );

== async_webgl.js ==

onmessage = function(event) {  
  if (event["name"] == "start_async") {
    var glWorker = event["context"];

    // This is where the magic happens, the context
    // can be permanently bound to the current thread
    // which exposes the context. Since nothing
    // else can bind to it only the worker can access it.
    var gl = glWorker.bind();
    // We let the context talk to the compositor to allow it 
    // to composite asynchronously.  
  }
}

My questions:
- Are there any fundamental limitations that I missed?
- What do you think?

	Benoit Jacob [:bjacob] (mostly away)
	Comment 1 • 13 years ago

The ability to run WebGL in a Web Worker has definitely been discussed before, but I can't remember the conclusions. Maybe search the public_webgl archives...

	Benoit Girard (:BenWa) Reporter
	Comment 2 • 13 years ago

Here is what I found from searching:
http://groups.google.com/group/webgl-dev-list/browse_thread/thread/05b5a68d31bd3d36?pli=1

This thread is just looking for a way to mitigate texture uploads. This would help by taking it off the main thread.

Not really related:
http://www.khronos.org/webgl/public-mailing-list/archives/1104/msg00051.html

	Benoit Girard (:BenWa) Reporter
	Comment 3 • 13 years ago

Benoit Jacob told me that Vivien might have an opinion on this.

I'd like to discuss how complicated this is and what we think the win would be.

	Joe Drew (not getting mail)
	Comment 4 • 13 years ago

We currently don't support canvas in web workers, but we could (if we used threadsafe Cairo or our Azure canvas implemented on top of Skia). This is very analogous to that.

	Benoit Girard (:BenWa) Reporter
	Comment 5 • 13 years ago

Here's a bit of the low level explanation of how this could be implemented with async compositor. Note: We would need a fall back if async composition isn't supported.

glWorker = canvas.getContext("experimental-webworker-webgl");
^== Create an internal CanvasLayer for the canvas with no GLContext attached. This object now keeps a reference to the internal composition layer.

var gl = glWorker.bind();
^== Opens an IPC connection with the compositor, creates a GLContext for that specific web worker thread, invalidates glWorker so that it can't be used again.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 6 • 13 years ago

We could allow something like this yeah. However:

1. The magic should happen in the postMessage. As soon as a window postMessages a
   context to a worker it should lose any ability to interact with it.
2. By "any ability" I also include the ability to read data from it, including
   calling .getImageData on the context itself, calling .toDataURL or calling
   .getContext. In theory asynchronous functions like toBlob could be ok though seems
   unadvisable since you could get a half-painted picture.
3. Changes to the canvas elements width/height wouldn't clear the canvas or change
   it's actual size.
4. The page would have no ability to synchronize main-thread animations happening
   through .requestAnimationFrame
5. We'd need some API to tell the context that the picture is "fully painted" and
   is ready to be displayed on the screen. Currently we implicitly do that by
   returning to the event loop.

	Benoit Girard (:BenWa) Reporter
	Comment 7 • 13 years ago

#1 Are there any reason why you think this would be better then explicitly binding?
#2 Umm, that's an interesting point. The other threads should never see a context so calling a method like getImageData should be impossible. I'm not sure how the other methods should fail (throw exceptions?)
#3 AFAIK the glViewport doesn't have to match the pixels width/height so this isn't a deal breaker on it own, but it should be easy for the implementer to handle this case so perhaps something extra is needed here.
#4 I don't think this is a problem. If you request an async context that's just the price you have to pay. This is also a problem with resize. This is a problem we face in general with async composition.
#5 I think this will also happen when the web worker returns to it's IPC event loop, but it's hard to say without having written any code for that.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 8 • 13 years ago

(In reply to Benoit Girard (:BenWa) from comment #7)
> #1 Are there any reason why you think this would be better then explicitly
> binding?

If the "ownership" is transferred when the worker calls .bind() rather than at postMessage time, then it'll happen at an essentially random time from the main page's point of view. That's a recipe for races and bugs in the page.

> #2 Umm, that's an interesting point. The other threads should never see a
> context so calling a method like getImageData should be impossible. I'm not
> sure how the other methods should fail (throw exceptions?)

Yeah, any and all methods should probably fail by throwing exceptions.

> #4 I don't think this is a problem. If you request an async context that's
> just the price you have to pay. This is also a problem with resize. This is
> a problem we face in general with async composition.

I think the long-term plan is that resize and scroll events are fired at the same time as requestAnimationFrame callbacks happen, so it's not entirely true that they are equivalent.

But it might not be a big deal. Definitely something that we'd need to get developer feedback on though.

Another option is to simply make requestAnimationFrame available in workers too.

> #5 I think this will also happen when the web worker returns to it's IPC
> event loop, but it's hard to say without having written any code for that.

That would certainly be one solution. However one point of workers is to enable writing simpler code which doesn't have to return to the event loop constantly. One option would be to have an API like:

context.startDrawing(function() {
  ...drawing code here...
});

The callback is called synchronously which means that it's just as simple to write code for as normal calls. All your local variables are captured through the magic of JS closures.

As soon as the callback returns the drawing is "done" and is ok to be painted.

	Benoit Girard (:BenWa) Reporter
	Comment 9 • 13 years ago

(In reply to Jonas Sicking (:sicking) from comment #8)
> > #5 I think this will also happen when the web worker returns to it's IPC
> > event loop, but it's hard to say without having written any code for that.
> 
> That would certainly be one solution. However one point of workers is to
> enable writing simpler code which doesn't have to return to the event loop
> constantly. One option would be to have an API like:
> 
> context.startDrawing(function() {
>   ...drawing code here...
> });
> 
> The callback is called synchronously which means that it's just as simple to
> write code for as normal calls. All your local variables are captured
> through the magic of JS closures.
> 
> As soon as the callback returns the drawing is "done" and is ok to be
> painted.

You make a good point here. Game likes to run in a 'game loop'. Although I don't think this is possible with web workers because they need to yield to get new messages?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 10 • 13 years ago

Yes, you currently need to yield to get message, however there are proposals to change that so I wouldn't rely on that being the case forever.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 11 • 13 years ago

(In reply to Benoit Girard (:BenWa) from comment #3)
> Benoit Jacob told me that Vivien might have an opinion on this.
> 
> I'd like to discuss how complicated this is and what we think the win would
> be.

Not sure why Benoit Jacob mentionned me since I have no idea about how this stuff works at all. 

What I know is that canvas (2d) compositing on a web worker could be a great improvement for Pdf.js.

On Pdf.js every page is a canvas, and to save some memory pages are rendered only when they are visible. This doesn't fit well with smooth panning since when the canvas is draw, panning stops.

	Benoit Jacob [:bjacob] (mostly away)
	Comment 12 • 13 years ago

(In reply to Benoit Girard (:BenWa) from comment #3)
> Benoit Jacob told me that Vivien might have an opinion on this.
> 
> I'd like to discuss how complicated this is and what we think the win would
> be.

I meant Cedric Vivier !!!

	Benoit Girard (:BenWa) Reporter
	Comment 13 • 13 years ago

I was confused, I double checked on IRC. Clearly a miss communication.

	Benoit Jacob [:bjacob] (mostly away)
	Comment 14 • 13 years ago

(In reply to Benoit Girard (:BenWa) from comment #13)
> I was confused, I double checked on IRC. Clearly a miss communication.

Yeah I remember now when you asked but I thought you only wanted to check the spelling in Vivien's name, I didn't make the connection with this bug.

	Cedric Vivier [:cedricv]
	Comment 15 • 13 years ago

I'm not sure this is really a win for Javascript developers. Makes WebGL development even harder than it already is imho.

Could we implement out-of-process WebGL rendering on our side only without JS developers having to render in workers to get better performance.
That's one thing Chromium is doing great afaik (WebGL rendering is done in GPU processes).

	Kelsey Gilbert [:jgilbert]
	Comment 16 • 13 years ago

I'll add that to keep the web workers from running wild with rendering, we should (at some point) also give them access to a SwapBuffers() like function.

Also, without locking resources, it would be hard to do good transparent off-thread rendering. The extra time has to come out somewhere, either as copying data temporarily for the uploads, or blocking on access attempts to the data until the function clears. Also, a couple calls require synchronization anyways, which is not fun to add to such a system.

	Patrick Walton (:pcwalton)
	Comment 17 • 13 years ago

(In reply to Jeff Gilbert [:jgilbert] from comment #16)
> I'll add that to keep the web workers from running wild with rendering, we
> should (at some point) also give them access to a SwapBuffers() like
> function.
> 
> Also, without locking resources, it would be hard to do good transparent
> off-thread rendering. The extra time has to come out somewhere, either as
> copying data temporarily for the uploads, or blocking on access attempts to
> the data until the function clears. Also, a couple calls require
> synchronization anyways, which is not fun to add to such a system.

I've discussed this with Dave Herman. One possibility, which would be very helpful not only for WebGL but also for 2D canvas (pdf.js in particular desperately wants this), would be to allow users to create pixel buffers that match a canvas, transfer them to and from workers, swap the canvas backing store with a pixel buffer, and acquire 2D contexts on pixel buffers. The API might look something like:

// Create a matching pixel buffer with the current width and height of the canvas.
// Pixel buffers are opaque (to avoid leaking platform-specific info about the current bit
// depth/color space to the web), but content can get a CanvasRenderingContext with the
// getContext() call. 
PixelBuffer HTMLCanvasElement::createPixelBuffer();
// Returns a canvas rendering context that allows rendering into the pixel buffer.
// Example: getContext('2d') will return a CanvasRenderingContext2D.
CanvasRenderingContext PixelBuffer::getContext(DOMString contextType);
// Swaps the current canvas backing store with the given pixel buffer and returns the
// original pixel buffer.
PixelBuffer HTMLCanvasElement::swapPixelBuffer(PixelBuffer newPixelBuffer);

Coupled with a zero-copy mechanism for transferring pixel buffers from worker to worker, this would allow users to implement double (or triple) buffering.

	Benoit Girard (:BenWa) Reporter
	Comment 18 • 13 years ago

(In reply to Patrick Walton (:pcwalton) from comment #17)

If I understand your suggestion then you need to coordinate with the main thread for rendering. The key idea of my proposal is to allow rendering without hitting the main thread (after setup). This is important for responsiveness because rendering doesn't block the main thread and the main thread doesn't block rendering. Perhaps you can elaborate more on the advantages.

(In reply to Jeff Gilbert [:jgilbert] from comment #16)

Don't we already have these problems without off main thread compositing regardless?

	Patrick Walton (:pcwalton)
	Comment 19 • 13 years ago

(In reply to Benoit Girard (:BenWa) from comment #18)
> (In reply to Patrick Walton (:pcwalton) from comment #17)
> 
> If I understand your suggestion then you need to coordinate with the main
> thread for rendering. The key idea of my proposal is to allow rendering
> without hitting the main thread (after setup). This is important for
> responsiveness because rendering doesn't block the main thread and the main
> thread doesn't block rendering. Perhaps you can elaborate more on the
> advantages.

Well, you would have to synchronize with the main thread, but the part on the main thread could be very small: just a pointer swap to swap buffers.

	Benoit Girard (:BenWa) Reporter
	Comment 20 • 13 years ago

(In reply to Patrick Walton (:pcwalton) from comment #19)
> Well, you would have to synchronize with the main thread, but the part on
> the main thread could be very small: just a pointer swap to swap buffers.

Why do you think that is preferable to my suggestion? Can you elaborate on the advantage? It seems to me like your design gives more flexibility but doesn't allow rendering to continue while the main thread is blocked. Once off-main-thread-compositing is completed allowing things to render without the main thread will give big improvement to the responsiveness of applications. This will be more obvious once we have plugins, video (and a bit later CSS animations) using it.

	Patrick Walton (:pcwalton)
	Comment 21 • 13 years ago

(In reply to Benoit Girard (:BenWa) from comment #20)
> (In reply to Patrick Walton (:pcwalton) from comment #19)
> > Well, you would have to synchronize with the main thread, but the part on
> > the main thread could be very small: just a pointer swap to swap buffers.
> 
> Why do you think that is preferable to my suggestion? Can you elaborate on
> the advantage? It seems to me like your design gives more flexibility but
> doesn't allow rendering to continue while the main thread is blocked. Once
> off-main-thread-compositing is completed allowing things to render without
> the main thread will give big improvement to the responsiveness of
> applications. This will be more obvious once we have plugins, video (and a
> bit later CSS animations) using it.

Hmm, I see, I guess it doesn't work if other tabs are executing long-running scripts. Ignore me then.

	Benoit Girard (:BenWa) Reporter
	Comment 22 • 13 years ago

I think being able to pass pixel buffers between workers and the main thread could be useful so I'm not trying to say it's a bad idea. But I think its use case is independent from my suggestions.

	Kelsey Gilbert [:jgilbert]
	Comment 23 • 13 years ago

It seems that for most cases, this doesn't win us much, since we're still resolving on the compositing thread (which will eventually be non-main-thread, but not yet). However, this should help with more complicated rendering, specifically any multi-pass rendering that has dependent passes.

Ideally, we could just do all the graphics stuff on the main JS thread (and put all the real computation in webworkers. Really, similar to how it's done in Java), but since that's also main-thread for us, it's not really viable. (At least that's my understanding) While getting off-main-thread JS (Electrolysis?) would obviate this need, it would still be nice to have the choice, so it's probably a good idea even in the long run.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 24 • 13 years ago

I think the ability to use WebGL (and 2D-canvas) from Workers would be really useful. Apart from GPU flushing issues, it's really the only way for Web developers to get reliable frame rates in the face of unpredictable main-thread delays. It would also be useful for real-time video processing with ProcessedMediaStream.

I would probably design the API so that when a canvas context is transferred in a message to the Worker, all other contexts for the canvas become completely non-functional. Other canvas APIs like toDataURL and being used as a source to drawImage would get a snapshot of the canvas contents from the last time the Worker yielded.

Unfortunately adding APIs to Workers currently requires writing raw JSAPI. So making more APIs like WebGL available from Workers is prohibitively painful right now. Need to wait until we have a WebIDL bindings framework in place that can work both on the main thread and in Workers, and is fast.

	Benoit Girard (:BenWa) Reporter
	Comment 25 • 13 years ago

(In reply to Jeff Gilbert [:jgilbert] from comment #23)
> It seems that for most cases, this doesn't win us much, since we're still
> resolving on the compositing thread (which will eventually be
> non-main-thread, but not yet). However, this should help with more
> complicated rendering, specifically any multi-pass rendering that has
> dependent passes.
> 

I'm suggesting that we move the resolve within the worker. Thus this would be a big win for the compositor who will no longer have to block on the compositor. We already have the main thread do the resolve with e10s or OMTC since we can't share the GLContext across thread context.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)
> I would probably design the API so that when a canvas context is transferred
> in a message to the Worker, all other contexts for the canvas become
> completely non-functional. 

Right that's what I was trying to suggest in my initial proposal.

> 
> Unfortunately adding APIs to Workers currently requires writing raw JSAPI.
> So making more APIs like WebGL available from Workers is prohibitively
> painful right now. Need to wait until we have a WebIDL bindings framework in
> place that can work both on the main thread and in Workers, and is fast.

That's sad. Does someone have the bug number for this change so we can make it block this?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 26 • 13 years ago

Although I think this feature would be useful, I don't expect many developers would use it since it would be quite hard to write code to do all rendering from a Worker. So I don't think you should see this as any kind of performance solution for OMTC. You should probably have a separate bug tracking optimizations of GPU flushing for main-thread WebGL and OMTC.

	Dan Mosedale (:dmosedale, :dmose)
	Comment 27 • 12 years ago

At GDC, one of the (non-Google) speakers claimed that the Chrome team was working this, though they hadn't announced it yet.  I have no insight into whether that's actually true.

	Kelsey Gilbert [:jgilbert]
	Comment 28 • 12 years ago

(In reply to Dan Mosedale (:dmose) from comment #27)
> At GDC, one of the (non-Google) speakers claimed that the Chrome team was
> working this, though they hadn't announced it yet.  I have no insight into
> whether that's actually true.

WebGL in workers was an important TODO at the last Khronos F2F, so all WebGL implementors are working towards WebGL in workers.

	Benoit Girard (:BenWa) Reporter
	Comment 29 • 12 years ago

I think it's important to have a solution to lets WebGL post directly to the compositor rather then simply have the worker offload computation.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 30 • 12 years ago

Yes.

The new-DOM-bindings team's next project is going to be canvas-2d but probably could be WebGL instead. That would unblock the biggest issue here. (I hope new-DOM-bindings can work in Workers!)

There are a couple of standards-related issues here. Due to the fact that we can't use DOM elements in a worker, the "canvas" attribute can't work (it could just return null I guess), and texImage2D_dom and texSubImage2D_dom would not be usable. Also we need an API to actually pass control of a WebGL canvas to a worker. I'm not sure what that should look like.

	Benoit Girard (:BenWa) Reporter
	Comment 31 • 12 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30)
I'm not sure what that should look like.

What do you think of my proposal in Comment 0?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 32 • 12 years ago

Oops, sorry.

Something like that sounds right. It probably should be tied into the postMessage Transferable feature, i.e., a WebGL context can be Transferable, passing a WebGL context without transferring it would cause a structured clone failure (as now), but otherwise you get a WebGL context on the other side.

We don't necessarily need a new context type. It would be simpler to just be able to transfer existing WebGL contexts.

Jonas' comments are also apropos. When a WebGL context is transferred to a Worker, the canvas has to become a complete black box.

I'm not sure how resizing should work. You will want to be able to resize such canvases. Maybe you allow resizing from the main thread, but the WebGL context owned by the Worker gets completely disconnected from the canvas, with ownership returning to the page?

For frame rate control, we will need some new API to tell the worker when composition has happened and to allow the worker to swap buffers.

	Kelsey Gilbert [:jgilbert]
	Comment 33 • 12 years ago

I don't see any real reason resizing couldn't be async. We're already talking about lazy buffer allocation in another bug, as right now changing the width, then the height yields two resize operations, likely with large buffers on the GPU.

	Boris Zbarsky [:bzbarsky]
	Comment 34 • 12 years ago

> The new-DOM-bindings team's next project is going to be canvas-2d but probably could be
> WebGL instead.

I'd been thinking of doing both in some order as the next set of things, yes.  We could try webgl before canvas2d.

> I hope new-DOM-bindings can work in Workers

That's why we started with XMLHttpRequest, actually, since it's exposed to both workers and non-workers.

That said, the memory management in workers is somewhat different from main-thread.  I do think we want to make it possible to use the same object in both, though...

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 35 • 12 years ago

How are you handling that with XHR?

	Boris Zbarsky [:bzbarsky]
	Comment 36 • 12 years ago

The worker XHR impl is completely different (since it has to proxy all the necko access to the main thread, for example)...

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 37 • 12 years ago

To be clear, the worker XHR implementation creates an nsXMLHttpRequest on the main thread, and proxies calls and events between the threads.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 38 • 12 years ago

I'd appreciate it if this was brought up on whatwg@whatwg.org...

	(dormant account)
	Comment 39 • 12 years ago

pwalton suggested that we need an OMTC tab strip implemented in canvas if we are to have a responsive tab strip. I'm told that even though this this bug is about WebGL, it applies equally to 2d canvas.

	Patrick Walton (:pcwalton)
	Comment 40 • 12 years ago

I don't think it's the only way to have a responsive tab strip, but it's one I think is worth investigating.

	Joe Drew (not getting mail)
	Comment 41 • 12 years ago

This is off-topic for this bug, but please CC me on the bug where we have gathered the data showing where we're spending our time while painting the tab strip. It seems like it'd be better to make the tab strip cheap to paint.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 42 • 12 years ago

(In reply to Joe Drew (:JOEDREW!) from comment #41)

Bug 750417.

	(dormant account)
	Comment 43 • 12 years ago

(In reply to Joe Drew (:JOEDREW!) from comment #41)
> This is off-topic for this bug, but please CC me on the bug where we have
> gathered the data showing where we're spending our time while painting the
> tab strip. It seems like it'd be better to make the tab strip cheap to paint.

bug 593680 covers general tab strip slowness.

We also need ability to make the tab strip async(ie to make the throbber, tab animations not jank during page load), which seems on topic for this bug

	Benoit Girard (:BenWa) Reporter
	Comment 44 • 12 years ago

(In reply to Taras Glek (:taras) from comment #43)
> 
> We also need ability to make the tab strip async(ie to make the throbber,
> tab animations not jank during page load), which seems on topic for this bug

It's not, please don't scope creep this bug otherwise it wont get done. This bug is about a suggestion to extended WebGL. If this feature is useful for something else feel free to make it block this bug.

	Benoit Jacob [:bjacob] (mostly away)
	Comment 45 • 12 years ago

Talking of getting this done: BenWa, when you get a chance, could you draft a spec proposal? I don't see any other next step to make progress here.

	Benoit Girard (:BenWa) Reporter
	Comment 46 • 12 years ago

(In reply to Benoit Jacob [:bjacob] from comment #45)
> Talking of getting this done: BenWa, when you get a chance, could you draft
> a spec proposal? I don't see any other next step to make progress here.

I wont have time until Fennec ships, after that I'll see what my priorities are. Earliest for me would be Q3.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 47 • 11 years ago

I will probably be driving this.

	David Bruant
	Comment 48 • 11 years ago

I don't see it mentioned in this bug, so I'll share it.
CanvasProxy http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#canvasproxy
Something very close to what is described here. Note that it's a transferable.

On a side note, there is more and more discussion about making process-isolated iframes (Servo, Blink, different standard mailing-lists). If iframes could be process-isolated, I begin to wonder what the benefits of WebGL in worker really are (since you can do WebGL *and lots of other things* in an iframe).
Maybe a bug isn't the right place to discuss this. I'm happy to join if someone starts a thread on the topic (I'm not sure which ML would be mosy appropriate)

	Benoit Jacob [:bjacob] (mostly away)
	Comment 49 • 11 years ago

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #47)
> I will probably be driving this.

\o/

CC-wise you'll find that the WebGL implementation is fairly standard, with the only non-trivial bits being one or two custom overloads of ImplCycleCollection{Traverse,Unlink}. There is one in WebGLVertexAttribData.h and one in WebGLObjectModel.h.

Note that WebGLContext is intentionally very conservative in using strong references to limit CC overhead. For instance, the WebGLContext only has an array of strong references (mBound2DTextures) to currently bound textures, whereas not-currently-bound textures are intentionally not held by a strong reference by the WebGLContext; instead there is only a list of plain pointers to them (mTextures). Thus in WebGLContext.cpp, where we implement CC for WebGLContext, we only have to traverse mBound2DTextures, not mTextures. And so on.

The other aspect of this work is going to be hooking up with the compositor; let us know when you get there.

	Benoit Jacob [:bjacob] (mostly away)
	Comment 50 • 11 years ago

Oh, and the big documentation comments about WebGL reference counting (why objects have *2* refcounts) in WebGLObjectModel.h is recommended reading.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 51 • 11 years ago

(In reply to David Bruant from comment #48)
> I don't see it mentioned in this bug, so I'll share it.
> CanvasProxy
> http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-
> element.html#canvasproxy
> Something very close to what is described here. Note that it's a
> transferable.
> 
> On a side note, there is more and more discussion about making
> process-isolated iframes (Servo, Blink, different standard mailing-lists).
> If iframes could be process-isolated, I begin to wonder what the benefits of
> WebGL in worker really are (since you can do WebGL *and lots of other
> things* in an iframe).
> Maybe a bug isn't the right place to discuss this. I'm happy to join if
> someone starts a thread on the topic (I'm not sure which ML would be mosy
> appropriate)

CanvasProxy is not a very good design.  We're going to have an alternate proposal.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 52 • 11 years ago

(In reply to David Bruant from comment #48)
> On a side note, there is more and more discussion about making
> process-isolated iframes (Servo, Blink, different standard mailing-lists).
> If iframes could be process-isolated, I begin to wonder what the benefits of
> WebGL in worker really are (since you can do WebGL *and lots of other
> things* in an iframe).
> Maybe a bug isn't the right place to discuss this. I'm happy to join if
> someone starts a thread on the topic (I'm not sure which ML would be mosy
> appropriate)

The benefit is that we can do off main thread WebGL in the next few months.  Process isolated iframes in Gecko are likely years away.

	Boris Zbarsky [:bzbarsky]
	Comment 53 • 11 years ago

> On a side note, there is more and more discussion about making process-isolated iframes

Those are an implementation detail, not a spec requirement.

	David Bruant
	Comment 54 • 11 years ago

(In reply to Boris Zbarsky (:bz) from comment #53)
> > On a side note, there is more and more discussion about making process-isolated iframes
> 
> Those are an implementation detail, not a spec requirement.
I know, I know, but I was wondering whether the work of this bug wasn't largely overlapping with the work of process-isolating iframes.
Kyle answered my concern (few months VS years).

	Kelsey Gilbert [:jgilbert]
	Comment 55 • 11 years ago

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #51)
> CanvasProxy is not a very good design.  We're going to have an alternate
> proposal.

I didn't catch this first time around, but there's already a draft for this kicking around the WebGL WG. IIRC, we had a pretty good design for this already, though it naturally diverges from the CanvasProxy spec.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 56 • 11 years ago

Yeah, I know.  I've been talking to vlad about all of this ;-)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 57 • 11 years ago

Is this new design written down anywhere?

	Morris Tseng [:mtseng] [:Morris] (Inactive) Assignee
	Comment 58 • 11 years ago

This patch add function to allow off-main-thread canvas send ipc message to composition thread by using ImageBridge

	Morris Tseng [:mtseng] [:Morris] (Inactive) Assignee
	Comment 59 • 11 years ago

this patch create webidl interfaces for WorkerCanvas

	Morris Tseng [:mtseng] [:Morris] (Inactive) Assignee
	Comment 60 • 11 years ago

glue up all codes and let WebGL in worker works

	Morris Tseng [:mtseng] [:Morris] (Inactive) Assignee
	Comment 61 • 11 years ago

example for webgl in workers

	Nicolas Silva [:nical]
	Comment 62 • 11 years ago

(In reply to Morris Tseng [:mtseng] from comment #58)
> Created attachment 819664 [details] [diff] [review]
> (WIP)WebGLinWorkers-step0
> 
> This patch add function to allow off-main-thread canvas send ipc message to
> composition thread by using ImageBridge

Awesome, I gave it a look and it looks good to me.

	Kelsey Gilbert [:jgilbert]
	Comment 63 • 11 years ago

Comment on attachment 819666 [details] [diff] [review]
(WIP)WebGLinWorkers-step2

Review of attachment 819666 [details] [diff] [review]:
-----------------------------------------------------------------

Before finishing this up, we should have a spec document for how this is supposed to work. It's great to see a WIP, though.

::: content/canvas/src/WebGLContext.cpp
@@ +862,5 @@
> +        WorkerCanvas* canvas = userdata->mContent;
> +        WebGLContext* context = static_cast<WebGLContext*>(canvas->GetContext());
> +
> +        // Present our screenbuffer, if needed.
> +        context->PresentScreenBuffer();

We don't want to (and, I think, can't) do this for workers. Workers will need to explicitly call `commit` in order to Present frames.

::: dom/webidl/HTMLCanvasElement.webidl
@@ +31,5 @@
>    void toBlob(FileCallback _callback,
>                optional DOMString type = "",
>                optional any encoderOptions);
> +
> +  WorkerCanvas transferControlToProxy();

Why do we not call this a CanvasProxy, as the current spec does?

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 64 • 11 years ago

There's a big thread on whatwg about this with a new spec proposal, summarized at https://wiki.mozilla.org/User:Roc/WorkerCanvasProposal.

	Morris Tseng [:mtseng] [:Morris] (Inactive) Assignee
	Comment 65 • 11 years ago

(In reply to Jeff Gilbert [:jgilbert] from comment #63)
> Comment on attachment 819666 [details] [diff] [review]
> (WIP)WebGLinWorkers-step2
> 
> Review of attachment 819666 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Before finishing this up, we should have a spec document for how this is
> supposed to work. It's great to see a WIP, though.
> 
> ::: content/canvas/src/WebGLContext.cpp
> @@ +862,5 @@
> > +        WorkerCanvas* canvas = userdata->mContent;
> > +        WebGLContext* context = static_cast<WebGLContext*>(canvas->GetContext());
> > +
> > +        // Present our screenbuffer, if needed.
> > +        context->PresentScreenBuffer();
> 
> We don't want to (and, I think, can't) do this for workers. Workers will
> need to explicitly call `commit` in order to Present frames.

Yes, the commmit would call Present frames. Above code will be removed.

	Morris Tseng [:mtseng] [:Morris] (Inactive) Assignee
	Comment 66 • 11 years ago

more complex demo use webgl in workers. the code are modified version of below link's example: http://learningwebgl.com/blog/?p=370. So the result should be as same as the link provided

	Morris Tseng [:mtseng] [:Morris] (Inactive) Assignee
	Comment 71 • 11 years ago

Rebase patches to latest mozilla-central codebase and most of features are completed.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 72 • 11 years ago

Comment on attachment 827917 [details] [diff] [review]
(WIP)WebGLinWorkers-step0

nical, can you take a look at this for us?

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 73 • 11 years ago

Comment on attachment 827919 [details] [diff] [review]
(WIP)WebGLinWorkers-step1

Setting f?me so I don't forget to look at these.

	Nicolas Silva [:nical]
	Comment 74 • 11 years ago

Comment on attachment 827917 [details] [diff] [review]
(WIP)WebGLinWorkers-step0

Review of attachment 827917 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks good.

::: gfx/gl/SurfaceStream.h
@@ +29,5 @@
>  public:
>      typedef enum {
>          MainThread,
> +        OffMainThread,
> +        WorkerThread

Did you modify ChooseGLStreamType somewhere to take this new value into account? I didn't find it anywhere. Looking at the current implem it would make the value WorkerThread fallback to the MainThread code path which looks unintentional.
What behavior change with respect to SurfaceStream do we need with respect to the OffMainThread case?

::: gfx/layers/AsyncCanvasRenderer.h
@@ +6,5 @@
> +
> +#ifndef mozilla_AsyncCanvasRenderer_h__
> +#define mozilla_AsyncCanvasRenderer_h__
> +
> +#include "mozilla/layers/CanvasClient.h"

You should forward reference CanvasClient and only #include it in the .cpp file.

@@ +12,5 @@
> +
> +namespace mozilla {
> +namespace layers {
> +
> +class AsyncCanvasRenderer {

I'd like to see some class documentation.

::: gfx/layers/client/CanvasClient.h
@@ +69,5 @@
>  protected:
>    TextureInfo mTextureInfo;
>  };
>  
> +class CanvasClientBridge : public CanvasClient

Please add a doc comment explaining that this is a place-holder for the layer on the main thread only meant to forward the AsyncID, while the "actual" CanvasClient is on the ImageBridgeChild thread.

::: gfx/layers/ipc/ImageBridgeParent.cpp
@@ +95,5 @@
>  ImageBridgeParent::RecvUpdateNoSwap(const EditArray& aEdits)
>  {
>    InfallibleTArray<EditReply> noReplies;
>    bool success = RecvUpdate(aEdits, &noReplies);
> +  //NS_ABORT_IF_FALSE(noReplies.Length() == 0, "RecvUpdateNoSwap requires a sync Update to carry Edits");

The final patch should not have this commented out.

	Morris Tseng [:mtseng] [:Morris] (Inactive) Assignee
	Comment 75 • 11 years ago

(In reply to Nicolas Silva [:nical] from comment #74)
> Comment on attachment 827917 [details] [diff] [review]
> (WIP)WebGLinWorkers-step0
> 
> Review of attachment 827917 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall looks good.
> 
> ::: gfx/gl/SurfaceStream.h
> @@ +29,5 @@
> >  public:
> >      typedef enum {
> >          MainThread,
> > +        OffMainThread,
> > +        WorkerThread
> 
> Did you modify ChooseGLStreamType somewhere to take this new value into
> account? I didn't find it anywhere. Looking at the current implem it would
> make the value WorkerThread fallback to the MainThread code path which looks
> unintentional.
> What behavior change with respect to SurfaceStream do we need with respect
> to the OffMainThread case?

OffMainThread case use async version of SurfaceStream which is waiting for compositor taking the produced frame. In the WorkerThread case, sometimes Compositor thread will go away but not worker thread (e.g. If we switch to another tab and switch back). If we use async version of SurfaceStream in this case, WorkerThread is going to enter a deadlock because it will wait for compositor forever. In order to avoid deadlock, I use non-wait version of SurfaceStream.

> 
> ::: gfx/layers/AsyncCanvasRenderer.h
> @@ +6,5 @@
> > +
> > +#ifndef mozilla_AsyncCanvasRenderer_h__
> > +#define mozilla_AsyncCanvasRenderer_h__
> > +
> > +#include "mozilla/layers/CanvasClient.h"
> 
> You should forward reference CanvasClient and only #include it in the .cpp
> file.
> 

OK!
> @@ +12,5 @@
> > +
> > +namespace mozilla {
> > +namespace layers {
> > +
> > +class AsyncCanvasRenderer {
> 
> I'd like to see some class documentation.
> 
> ::: gfx/layers/client/CanvasClient.h
> @@ +69,5 @@
> >  protected:
> >    TextureInfo mTextureInfo;
> >  };
> >  
> > +class CanvasClientBridge : public CanvasClient
> 
> Please add a doc comment explaining that this is a place-holder for the
> layer on the main thread only meant to forward the AsyncID, while the
> "actual" CanvasClient is on the ImageBridgeChild thread.
> 
I'd add some document here.

> ::: gfx/layers/ipc/ImageBridgeParent.cpp
> @@ +95,5 @@
> >  ImageBridgeParent::RecvUpdateNoSwap(const EditArray& aEdits)
> >  {
> >    InfallibleTArray<EditReply> noReplies;
> >    bool success = RecvUpdate(aEdits, &noReplies);
> > +  //NS_ABORT_IF_FALSE(noReplies.Length() == 0, "RecvUpdateNoSwap requires a sync Update to carry Edits");
> 
> The final patch should not have this commented out.

	Nicolas Silva [:nical]
	Comment 76 • 11 years ago

(In reply to Morris Tseng [:mtseng] from comment #75)
> OffMainThread case use async version of SurfaceStream which is waiting for
> compositor taking the produced frame. In the WorkerThread case, sometimes
> Compositor thread will go away but not worker thread (e.g. If we switch to
> another tab and switch back). If we use async version of SurfaceStream in
> this case, WorkerThread is going to enter a deadlock because it will wait
> for compositor forever. In order to avoid deadlock, I use non-wait version
> of SurfaceStream.

Ok, makes sense. Please add a comment in SurfaceStream::ChooseGLStreamType to explain this. Because this function reads like this: "if we are doing OMTC do A, else do B" and the worker thread scenario can also be doing OMTC so we need to make it apparent here that we consciously choose the "non-OMTC" branch even though we may be dong off-main-thread compositing.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 77 • 11 years ago

Some general comments:

For now we should make the WorkerCanvas and the transferControlToWorker method be preference controlled (e.g. http://mxr.mozilla.org/mozilla-central/source/dom/webidl/MediaStreamEvent.webidl#14 and http://mxr.mozilla.org/mozilla-central/source/dom/webidl/AudioContext.webidl#87).

WebGLContext adds and removes itself from a global array of contexts in the WebGLMemoryReporterWrapper (e.g. http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLMemoryReporterWrapper.h#42).  Obviously without any locking that's going to cause problems.  But even if we do add locking we can't report memory from the main thread, because that relies on looking at member variables of the context which could also be changing (http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLMemoryReporterWrapper.h#55).  I think for now we should just only add ourselves to this list on the main thread, and we'll fix this in a separate bug before we remove the preference.

WebGLContext::UpdateLastUseIndex has a similar problem, so lets also skip calling that off the main thread for now.

80 character lines everywhere please.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 78 • 11 years ago

Comment on attachment 827919 [details] [diff] [review]
(WIP)WebGLinWorkers-step1

Review of attachment 827919 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/WorkerCanvas.cpp
@@ +10,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +

nit: extra newline

@@ +45,5 @@
> +  return WorkerCanvasBinding::Wrap(aCx, aScope, this);
> +}
> +
> +already_AddRefed<WorkerCanvas>
> +WorkerCanvas::Constructor(const GlobalObject& global, uint32_t width, uint32_t height, ErrorResult& aRv)

Gecko style is to name all arguments aFoo (and member variables mFoo, statics sFoo, globals gFoo, and constants kFoo).  Fix global, width, and height please.

@@ +52,5 @@
> +  return workerCanvas.forget();
> +}
> +
> +already_AddRefed<nsISupports>
> +WorkerCanvas::GetContext(JSContext* aCx, const nsAString& aContextId, JS::Handle<JS::Value> aContextOptions, ErrorResult& rv)

aRv.

@@ +58,5 @@
> +  return nullptr;
> +}
> +
> +void
> +WorkerCanvas::ToBlob(JSContext* cx,

Here too.

::: content/canvas/src/WorkerCanvas.h
@@ +36,5 @@
> +  WorkerCanvas* GetParentObject() const;
> +
> +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> +
> +  static already_AddRefed<WorkerCanvas> Constructor(const GlobalObject& global, uint32_t width, uint32_t height, ErrorResult& aRv);

And in the header.

@@ +49,5 @@
> +    return mHeight;
> +  }
> +
> +  // Mark this as resultNotAddRefed to return raw pointers
> +  already_AddRefed<nsISupports> GetContext(JSContext* cx, const nsAString& contextId, JS::Handle<JS::Value> contextOptions, ErrorResult& aRv);

You can remove the comment from the code generator.

@@ +53,5 @@
> +  already_AddRefed<nsISupports> GetContext(JSContext* cx, const nsAString& contextId, JS::Handle<JS::Value> contextOptions, ErrorResult& aRv);
> +
> +  void ToBlob(JSContext* cx, FileCallback& callback, const nsAString& type, const Optional<JS::Handle<JS::Value> >& encoderOptions, ErrorResult& aRv);
> +
> +  bool Commit();

We decided to remove this.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 79 • 11 years ago

Comment on attachment 827920 [details] [diff] [review]
(WIP)WebGLinWorkers-step2

Review of attachment 827920 [details] [diff] [review]:
-----------------------------------------------------------------

Looking pretty good.  Lets get these comments addressed and then we'll get a <canvas> person to review the canvas parts and I'll look over the DOM stuff more closely.

::: content/canvas/src/WorkerCanvas.cpp
@@ +77,5 @@
> +{
> +  return str.EqualsLiteral("webgl") ||
> +         str.EqualsLiteral("experimental-webgl") ||
> +         str.EqualsLiteral("moz-webgl");
> +}

Don't duplicate this code.  Move it somewhere where it can be shared between this and HTMLCanvasElement (maybe CanvasUtils.h/cpp?).

@@ +96,5 @@
> +    if (!cp) {
> +      mCurrentContext = nullptr;
> +      rv.Throw(NS_ERROR_FAILURE);
> +      return nullptr;
> +    }

I don't think any of this code to ensure that the context participates in cycle collection is useful anymore.  No need to copy it over here.

@@ +192,5 @@
> +                               nsICanvasRenderingContextInternal **aContext)
> +{
> +  NS_ENSURE_ARG(aContext);
> +
> +  if (aContextId.EqualsLiteral("2d")) {

We definitely don't want to do this.  2d should fail for now.  It's going to take a lot more work to get the 2d backend working.

::: content/canvas/src/WorkerCanvas.h
@@ +8,5 @@
>  #define mozilla_dom_WorkerCanvas_h__
>  
>  #include "mozilla/Attributes.h"
>  #include "mozilla/ErrorResult.h"
> +#include "mozilla/ReentrantMonitor.h"   // for ReentrantMonitor, etc

I don't think you are using this, so remove the include?

@@ +13,5 @@
>  #include "mozilla/dom/BindingDeclarations.h"
>  #include "nsCycleCollectionParticipant.h"
>  #include "nsWrapperCache.h"
> +#include "nsSize.h"
> +#include "nsICanvasElementExternal.h"

It also doesn't look like you're using this?  We want to remove nsICanvasElementExternal eventually anyways.

@@ +46,5 @@
>    NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>    NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(WorkerCanvas)
>  
>  public:
> +  WorkerCanvas(uint32_t aWidth, uint32_t aHeight, bool aOpaque, AsyncCanvasRenderer* aRenderer);

80 character lines please.

::: dom/webidl/HTMLCanvasElement.webidl
@@ +31,5 @@
>    void toBlob(FileCallback _callback,
>                optional DOMString type = "",
>                optional any encoderOptions);
> +
> +  WorkerCanvas transferControlToWorker();

Nit: Please put this on a separate "partial interface" bit for now.

::: dom/workers/WorkerPrivate.cpp
@@ +3767,5 @@
>          static_cast<nsIRunnable*>(event)->Run();
> +        for (uint32_t index = 0; index < mInvalidateWorkerCanvas.Length(); index++) {
> +          mInvalidateWorkerCanvas[index]->Commit();
> +        }
> +        mInvalidateWorkerCanvas.Clear();

I think you should put this in a separate method and just call it from here.  "CommitCanvases" or something.

	Morris Tseng [:mtseng] [:Morris] (Inactive) Assignee
	Comment 80 • 11 years ago

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #79)
> Comment on attachment 827920 [details] [diff] [review]
> (WIP)WebGLinWorkers-step2
> 
> Review of attachment 827920 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking pretty good.  Lets get these comments addressed and then we'll get a
> <canvas> person to review the canvas parts and I'll look over the DOM stuff
> more closely.
> 
> ::: content/canvas/src/WorkerCanvas.cpp
> @@ +77,5 @@
> > +{
> > +  return str.EqualsLiteral("webgl") ||
> > +         str.EqualsLiteral("experimental-webgl") ||
> > +         str.EqualsLiteral("moz-webgl");
> > +}
> 
> Don't duplicate this code.  Move it somewhere where it can be shared between
> this and HTMLCanvasElement (maybe CanvasUtils.h/cpp?).
> 

I duplicate lots of code from HTMLCanvasElemnt class(e.g GetContext(), ToBlob() and other utility functions). I think I should move those codes to CanvasUtils.h as well. Any opinions?

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 81 • 11 years ago

CanvasUtils or a base class?  It seems like you really just want an abstract base class here for both of these, even if it just has static helpers for now if that's simpler.  I'd support doing a base class shared by both of the canvas element-ish impls.

	Morris Tseng [:mtseng] [:Morris] (Inactive) Assignee
	Comment 82 • 11 years ago

update patch to address comment

	Morris Tseng [:mtseng] [:Morris] (Inactive) Assignee
	Comment 83 • 11 years ago

update patch to address comment

	Morris Tseng [:mtseng] [:Morris] (Inactive) Assignee
	Comment 84 • 11 years ago

Move common functions which are related to CanvasRenderingContext to a base class.

	Morris Tseng [:mtseng] [:Morris] (Inactive) Assignee
	Comment 85 • 11 years ago

update patch to address comment

	Nicolas Silva [:nical]
	Comment 86 • 11 years ago

Comment on attachment 8335128 [details] [diff] [review]
(WIP)WebGLinWorkers-step0

Review of attachment 8335128 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but I'd like to understand what's happening with the assertion in ImageBridgeParent before I give you the r+

::: content/html/content/public/HTMLCanvasElement.h
@@ +14,5 @@
>  #include "nsError.h"
>  
>  #include "nsICanvasElementExternal.h"
>  #include "mozilla/gfx/Rect.h"
> +#include <map>

nit: if the AsyncCanvasMap is going to be static, I would prefer to keep the include and declaration in the .cpp

::: gfx/layers/AsyncCanvasRenderer.cpp
@@ +20,5 @@
> +  , mCanvasClientAsyncID(0)
> +  , mCanvasClient(nullptr)
> +  , mGLContext(nullptr)
> +{
> +}

nit: MOZ_COUNT_CTOR here and MOZ_COUNT_DTOR in the destructor below.

::: gfx/layers/ipc/ImageBridgeParent.cpp
@@ -95,5 @@
>  ImageBridgeParent::RecvUpdateNoSwap(const EditArray& aEdits)
>  {
>    InfallibleTArray<EditReply> noReplies;
>    bool success = RecvUpdate(aEdits, &noReplies);
> -  NS_ABORT_IF_FALSE(noReplies.Length() == 0, "RecvUpdateNoSwap requires a sync Update to carry Edits");

In what cases would we hit this assertion? Looks like we should keep it and fix what's causing us to generate replies in a async transactions.

	Morris Tseng [:mtseng] [:Morris] (Inactive) Assignee
	Comment 87 • 11 years ago

I updated patch to address comment. And also put the assertion code in ImageBridgeParent back.

	Morris Tseng [:mtseng] [:Morris] (Inactive) Assignee
	Comment 88 • 11 years ago

Because patch step0 change, I have to change some dependency codes.

	Morris Tseng [:mtseng] [:Morris] (Inactive) Assignee
	Comment 89 • 11 years ago

Because patch step0 change, I have to change some dependency codes.

	Till Schneidereit [:till]
	Comment 90 • 10 years ago

Morris, what's the status on this? The Shumway project would be very interested in having this.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 91 • 10 years ago

The patch needs to be unbitrotted and reviewed.  Unfortunately reviewer bandwidth (i.e. me) has been very constrained :(

	Jon Morton [:jmorton]
	Comment 92 • 10 years ago

Unbitrotted version of mtseng's part0.
Most of it is essentially same, except for CanvasClientSurfaceStream::Update*.

	Jon Morton [:jmorton]
	Comment 93 • 10 years ago

Oops! Patch was missing new AsyncCanvasRenderer files, here's a new version

	Jon Morton [:jmorton]
	Comment 94 • 10 years ago

Unbitrotted version of mtseng's part 1 - WebIDL skeleton.
Mostly unchanged except for some binding updates.

	Nicolas Silva [:nical]
	Comment 95 • 10 years ago

Comment on attachment 8439738 [details] [diff] [review]
WorkerCanvas-WIP-Part-1-OMT-canvas-with-ImageBridge.patch

Review of attachment 8439738 [details] [diff] [review]:
-----------------------------------------------------------------

This is a big patch so It'll take time for me to review it. So far it looks good.

	Nicolas Silva [:nical]
	Comment 96 • 10 years ago

Comment on attachment 8439738 [details] [diff] [review]
WorkerCanvas-WIP-Part-1-OMT-canvas-with-ImageBridge.patch

Review of attachment 8439738 [details] [diff] [review]:
-----------------------------------------------------------------

Note that this needs to block on ImageBridge being activated by default on all platforms. This should be the case soon (depends on whether linux OMTC will have async video by default or not).

	Kelsey Gilbert [:jgilbert]
	Comment 97 • 10 years ago

Comment on attachment 8439738 [details] [diff] [review]
WorkerCanvas-WIP-Part-1-OMT-canvas-with-ImageBridge.patch

Review of attachment 8439738 [details] [diff] [review]:
-----------------------------------------------------------------

I'm going the cherry-pick this function to illustrate some style issues.
I'm not going to r- it, since it's not my files, but I'd like to point this out. :)

::: gfx/layers/ipc/ImageBridgeChild.cpp
@@ +242,5 @@
> +static void CreateCanvasClientSync(RefPtr<CanvasClient>* result,
> +                                  ReentrantMonitor* barrier,
> +                                  CanvasClient::CanvasClientType aType,
> +                                  TextureFlags aFlag,
> +                                  bool *aDone)

static void CreateCanvasClientSync(RefPtr<CanvasClient>* result,
                                   ReentrantMonitor* barrier,
                                   CanvasClient::CanvasClientType aType,
                                   TextureFlags aFlag,
                                   bool *aDone)

There are a few style issues here:
* Bad indentation on 'hanging' args.
* Type and function name on same line at base scope.
* Outvars unmarked and interspersed with normal arguments.
* Mixed 'aFoo' and 'foo' namings.
* Both star-to-left and star-to-right.
(Non-outvar passed as pointer in C++ sucks, but is needed here because of how this is called)

All in all, ouch. Try something closer to:
static void
CreateCanvasClientSync(ReentrantMonitor* const barrier,
                       CanvasClient::CanvasClientType type,
                       TextureFlags flag,
                       RefPtr<CanvasClient>* const out_result,
                       bool* const out_done)

	Jon Morton [:jmorton]
	Comment 98 • 10 years ago

(In reply to Jeff Gilbert [:jgilbert] from comment #97)
> Comment on attachment 8439738 [details] [diff] [review]
> WorkerCanvas-WIP-Part-1-OMT-canvas-with-ImageBridge.patch
> 
> Review of attachment 8439738 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm going the cherry-pick this function to illustrate some style issues.
> I'm not going to r- it, since it's not my files, but I'd like to point this
> out. :)
> 

Ouch indeed... this part was copied from the original patch, which appears to be copied from the existing CreateImageClientSync. I should have noticed and fixed this before, though. I'll update this and the original (and any other bad style I can find) for the new patch -- thanks!

	Jon Morton [:jmorton]
	Comment 100 • 10 years ago

Updated Part 1
 * Fix code style issues
 * Fix crash on canvas cleanup
 * Resolve conflicts in changes on m-c to CanvasClientSurfaceStream

	Jon Morton [:jmorton]
	Comment 101 • 10 years ago

Updated Part 2 - FileCallback? -> FileCallback in WorkerCanvas.webidl so it's the same as HTMLCanvasElement

	Jon Morton [:jmorton]
	Comment 102 • 10 years ago

Combined version of bitrotted step 1.5 and step 2 patches, with numerous updates and fixes.

Should Work pretty decently at this point, except for an occasional memory corruption bug we're still trying to track down and a thread safety bug in ANGLE which should be fixed after bug 1010371 lands.

Tests and examples I've been using are at https://github.com/Jonanin/WorkerCanvasTests

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 103 • 10 years ago

Comment on attachment 8445611 [details] [diff] [review]
0002-WorkerCanvas-WIP-Part-2-WebIDL.patch

Review of attachment 8445611 [details] [diff] [review]:
-----------------------------------------------------------------

You should squash this with the next patch.  It's not very useful on its own.

::: dom/webidl/WorkerCanvas.webidl
@@ +7,5 @@
> + * http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2013-October/041074.html
> + */
> +
> +[Pref="gfx.workercanvas.enabled",
> + Constructor(unsigned long width, unsigned long height)]

This isn't really useful without toBlob/etc, so lets go ahead and remove the constructor for now.

I suspect that Pref="" does the wrong thing on workers today ... we should investigate that.

@@ +19,5 @@
> +  nsISupports? getContext(DOMString contextId,
> +                          optional any contextOptions = null);
> +
> +  [Throws]
> +  void toBlob(FileCallback? _callback,

toBlob is unimplemented for now, so just leave that off and we can add it back later.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 104 • 10 years ago

Comment on attachment 8445617 [details] [diff] [review]
0003-WorkerCanvas-WIP-Part-3.patch

Review of attachment 8445617 [details] [diff] [review]:
-----------------------------------------------------------------

There's still more to do here, so for now just some style things.

::: content/canvas/src/WorkerCanvas.h
@@ +96,5 @@
> +  void Invalidate();
> +
> +  bool CommitFrameToCompositor();
> +
> +  virtual bool GetIsOpaque()

You should mark all of these methods that override base class methods MOZ_OVERRIDE.

@@ +129,5 @@
>    // Mapped to HTMLCanvasElement's moz_opaque
>    bool mOpaque;
>    bool mAttrDirty;
> +  nsRefPtr<layers::AsyncCanvasRenderer> mCanvasRenderer;
> +  layers::CanvasClient* mCanvasClient;

mCanvasClient should be a smart pointer

::: gfx/thebes/gfxPrefs.h
@@ +233,5 @@
> +  DECL_GFX_PREF(Once, "webgl.prefer-egl",                      WebGLPreferEGL, bool, false);
> +  DECL_GFX_PREF(Once, "webgl.prefer-native-gl",                WebGLPreferNativeGL, bool, false);
> +#endif
> +
> +  DECL_GFX_PREF(Once, "webgl.force-layers-readback",           WebGLForceLayersReadback, bool, false);  

whitespace at EOL

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 105 • 10 years ago

Also, you should add something in all of the commit messages here crediting Morris Tseng for writing the original patches.

	Jon Morton [:jmorton]
	Comment 106 • 10 years ago

Updated Part 1 - Notable change is in ImageBridgeChild::FlushCanvasClientTexture

	Jon Morton [:jmorton]
	Comment 109 • 10 years ago

Most changes since last patch set, which includes:
 - Several fixes for crashes with WorkerCanvas and ImageBridgeChild shutdown
 - Support for WebGLContext.canvas returning WorkerCanvas
 - Removed lots of Main-thread-only stubs in WebGLContext
 - (Not in this interdiff, oops) move all WebGLContext pref access to gfxPrefs to support OMT access
 - (Not in this interdiff, oops) Add WorkerCanvasObserver (nsIThreadObserver) to manage committing invalidated canvas frames
 - Misc changes/fixes during rebase to m-c.

	Jon Morton [:jmorton]
	Comment 110 • 10 years ago

I have also been working to port the WebGL conformance suite to work in Web Workers, which I will be attaching soon. At this point, almost all of the tests pass, with these exceptions:
 - HTMLCanvasElement features not implemented in WorkerCanvas (toDataURL, toBlob, texImage2D)
 - Tests that depend on certain DOM manipulations
 - Extra tests in the more/ directory that depend on an entirely different testing library that I haven't ported

	Nicolas Silva [:nical]
	Comment 111 • 10 years ago

Comment on attachment 8468112 [details] [diff] [review]
0001-WorkerCanvas-WIP-Part-1-OMT-canvas-with-ImageBridge.patch

Review of attachment 8468112 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, I just want to understand the reason for the connect() call in the async case (see my comment below)

::: gfx/layers/client/CanvasClient.h
@@ +103,5 @@
> +  virtual void Update(gfx::IntSize aSize, ClientCanvasLayer* aLayer) MOZ_OVERRIDE
> +  {
> +  }
> +
> +  virtual void UpdateAsync(AsyncCanvasRenderer* aRenderer);

MOZ_OVERRIDE

@@ +138,4 @@
>  
>    virtual void Update(gfx::IntSize aSize, ClientCanvasLayer* aLayer) MOZ_OVERRIDE;
>  
> +  virtual void UpdateAsync(AsyncCanvasRenderer* aRenderer)

MOZ_OVERRIDE

::: gfx/layers/client/ClientCanvasLayer.cpp
@@ +64,2 @@
>      SurfaceCaps caps;
> +    

nit: trailing space

@@ +185,3 @@
>      }
>      if (HasShadow()) {
>        mCanvasClient->Connect();

Isn't this supposed to only happen only in the non-async case? There is no need to connect a CanvasClientBridge since the latter is a placeholder for the one that is connected through ImageBridge.

::: gfx/layers/moz.build
@@ +246,4 @@
>      'apz/testutil/APZTestData.cpp',
>      'apz/util/ActiveElementManager.cpp',
>      'apz/util/APZCCallbackHelper.cpp',
> +    'AsyncCanvasRenderer.cpp',    

nit: trailing spaces

	(no longer active)
	Comment 112 • 10 years ago

I'm still struggling to do these reviews today...

	(no longer active)
	Comment 113 • 10 years ago

Comment on attachment 8468113 [details] [diff] [review]
0002-WorkerCanvas-WIP-Part-2-Implementation.patch

Review of attachment 8468113 [details] [diff] [review]:
-----------------------------------------------------------------

I have quite a few comments below, but the biggest issue is that I'm not very familiar with the gfx side of things, and am not familiar with the processing model we're trying to implement here (I have basically been interpolating between roc's proposal and the whatwg spec and have been trying to read khuey's mind too).  So you won't find any useful comments on the overall approach, but that's because I mostly took things at face value on the approach.  Specifically, I didn't review the gfx related stuff closely at all.  This should definitely be reviewed by someone familiar with WebGL in addition to the usual DOM/worker related reviews.

::: content/html/content/public/HTMLCanvasElement.h
@@ +73,5 @@
>    void SetHeight(uint32_t aHeight, ErrorResult& aRv)
>    {
> +    if (!mWorkerCanvas) {
> +      SetUnsignedIntAttr(nsGkAtoms::height, aHeight, aRv);
> +    }

Doing nothing when we have a worker context here seems wrong.  Should we throw perhaps?

@@ +83,5 @@
>    void SetWidth(uint32_t aWidth, ErrorResult& aRv)
>    {
> +    if (!mWorkerCanvas) {
> +      SetUnsignedIntAttr(nsGkAtoms::width, aWidth, aRv);
> +    }

Here too.

@@ +115,5 @@
>    void SetMozOpaque(bool aValue, ErrorResult& aRv)
>    {
> +    if (!mWorkerCanvas) {
> +      SetHTMLBoolAttr(nsGkAtoms::moz_opaque, aValue, aRv);
> +    }

Here too.

::: content/html/content/src/HTMLCanvasElement.cpp
@@ +526,3 @@
>    }
> +  nsRefPtr<WorkerCanvas> workerCanvas = mWorkerCanvas;
> +  return workerCanvas.forget();

I think you need to throw InvalidStateError if transferControlToWorker has been previously called per spec.

@@ +779,5 @@
>  HTMLCanvasElement::GetCanvasLayer(nsDisplayListBuilder* aBuilder,
>                                    CanvasLayer *aOldLayer,
>                                    LayerManager *aManager)
>  {
> +  if (mCurrentContext) {

This and the next function needs to be reviewed by a gfx guy.

::: dom/canvas/CanvasRenderingContextHelper.cpp
@@ +149,5 @@
> +  rv = mCurrentContext->SetDimensions(sz.width, sz.height);
> +  if (NS_FAILED(rv)) {
> +    mCurrentContext = nullptr;
> +    mCurrentContextId.Truncate();
> +    return rv;

Nit: please remove this.

::: dom/canvas/WebGLContext.cpp
@@ +101,1 @@
>          return;

What makes these silent early returns OK?

@@ +291,5 @@
>      mPixelStorePackAlignment = 4;
>      mPixelStoreUnpackAlignment = 4;
>  
> +    if (NS_IsMainThread()) {
> +        // XXX jmorton: not thread safe

Please file a follow-up bug to get about:memory hooked up to WebGL on workers and CC :njn and :erahm on it, and include the bug# here.

@@ +447,5 @@
>  void
>  WebGLContext::Invalidate()
>  {
> +    if (mWorkerCanvas)
> +        mWorkerCanvas->Invalidate();

Shouldn't you do this after the mInvalidated check?

@@ +643,5 @@
> +    if (!gFeatureStatusReady) {
> +        MOZ_ASSERT(NS_IsMainThread(), "WebGLContext::UpdateFeatureStatus must be"
> +            "called from the main thread before creating an OMT webgl context!");
> +        UpdateFeatureStatus();
> +    }

Typically we'd have a function such as MaybeUpdateFeatureStatus() which does the gFeatureStatusReady check internally, and we just call it unconditionally.

@@ +777,5 @@
>      MOZ_ASSERT(kMaxWebGLContextsPerPrincipal < kMaxWebGLContexts);
>  
> +    if (!NS_IsMainThread()) {
> +        return;
> +    }

Does this not break losing the WebGL context on workers?

@@ +930,5 @@
>  void WebGLContext::UpdateLastUseIndex()
>  {
> +    if (!NS_IsMainThread()) {
> +        return;
> +    }

Ditto.

@@ +1053,5 @@
> +{
> +    if (mCanvasElement) {
> +        aRetVal.SetValue().SetAsHTMLCanvasElement() = mCanvasElement;
> +    }
> +    else if (mWorkerCanvas) {

Can this be false here?  If not, please convert this to an unconditional else and MOZ_ASSERT(mWorkerCanvas); inside it.

@@ +1350,5 @@
>      }
> +
> +    NS_IMETHOD Cancel() {
> +        return NS_OK;
> +    }

This too.

::: dom/canvas/WebGLContextExtensions.cpp
@@ +163,4 @@
>              break;
>      }
>  
> +    if (NS_IsMainThread() && (Preferences::GetBool("webgl.enable-draft-extensions", false) || IsWebGL2())) {

I don't understand why these are restricted to the main thread...

::: dom/canvas/WebGLContextGL.cpp
@@ +2067,5 @@
>      if (IsContextLost())
>          return;
>  
> +    if (mCanvasElement && mCanvasElement->IsWriteOnly() &&
> +                          !nsContentUtils::IsCallerChrome()) {

I think this breaks tainting the canvas, doesn't it?

::: dom/canvas/WebGLContextLossTimer.cpp
@@ +16,5 @@
> +// timer's EventTarget to use our own cancelable runnable
> +
> +class ContextLossWorkerEventTarget MOZ_FINAL : public nsIEventTarget {
> +public:
> +    ContextLossWorkerEventTarget(nsCOMPtr<nsIEventTarget> aEventTarget)

This can take an nsIEventTarget*.  Also, please make it explicit.

@@ +26,5 @@
> +    NS_DECL_NSIEVENTTARGET
> +    NS_DECL_THREADSAFE_ISUPPORTS
> +
> +protected:
> +    virtual ~ContextLossWorkerEventTarget() {}

The destructor doesn't need to be virtual.

@@ +34,5 @@
> +};
> +
> +class ContextLossWorkerRunnable MOZ_FINAL : public nsICancelableRunnable {
> +public:
> +    ContextLossWorkerRunnable(nsIRunnable* aRunnable)

Nit: explicit.

@@ +45,5 @@
> +
> +    NS_FORWARD_NSIRUNNABLE(mRunnable->)
> +
> +protected:
> +    virtual ~ContextLossWorkerRunnable() {}

Nit: make this non-virtual please.

::: dom/canvas/WorkerCanvas.cpp
@@ +57,5 @@
> +
> +WorkerCanvas*
> +WorkerCanvas::GetParentObject() const
> +{
> +  return nullptr;

Shouldn't this return the worker global or something?

@@ +127,5 @@
> +
> +  return rv;
> +}
> +
> +bool

The caller of this function drops the return value on the floor.  Please return void.

::: dom/canvas/WorkerCanvas.h
@@ +45,5 @@
> +  static already_AddRefed<WorkerCanvas>
> +  Constructor(const GlobalObject& aGlobal,
> +              uint32_t aWidth,
> +              uint32_t aHeight,
> +              ErrorResult& aRv);

The WorkerCanvas.webidl file doesn't have a [Constructor] so this can be removed, which means that we can simplify some code here (for example, we can enforce that mCanvasRenderer will never be null.)

Note that https://wiki.mozilla.org/User:Roc/WorkerCanvasProposal includes a [Constructor] but I don't understand what the user is supposed to do with a WorkerCanvas that they have constructed themselves.  I consider that a bug in roc's proposal.

@@ +81,5 @@
> +      CanvasAttrChanged();
> +    }
> +  }
> +
> +  nsICanvasRenderingContextInternal* GetContext()

Nit: const.

@@ +113,5 @@
> +  GetContextHelper(const nsAString& aContextId,
> +                   nsICanvasRenderingContextInternal** const outContext)
> +                   MOZ_OVERRIDE;
> +
> +  void SetDisabled()

I think "neutered" is a better name for these.

@@ +118,5 @@
> +  {
> +    mDisabled = true;
> +  }
> +
> +  bool IsDisabled()

Nit: const.

@@ +139,5 @@
> +  bool mOpaque;
> +  bool mAttrDirty;
> +  nsRefPtr<layers::AsyncCanvasRenderer> mCanvasRenderer;
> +  layers::CanvasClient* mCanvasClient;
> +  bool mDisabled;

Nit: please reorder the boolean members for better packing.

::: dom/webidl/WorkerCanvas.webidl
@@ +6,5 @@
> + * For more information on this interface, please see
> + * http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2013-October/041074.html
> + */
> +
> +[Exposed=(Window,Worker)]

[Pref="gfx.workercanvas.enabled"].

Also, please updte test_interfaces.html with this name + the pref for it as well.

::: dom/workers/WorkerCanvasObserver.h
@@ +22,5 @@
> + */
> +class WorkerCanvasObserver MOZ_FINAL : public nsIThreadObserver
> +{
> +public:
> +  NS_DECL_THREADSAFE_ISUPPORTS

Do you actually need thread-safe refcounting for this?  IINM nsIThreadObservers are not expected to be thread safe.

@@ +31,5 @@
> +  void
> +  InvalidateWorkerCanvas(WorkerCanvas* aWorkerCanvas);
> +
> +protected:
> +  virtual ~WorkerCanvasObserver();

Please make the dtor non-virtual.  (virtual dtors for final classes don't make any sense, and waste an entry in the vtable!)

::: dom/workers/WorkerPrivate.cpp
@@ +426,5 @@
> +    if (aTag == SCTAG_DOM_CANVAS) {
> +      WorkerCanvas* canvas = static_cast<WorkerCanvas*>(aData);
> +      nsRefPtr<WorkerCanvas> child = canvas->CloneForWorker();
> +      JS::Rooted<JSObject*> obj(aCx, child->WrapObject(aCx));
> +      if (obj && JS_WrapObject(aCx, &obj)) {

Someone like bz/smaug should review the wrapping and unwrapping here.

@@ +431,5 @@
> +        aReturnObject.set(obj);
> +        return true;
> +      }
> +    }
> +    return false;

I think you need to call Error(aCx, <errorcode-for-the-mainthread-case>) everywhere that structured cloning fails, otherwise the content won't get an exception.  (And you should test that these properly throw as well.)

@@ +441,5 @@
> +                void** aContent, uint64_t* aExtraData)
> +  {
> +    WorkerCanvas* canvas = nullptr;
> +    nsresult rv = UNWRAP_OBJECT(WorkerCanvas, aObj, canvas);
> +    if (NS_SUCCEEDED(rv) && canvas && !canvas->IsDisabled()) {

The spec seems to suggest that it should be possible to transfer the same canvas proxy twice.  I think that is a spec bug and what you're doing here is the right thing to do.

::: dom/workers/WorkerPrivate.h
@@ +745,4 @@
>    nsTArray<ParentType*> mChildWorkers;
>    nsTArray<WorkerFeature*> mFeatures;
>    nsTArray<nsAutoPtr<TimeoutInfo>> mTimeouts;
> +  RefPtr<WorkerCanvasObserver> mWorkerCanvasObserver;

Nit: please use nsRefPtr.

	(no longer active)
	Comment 114 • 10 years ago

Comment on attachment 8468114 [details] [diff] [review]
0003-WorkerCanvas-WIP-Part-3-Tests.patch

Review of attachment 8468114 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good overall.  But please file a follow-up to run the entire WebGL test suite in workers somehow if one is not already on file.  We need to test this much more rigorously before shipping this feature.

::: dom/canvas/test/test_workercanvas_disable.html
@@ +13,5 @@
> +
> +function runTest() {
> +
> +  var htmlCanvas = document.getElementById("c");
> +  var worker = new Worker("workercanvas_disable.js");

Nit: s/disable/neuter/ in this test (including the file names.)

@@ +38,5 @@
> +  /* check parent workercanvas is disabled after being transfered */
> +  workerCanvas.width = 128;
> +  is(workerCanvas.width, 64, "Can't change transfered worker canvas width");
> +  workerCanvas.height = 128;
> +  is(workerCanvas.height, 64, "Can't change transfered worker canvas height");

It's not clear what the spec says, but like I said before, I think instead of silently ignoring these setters, they should all throw.

::: dom/canvas/test/workercanvas.js
@@ +25,5 @@
> +        gl = canvas.getContext("webgl");
> +    } catch (e) {}
> +
> +    if (!gl) {
> +        todo("WebGL is unavailable");

How can this happen?

@@ +37,5 @@
> +
> +    var fragSrc = "precision mediump float; \
> +                   void main(void) { \
> +                     gl_FragColor = vec4(0.0, 1.0, 0.0, 1.0); \
> +                   }";

/me pretends he knows WebGL...

@@ +40,5 @@
> +                     gl_FragColor = vec4(0.0, 1.0, 0.0, 1.0); \
> +                   }";
> +
> +    // Returns a valid shader, or null on errors.
> +    var createShader = function(src, t) {

Please define functions like one normally would.

@@ +65,5 @@
> +            var str = "Shader program linking failed:";
> +            str += "\nShader program info log:\n" + gl.getProgramInfoLog(prog);
> +            str += "\n\nVert shader log:\n" + gl.getShaderInfoLog(vs);
> +            str += "\n\nFrag shader log:\n" + gl.getShaderInfoLog(fs);
> +            console.log(str);

Shouldn't this be a test failure instead of a console log which will be ignored?

@@ +171,5 @@
> +        }
> +
> +        var count = 0;
> +
> +        var iid = setInterval(function() {

Why the 10ms intervals?  If you just want to yield to the event loop, please use a 0 timeout.

@@ +212,5 @@
> +                    ok(true, "Worker is done");
> +                    finish();
> +                }, 10);
> +            }, 10);
> +        }, 10);

Ditto.

	Till Schneidereit [:till]
	Comment 115 • 10 years ago

Not relevant to shumway anymore

	Nicolas Silva [:nical]
	Comment 116 • 10 years ago

Comment on attachment 8468112 [details] [diff] [review]
0001-WorkerCanvas-WIP-Part-1-OMT-canvas-with-ImageBridge.patch

Review of attachment 8468112 [details] [diff] [review]:
-----------------------------------------------------------------

Marking r- because there are a few details to change and I want bugzilla to stop nagging me. This is not far from being landable though.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 117 • 9 years ago

Comment on attachment 8468113 [details] [diff] [review]
0002-WorkerCanvas-WIP-Part-2-Implementation.patch

These are pretty bitrotted at this point, unfortunately.  I think baku is going to try to dust them off.

	Andrea Marchesini [:baku]
	Comment 119 • 9 years ago

These 2 patches are the rebasing of the first 2 patches written by Jon Morton.
Often it was a rewrite because the patches do not apply anymore and gfx is changed a lot.

These patches don't work. One of the issues I have is with CopyableCanvasLayer::UpdateTarget.

A feedback about how to proceed would be great! Thanks.

	Jukka Jylänki
	Comment 120 • 9 years ago

Here's some thoughts from Emscripten perspective. In Emscripten, moving execution to workers is forced mainly by these:
   a. main thread can't run a blocking main loop, and it may not be possible to port the codebase to run an async main loop. (king.com as an example)
   b. synchronous file IO from an IndexedDB (or sync XHR for that matter) is not accessible in main thread, and it may not be possible to port the codebase to run async file IO (UE 4 as an example).
   c. SharedArrayBuffer won't most likely be accessible to main thread, but only to workers (any native applications that uses multiple threads).
   d. some native applications use GL in a multithreaded manner, e.g. have a dedicated GL rendering thread, and/or streaming GL resource loading thread.

To resolve all of these, the goal is to move execution of Emscripten compiled apps fully to web workers, which brings up the need to do WebGL in workers. To illustrate the use cases, here's two code patterns of what we are looking to port. I'll use the EGL API as an example, because it matches almost perfectly with the various existing code we are seeing in the wild.

Application A: fully single-threaded application:

int main()
{
	glContext = glCreateContext();
	initApplication();
	for(;;)
	{
		updateGameOneFrame();
		// render
		glClear(...);
		for(each object)
		{
			glBindBuffer(...);
			glVertexAttribPointer(...);
			glDrawArrays(...);
		}
		eglSwapBuffers(); // Post the rendered content to be visible on the screen: https://www.khronos.org/registry/egl/sdk/docs/man/html/eglSwapBuffers.xhtml
	}
}


Here the application runs its own main loop, and therefore it cannot communicate by receiving postMessage() events from its parent thread. This means that to be able to port these types of applications, we should be able to:
   1. create a GL context in a worker.
   2. be able to swap buffers in a worker.

For 1. we could fake the GL context creation and create it advance in the main thread, however that means we will have to guess the context parameters (msaa or not, stencil or not, depth buffer or not) on behalf of the application. That is probably not a huge deal, but a small difference to native nevertheless.

For 2. since the application is running its own main loop, it means it can't reuse a requestAnimationFrame or an anonymous function callback-based (comment 8) approach, but it should have the ability to synchronously call to present to signal the frame boundaries.

In general, Emscripten applications that run in a web worker will have their worker message queues always empty and they will never receive a postMessage() event (but they might rarely do a postMessage() themselves) since they are running their own main loops.

Another use case is when the GL rendering code is split across multiple threads:

Application B: multithreaded GL usage:

/* thread X, e.g. main thread */
void init()
{
   someGlobalVariableHoldingTheGLContext = glCreateContext();
   globalMutexForGLContext = new mutex;
}

/* thread Y, e.g. rendering thread */
void render()
{
   acquire(globalMutexForGLContext);
   eglMakeCurrent(..., someGlobalVariableHoldingTheGLContext); // Acquire the context for rendering in this thread: https://www.khronos.org/registry/egl/sdk/docs/man/html/eglMakeCurrent.xhtml
   glBindBuffer(...);
   glVertexAttribPointer(...);
   glDrawArrays(...);
   eglMakeCurrent(..., 0); // Not doing rendering any more, release context for this thread.
   release(globalMutexForGLContext);
}

/* thread Z, e.g. threaded resource creation */
for(;;)
{
	while(nothing to load) sleep();
	http_request(url_for_texture);
	acquire(globalMutexForGLContext);
	eglMakeCurrent(..., someGlobalVariableHoldingTheGLContext); // Acquire the context for creating resources.
	glGenTextures(...);
	glActiveTexture(...);
	glBindTexture(...);
	glTexImage2D(...);
	eglMakeCurrent(..., 0); // Finished creating resources, release context for this thread.
	release(globalMutexForGLContext);
}

This brings about a third obstacle:
   3. be able to access the GL context from multiple threads

For this, a web worker should be able to call a function to obtain access to the GL context. In native GL world, this only succeeds when the GL context is in an unbound state, i.e. eglMakeCurrent is not "stealing", but aborts with EGL_BAD_ACCESS if some other thread currently has the GL context active to it. All threads will cooperatively release their use of the context when they don't need it anymore.

How do these requirements 1.-3. look to you guys from the perspective of the current patches?

	Kenneth Russell
	Comment 121 • 9 years ago

May I comment on the feasibility of these requirements from the API standpoint rather than just Firefox's implementation and patches in progress?

There is a fundamental problem with the ability to access the GL context from multiple threads in the multi-worker SharedArrayBuffer model. Emscripten's OpenGL ES access is implemented in terms of WebGL, which is implemented with a bunch of JavaScript wrappers around the underlying OpenGL resources. These wrappers are instantiated in the JavaScript context of the thread which fetched the WebGL rendering context. Since all of the workers that have access to the SharedArrayBuffer actually have separate JavaScript execution contexts, none of them will be able to see those objects.

I gather that the people working on asm.js's threading model are discussing this actively, so hopefully progress will be made on this problem.

In the meantime, if we just consider the problem of a single thread in the worker group pushing OpenGL frames to the browser's compositor, https://wiki.whatwg.org/wiki/OffscreenCanvas is a proposal to enable that. Please comment on the ongoing discussion "Canvas image to blob/dataurl within Worker" on whatwg@whatwg.org.

	Luke Wagner [:luke]
	Comment 122 • 9 years ago

(In reply to Jukka Jylänki from comment #120)
Whoaa now, I don't think we need to be allowing rendering from-multiple threads for this feature to be massively useful.  Other than the issue of async resource creation (which it seems like we can optimize separately w/in current API of WebGL to a degree), aren't most C++ apps keeping all their GL activity on a single thread anyway?  Also, as Kenneth says, we have a much bigger issue with sharing Web API resources between threads, one that we'll likely need to address directly in the future --- but only  after spending some time to better understand the problem and collect use cases.  For now, it seems like we have a quite workable solution if we can only allow a single worker to access a given webgl canvas.

The only only tweak I was hoping to discuss on this bug (and if it's not crazy, take it to the broader WHATWG discussion) which prompted me to poke Jukka to comment is: rather than requiring workers to return to their event loop to render the next frame, could we provide a blocking (worker-only) API to present() so that the app can maintain its existing event-loop structure.  This has shown up repeatedly over the years as one of the big reasons apps have trouble porting with Emscripten (vim being another example (one near to my heart ;) having apparently many event loops scattered around).

	Jukka Jylänki
	Comment 123 • 9 years ago

(In reply to Kenneth Russell from comment #121)
> There is a fundamental problem with ...

I brought up the multithreaded context case since comments above were already discussing changes to how a context is activated in a worker. You're right that it alone won't enable multithreaded access to the GL context, but I did not want to start expanding the focus of this bug item, only comment to the existing proposed changes in terms of context activation.

(In reply to Luke Wagner [:luke] from comment #122)
> (In reply to Jukka Jylänki from comment #120)
> Whoaa now, I don't think we need to be allowing rendering from-multiple
> threads for this feature to be massively useful.

That's right. I was merely asking what how the current patches look in terms of items 1. - 3. . Indeed item 2. is the most important one, since the prospect of being able to have applications run their own blocking main loop would enable porting much more applications. Multithreaded GL context access is much more rare.

	Kenneth Russell
	Comment 124 • 9 years ago

(In reply to Luke Wagner [:luke] from comment #122)
> 
> The only only tweak I was hoping to discuss on this bug (and if it's not
> crazy, take it to the broader WHATWG discussion) which prompted me to poke
> Jukka to comment is: rather than requiring workers to return to their event
> loop to render the next frame, could we provide a blocking (worker-only) API
> to present() so that the app can maintain its existing event-loop structure.
> This has shown up repeatedly over the years as one of the big reasons apps
> have trouble porting with Emscripten (vim being another example (one near to
> my heart ;) having apparently many event loops scattered around).

Hopefully that need has been captured in one API proposal being discussed on the WHATWG mailing list: please see https://wiki.whatwg.org/wiki/OffscreenCanvas . Both the 2D and WebGL contexts will add a "commit" method which will allow explicit presentation of a frame into an HTMLCanvasElement, and that method can be used both from workers and from the main thread. However, your point is a good one that it needs to block so that if the worker doesn't return control to its event loop, that it doesn't run uncontrollably quickly. I can see that that behavior could be implemented as a heuristic inside the browser but should it be made explicit in the proposal? I'm guessing it should.

	Luke Wagner [:luke]
	Comment 125 • 9 years ago

(In reply to Kenneth Russell from comment #124)
Ah hah, I had been looking at https://wiki.whatwg.org/wiki/WorkerCanvas.

> I can see that that
> behavior could be implemented as a heuristic inside the browser but should
> it be made explicit in the proposal? I'm guessing it should.

Making it explicit would also address the limitation bullet: "A known good way to drive an animation loop from a worker is needed. requestAnimationFrame or a similar API needs to be defined on worker threads."

	Morris Tseng [:mtseng] [:Morris] (Inactive) Assignee
	Comment 126 • 9 years ago

I rebased those patches today and hope we can run it normally. But I failed :( I always hit this crash [1] when I transfer canvas to worker. Kyle, do you have any idea about this?

[1]: https://dxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.cpp#920

	Morris Tseng [:mtseng] [:Morris] (Inactive) Assignee
	Comment 127 • 9 years ago

BTW, here is callstack.

  * frame #0: 0x0000000104b1f595 XUL`(anonymous namespace)::Wrap(cx=0x0000000139aee340, existing=JS::HandleObject at 0x000000013dacb2a8, obj=JS::HandleObject at 0x000000013dacb2a0) + 117 at RuntimeService.cpp:920
    frame #1: 0x00000001076443af XUL`JSCompartment::wrap(this=0x000000013c2c4800, cx=0x0000000139aee340, obj=JS::MutableHandleObject at 0x000000013dacb5f0, existingArg=JS::HandleObject at 0x000000013dacb5e8) + 3119 at jscompartment.cpp:447
    frame #2: 0x0000000106fbee43 XUL`JSCompartment::wrap(this=0x000000013c2c4800, cx=0x0000000139aee340, vp=JS::MutableHandleValue at 0x000000013dacb7a0, existing=JS::HandleObject at 0x000000013dacb798) + 1011 at jscompartmentinlines.h:117
    frame #3: 0x0000000107644595 XUL`JS_WrapValue(cx=0x0000000139aee340, vp=JS::MutableHandleValue at 0x000000013dacb808) + 117 at jsapi.cpp:1014
    frame #4: 0x0000000103b75cfa XUL`bool mozilla::dom::binding_detail::DoGetOrCreateDOMReflector<mozilla::dom::WorkerCanvas, (mozilla::dom::binding_detail::GetOrCreateReflectorWrapBehavior)0>(cx=0x0000000139aee340, value=0x000000013c234dc0, rval=MutableHandle<JS::Value> at 0x000000013dacb8a0) + 666 at BindingUtils.h:978
    frame #5: 0x0000000103b7592d XUL`bool mozilla::dom::GetOrCreateDOMReflector<mozilla::dom::WorkerCanvas>(cx=0x0000000139aee340, value=0x000000013c234dc0, rval=MutableHandle<JS::Value> at 0x000000013dacb8d8) + 45 at BindingUtils.h:997
    frame #6: 0x0000000104b80b6a XUL`(anonymous namespace)::WorkerStructuredCloneCallbacks::ReadTransfer(aCx=0x0000000139aee340, aReader=0x000000013dacbd10, aTag=4294934544, aData=0x000000013c234dc0, aExtraData=0, aClosure=0x0000000000000000, aReturnObject=MutableHandle<JSObject *> at 0x000000013dacb980) + 170 at WorkerPrivate.cpp:699
    frame #7: 0x0000000107148c05 XUL`JSStructuredCloneReader::readTransferMap(this=0x000000013dacbd10) + 1285 at StructuredClone.cpp:1783
    frame #8: 0x0000000107141a06 XUL`JSStructuredCloneReader::read(this=0x000000013dacbd10, vp=JS::MutableHandleValue at 0x000000013dacbc90) + 38 at StructuredClone.cpp:1818
    frame #9: 0x0000000107141949 XUL`ReadStructuredClone(cx=0x0000000139aee340, data=0x00000001399ecd00, nbytes=112, vp=JS::MutableHandleValue at 0x000000013dacbd08, cb=0x000000010a5bf030, cbClosure=0x0000000000000000) + 169 at StructuredClone.cpp:377
    frame #10: 0x0000000107149091 XUL`JS_ReadStructuredClone(cx=0x0000000139aee340, buf=0x00000001399ecd00, nbytes=112, version=5, vp=JS::MutableHandleValue at 0x000000013dacbec0, optionalCallbacks=0x000000010a5bf030, closure=0x0000000000000000) + 209 at StructuredClone.cpp:1895
    frame #11: 0x00000001071496cc XUL`JSAutoStructuredCloneBuffer::read(this=0x00000001399ecdb0, cx=0x0000000139aee340, vp=JS::MutableHandleValue at 0x000000013dacbf28, optionalCallbacks=0x000000010a5bf030, closure=0x0000000000000000) + 268 at StructuredClone.cpp:2053
    frame #12: 0x0000000104b85001 XUL`(anonymous namespace)::MessageEventRunnable::DispatchDOMEvent(this=0x00000001399ecd80, aCx=0x0000000139aee340, aWorkerPrivate=0x0000000136aaa800, aTarget=0x0000000130579a00, aIsMainThread=false) + 225 at WorkerPrivate.cpp:1257
    frame #13: 0x0000000104b84f00 XUL`(anonymous namespace)::MessageEventRunnable::WorkerRun(this=0x00000001399ecd80, aCx=0x0000000139aee340, aWorkerPrivate=0x0000000136aaa800) + 768 at WorkerPrivate.cpp:1336
    frame #14: 0x0000000104b66069 XUL`mozilla::dom::workers::WorkerRunnable::Run(this=0x00000001399ecd80) + 1849 at WorkerRunnable.cpp:356
    frame #15: 0x0000000101708a2a XUL`nsThread::ProcessNextEvent(this=0x0000000130882960, aMayWait=false, aResult=0x000000013dacc5fe) + 2042 at nsThread.cpp:868
    frame #16: 0x0000000101763ec7 XUL`NS_ProcessNextEvent(aThread=0x0000000130882960, aMayWait=false) + 151 at nsThreadUtils.cpp:265
    frame #17: 0x0000000104b591b1 XUL`mozilla::dom::workers::WorkerPrivate::DoRunLoop(this=0x0000000136aaa800, aCx=0x0000000139aee340) + 2161 at WorkerPrivate.cpp:5271

	Kelsey Gilbert [:jgilbert]
	Comment 128 • 9 years ago

We're retargeting this bug for just the commit() component of WebGL+Workers.
There will be a follow-up bug for the explicit buffer management part.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 129 • 9 years ago

(In reply to Morris Tseng [:mtseng] from comment #126)
> I rebased those patches today and hope we can run it normally. But I failed
> :( I always hit this crash [1] when I transfer canvas to worker. Kyle, do
> you have any idea about this?
> 
> [1]:
> https://dxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.
> cpp#920

Not offhand.  Do you see this with Andrea's patches?  Or something else?

	Morris Tseng [:mtseng] [:Morris] (Inactive) Assignee
	Comment 130 • 9 years ago

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #129)
> Not offhand.  Do you see this with Andrea's patches?  Or something else?
I compare with both Andrea and Jon's patches. But there is no big difference in the worker stuff. The MOZ_CRASH code added in bug 1092102 which is related to worker debug things. So I guess bug 1092102 is assume Wrap function is only called from debugger compartment to debuggee compartment or vice versa. But in the worker canvas case, we actually want to wrap a WorkerCanvas object from main thread to worker thread. So it hits this MOZ_CRASH. I try to ignore the MOZ_CRASH for testing, but it will crash elsewhere. I'll keep digging into this!

	Morris Tseng [:mtseng] [:Morris] (Inactive) Assignee
	Comment 131 • 9 years ago

Ah, I see there is tiny difference causes this issue. Andrea's version shares the same WorkerCanvas on main thread and worker but Jon's version uses cloned WorkerCanvas on worker. So, it would crash if we share the same WorkerCanvas. With this modification, WorkerCanvas is able to transfer to worker without error. Next, I'll fix graphics related code to make it works again!

	Andrea Marchesini [:baku]
	Comment 132 • 9 years ago

My patches are not ready at all. I just tried to rebase the existing code but I think I'll go for a full rewrite.

	Morris Tseng [:mtseng] [:Morris] (Inactive) Assignee
	Comment 133 • 9 years ago

(In reply to Andrea Marchesini (:baku) from comment #132)
> My patches are not ready at all. I just tried to rebase the existing code
> but I think I'll go for a full rewrite.
I have a workable patches now. I'll upload it asap.

	Morris Tseng [:mtseng] [:Morris] (Inactive) Assignee
	Comment 136 • 9 years ago

Those patches are rebased to latest m-c and can run the demo. I also addressed most reviewer's comment above. But in this patch, the result image would be upside down. I'll fix this problem.