Devon Govett on twitter: """ that's true re canvas, but the ability to create off screen image buffers like canvas in workers could be nice not only to get its data, to actually do rendering on a separate thread using the canvas API then you could generate images in separate threads and transfer them back to the main thread to draw """ https://twitter.com/devongovett/status/256529418267283456 https://twitter.com/devongovett/status/256531007082221568 https://twitter.com/devongovett/status/256529643283308544 See related bug 709490 which is about WebGL from workers. Word from Andreas and JeffM is fonts/text will hurt, but there may be a way forward. Andreas channels Ken Thompson: "When in doubt, use brute force". Cc'ing jfkthame for his thoughts. Paul Bakaus (https://twitter.com/pbakaus) is quite keen to see this. Please cc:, help fix the summary, etc. as needed. Also, someone take this bug and hack on it! /be

This is highly relevant to the pdf.js interests.

I love this idea but please don't let this extra API surface in any way bog down the workers. Part of why they're so great is they're so lightweight and quick. The downside of this approach is that it could create an even more slippery slope of stuff that people would like to have inside a worker, making it more and more like a regular `window` frame instance. XHR, sockets, history, appcache... geo, requestAnimationFrame, getUserMedia, Social API, etc... all these and tons more (all device APIs, etc) are ones people can make arguments for being accessible from the worker as well, for the same reasons (doing background processing). Some already are there obviously (XHR, sockets, etc), but it could be a sliding slope. I'm not talking down this idea, sounds like it'll be quite helpful. But please do give some thought to a coherent way this line gets drawn so we don't see an arms race between browsers of trying to stuff more and more stuff into workers. :)

What I proposed in bug 709490 is to provide a way to pass the context to the worker where only the context would be accessible rather then the canvas element. The canvas would also become inaccessible from the main thread to prevent headaches. It shouldn't make thing much worse. I see an extension like this being critical if we plan on supporting bigger projects like games and pdf.js while providing great user experience. Are there any difference to what is being suggested here compared to bug 709490?

(In reply to Benoit Girard (:BenWa) from comment #3) > What I proposed in bug 709490 is to provide a way to pass the context to the > worker where only the context would be accessible rather then the canvas > element. The canvas would also become inaccessible from the main thread to > prevent headaches. if you pass the reference of the context from the window to the worker, and that parent canvas object is still attached to the DOM, does it stay attached to the DOM and visible, but just untouchable? Then, if you pass back a context reference from worker to window, does it automatically re-embed itself in the canvas in the DOM and then be accessible?

We could do a more explicit API. other = context.suspend() => object "other" that can be mutated instead, the original context throws when touched context.resume() => all other references becomes dead, context becomes live again Names are for illustration purposes only.

Perhaps rather than transferring the whole canvas or the context around, there would be some API to create the headless canvas in the worker itself. Then, if you wanted to render what was drawn in the worker canvas to the screen, you would getImageData and transfer the pixel data back to the main thread to be drawn with putImageData. Otherwise how would you create a new canvas in the worker? Would you have to create it in the main thread with document.createElement and then transfer it to the worker? That seems a bit silly. I think passing image data around is much simpler than trying to transfer the entire canvas. Thoughts on that?

Maybe a bit off the deep end, but does mediastream/webRTC provide what we need to create some concept of a "canvas stream"? We could use an effects API to set up a renderer worker, and then include the resulting stream in the page through a specially-constructed URI on a <video>. When the worker pushed a new frame, it would automatically trigger a recomposition (wherever that might be happening). If the main DOM wanted to shut that off, it could stop() the video. If the main DOM wanted a snapshot of the worker's output, it could drawImage() the <video> onto a main-thread canvas.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #7) +1 to this idea. more flexible in that it solves both single frame updates (for canvas use-case) and live-stream updates. I assume then that we could do the same for audio, right? But, the same concern would exist, that we'd need to actually have these creation APIs (canvas, WebAudio, etc) available *in the worker* to create the content that is then pumped down the stream. But I do like that we could avoid the ugliness of having to encode data into ArrayBuffer so that as a TransferableObject it can be moved without copy, etc. This makes the data transfer part of this request really clean, IMHO.

(In reply to Devon Govett from comment #6) > there would be some API to create the headless canvas in the worker itself. We specifically want a canvas that is presenting directly to the screen from the worker. The DOM thread has too many events that take longer then 15ms making it impossible to implement a canvas application that can paint at 60 FPS smoothly i.e. every 16ms without missing a beat. Try the 'Gecko Profiler' extension to see just how much contention that main thread has.

(In reply to Kyle Simpson from comment #8) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #7) > But, the same concern would exist, that we'd need to actually have these > creation APIs (canvas, WebAudio, etc) available *in the worker* to create > the content that is then pumped down the stream. > I was thinking that the worker would do something like requestAnimationFrame(), and a callback / event would hand it a pre-constructed context.

(However that kind of style would fit into mediastreams, which I don't know well.)

(Note: I'm not really a platform person. I don't have a big picture about thebes, fonts and thread safety. Maybe the following is totally wrong, but I hope they get the discussion in the right direction.) - Rendering to a canvas' context is done using the Thebes library, which in most cases uses Cairo (correct me if I'm wrong). Worker run in their own thread. Is our current thebes library thread save? Can we access a thebes object on the main thread from a worker thread without issues? - There was some discusson on the WHATWG about this: "[whatwg] Offscreen canvas (or canvas for web workers)" [1] - For PDF.JS purpose, we need a way to load fonts and use them in the worker. Roc poined out in a different thead on the WHATWG [2], that this is complicated to do: Quoting roc: > Drawing text on a worker thread means making CSS font parsing, associated > font objects, text shaping, and bidi processing all usable from multiple > threads. That would be a much bigger problem for us [Gecko] than the actual > drawing... - I don't think the problem here is to come up with some API to glue the WorkerCanvas and the main thread together. The main problem is the "technical" - how to make bidi etc. thread save like roc pointed out. Maybe the solution is to spawn a new process to get around the thread-save problem. --- For the API discussion, I see three main approaches for canvas worker support: a) Create the canvas on the main thread and send a reference of the context to the worker. The worker then operates on the context. As it's the same context as on the main thread, changes will show up automatically in the browser. b) The worker creates a WorkerCanvas, gets a context from it and operate on this context. Once something should be rendered, the workerCanvas "postMessage" the imageData of the context to the main thread, which then uses `putImageData` to draw the worker's imageData to a canvas on the page. c) Inside the worker, a WorkerCanvas is created. Some reference (blobURL?) of this canvas is send to the main thread. On the main thread, the `src` of an image is set to the reference. Whenever the image needs a repaint, the current content of the WorkerCanvas is copied from the Worker and used. (I think this is somewhat what Chris was up to with the mediastream/webRTC stuff). Personally, I don't like a) as the worker and main thread should be disconnected. c) can be seen as a future iteration of b). Julian --- [1]: http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2010-February/025254.html [2]: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2009-December/024478.html

(In reply to Chris Jones [:cjones] [:warhammer] from comment #7) > Maybe a bit off the deep end, but does mediastream/webRTC provide what we > need to create some concept of a "canvas stream"? We could use an effects > API to set up a renderer worker, and then include the resulting stream in > the page through a specially-constructed URI on a <video>. We could do that. I don't think it would be the most natural API though for stuff like games that just want to draw to the screen. For games and other real-time rendering applications, you you want to tie invocations of the worker to the screen refresh rate using a mechanism like requestAnimationFrame, and that might not fit well with a MediaStream-based API. We also have the issue that the queue of Image objects managed by MediaStreams isn't necessarily optimal for canvas. I think a protocol that hands ownership of a <canvas> to a worker is probably the way to go. Ideally the same API could be used for both 2D and WebGL contexts. (Having said that, I do want to eventually support using WebGL canvas in workers to process the video frames of MediaStreams, for things like applying special effects to incoming video streams before sending them over the wire.)

(In reply to Julian Viereck from comment #12) > - Rendering to a canvas' context is done using the Thebes library, which in > most cases uses Cairo (correct me if I'm wrong). Worker run in their own > thread. Is our current thebes library thread save? Can we access a thebes > object on the main thread from a worker thread without issues? I'm not sure if we build cairo to be thread-safe currently. I think it would be easy to do so, and we're reducing our use of cairo so the cost of doing is declining. (Canvas uses Azure these days, which only sometimes uses cairo.) > Quoting roc: > > Drawing text on a worker thread means making CSS font parsing, associated > > font objects, text shaping, and bidi processing all usable from multiple > > threads. That would be a much bigger problem for us [Gecko] than the actual > > drawing... This is still a problem. It's one reason why making WebGL accessible from a worker is currently more attractive than 2D canvas. > a) Create the canvas on the main thread and send a reference of the context > to the worker. The worker then operates on the context. As it's the same > context as on the main thread, changes will show up automatically in the > browser. For games, it really has to be something like this; see comment #9. There may be use-cases where creating a canvas on a worker and postMessaging results to the main thread are useful. They may need slightly different API.

I made a proposal for this earlier today in specese, hope it helps. Details here: http://lists.w3.org/Archives/Public/public-whatwg-archive/2012Nov/0199.html (I haven't made any special dispensations for text rendering in workers; if that's hard, we can always disable it from workers. Let me know how I can help with that, either on IRC or on the whatwg list. Thanks.)

I read over the proposal and discussed it a bit. It seems very interesting and at a quick glance covers the needs of bug 709490 which will be important for games to run at a smooth 60 FPS. I'll post a response next week.

Awesome Hixie, this is looking promising. I will chime in on the mailing list.

Hixie: Great to see that there's some traction on getting an API for off screen graphics. This will come in handy outside web workers as well. It would be great if this played along well with webRTC and the LocalMediaStream as well. It would be great to be able to open a media stream, create a new CanvasRenderingContext2d() and render directly to that from the stream without having to add unnecessary DOM elements. Right now you have to add a hidden video and canvas elements to the DOM, just to have an image source and a context to render to.

Needed for competitive parity and major content use cases like games and pdf.js.

Needed for Snappy/Performance work such as OMT thumbnails.

This appears to be fully specced now, though not finalised. Constructing with `new CanvasRenderingContext2D()` is possible in window and worker[1] environments, which can bind to a canvas or CanvasProxy[2], which implements Transferable. [1]: http://www.w3.org/html/wg/drafts/2dcontext/master/#canvasrenderingcontext2d [2]: https://html.spec.whatwg.org/multipage/scripting.html#canvasproxy

We're going to need this for the "next generation architecture" of FirefoxOS (see bug 1068062)

(In reply to David Flanagan [:djf] from comment #23) > We're going to need this for the "next generation architecture" of FirefoxOS > (see bug 1068062) That doesn't seem like the right bug number, what's the correct one?

Thanks. I meant bug 1168062. https://bugzilla.mozilla.org/show_bug.cgi?id=nga

This is a *ton* of work ... I hope you're talking to people about scheduling already ...

(In reply to David Flanagan [:djf] from comment #25) > Thanks. I meant bug 1168062. https://bugzilla.mozilla.org/show_bug.cgi?id=nga Would you mind expanding a bit more on the why you think this feature is related or point to sources explaining it? The doc linked from bug 1168062 which is this: https://wiki.mozilla.org/Gaia/Architecture_Proposal does not have "canvas" in there as all (at least Firefox search doesn't highlight anything). --- The reason why I am asking: I am in the process to write a proposal to shift the way we do App Development and thereby get better performance, easier to build apps, better accesibility and repsonsive design out of the box for free if the plan succeeds. The codename is Web* and I talked to Daniel Holbert, Alon Zakai, Bill Walker and a few others about it. Given there positive feedback, I intend to get a draft of the proposal out by Saturday. Given that Kyle things getting this done takes "a *ton* of work" - maybe it is worth considering alternative options like "Web*"?

Hi Roc, Recently I'd take time to look into this topic. Currently I have done with some WIPs in my local includes 1. Webidl for Canvas 2d worker. 2. Operating JS codes below and drawing to screen by passing image throuhg ImageBridge. ctxworker.beginPath(); ctxworker.arc(x_icon,y_icon,50,0,2*Math.PI); ctxworker.closePath(); ctxworker.fillStyle = '#8ED6FF'; ctxworker.fill(); ctxworker.stroke(); ctxworker.commit(); When I want to implement font functions in worker, it comes a big problems here. 1. In SetFont(), Style operations are under PresShell and mCanvasElement. Apparently we can't run it on worker thread 2. In FillText for drawing text, 2.1 It ensures user font set is up to date. In this part, Style operations is still needed. We can't run it on worker thread as well. 2.2 Operate BidiProcessor to measure and draw text. The operatioins of presShell seems not so much. Based on this, it might be possible doing some modifications and let BidiProcessor can run worker thread. For the style operations like SetFont() in [1], my idea is 3. If JS code runs in main thread, directly calling [2]. 4. If JS code runs in worker thread, implementating a runnable and posting [2] back to main thread to operate. [1] : https://dxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp#3110 [2] : https://dxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp#3113 I might not consider too much about any side effect for this idea. Could you have any suggestions for it? Any comment is appreciate.

Our text shaping code can only run on the main thread, so without a lot of work FillText will depend on the main thread. I think for now the best thing to do would be to simply not expose the text APIs at the WebIDL level in Workers. I'd rather not expose them at all than to expose them but have them be very slow. At least then JS can feature-detect that they're not supported. Applications that don't use much text can work around the issue themselves by drawing text into temporary canvases on the main thread and transferring those images to the worker. To make progress beyond that we'd have to start by making gfxTextRun construction work on non-main threads. That's already very hard because you probably need to make gfxFont/gfxFontGroup work on non-main threads at the same time, and there are platform dependencies, global caches that need to be shared (or made per-thread)... Drawing SVG glyphs off the main thread can't really be done at all :-(. This is hard.

I think we should buy some time on the text rendering issue by explicitly removing the requirement to render text in the OffscreenCanvas feature proposal, and leaving the door open for a future proposal to add text capability, which may involve different APIs than what we use in the main thread context. Also, there are a number of other APIs in CRC2D that have no business in Workers. Basically anything that interacts with the View or DOM: DrawFocusRingIfNeeded, ScrollPathIntoView, HitRegions. The Custom Paint proposal (part of Houdini) has similar issues, except that they need text. I wonder if we should re-organize the IDLs to split the 2D canvas interface into component interfaces (ImageData, Image drawing, Path rendering, text, drawing state, etc.) Then we could have something like OffscreenCanvasRenderingContext2D, that only implements a select group of these 2D interfaces, and Houdini could do something similar.

That sounds good.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29) > Our text shaping code can only run on the main thread, so without a lot of > work FillText will depend on the main thread. I think for now the best thing > to do would be to simply not expose the text APIs at the WebIDL level in > Workers. I'd rather not expose them at all than to expose them but have them > be very slow. At least then JS can feature-detect that they're not > supported. Applications that don't use much text can work around the issue > themselves by drawing text into temporary canvases on the main thread and > transferring those images to the worker. > > To make progress beyond that we'd have to start by making gfxTextRun > construction work on non-main threads. That's already very hard because you > probably need to make gfxFont/gfxFontGroup work on non-main threads at the > same time, and there are platform dependencies, global caches that need to > be shared (or made per-thread)... Drawing SVG glyphs off the main thread > can't really be done at all :-(. This is hard. Thanks for the great suggestion. I will than do the following implementations toward this way.

Hi Morris, The attached patch intends to modify webidl for Canvas 2d on worker. The modifications kept some ctx operations like text, filter, ... that interacts with the View or DOM only on main thread. Others can works on both main/worker thread. Could you please have feedback for it? Thanks.

Comment on attachment 8673447 [details] [diff] [review] part-1-Modify-webidl-for-2d-canvas.patch Review of attachment 8673447 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/CanvasRenderingContext2D.webidl @@ +41,4 @@ > > + // For OffscreenCanvas > + // Reference: https://wiki.whatwg.org/wiki/OffscreenCanvas > + void commit(); Prefer move this function to separate partial interface since this is not in standard spec. And this method should also behind the pref. See https://dxr.mozilla.org/mozilla-central/source/dom/webidl/WebGLRenderingContext.webidl?case=true&from=WebGLRenderingContext.webidl#794 @@ +114,5 @@ > +partial interface CanvasRenderingContext2D { > + > + // state > + void save(); // push state on state stack > + void restore(); // pop state stack and restore state Does it mean we cannot save and restore state on worker? I think we should able to save and restore sate on worker. @@ +119,5 @@ > + > + [NewObject, Throws] > + CanvasPattern createPattern(CanvasImageSource image, [TreatNullAs=EmptyString] DOMString repetition); > + > + [SetterThrows, Func="mozilla::dom::CanvasRenderingContext2D::PrefCanvasFiltersEnabled"] Since this interface only expose to main-thread. You can use "Pref="canvas.filters.enabled" instead of Func. Am I right? @@ +122,5 @@ > + > + [SetterThrows, Func="mozilla::dom::CanvasRenderingContext2D::PrefCanvasFiltersEnabled"] > + attribute DOMString filter; // (default empty string = no filter) > + > + [Throws, Func="mozilla::dom::CanvasRenderingContext2D::PrefCanvasFocusringEnabled"] void drawFocusIfNeeded(Element element); ditto @@ +124,5 @@ > + attribute DOMString filter; // (default empty string = no filter) > + > + [Throws, Func="mozilla::dom::CanvasRenderingContext2D::PrefCanvasFocusringEnabled"] void drawFocusIfNeeded(Element element); > +// NOT IMPLEMENTED void drawSystemFocusRing(Path path, HTMLElement element); > + [Func="mozilla::dom::CanvasRenderingContext2D::PrefCanvasCustomfocusringEnabled"] boolean drawCustomFocusRing(Element element); ditto @@ +148,5 @@ > [Throws, LenientFloat] > void drawImage(CanvasImageSource image, double sx, double sy, double sw, double sh, double dx, double dy, double dw, double dh); > > // hit regions > + [Throws, Func="mozilla::dom::CanvasRenderingContext2D::PrefCanvasHitregionsEnabled"] void addHitRegion(optional HitRegionOptions options); ditto @@ +149,5 @@ > void drawImage(CanvasImageSource image, double sx, double sy, double sw, double sh, double dx, double dy, double dw, double dh); > > // hit regions > + [Throws, Func="mozilla::dom::CanvasRenderingContext2D::PrefCanvasHitregionsEnabled"] void addHitRegion(optional HitRegionOptions options); > + [Func="mozilla::dom::CanvasRenderingContext2D::PrefCanvasHitregionsEnabled"] void removeHitRegion(DOMString id); ditto @@ +150,5 @@ > > // hit regions > + [Throws, Func="mozilla::dom::CanvasRenderingContext2D::PrefCanvasHitregionsEnabled"] void addHitRegion(optional HitRegionOptions options); > + [Func="mozilla::dom::CanvasRenderingContext2D::PrefCanvasHitregionsEnabled"] void removeHitRegion(DOMString id); > + [Func="mozilla::dom::CanvasRenderingContext2D::PrefCanvasHitregionsEnabled"] void clearHitRegions(); ditto @@ +158,5 @@ > ImageData createImageData(double sw, double sh); > [NewObject, Throws] > ImageData createImageData(ImageData imagedata); > [NewObject, Throws] > ImageData getImageData(double sx, double sy, double sw, double sh); I think we can manipulate ImageData on worker. So I think this cannot restrict to main thread. BTW, I don't think we divide whole interface to worker-part and non worker part is a good solution. As comment 30 says, we should divide interface to multiple components and decide each components are exposed to worker or not. That's would be more organized compared to divide into 2 parts. @@ +269,3 @@ > interface CanvasDrawingStyles { > + > +}; We should move line cap/joins property to this common interface and only expose text settings to main-thread only. @@ +327,5 @@ > // addColorStop should take a double > void addColorStop(float offset, DOMString color); > }; > > interface CanvasPattern { Those interfaces such as CanvasPattern and Path2D should be exposed to worker, too. ::: dom/webidl/HTMLCanvasElement.webidl @@ +62,5 @@ > // To be called when rendering to the context is done. > void done(); > }; > > +typedef (HTMLCanvasElement or OffscreenCanvas) CanvasObj; You should modify WebGLRenderingContext.webidl to use this typedef as well.

Hi Morris, Thanks for your all suggestions in Comment 34. Just like you said in Comment, save/restore and createImageData/getImageData can run in Worker. Also, line cap/joins are all the same. The attached file is the modified WIP based on Comment 34, Please also give a feedback for me. Thanks

Comment on attachment 8674759 [details] [diff] [review] part1-v2-Modify-webidl-for-2d-canvas.patch Review of attachment 8674759 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +3409,5 @@ > > +/* static */ bool > +CanvasRenderingContext2D::PrefCanvasPathEnabled(JSContext* aCx, JSObject* aObj) > +{ > + return gfxPrefs::CanvasPathEnabled(); We cannot rely on gfxPrefs here because the initialization of gfxPrefs is not early enough. Check the OffscreenCanvas [1] to use another mechanism to get prefs. [1]: https://dxr.mozilla.org/mozilla-central/source/dom/canvas/OffscreenCanvas.cpp#213 ::: dom/canvas/CanvasRenderingContext2D.h @@ +533,5 @@ > > // return true and fills in the bound rect if element has a hit region. > bool GetHitRegionRect(Element* aElement, nsRect& aRect) override; > > + static bool PrefCanvasPathEnabled(JSContext* aCx, JSObject* aObj); tailing space. ::: dom/webidl/CanvasRenderingContext2D.webidl @@ +40,4 @@ > > + // For OffscreenCanvas > + // Reference: https://wiki.whatwg.org/wiki/OffscreenCanvas > + void commit(); Move this part to another partial interface. @@ +154,5 @@ > +[Exposed=Window] > +partial interface CanvasRenderingContext2D { > + [NewObject, Throws] > + // Methods to creates a pattern using the specified image (a CanvasImageSource). > + CanvasPattern createPattern(CanvasImageSource image, [TreatNullAs=EmptyString] DOMString repetition); createPattern should expose to worker. @@ +195,5 @@ > + void drawImage(CanvasImageSource image, double dx, double dy); > + [Throws, LenientFloat] > + void drawImage(CanvasImageSource image, double dx, double dy, double dw, double dh); > + [Throws, LenientFloat] > + void drawImage(CanvasImageSource image, double sx, double sy, double sw, double sh, double dx, double dy, double dw, double dh); We definitely able to drawImage on worker. Although most CanvasImageSource cannot pass to worker, but ImageBitmap could pass to worker. So we should support drawImage on worker.

Hi Morris, The WIPv3 was attached for your suggestions. Could you please also have a feedback for me? Thanks

Comment on attachment 8676108 [details] [diff] [review] part1-v3-Modify-webidl-for-2d-canvas.patch Review of attachment 8676108 [details] [diff] [review]: ----------------------------------------------------------------- f+ with nit fixed. ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +3416,5 @@ > + } else { > + workers::WorkerPrivate* workerPrivate = workers::GetWorkerPrivateFromContext(aCx); > + MOZ_ASSERT(workerPrivate); > + return workerPrivate->CanvasPathEnabled(); > + } nit: indent @@ +3426,5 @@ > + if (NS_IsMainThread()) { > + return true; > + } > + > + return PrefEnabled(aCx, aObj); nit: indent. Since you only use this function in this patch. You can merge PrefEnabled and PrefCanvasPathEnabled to a single function.

(In reply to Morris Tseng [:mtseng] from comment #38) > Comment on attachment 8676108 [details] [diff] [review] > part1-v3-Modify-webidl-for-2d-canvas.patch > > Review of attachment 8676108 [details] [diff] [review]: > ----------------------------------------------------------------- > > f+ with nit fixed. > > ::: dom/canvas/CanvasRenderingContext2D.cpp > @@ +3416,5 @@ > > + } else { > > + workers::WorkerPrivate* workerPrivate = workers::GetWorkerPrivateFromContext(aCx); > > + MOZ_ASSERT(workerPrivate); > > + return workerPrivate->CanvasPathEnabled(); > > + } > > nit: indent > > @@ +3426,5 @@ > > + if (NS_IsMainThread()) { > > + return true; > > + } > > + > > + return PrefEnabled(aCx, aObj); > > nit: indent. > > Since you only use this function in this patch. You can merge PrefEnabled > and PrefCanvasPathEnabled to a single function. Thanks for the feedback. The modification for the nit would be +/* static */ bool +CanvasRenderingContext2D::PrefCanvasPathEnabled(JSContext* aCx, JSObject* aObj) +{ + if (NS_IsMainThread()) { + return Preferences::GetBool("canvas.path.enabled"); + } else { + workers::WorkerPrivate* workerPrivate = workers::GetWorkerPrivateFromContext(aCx); + MOZ_ASSERT(workerPrivate); + return workerPrivate->CanvasPathEnabled(); + } +}

Hi smaug, The attached patch is my webidl modifications for 2d canvas on worker thread. The basic ideas are 1. Add the method commit() to send frame to Compositer. 2. As suggestions in Comment 30, separating interfaces of 2d canvas into different groups. Could you please have a review for me? Thanks

So there are lots of webidl changes. Since CanvasRenderingContext2D is exposed to window and workers, could you then just add [Exposed=Window] to those methods and properties which are window-only, and not split the interface to so many pieces? And only non-standard (they will be in some spec, right) should go to their own partial interface.

Comment on attachment 8676663 [details] [diff] [review] part1-v4-Modify-webidl-for-2d-canvas.patch In other words, keep the changes at minimum and CanvasRenderingContext2D should look as much the same as in the WhatWG HTML spec as possible.

comment 30 and splitting interface to smaller pieces is more like a spec issue. Let's not do that before the spec has changed. And it wouldn't mean splitting interface to smaller partial interfaces, but to create new interfaces which CanvasRenderingContext2D then 'implements'. And now I see commit() is in the spec, so no need for partial for it.

(In reply to Olli Pettay [:smaug] from comment #41) > So there are lots of webidl changes. Since CanvasRenderingContext2D is > exposed to window and workers, could you then just add [Exposed=Window] to > those methods and properties which are window-only, and not split the > interface to so many pieces? > > And only non-standard (they will be in some spec, right) should go to their > own partial interface. Hi :smaug, Thanks for your suggestions. Based on your comment, the idea is exposing [Exposed=(Window,Worker)] for interface CanvasRenderingContext2D. For the methods which only exposing to main-thread, there were many trials as below to filter out to expose to Worker. Taking the method filter() as example. [Pref="canvas.filters.enabled", SetterThrows] attribute DOMString filter; // (default empty string = no filter) At first, since the attribute [Pref=] couldn't be used once the interface exposing to Worker, Changing [Pref=] to [Func=]. Pref="canvas.filters.enabled" to Func="CanvasRenderingContext2D::PrefCanvasFiltersEnabled" 1. Tried adding [Exposed=Window] prefix to filter(). [Exposed=Window, Func="CanvasRenderingContext2D::PrefCanvasFiltersEnabled", SetterThrows] attribute DOMString filter; // (default empty string = no filter) I found filter() can still be called into it in Workers. Does the result correct to me? 2. Tried adding anothr [Func=] prefix to filter(). [Func="CanvasRenderingContext2D::CanvasInterfaceInWorkerDisabled", Func="CanvasRenderingContext2D::PrefCanvasFiltersEnabled", SetterThrows] attribute DOMString filter; // (default empty string = no filter) More details to look into the behavior for calling into filter(), I found the above two [Func] not act as *AND* relationship as I expected. Instead, they act as *OR* relationship. From the link defined for WebIDL https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings Just like he statements said, "this extended attibute can be specified together with [AvailableIn], [CheckAnyPermissions], [CheckAllPermissions], [ChromeOnly], and [Pref]. If more than one of these is specified, all conditions will need to test true for the interface or interface member to be exposed." The above statements seems not mentioning about how the code will actual do when two [Func] are combined as conditions. Is that a bug we need to file? 3. If 2. is the expected behavior, it might have another way to solve this situation. Is it a better way to merge above two Func into one to operate. The detail is [Func="CanvasRenderingContext2D::PrefCanvasFiltersEnabled", SetterThrows] attribute DOMString filter; // (default empty string = no filter) In .cpp file, implementing these two different purpose of operations to decide the return value. /* static */ bool CanvasRenderingContext2D::PrefCanvasFiltersEnabled(JSContext* aCx, JSObject* aObj) { 1. Check if Canvas is in main-thread on Worker. 2. Check if canvas.filters.enabled is enable or not. } It seems that if 2. is a problem need to be fixed, we don't use 3. as consideration. May I have your advice to move next? Really Thanks.

(In reply to Vincent Liu[:vliu] from comment #45) > At first, since the attribute [Pref=] couldn't be used once the interface > exposing to Worker, Changing [Pref=] to [Func=]. > > Pref="canvas.filters.enabled" to > Func="CanvasRenderingContext2D::PrefCanvasFiltersEnabled" > > 1. Tried adding [Exposed=Window] prefix to filter(). > > [Exposed=Window, > Func="CanvasRenderingContext2D::PrefCanvasFiltersEnabled", > SetterThrows] > attribute DOMString filter; // (default empty string = no filter) > > I found filter() can still be called into it in Workers. Does the > result correct to me? .filter property shouldn't be in the interface object at all if Exposed=Window and you're in a worker. Does it matter whether or not there is Func ? Anyhow, sounds like a bug. But filter isn't from HTML spec, so it could go to a partial interface for now > > 2. Tried adding anothr [Func=] prefix to filter(). > > [Func="CanvasRenderingContext2D::CanvasInterfaceInWorkerDisabled", > Func="CanvasRenderingContext2D::PrefCanvasFiltersEnabled", ... > > The above statements seems not mentioning about how the code will > actual do when two [Func] are combined as conditions. never seen multiple Funcs used. Didn't know that would even compile ;) Sounds like we should prevent use of multiple funcs and just force one to write a proper one dealing with the necessary checks > Is that a bug we need > to file? perhaps worth yes. > 3. If 2. is the expected behavior, it might have another way to solve this > situation. Is it a better way to merge above two Func into one to operate. You could indeed just have one Func which deals with perf and right context (Window). > { > 1. Check if Canvas is in main-thread on Worker. I guess you mean s/on/or/ > 2. Check if canvas.filters.enabled is enable or not. > } and if you're in worker you won't even do this check, right? > It seems that if 2. is a problem need to be fixed, we don't use 3. as > consideration. May I have your advice to move next? Really Thanks. Well, 2 is a bug, but I'd say we should support only one Func. Otherwise .webidl gets even messier. The real bug seems to be Exposed not working. Exposed=Window does work with XMLHttpRequest for example, but that doesn't have Func. Is there perhaps some issue with Func + Exposed combination. Might be worth to check the generated code and file a bug about Exposed.

(In reply to Olli Pettay [:smaug] from comment #46) > > It seems that if 2. is a problem need to be fixed, we don't use 3. as > > consideration. May I have your advice to move next? Really Thanks. > Well, 2 is a bug, but I'd say we should support only one Func. Otherwise > .webidl gets even messier. > The real bug seems to be Exposed not working. > Exposed=Window does work with XMLHttpRequest for example, but that doesn't > have Func. Is there perhaps some issue with > Func + Exposed combination. > Might be worth to check the generated code and file a bug about Exposed. Thanks for your good suggestion to look into the implement of XMLHttpRequest. From looked into it, I realized the difference and understood how it works for XMLHttpRequest. Based on similar design, the proposed patch was attached. The ideas are 1. keep standard in the interface part 2. For non-standard, I separate them into two partial interfaces. One is for Both threads and the other is for Window-only. Could you please also have a super review for the proposed patch? Really thanks

Comment on attachment 8687082 [details] [diff] [review] 0001-part1-v5-Modify-webidl-for-2d-canvas.patch >+ }, >+ 'workers': False, >+ 'headerFile': 'mozilla/dom/CanvasRenderingContext2D.h', >+ 'nativeType': 'mozilla::dom::CanvasRenderingContext2D' >+}, { >+ 'workers': True, >+ 'headerFile': 'mozilla/dom/CanvasRenderingContext2D.h', >+ 'nativeType': 'mozilla::dom::CanvasRenderingContext2D', >+}], Why do we need this? Don't both main thread and workers could just use the same binding given that the native type is anyway the same. > CanvasRenderingContext2D::WrapObject(JSContext *cx, JS::Handle<JSObject*> aGivenProto) > { >- return CanvasRenderingContext2DBinding::Wrap(cx, this, aGivenProto); >+ if (NS_IsMainThread()) { >+ return CanvasRenderingContext2DBinding::Wrap(cx, this, aGivenProto); >+ } else { >+ return CanvasRenderingContext2DBinding_workers::Wrap(cx, this, aGivenProto); >+ } and here, why different bindings? >+already_AddRefed<ImageData> >+CanvasRenderingContext2D::GetImageData(double aSx, double aSy, >+ double aSw, double aSh, >+ ErrorResult& error) >+{ >+ MOZ_ASSERT(NS_IsMainThread()); >+ >+ workers::WorkerPrivate* workerPrivate = workers::GetCurrentThreadWorkerPrivate(); This doesn't make sense ;) You assert that you're in main thread, but access workerprivate. >+already_AddRefed<ImageData> >+CanvasRenderingContext2D::CreateImageData(double sw, >+ double sh, ErrorResult& error) >+{ >+ MOZ_ASSERT(NS_IsMainThread()); >+ >+ workers::WorkerPrivate* workerPrivate = workers::GetCurrentThreadWorkerPrivate(); >+ JSContext* cx = workerPrivate->GetJSContext(); ditto >+already_AddRefed<ImageData> >+CanvasRenderingContext2D::CreateImageData(ImageData& imagedata, >+ ErrorResult& error) >+{ >+ MOZ_ASSERT(NS_IsMainThread()); >+ >+ workers::WorkerPrivate* workerPrivate = workers::GetCurrentThreadWorkerPrivate(); >+ JSContext* cx = workerPrivate->GetJSContext(); and here. Do you really need different binding here? Couldn't you just check the current thread and operate on that? >+ CreateImageData(double sw, double sh, arguments should be named aName. Same also elsewhere >+ >+ // For OffscreenCanvas >+ // Reference: https://wiki.whatwg.org/wiki/OffscreenCanvas >+ void commit(); commit is in the HTML spec now, so remove the comment before the method. >-// NOT IMPLEMENTED void scrollPathIntoView(); >-// NOT IMPLEMENTED void scrollPathIntoView(Path path); I wouldn't move these. They are in the spec, just not implemented.

Hi Morris, This WIP contains the part how commit flame to worker and update flame in CanvasClient. Could you have a feedback for it? Thanks

Hi Morris, The attached WIP contains the part to keep CSSParserImpl working in thread safe mode. Also including error handling in ParseColor() operated by Canvas. Could you please have a review for it? Thanks

(In reply to Olli Pettay [:smaug] from comment #48) > Comment on attachment 8687082 [details] [diff] [review] > 0001-part1-v5-Modify-webidl-for-2d-canvas.patch > > >+ }, > >+ 'workers': False, > >+ 'headerFile': 'mozilla/dom/CanvasRenderingContext2D.h', > >+ 'nativeType': 'mozilla::dom::CanvasRenderingContext2D' > >+}, { > >+ 'workers': True, > >+ 'headerFile': 'mozilla/dom/CanvasRenderingContext2D.h', > >+ 'nativeType': 'mozilla::dom::CanvasRenderingContext2D', > >+}], > > Why do we need this? Don't both main thread and workers could just use the > same binding given that the > native type is anyway the same. > > > CanvasRenderingContext2D::WrapObject(JSContext *cx, JS::Handle<JSObject*> aGivenProto) > > { > >- return CanvasRenderingContext2DBinding::Wrap(cx, this, aGivenProto); > >+ if (NS_IsMainThread()) { > >+ return CanvasRenderingContext2DBinding::Wrap(cx, this, aGivenProto); > >+ } else { > >+ return CanvasRenderingContext2DBinding_workers::Wrap(cx, this, aGivenProto); > >+ } > and here, why different bindings? > In 2D canvas, most methods are exposed in both main/worker thread. For these methods, they have the same implementations for their use separately. To reuse these methods,plus to filter out some other methods which only exposed to Window, binding different ::Wrap() in ::WrapObject() stage is a good reason to do that. Recap sentense you comment here > Why do we need this? Don't both main thread and workers could just use the > same binding given that the > native type is anyway the same. Could you please have some detail descriptions to speak out the concern if I have code implementation in Bindings.conf ? Maybe there are missing I didn't to consider? Thanks > >+already_AddRefed<ImageData> > >+CanvasRenderingContext2D::GetImageData(double aSx, double aSy, > >+ double aSw, double aSh, > >+ ErrorResult& error) > >+{ > >+ MOZ_ASSERT(NS_IsMainThread()); > >+ > >+ workers::WorkerPrivate* workerPrivate = workers::GetCurrentThreadWorkerPrivate(); > This doesn't make sense ;) > You assert that you're in main thread, but access workerprivate. > Sorry for the bad code implementation. this funcion supposed to be run in Worker thread. I should re-implementation MOZ_ASSERT() as below. MOZ_ASSERT(!NS_IsMainThread()); > > >+already_AddRefed<ImageData> > >+CanvasRenderingContext2D::CreateImageData(double sw, > >+ double sh, ErrorResult& error) > >+{ > >+ MOZ_ASSERT(NS_IsMainThread()); > >+ > >+ workers::WorkerPrivate* workerPrivate = workers::GetCurrentThreadWorkerPrivate(); > >+ JSContext* cx = workerPrivate->GetJSContext(); > ditto > ditto > > >+already_AddRefed<ImageData> > >+CanvasRenderingContext2D::CreateImageData(ImageData& imagedata, > >+ ErrorResult& error) > >+{ > >+ MOZ_ASSERT(NS_IsMainThread()); > >+ > >+ workers::WorkerPrivate* workerPrivate = workers::GetCurrentThreadWorkerPrivate(); > >+ JSContext* cx = workerPrivate->GetJSContext(); > and here. > > Do you really need different binding here? Couldn't you just check the > current thread and operate on that? > This function supposed to be run only in worker thread. I think using MOZ_ASSERT() is better than checking current thread. > > >+ CreateImageData(double sw, double sh, > arguments should be named aName. Same also elsewhere > It also confused to me there are lots of argument *Name* didn't be named aName before I modified the code. It seems it depends on the coding style by reviewer. > >+ > >+ // For OffscreenCanvas > >+ // Reference: https://wiki.whatwg.org/wiki/OffscreenCanvas > >+ void commit(); > commit is in the HTML spec now, so remove the comment before the method. > > > >-// NOT IMPLEMENTED void scrollPathIntoView(); > >-// NOT IMPLEMENTED void scrollPathIntoView(Path path); > I wouldn't move these. They are in the spec, just not implemented. Yes, you are right. I will modify them in the next version of patch.

(In reply to Vincent Liu[:vliu] from comment #51) > > In 2D canvas, most methods are exposed in both main/worker thread. For these > methods, they have the same implementations for their use separately. > To reuse these methods,plus to filter out some other methods which only > exposed to Window, binding different ::Wrap() in ::WrapObject() stage is a > good > reason to do that. > > Recap sentense you comment here > > Why do we need this? Don't both main thread and workers could just use the > > same binding given that the > > native type is anyway the same. > > Could you please have some detail descriptions to speak out the concern if I > have code implementation in Bindings.conf ? Maybe there are missing I didn't > to consider? Thanks Why doesn't Exposed=Window or Exposed=Worker work? As far as I see, this is exactly the case where it is supposed to be used. Expose something on Window context only. But if Exposed doesn't work here, could you at least try using implicitJSContext also for worker bindings so that the C++ side could stay the same. And please file a bug about Exposed not working. in comment 45 you did indeed say exposed doesn't work, but is it the combination of Exposed and Func? Could the relevant Func check that if we're not in main thread, return false? But I could live with having separate worker bindings for now. It just indicates bug somewhere. The r- was mostly about those wrong MOZ_ASSERTs which indicated the code wasn't run on debug builds. > > > > >+ CreateImageData(double sw, double sh, > > arguments should be named aName. Same also elsewhere > > > > It also confused to me there are lots of argument *Name* didn't be named > aName before I modified the code. It seems it depends on the coding style by > reviewer. Yeah, not all reviewers care about Mozilla coding style as much as I (and I guess most of DOM team ;) ). aName coding style has been there for ages https://developer.mozilla.org/en-US/docs/Mozilla_Coding_Style_Guide and these days we're trying to be more consistent how code is written. Some, especially older code, uses still random styles. And js/* is all different too. So, new code should follow the coding style. (but please don't change the coding style of the old code, that may happen in some other bug especially if we get some tool to fix coding style automatically.) Consistent coding style makes code easier to read and review, and I'd say also write.

Ok, there is https://bugzilla.mozilla.org/show_bug.cgi?id=1048695 open already.

So, just use separate bindings for workers after all. Sorry about the hassle. Didn't know about the webidl bug.

Comment on attachment 8688898 [details] [diff] [review] Bug-801176-part2-v1-Modify-Commit-frame-For-Can.patch Review of attachment 8688898 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +1414,5 @@ > + if (glue && glue->GetGrContext() && glue->GetGLContext()) { > + mTarget = Factory::CreateDrawTargetSkiaWithGrContext(glue->GetGrContext(), size, format); > + if (mTarget) { > + AddDemotableContext(this); > + if (NS_IsMainThread()) { Only create BufferProvider in main thread? @@ +5767,5 @@ > + if (!bufferProvider->IsValid()) { > + return nullptr; > + } > + return bufferProvider.forget(); > +} This version is the same as layerManager's version. Can we merge two version into one? ::: dom/canvas/OffscreenCanvas.cpp @@ +147,5 @@ > > if (factory) > screen->Morph(Move(factory)); > + } > + } else { I'll add one more CanvasContextType in bug 1172796. So I think this "else" should change to "else if (mContextType == CanvasContexType::Canvas2D" @@ +158,5 @@ > + TextureFlags flags = TextureFlags::ORIGIN_BOTTOM_LEFT; > + mCanvasClient = ImageBridgeChild::GetSingleton()-> > + CreateCanvasClient(CanvasClient::CanvasClientSurface, flags).take(); > + mCanvasRenderer->SetCanvasClient(mCanvasClient); > + } I think above codes is much similar with WebGL part. I think you should prevent duplicate those codes. @@ +200,5 @@ > + mCanvasRenderer->NotifyElementAboutInvalidation(); > + ImageBridgeChild::GetSingleton()-> > + UpdateAsyncCanvasRenderer(mCanvasRenderer); > + } > + } else { The same problems. 1. CnavasContextType check. 2. Code duplicate in those 2 blocks. ::: dom/canvas/OffscreenCanvas.h @@ +167,4 @@ > > uint32_t mWidth; > uint32_t mHeight; > + CanvasContextType mContextType; In the base class CanvasRenderingContextHelper have same attribute called mCurrentContextType. So we don't need this memeber here. ::: gfx/layers/AsyncCanvasRenderer.cpp @@ +236,5 @@ > + return; > + } > + > + gfx::IntSize CanvasSize = GetSize(); > + RefPtr<gfx::SourceSurface> mSurface = mBufferProvider->GetSnapshot(); don't use m prefix for local variable. @@ +310,5 @@ > + return TextureClient::CreateForRawBufferAccess(mCanvasClient->GetForwarder(), > + aFormat, aSize, backend, > + aFlags); > +#endif > +} Why we need this? You can just modify the version of CnavasClient2D. ::: gfx/layers/AsyncCanvasRenderer.h @@ +12,4 @@ > #include "mozilla/Mutex.h" > #include "mozilla/RefPtr.h" // for nsAutoPtr, nsRefPtr, etc > #include "nsCOMPtr.h" // for nsCOMPtr > +#include "PersistentBufferProvider.h" Using forward declaration. Don't include this file. ::: gfx/layers/client/CanvasClient.cpp @@ +156,5 @@ > > +void > +CanvasClient2D::Updated() > +{ > + if (!AddTextureClient(mBuffer)) { This logic differ from original logic which check bufferCreated. I'm afraid of the behavior will different if we change this logic. @@ +167,5 @@ > + t->mTextureClient = mBuffer; > + t->mPictureRect = nsIntRect(nsIntPoint(0, 0), mBuffer->GetSize()); > + t->mFrameID = mFrameID; > + GetForwarder()->UseTextures(this, textures); > + mBuffer->SyncWithObject(GetForwarder()->GetSyncObject()); Same as above, the "updated" check is removed. Are you sure this is correct?

Comment on attachment 8688899 [details] [diff] [review] Bug-801176-part3-v1-keep-CSSParser-Operation-As.patch Review of attachment 8688899 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +684,5 @@ > + } else { > + if (!nsRuleNode::ComputeColor(value, nullptr, nullptr, color)) { > + rv.Throw(NS_ERROR_ILLEGAL_VALUE); > + return; > + } You can merge those two conditions into one. And those two conditions should throw same error. @@ +1017,5 @@ > nsCSSValue value; > + if (NS_IsMainThread()) { > + nsIDocument* document = mCanvasElement > + ? mCanvasElement->OwnerDoc() > + : nullptr; You can using this code: nsIDocument* document = NS_IsMainThread() && mCanvasElement ? mCanvasElement->OwnerDoc() : nullptr; to avoid those if else condition. Because two blocks are too similar. No code duplicate. ::: layout/style/nsCSSParser.cpp @@ +16232,5 @@ > + mImpl = static_cast<void*>(impl); > + } else { > + CSSParserImpl *impl = new CSSParserImpl(); > + mImpl = static_cast<void*>(impl); > + } Code dup. @@ +16245,5 @@ > + gFreeList = impl; > + } else { > + CSSParserImpl *impl = static_cast<CSSParserImpl*>(mImpl); > + impl->Reset(); > + } Code dup.

Hi Morris, The attached patch is for modifying coding style in CanvasRenderingContext2D .cpp/.h. Could you please have a feedback? Thanks

(In reply to Morris Tseng [:mtseng] from comment #55) > Comment on attachment 8688898 [details] [diff] [review] > Bug-801176-part2-v1-Modify-Commit-frame-For-Can.patch > > Review of attachment 8688898 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/canvas/CanvasRenderingContext2D.cpp > @@ +1414,5 @@ > > + if (glue && glue->GetGrContext() && glue->GetGLContext()) { > > + mTarget = Factory::CreateDrawTargetSkiaWithGrContext(glue->GetGrContext(), size, format); > > + if (mTarget) { > > + AddDemotableContext(this); > > + if (NS_IsMainThread()) { > > Only create BufferProvider in main thread? Yes, It is the case for Skia GL.

Hi Morris, Thanks for the feedback to let the code more better than previous. The patch for v2 was attached. Could you please also have feedback for me? Thanks

(In reply to Olli Pettay [:smaug] from comment #53) > Ok, there is https://bugzilla.mozilla.org/show_bug.cgi?id=1048695 open > already. The proposed patch is based on the patch landing in Bug 1048695. After that, I don't need to wrap different object to expose methods only for Window. Instead, declaring them in webidl.

Comment on attachment 8693577 [details] [diff] [review] 0001-part1-v6-Modify-webidl-for-2d-canvas.patch >+CanvasRenderingContext2D::GetCanvas(Nullable<dom::OwningHTMLCanvasElementOrOffscreenCanvas>& aRetval) >+{ >+ if (mCanvasElement) { >+ MOZ_RELEASE_ASSERT(!mOffscreenCanvas); >+ aRetval.SetValue().SetAsHTMLCanvasElement() = mCanvasElement; >+ } else if (mOffscreenCanvas) { >+ aRetval.SetValue().SetAsOffscreenCanvas() = mOffscreenCanvas; >+ } else { >+ aRetval.SetNull(); >+ } >+} Please use 2 spaces for indentation >+CanvasRenderingContext2D::Commit() >+{ >+ if (mOffscreenCanvas) { >+ mOffscreenCanvas->CommitFrameToCompositor(); >+ } >+} ditto >+++ b/dom/canvas/CanvasRenderingContext2D.h >@@ -46,6 +46,7 @@ class OwningStringOrCanvasGradientOrCanvasPattern; > class TextMetrics; > class CanvasFilterChainObserver; > class CanvasPath; >+class OwningHTMLCanvasElementOrOffscreenCanvas; > > extern const mozilla::gfx::Float SIGMA_MAX; > >@@ -79,6 +80,11 @@ public: > return mCanvasElement->GetOriginalCanvas(); > } > >+ void GetCanvas(Nullable<dom::OwningHTMLCanvasElementOrOffscreenCanvas>& aRetval); >+ >+ // For OffscreenCanvas >+ void Commit(); I would just drop the '// For OffscreenCanvas' comment given that Commit() in HTML spec. >+[Exposed=(Window,Worker), >+ Func="mozilla::dom::OffscreenCanvas::PrefEnabledOnWorkerThread"] > interface CanvasRenderingContext2D { > > // back-reference to the canvas. Might be null if we're not > // associated with a canvas. >- readonly attribute HTMLCanvasElement? canvas; >+ readonly attribute CanvasObj? canvas; >+ >+ // For OffscreenCanvas >+ // Reference: https://wiki.whatwg.org/wiki/OffscreenCanvas >+ void commit(); commit() is defined in HTML spec, so drop '// For OffscreenCanvas // Reference: https://wiki.whatwg.org/wiki/OffscreenCanvas' >-[Pref="canvas.path.enabled", >+[Exposed=(Window,Worker), >+ Func="CanvasRenderingContext2D::PrefCanvasPathEnabled", > Constructor, > Constructor(Path2D other), > Constructor(DOMString pathString)] > interface Path2D > { >+ [Func="mozilla::dom::OffscreenCanvas::PrefEnabledOnWorkerThread"] > void addPath(Path2D path, optional SVGMatrix transformation); > }; So if canvas path is enabled, we expose the interface in workers, but the interface just doesn't have addPath. (it does have the methods defined in CanvasPathMethods). I don't think we want that. I guess PrefCanvasPathEnabled could call also OffscreenCanvas::PrefEnabledOnWorkerThread when called on non-mainthread. And drop the Func before addPath. Those fixed, r+

Comment on attachment 8693480 [details] [diff] [review] 0001-part0-v1-Modify-Coding-Style-for-2d-Canvas.patch Review of attachment 8693480 [details] [diff] [review]: ----------------------------------------------------------------- Mostly the * and & position errors. ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +432,4 @@ > public: > typedef CanvasRenderingContext2D::ContextState ContextState; > > + AdjustedTargetForShadow(CanvasRenderingContext2D *aCtx, CanvasRenderingContext2D* @@ +600,4 @@ > private: > > gfx::Rect > + MaxSourceNeededBoundsForFilter(const gfx::Rect& aDestBounds, CanvasRenderingContext2D *aCtx) ditto @@ +617,5 @@ > return gfx::Rect(sourceGraphicNeededRegion.GetBounds()); > } > > gfx::Rect > + MaxSourceNeededBoundsForShadow(const gfx::Rect& aDestBounds, CanvasRenderingContext2D *aCtx) ditto @@ +634,4 @@ > } > > gfx::Rect > + BoundsAfterFilter(const gfx::Rect& aBounds, CanvasRenderingContext2D *aCtx) ditto @@ +1000,4 @@ > } > > JSObject* > +CanvasRenderingContext2D::WrapObject(JSContext *aCx, JS::Handle<JSObject*> aGivenProto) ditto @@ +1145,4 @@ > } > > void > +CanvasRenderingContext2D::Redraw(const gfx::Rect &aR) ditto @@ +1570,5 @@ > } > > NS_IMETHODIMP > +CanvasRenderingContext2D::InitializeWithSurface(nsIDocShell *aShell, > + gfxASurface *aSurface, ditto ::: dom/canvas/CanvasRenderingContext2D.h @@ +67,4 @@ > public: > CanvasRenderingContext2D(); > > + virtual JSObject* WrapObject(JSContext *aCx, JS::Handle<JSObject*> aGivenProto) override; JSContext* aCx @@ +439,5 @@ > } > return nullptr; > } > + NS_IMETHOD SetDimensions(int32_t aWidth, int32_t aHeight) override; > + NS_IMETHOD InitializeWithSurface(nsIDocShell *aShell, gfxASurface *aSurface, int32_t aWidth, int32_t aHeight) override; nsIDocShell* gfxASurface* @@ +470,2 @@ > // this rect is in canvas device space > + void Redraw(const mozilla::gfx::Rect &aR); Rect& @@ +470,3 @@ > // this rect is in canvas device space > + void Redraw(const mozilla::gfx::Rect &aR); > + NS_IMETHOD Redraw(const gfxRect &aR) override { Redraw(ToRect(aR)); return NS_OK; } gfxRect& @@ +482,4 @@ > virtual void DidRefresh() override; > > // this rect is in mTarget's current user space > + void RedrawUser(const gfxRect &aR); ditto @@ +558,2 @@ > > + nsresult InitializeWithTarget(mozilla::gfx::DrawTarget *aSurface, DrawTarget* @@ +676,4 @@ > nsLayoutUtils::SurfaceFromElementResult > CachedSurfaceFromElement(Element* aElement); > > + void DrawImage(const CanvasImageSource &aImgElt, CanvasImageSource& @@ +1048,4 @@ > friend class AdjustedTargetForFilter; > > // other helpers > + void GetAppUnitsValues(int32_t *aPerDevPixel, int32_t *aPerCSSPixel) int32_t*

Hi Cameron, The proposed patch are trying to implement for 1. Keep gFreeList is used only in main-thread. 2. Add THREADSAFE_REFCOUNTING for ErrorReport Could you please have a review for me? Thanks

Hi Cameron, Thanks to talk to your last week for the patch review. Could you also have a review for me? Thanks

Hi roc, The proposed patch attends to modify coding styles in CanvasRenderingContext2D. Could you please have a review for me? Thanks

Hi roc, The proposed patch intends to implement. 1. Create canvas context in Worker thread. 2. Commit Frame to Compositor in Worker thread. Could you please have a review for me? Really thanks.

Comment on attachment 8698747 [details] [diff] [review] part3-v3-keep-CSSParser-Operation-As-ThreadSafe.patch Review of attachment 8698747 [details] [diff] [review]: ----------------------------------------------------------------- Some comments here that I didn't think of when going through the patch with you in person. Thanks! ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +1029,5 @@ > *aColor = value.GetColorValue(); > } else { > // otherwise resolve it > + > + // Skip resolving because we can't access DOM to operate in off main-thread. In CanvasGradient::AddColorStop we call nsRuleNode::ComputeColor to handle non-numeric specified color values by passing in null for the nsPresContext/nsStyleContext. Can we just do the same here? Otherwise we won't handle any color keyword values. ::: layout/style/nsCSSParser.cpp @@ +2115,5 @@ > > + // Skip clear/output ErrorReporter object because we can't access DOM to operate in > + // off main-thread. > + if (NS_IsMainThread()) { > + if (aSuppressErrors) { Looking in ErrorReporter, I realise we need to do a little more work to ensure that we don't use it unsafely off the main thread. Specifically, I think we need to: * avoid touching sSpecCache in ~ErrorReporter (i.e. just skip that work) when !NS_IsMainThread() * make ShouldReportErrors return false if !NS_IsMainThread() (and add a comment there that says we only need to do CSS parsing off the main thread for canvas-on-workers currently, where we don't need to report any CSS parser errors) The second one might not be necessary, since we're pretty much guaranteed to have initialized sConsoleService etc. by the time we're running canvas-on-worker scripts, but better to avoid touching those global variables. With ShouldReportErrors always returning false, we can avoid modifying ParseColorString to check NS_IsMainThread -- even if we called OUTPUT_ERROR we'll return early in ErrorReporter::OutputError (and most other methods on ErrorReporter). @@ +16882,2 @@ > } else { > impl = new CSSParserImpl(); Assert in here that aLoader and aSheet are null.

Hi Cameron, Thanks for the comment for my missing for skipping global variables in ErrorReporter and error handling. The attached file is my proposed patch.

Comment on attachment 8699362 [details] [diff] [review] part2-v3-Commit-frame-For-Canvas.patch Review of attachment 8699362 [details] [diff] [review]: ----------------------------------------------------------------- Please add a useful commit message. And please provide 8 lines of context instead of 3.

Comment on attachment 8700485 [details] [diff] [review] part3-v4-keep-CSSParser-Operation-As-ThreadSafe.patch Review of attachment 8700485 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: layout/style/ErrorReporter.cpp @@ +153,4 @@ > // Schedule deferred cleanup for cached data. We want to strike a > // balance between performance and memory usage, so we only allow > // short-term caching. > + if (NS_IsMainThread() && sSpecCache && sSpecCache->IsInUse() && !sSpecCache->IsPending()) { Please wrap this at 80 columns.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #69) > Comment on attachment 8699362 [details] [diff] [review] > part2-v3-Commit-frame-For-Canvas.patch > > Review of attachment 8699362 [details] [diff] [review]: > ----------------------------------------------------------------- > > Please add a useful commit message. And please provide 8 lines of context > instead of 3. The proper commit message and line of context were changed. Could you please have review for me? Thanks

Hi Morris, The attached patch is Mochitest for basic offscreencanvas operations. Could you please have a feedback for me? Thanks

Comment on attachment 8700952 [details] [diff] [review] part4-v1: Mochitests for basic offscreencanvas operations Review of attachment 8700952 [details] [diff] [review]: ----------------------------------------------------------------- test_offscreencanvas_toimagebitmap.html and test_offscreencanvas_toblob.html should test for 2d case as well. Since you modify the file name of test case, you need modify mochitest.ini. ::: dom/canvas/test/test_offscreencanvas_basic_operations.html @@ +65,5 @@ > + [offscreenCanvas]); > + } > + > + startWorker(createCanvas(), '2d'); > + startWorker(createCanvas(), 'webgl'); You cannot call two startWorker simultaneously. Because it will call SimpleTest.finish twice which mochitest will report error. The better way is calling first startWorker, wait it finish, then call second startWorker. ::: gfx/layers/AsyncCanvasRenderer.cpp @@ +293,5 @@ > + if (mGLContext) { > + surface = GetSurface(); > + } else { > + RefPtr<gfx::SourceSurface> sourceSurface = mBufferProvider->GetSnapshot(); > + surface = sourceSurface->GetDataSurface(); Put this logic in the GetSurface(). @@ +303,5 @@ > > + // Only handle y flip in webgl. > + RefPtr<gfx::DataSourceSurface> dataSurf = !!mGLContext > + ? gl::YInvertImageSurface(surface) > + : surface; Do we need "!!" here?

Comment on attachment 8700879 [details] [diff] [review] part2-v3-Let-Canvas-2d-context-works-on-Workers.patch Review of attachment 8700879 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/AsyncCanvasRenderer.cpp @@ +245,5 @@ > + RefPtr<gfx::SourceSurface> surface = mBufferProvider->GetSnapshot(); > + > + if (surface) { > + NS_ASSERTION(surface, "Must have surface to draw!"); > + if (surface) { Redundant "if"

Hi Morris, The attached WIP is Mochitest for 2d offscreen canvas. Could you please have a feedback? Thanks

(In reply to Cameron McCormack (:heycam) from comment #67) > Comment on attachment 8698747 [details] [diff] [review] > part3-v3-keep-CSSParser-Operation-As-ThreadSafe.patch > > @@ +16882,2 @@ > > } else { > > impl = new CSSParserImpl(); > > Assert in here that aLoader and aSheet are null. Hi heycam, It seems that adding assert check for aLoader and aSheet might have problem. The below link is one case to pass nullptr aLoader. Could you please double confirm with that? Thanks https://dxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp#1018

(In reply to Vincent Liu[:vliu] from comment #77) > It seems that adding assert check for aLoader and aSheet might have problem. > The below link is one case to pass nullptr aLoader. Could you please double > confirm with that? Thanks > > https://dxr.mozilla.org/mozilla-central/source/dom/canvas/ > CanvasRenderingContext2D.cpp#1018 There is a comment in there that we pass in the loader so that we can report errors in parsing correctly, but when I test with: <canvas></canvas> <script> var cx = document.querySelector("canvas").getContext("2d"); cx.shadowColor = "xxx"; console.log(cx.shadowColor); </script> I don't see any error in the console. So if that was the intention I don't think it's working. Can you just add "&& !NS_IsMainThread()" to that condition, so that we do pass in null for workers? And then file a bug about the main thread console error reporting not working here (so we can decide to remove it or fix it).

Comment on attachment 8710300 [details] [diff] [review] part4-v1: Mochitest for adding 2d offscreen canvas. Review of attachment 8710300 [details] [diff] [review]: ----------------------------------------------------------------- First round review for the formatting. ::: dom/canvas/test/offscreencanvas.js @@ +41,5 @@ > postMessageGeneral({type: "draw", count: count}); > } > > +function sendBlob(canvasType, blob) { > + postMessageGeneral({type: "blob",canvasType: canvasType, blob: blob}); space after comma. @@ +59,5 @@ > +//-------------------------------------------------------------------- > +// Canvas 2D Drawing Functions > +//-------------------------------------------------------------------- > +function createDrawFunc2D(canvas) { > + var Context2d; variable starts with lower case. @@ +298,2 @@ > draw("", false); > + var imgBitmap = canvas.transferToImageBitmap(); Too many space before "=". ::: dom/canvas/test/offscreencanvas_serviceworker_inner.html @@ +33,5 @@ > +function CreateMessagChannel () { > + var msgChannel = new MessageChannel(); > + msgChannel.port1.onmessage = function(evt) { > + parent.postMessage(evt.data, "*"); > +} indent. @@ +40,3 @@ > > +var offscreen2dCanvas = transferCtrlToOffscreen(createCanvas()); > +var msgChannel2d = CreateMessagChannel (); unnecessary space @@ +40,5 @@ > > +var offscreen2dCanvas = transferCtrlToOffscreen(createCanvas()); > +var msgChannel2d = CreateMessagChannel (); > +var offscreenWebGLCanvas = transferCtrlToOffscreen(createCanvas()); > +var msgChannelwebgl = CreateMessagChannel (); ditto @@ +47,5 @@ > + navigator.serviceWorker.controller.postMessage({canvasType: '2d', > + testType: 'basic', > + canvas: offscreen2dCanvas}, > + [offscreen2dCanvas, > + msgChannel2d.port2]); missing one space here. @@ +54,5 @@ > + navigator.serviceWorker.controller.postMessage({canvasType: 'webgl', > + testType: 'basic', > + canvas: offscreenWebGLCanvas}, > + [offscreenWebGLCanvas, > + msgChannelwebgl.port2]); ditto ::: dom/canvas/test/test_offscreencanvas_many.html @@ +67,5 @@ > } > > var offscreenCanvas = canvas.transferControlToOffscreen(); > + worker.postMessage({canvasType: canvasType, testType: testType, > + canvas: offscreenCanvas}, [offscreenCanvas]); missing space. ::: dom/canvas/test/test_offscreencanvas_neuter.html @@ +37,5 @@ > + ok(!htmlCanvas.getContext("2d"), "Can't get 2d Context after transferControlToOffscreen"); > + ok(!htmlCanvas.getContext("webgl"), "Can't get webgl Context after transferControlToOffscreen"); > + ok(!htmlCanvas.getContext("webgl2"), "Can't get webgl2 Context after transferControlToOffscreen"); > + ok(!htmlCanvas.getContext("bitmaprenderer") > + , "Can't get bitmaprenderer Context after transferControlToOffscreen"); put comma at previous line and fix line indent. ::: dom/canvas/test/test_offscreencanvas_serviceworker.html @@ +25,3 @@ > if (msg.type == "finish") { > registration.unregister().then(function() { > + if (--stillRunning == 0) missing brackets. ::: dom/canvas/test/test_offscreencanvas_sharedworker.html @@ +40,5 @@ > ); > +} > + > +function runTest() { > + remove extra line. ::: dom/canvas/test/test_offscreencanvas_toblob.html @@ +91,5 @@ > ok(offscreenCanvas, "Expected transferControlToOffscreen to succeed"); > > + // Start to test toblob for 2d > + worker.postMessage({canvasType: '2d', testType: 'toblob', > + canvas: offscreenCanvas}, [offscreenCanvas]); indent.

Comment on attachment 8710300 [details] [diff] [review] part4-v1: Mochitest for adding 2d offscreen canvas. Review of attachment 8710300 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/test/offscreencanvas.js @@ +34,5 @@ > } > > +function finish2D() { > + postMessageGeneral({type: "finish2D"}); > +} You don't need this extra function. See comment in test_offscreencanvas_toimagebitmap.html. @@ +58,5 @@ > +} > +//-------------------------------------------------------------------- > +// Canvas 2D Drawing Functions > +//-------------------------------------------------------------------- > +function createDrawFunc2D(canvas) { Maybe you can merge this function to createDrawFunc with an extra parameter "type". Since two function is very similar. @@ +229,2 @@ > var canvas = offscreenCanvas; > + if ((testType == "imagebitmap") || (testType == "changesize")) { Why we need construct a new OffscreenCanvas for "changesize" case? ::: dom/canvas/test/test_offscreencanvas_dynamic_fallback.html @@ +76,5 @@ > + } else { > + initWithMask = true; > + startWorker(createCanvas(initWithMask), canvasType, testType, > + initWithMask); > + } This is so tricky and hard to maintenance. See my suggestion below. @@ +94,3 @@ > } > > + startWorker(createCanvas(false), '2d', 'fallback', false); We can let startWorker return a Promise. Then we can do something like this. startWorker(createCanvas(false), '2d', 'fallback', false).then( startWorker(createCanvas(true), '2d', 'fallback', true).then( startWorker(createCanvas(false), 'webgl', 'fallback', false).then( startWorker(createCanvas(true), 'webgl', 'fallback', true).then( SimpleTest.finish() ))))); ::: dom/canvas/test/test_offscreencanvas_neuter.html @@ +52,5 @@ > "Can't change transfered worker canvas height"); > > SimpleTest.doesThrow( > function() { offscreenCanvas.getContext("2d") }, > + "Can't get 2d Context on transfered worker canvas"); indent. @@ +57,4 @@ > > SimpleTest.doesThrow( > function() { offscreenCanvas.getContext("webgl") }, > + "Can't get webgl Context on transfered worker canvas"); indent. @@ +61,4 @@ > > SimpleTest.doesThrow( > function() { offscreenCanvas.getContext("webgl2") }, > + "Can't get webgl2 Context on transfered worker canvas"); indent @@ +64,5 @@ > + "Can't get webgl2 Context on transfered worker canvas"); > + > + SimpleTest.doesThrow( > + function() { offscreenCanvas.getContext("bitmaprenderer") }, > + "Can't get bitmaprenderer Context on transfered worker canvas"); indent. ::: dom/canvas/test/test_offscreencanvas_toblob.html @@ +89,2 @@ > > + var offscreenCanvas = TransferCtrlToOffscreen(createCanvas()); extra space after = ::: dom/canvas/test/test_offscreencanvas_toimagebitmap.html @@ +52,5 @@ > "ImageBitmap has been transferred, toDataURL will throw."); > + } > + if (msg.type == "finish2D") { > + // Start to test sizechange for webgl > + worker.postMessage({canvasType: 'webgl', testType: 'imagebitmap'}); If you get first finish, then you know that is the finish of test for 2d case, then you can start for webgl. No need add a new state "finish2D" for this case.

The part4-v2 was attached for the feedbacks from Comment 79 and 80. (In reply to Morris Tseng [:mtseng] from comment #80) > Comment on attachment 8710300 [details] [diff] [review] > part4-v1: Mochitest for adding 2d offscreen canvas. > > Review of attachment 8710300 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/canvas/test/test_offscreencanvas_toimagebitmap.html > @@ +52,5 @@ > > "ImageBitmap has been transferred, toDataURL will throw."); > > + } > > + if (msg.type == "finish2D") { > > + // Start to test sizechange for webgl > > + worker.postMessage({canvasType: 'webgl', testType: 'imagebitmap'}); > > If you get first finish, then you know that is the finish of test for 2d > case, then you can start for webgl. No need add a new state "finish2D" for > this case. I tried to use Promise().then() operation to change the original behavior.

Comment on attachment 8712489 [details] [diff] [review] part4-v2: Mochitest for adding 2d offscreen Review of attachment 8712489 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/test/offscreencanvas_serviceworker_inner.html @@ +31,3 @@ > } > > +function CreateMessagChannel () { nit: function starts with lower case. ::: dom/canvas/test/test_offscreencanvas_toblob.html @@ +19,5 @@ > + document.body.appendChild(htmlCanvas); > + return htmlCanvas; > +} > + > +function TransferCtrlToOffscreen(canvas) { nit: start with lower case.

Rebase based on part0-v2.

Based on the patch in Comment 83. Pushed to mozilla-inbound try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c21edf4f19a https://hg.mozilla.org/integration/mozilla-inbound/rev/ed8431be3eeb

Reopen this bug since there are some works need to be done.

Comment on attachment 8700879 [details] [diff] [review] part2-v3-Let-Canvas-2d-context-works-on-Workers.patch Review of attachment 8700879 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/OffscreenCanvas.cpp @@ +125,1 @@ > contextType == CanvasContextType::ImageBitmap)) You support all context types now. So you can remove this check completely.

Hi Morris, Can you please have a review for the patch? Thanks.

Hi James, Since the patches in this bug are landed, Canvas 2d will actually has API commit() exposing to JS. After that, the interface check for commit() seems can removed from web-platform-test for FALSE checking. I'd proposed a patch here for this purpose. Could you please have a review for me? Thanks

Hi Cameron, The proposed patch intends to fix memory leak when operating CSSParserImpl in Worker. Could you please have a review for me? Thanks

Hi roc, The proposed patch intends to fix assertions when 2d Canvas Worker runs in debug build.

Hi roc, The proposed patch handles the case when canvas is anonymous in Worker. Please have a review. Thanks.

Comment on attachment 8723477 [details] [diff] [review] part5-v1: Fix Assertions in debug build. Review of attachment 8723477 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +1424,5 @@ > nsContentUtils::PersistentLayerManagerForDocument(ownerDoc); > } > > + if (NS_IsMainThread() && > + mode == RenderingMode::OpenGLBackendMode && Why is this main-thread-only? Shouldn't we support Skia OpenGL canvases off main thread? @@ +1468,5 @@ > + if (NS_IsMainThread()) { > + static bool registered = false; > + if (!registered) { > + registered = true; > + RegisterStrongMemoryReporter(new Canvas2dPixelsReporter()); Shouldn't we support memory reporting for off-main-thread canvases?

Comment on attachment 8723482 [details] [diff] [review] part9-v1: Fix build break by using namespace doesn't include in proper scope. Review of attachment 8723482 [details] [diff] [review]: ----------------------------------------------------------------- This patch should be folded into one of the other patches I guess? We want every landed revision to build.

Comment on attachment 8723484 [details] [diff] [review] part6-v1: SetNull for the return of GetCanvas() if canvas is anonymous in Worker. Review of attachment 8723484 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +3459,5 @@ > if (mCanvasElement) { > MOZ_RELEASE_ASSERT(!mOffscreenCanvas); > + > + if (mCanvasElement->IsInNativeAnonymousSubtree()) { > + aRetval.SetNull(); This is very confusing. The patch comment says "if canvas is anonymous in Worker" but Workers don't have elements, anonymous or not.

Comment on attachment 8723460 [details] [diff] [review] part11-v1: Fix memory leak in nsCSSParser. Review of attachment 8723460 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsCSSParser.cpp @@ +17197,5 @@ > if (NS_IsMainThread()) { > impl->mNextFree = gFreeList; > gFreeList = impl; > + } else { > + delete impl; So we can catch future similar mistakes, can you add MOZ_COUNT_CTOR/MOZ_COUNT_DTOR calls in CSSParserImpl's constructor/destructor? @@ +17204,5 @@ > > /* static */ void > nsCSSParser::Shutdown() > { > + if (NS_IsMainThread()) { I think Shutdown will only be called from the main thread, in nsLayoutStatics::Shutdown. Can you just assert NS_IsMainThread() instead?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #99) > Comment on attachment 8723477 [details] [diff] [review] > part5-v1: Fix Assertions in debug build. > > Review of attachment 8723477 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/canvas/CanvasRenderingContext2D.cpp > @@ +1424,5 @@ > > nsContentUtils::PersistentLayerManagerForDocument(ownerDoc); > > } > > > > + if (NS_IsMainThread() && > > + mode == RenderingMode::OpenGLBackendMode && > > Why is this main-thread-only? Shouldn't we support Skia OpenGL canvases off > main thread? It seems that I got asserion below. https://dxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp#1321 Even I remove this line, I can still meet asserts in other place. Supporting Skia OpenGL in off main thread is a great idea but it seems there should be take more time to dig into. How do you think I filing another bug for the following tracking? > @@ +1468,5 @@ > > + if (NS_IsMainThread()) { > > + static bool registered = false; > > + if (!registered) { > > + registered = true; > > + RegisterStrongMemoryReporter(new Canvas2dPixelsReporter()); > > Shouldn't we support memory reporting for off-main-thread canvases? The attached patch intends to support memory reporting for off-main-thread canvas. Could you please have a review for me? Thanks.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #101) > Comment on attachment 8723484 [details] [diff] [review] > part6-v1: SetNull for the return of GetCanvas() if canvas is anonymous in > Worker. > > Review of attachment 8723484 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/canvas/CanvasRenderingContext2D.cpp > @@ +3459,5 @@ > > if (mCanvasElement) { > > MOZ_RELEASE_ASSERT(!mOffscreenCanvas); > > + > > + if (mCanvasElement->IsInNativeAnonymousSubtree()) { > > + aRetval.SetNull(); > > This is very confusing. The patch comment says "if canvas is anonymous in > Worker" but Workers don't have elements, anonymous or not. I'd rewrote the commit message to clear the purpose of this patch. Could you please have a review? Thanks

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #100) > Comment on attachment 8723482 [details] [diff] [review] > part9-v1: Fix build break by using namespace doesn't include in proper scope. > > Review of attachment 8723482 [details] [diff] [review]: > ----------------------------------------------------------------- > > This patch should be folded into one of the other patches I guess? We want > every landed revision to build. Actually this build break happens when I run try server. It could be some code modifications in my patches are build in Unified_cpp_gfx_layers0 to cause this problem. I will fold this patch into one of my patches.

Comment on attachment 8725136 [details] [diff] [review] part6-v1: SetNull for the return of GetCanvas() if canvas is anonymous. Review of attachment 8725136 [details] [diff] [review]: ----------------------------------------------------------------- Why do we need this patch?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #106) > Comment on attachment 8725136 [details] [diff] [review] > part6-v1: SetNull for the return of GetCanvas() if canvas is anonymous. > > Review of attachment 8725136 [details] [diff] [review]: > ----------------------------------------------------------------- > > Why do we need this patch? My previous patch will cause TEST-UNEXPECTED-FAIL in test_anonymousContent_canvas.html when I run tryserver. 20:11:14 INFO - 478 INFO TEST-UNEXPECTED-FAIL | dom/base/test/test_anonymousContent_canvas.html | context's canvas property is null in anonymous content - got [object HTMLCanvasElement], expected null

Hi roc, I'd rewritten the commit message to explain the reason why we need this patch. The failed test was showed in [1] [1]: https://dxr.mozilla.org/mozilla-central/source/dom/base/test/test_anonymousContent_canvas.html The test fail message was showed in [2] [2]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7e6832c765b&selectedJob=16025813 From the fail message, it indicates that GetCanvas() I created in my patch doesn't consider the case which fit for test condition in [1]. I am not sure the comment message is clear enough for this patch. If not, can you please tell me what part I should modify? Or is any wrong for the patch to fix this problem? Thanks

Comment on attachment 8725566 [details] [diff] [review] part6-v1: Test fails if context's canvas property is null in anonymous content. Review of attachment 8725566 [details] [diff] [review]: ----------------------------------------------------------------- OK, this just fixes a regression in part1-v6-Modify-webidl-for-2d-canvas. Please fold this into that patch.

(In reply to Robert O'Callahan (:roc) (Exited; email my personal email if necessary) from comment #110) > Comment on attachment 8725566 [details] [diff] [review] > part6-v1: Test fails if context's canvas property is null in anonymous > content. > > Review of attachment 8725566 [details] [diff] [review]: > ----------------------------------------------------------------- > > OK, this just fixes a regression in part1-v6-Modify-webidl-for-2d-canvas. > Please fold this into that patch. The attached patch folding part6 into part 1.

(In reply to Vincent Liu[:vliu] from comment #105) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment > #100) > > Comment on attachment 8723482 [details] [diff] [review] > > part9-v1: Fix build break by using namespace doesn't include in proper scope. > > > > Review of attachment 8723482 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > This patch should be folded into one of the other patches I guess? We want > > every landed revision to build. > > Actually this build break happens when I run try server. It could be some > code modifications in my patches are build in Unified_cpp_gfx_layers0 to > cause this problem. I will fold this patch into one of my patches. The attach file folding part9-v1 into part2.

The attached patch folding part11 into part3

Rename the patch to fit the part number.

Rename the patch to fit the part number.

Rename the patch to fit the part number.

Rename the patch to fit the part number.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=00232251530b

https://hg.mozilla.org/integration/mozilla-inbound/rev/2022afa9aef2 https://hg.mozilla.org/integration/mozilla-inbound/rev/134a8c56335a https://hg.mozilla.org/integration/mozilla-inbound/rev/fc509e59a0cd https://hg.mozilla.org/integration/mozilla-inbound/rev/c090dc98c670 https://hg.mozilla.org/integration/mozilla-inbound/rev/0fb279e041e1 https://hg.mozilla.org/integration/mozilla-inbound/rev/e210473d793c https://hg.mozilla.org/integration/mozilla-inbound/rev/a2a7b1c4dc5b https://hg.mozilla.org/integration/mozilla-inbound/rev/624fb8512ba5 https://hg.mozilla.org/integration/mozilla-inbound/rev/82320c900699

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/953741f757ab - those crashes in test_offscreencanvas_dynamic_fallback.html on your try push weren't some sort of coincidence, they were yours. Mostly Windows and OS X 10.10 opt, so possibly a race you lose on faster machines.

Hi Jeff, The attached patch intends to add mutex to fix thread competition problem. I tried to rename sMemoryReportMutex defined in part5 patch to let this mutex is more general to be used by Context 2d. Could you please review it? Thanks

Hi Jeff, The attached patch intends to add mutex to fix thread competition problem. I tried to rename sMemoryReportMutex defined in part5 patch to let this mutex is more general to be used by Context 2d. Could you please review it? Thanks

Comment on attachment 8731159 [details] [diff] [review] part10-v1:Add mutex to solve threading save issue. Review of attachment 8731159 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +530,5 @@ > > explicit AdjustedTarget(CanvasRenderingContext2D* aCtx, > const gfx::Rect *aBounds = nullptr) > { > + StaticMutexAutoLock lock(sMutex); What data is this locking? @@ +4897,5 @@ > double aY, double aW, double aH, > const nsAString& aBgColor, > uint32_t aFlags, ErrorResult& aError) > { > + StaticMutexAutoLock lock(sMutex); What data is this locking?

(In reply to Jeff Muizelaar [:jrmuizel] from comment #123) > Comment on attachment 8731159 [details] [diff] [review] > part10-v1:Add mutex to solve threading save issue. > > Review of attachment 8731159 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/canvas/CanvasRenderingContext2D.cpp > @@ +530,5 @@ > > > > explicit AdjustedTarget(CanvasRenderingContext2D* aCtx, > > const gfx::Rect *aBounds = nullptr) > > { > > + StaticMutexAutoLock lock(sMutex); > > What data is this locking? > frame #2: 0x000000010807ac96 XUL`SkBitmap::copyTo(this=0x000000012434f698, dst=0x000000012434f698, dstColorType=kBGRA_8888_SkColorType, alloc=0x0000000000000000) const + 1302 at SkBitmap.cpp:893 frame #3: 0x000000010807afa6 XUL`SkBitmap::deepCopyTo(this=0x000000012434f698, dst=0x000000012434f698) const + 502 at SkBitmap.cpp:940 frame #4: 0x00000001036a2559 XUL`mozilla::gfx::SourceSurfaceSkia::DrawTargetWillChange(this=0x000000012434f670) + 121 at SourceSurfaceSkia.cpp:138 frame #5: 0x0000000103672685 XUL`mozilla::gfx::DrawTargetSkia::MarkChanged(this=0x000000012434f1f0) + 53 at DrawTargetSkia.cpp:1182 frame #6: 0x000000010367390e XUL`mozilla::gfx::DrawTargetSkia::Fill(this=0x000000012434f1f0, aPath=0x00000001243e8c70, aPattern=0x00007000056f9c30, aOptions=0x00007000056f9bc8) + 46 at DrawTargetSkia.cpp:530 frame #7: 0x00000001050af5b7 XUL`mozilla::dom::CanvasRenderingContext2D::Fill(this=0x000000011e939800, aWinding=0x00007000056f9dc8) + 631 at CanvasRenderingContext2D.cpp:2766 frame #8: 0x00000001047f19a8 XUL`mozilla::dom::CanvasRenderingContext2DBinding::fill(cx=0x0000000124c34400, obj=Handle<JSObject *> @ 0x00007000056f9df0, self=0x000000011e939800, args=0x00007000056f9e58) + 392 at CanvasRenderingContext2DBinding.cpp:3322 > @@ +4897,5 @@ > > double aY, double aW, double aH, > > const nsAString& aBgColor, > > uint32_t aFlags, ErrorResult& aError) > > { > > + StaticMutexAutoLock lock(sMutex); > > What data is this locking? frame #10: 0x000000010374ab66 XUL`mozilla::gfx::SourceSurfaceCGBitmapContext::DrawTargetWillChange(this=0x0000000195673180) + 166 at SourceSurfaceCG.cpp:346 frame #11: 0x000000010374d8f8 XUL`mozilla::gfx::SourceSurfaceCGBitmapContext::GetDataSurface(this=0x0000000195673180) + 40 at SourceSurfaceCG.h:135 frame #12: 0x00000001050b8b91 XUL`mozilla::dom::CanvasRenderingContext2D::DrawWindow(this=0x000000011eaa9800, aWindow=0x00000001361d8000, aX=0, aY=0, aW=500, aH=300, aBgColor=u"rgb(255,255,255)", aFlags=0, aError=0x00007fff5fbf3370) + 2001 at CanvasRenderingContext2D.cpp:5008 The drawing command like Fill() calls into DrawTargetWillChange() to make a copy. Similar operations also did in DrawWindow(). In Windows platform, the thread competition also happens in D2D1. Based on this, The patch can handle all of them.

(In reply to Vincent Liu[:vliu] from comment #124) I'm not in a hurry to have a global lock for non-global data. Can you explain why we can't have locks around the individual pieces of data that we're trying to ensure mutual access to?

(In reply to Jeff Muizelaar [:jrmuizel] from comment #125) > (In reply to Vincent Liu[:vliu] from comment #124) > I'm not in a hurry to have a global lock for non-global data. Can you > explain why we can't have locks around the individual pieces of data that > we're trying to ensure mutual access to? Hi Jeff, Please let me take some time to explain the current problem I saw. This issue happens when I run the following mochitest. ./mach mochitest --repeat 30 dom/canvas/test/dom/canvas/test/test_offscreencanvas_dynamic_fallback.html The above test will cause crash on main thread in [1] [1]: https://dxr.mozilla.org/mozilla-central/source/gfx/2d/SourceSurfaceD2D1.cpp#65 At this moment, Worker thread stops at [2] [2]: https://dxr.mozilla.org/mozilla-central/source/gfx/2d/SourceSurfaceD2D1.cpp#122 Since the code behavior from main and Worker are all come from CanvasRenderingContext2D, that's the reason I want to add mutex in there. But details to look into this problem, it might be more complexed than I throught when I proposed this patch. The thing is 1. Without fallback, Worker creates TextureClient and then makes a copy by UpdateTarget(). This copy only used by Compositer thread. Please see [3] The behavior has been ordered so it doesn't have any problem. Please correct me if I got anything wrong. [3]: https://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/CanvasClient.cpp#93 2. With fallback, main-thread may call snapshot to get SourceSurface at any time, even during the drawing PersistentBufferProvider in Worker. In this case, threading competition happens. 3. With fallback, even main-thread can get SourceSurface without any assertion happens, main-thread may get incomplete data because Commit() may not be called in Worker. Some possible ways to fix them are proposed below. 1. Both main-thread and compositor-thread are all read access the same TextureClient created in [3]. For this case, I am not sure I can get access to this TextureClient on main-thread. May I have your opinion if there there is any concern if I go go this way? 2. Also make a copy for main-thread to read. This way may consume performance and memory. If above two ways are all have concern to fix, may I have your suggestion? Really thanks.

Your question is probably better answered by Bas or Nical.

Just out of curiosity, how does the current code work with D2D? The D2D Moz2D backend isn't threadsafe and as far as I can tell this patch would also cause race conditions with the main thread on using the content D3D11 device.

Comment on attachment 8731159 [details] [diff] [review] part10-v1:Add mutex to solve threading save issue. I'm r- this until we have a resolution on what should be thread safe.

(In reply to Bas Schouten (:bas.schouten) from comment #128) > Just out of curiosity, how does the current code work with D2D? The D2D > Moz2D backend isn't threadsafe and as far as I can tell this patch would > also cause race conditions with the main thread on using the content D3D11 > device. Actually in my mochitest with my patch applied, I can reproduce it on Windows platform. Please see my part4 of my patches the detail about mochitest I implemented. The mochitest I ran is dom/canvas/test/test_offscreencanvas_dynamic_fallback.html. The attached file in this Comment records the call stack on both thread.

Hi Jeff, Bas, As mentioned in Comment 126 and Comment 130 ，I'd proposed a patch to fix it. The idea is make a copy for main-thread to read. Could you please have a review for me? Thanks.

Comment on attachment 8742205 [details] [diff] [review] part10-v1: Fix thread competition on dynamic fallback operations. r=jrmuizel, bas Review of attachment 8742205 [details] [diff] [review]: ----------------------------------------------------------------- r- for now as Moz2D cannot be used off main thread.

Comment on attachment 8742205 [details] [diff] [review] part10-v1: Fix thread competition on dynamic fallback operations. r=jrmuizel, bas Review of attachment 8742205 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/AsyncCanvasRenderer.cpp @@ +291,5 @@ > + return; > + } > + > + memcpy(map.GetData(), lockedBytes, map.GetStride() * mSurfaceForBasic->GetSize().height); > + } Why do we need to call? I saw GetSurface already had the code to mapped the mSurfaceForBasic. @@ +333,5 @@ > } else { > + if (mSurfaceForBasic) { > + RefPtr<gfx::DataSourceSurface> surfaceBasic; > + surfaceBasic = mSurfaceForBasic; > + return surfaceBasic.forget(); Could we merge the code with [1]? I don't want to see too many duplicated code. https://dxr.mozilla.org/mozilla-central/source/gfx/layers/AsyncCanvasRenderer.cpp#242

(In reply to Bas Schouten (:bas.schouten) from comment #132) > Comment on attachment 8742205 [details] [diff] [review] > part10-v1: Fix thread competition on dynamic fallback operations. > r=jrmuizel, bas > > Review of attachment 8742205 [details] [diff] [review]: > ----------------------------------------------------------------- > > r- for now as Moz2D cannot be used off main thread. May I have more questions to ask? 1. Does it means any canvas operations which are totally works on off the main thread, it still can't access to Moz2D? The part I don't understand is... in this case, there is only one thread accessing to Moz2D. It seems there is no thread competition in it. What part of concern it should have? 2. If the patches in this bug are landed with disabled by preference, do you think if it is ok? The reason why I want to say is if 2d canvas on Worker is actually needed in the future, at least we keep this feature disabled in mozilla-central. Once the thread-safe issues in Moz2D is fixed, this feature can then be enabled.

(In reply to Vincent Liu[:vliu] from comment #134) > (In reply to Bas Schouten (:bas.schouten) from comment #132) > > Comment on attachment 8742205 [details] [diff] [review] > > part10-v1: Fix thread competition on dynamic fallback operations. > > r=jrmuizel, bas > > > > Review of attachment 8742205 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > r- for now as Moz2D cannot be used off main thread. > > May I have more questions to ask? > > 1. Does it means any canvas operations which are totally works on off the > main thread, it still can't access to Moz2D? The part I don't understand > is... in this case, there is only one thread accessing to Moz2D. It seems > there is no thread competition in it. What part of concern it should have? There's no access on multiple threads for a single DrawTarget, but there is for example for a single backend. > 2. If the patches in this bug are landed with disabled by preference, do you > think if it is ok? > The reason why I want to say is if 2d canvas on Worker is actually needed > in the future, at least we keep this feature disabled in mozilla-central. > Once the thread-safe issues in Moz2D is fixed, this feature can then be > enabled. We could implement canvas on workers by proxying all the drawing calls to the main thread (although that would be a shame). I personally think we should be conservative with adding new features unless there's really compelling arguments and focus on improving stability and usability of Firefox at the moment though. Having said that, I'm fine with landing parts of these patches if they stand up to review, as long as they are truly preffed off and don't change anything that might cause stability issues in our current canvas code.

(In reply to Bas Schouten (:bas.schouten) from comment #135) > (In reply to Vincent Liu[:vliu] from comment #134) > > (In reply to Bas Schouten (:bas.schouten) from comment #132) > > > Comment on attachment 8742205 [details] [diff] [review] > > > part10-v1: Fix thread competition on dynamic fallback operations. > > > r=jrmuizel, bas > > > > > > Review of attachment 8742205 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > r- for now as Moz2D cannot be used off main thread. > > > > May I have more questions to ask? > > > > 1. Does it means any canvas operations which are totally works on off the > > main thread, it still can't access to Moz2D? The part I don't understand > > is... in this case, there is only one thread accessing to Moz2D. It seems > > there is no thread competition in it. What part of concern it should have? > > There's no access on multiple threads for a single DrawTarget, but there is > for example for a single backend. > Hi Bas, The main idea of part10 patch intends do the following. 1. Make a copy for main-thread use only every Commit() was called. 2. When fallback happens, return this copy to get the surface the last time Commit() was applied The creation of DataSourceSurface was called by gfx::Factory::CreateDataSourceSurfaceWithStride() and run memcpy() to make this copy. Based on this, I didn't operate it through DrawTarget. If you still have concern, please note it in Comment. Really thanks

Comment on attachment 8742205 [details] [diff] [review] part10-v1: Fix thread competition on dynamic fallback operations. r=jrmuizel, bas Add more comments to explain the part10 patch didn't share the drawTarget across threads based on previous. ># HG changeset patch ># User vincentliu <vliu@mozilla.com> > >Bug 801176 - part10-v1: Fix thread competition on dynamic fallback operations. r=jrmuizel, bas > >diff --git a/gfx/layers/AsyncCanvasRenderer.cpp b/gfx/layers/AsyncCanvasRenderer.cpp >index 090b6f7..3cf6672 100644 >--- a/gfx/layers/AsyncCanvasRenderer.cpp >+++ b/gfx/layers/AsyncCanvasRenderer.cpp >+ const gfx::IntSize& size = aTexture->GetSize(); >+ // This buffer would be used later for content rendering. So we choose >+ // B8G8R8A8 format here. >+ const gfx::SurfaceFormat format = gfx::SurfaceFormat::B8G8R8A8; >+ CreateCopiedDataSourceSurface(size, format); >+ if (mSurfaceForBasic) { >+ RefPtr<gfx::DataSourceSurface> dataSourceSurface = surface->GetDataSurface(); >+ if (!dataSourceSurface) { >+ return; >+ } >+ >+ const uint8_t* lockedBytes = dataSourceSurface->GetData(); >+ gfx::DataSourceSurface::ScopedMap map(mSurfaceForBasic, >+ gfx::DataSourceSurface::MapType::WRITE); >+ if (!map.IsMapped()) { >+ return; >+ } >+ >+ memcpy(map.GetData(), lockedBytes, map.GetStride() * mSurfaceForBasic->GetSize().height); >+ } 1. The creation of DataSourceSurface was called by CreateCopiedDataSourceSurface(). 2. run memcpy() to make this copy. It was done by every Commit() was called. >+ if (mSurfaceForBasic) { >+ RefPtr<gfx::DataSourceSurface> surfaceBasic; >+ surfaceBasic = mSurfaceForBasic; >+ return surfaceBasic.forget(); >+ } else { > return nullptr; > } When fallback happens, return this copy to get the surface the last time Commit() was applied

(In reply to Vincent Liu[:vliu] from comment #136) > (In reply to Bas Schouten (:bas.schouten) from comment #135) > > (In reply to Vincent Liu[:vliu] from comment #134) > > > (In reply to Bas Schouten (:bas.schouten) from comment #132) > > > > Comment on attachment 8742205 [details] [diff] [review] > > > > part10-v1: Fix thread competition on dynamic fallback operations. > > > > r=jrmuizel, bas > > > > > > > > Review of attachment 8742205 [details] [diff] [review]: > > > > ----------------------------------------------------------------- > > > > > > > > r- for now as Moz2D cannot be used off main thread. > > > > > > May I have more questions to ask? > > > > > > 1. Does it means any canvas operations which are totally works on off the > > > main thread, it still can't access to Moz2D? The part I don't understand > > > is... in this case, there is only one thread accessing to Moz2D. It seems > > > there is no thread competition in it. What part of concern it should have? > > > > There's no access on multiple threads for a single DrawTarget, but there is > > for example for a single backend. > > > > Hi Bas, > > The main idea of part10 patch intends do the following. > > 1. Make a copy for main-thread use only every Commit() was called. > 2. When fallback happens, return this copy to get the surface the last > time Commit() was applied > > The creation of DataSourceSurface was called by > gfx::Factory::CreateDataSourceSurfaceWithStride() and run memcpy() to make > this copy. Based on this, I didn't operate it through DrawTarget. > > If you still have concern, please note it in Comment. Really thanks I'm not sure what you're comment is saying here.. Moz2D calls are -not allowed- to be called off the main thread at the moment. That's as simple as it is. So, any Canvas call, like FillRect or anything of the likes -have- to be called -on the main thread-.

(In reply to Vincent Liu[:vliu] from comment #137) > Comment on attachment 8742205 [details] [diff] [review] > part10-v1: Fix thread competition on dynamic fallback operations. > r=jrmuizel, bas > > Add more comments to explain the part10 patch didn't share the drawTarget > across threads based on previous. So just to be clear. It -does not- matter whether a DrawTarget is or is -not- shared across multiple threads, a DrawTarget -cannot- be used -off the main thread- -ever-, currently.

(In reply to Bas Schouten (:bas.schouten) from comment #135) > Having said that, I'm fine with landing parts > of these patches if they stand up to review, as long as they are truly > preffed off and don't change anything that might cause stability issues in > our current canvas code. In current code base, there is a preference value gfx.offscreencanvas.enabled to enable/disable webgl on worker thread. [1]: https://dxr.mozilla.org/mozilla-central/source/dom/canvas/OffscreenCanvas.cpp#354 [2]: https://dxr.mozilla.org/mozilla-central/source/dom/canvas/OffscreenCanvas.cpp#342 Now the fefault value is disabled. From [1] and [2], it always return true for main-thread while false for Worker. In my part 1 patch, I also use this preference value to do do the same with webgl. But for the sentence you'd ever said parts of these patches, could you please have more explanation about this? I am not sure which parts of patches you want to tell me. Thanks. (In reply to Bas Schouten (:bas.schouten) from comment #138) > I'm not sure what you're comment is saying here.. Moz2D calls are -not > allowed- to be called off the main thread at the moment. That's as simple as > it is. So, any Canvas call, like FillRect or anything of the likes -have- to > be called -on the main thread-. I agree with your point. I believe landing these patches With preference disabled won't have any Moz2D calls off the main thread at this moment.

(In reply to Vincent Liu[:vliu] from comment #140) >... > > (In reply to Bas Schouten (:bas.schouten) from comment #138) > > I'm not sure what you're comment is saying here.. Moz2D calls are -not > > allowed- to be called off the main thread at the moment. That's as simple as > > it is. So, any Canvas call, like FillRect or anything of the likes -have- to > > be called -on the main thread-. > > I agree with your point. I believe landing these patches With preference > disabled won't have any Moz2D calls off the main thread at this moment. Wait - if these patches land, and I set gfx.offscreencanvas.enabled to true, I will get Moz2D calls off main thread? We don't want to do that. If the above meant "gfx.offscreencanvas.enabled is set to true or false, and we never get Moz2D calls off main thread", then no complaints.

Question regarding this feature on behalf of the team that is implementing it in Chrome: What is your plan for masking the methods of CanvasRenderingContext2D that have no business working with an OffscreenCanvas? Things like scrollPathIntoView, hit regions, drawFocusIfNeeded. The way we addressed this so far in Chrome was by creating a separate interface (OffscreenCanvasRenderingContext2D). We basically took the same approach as the Houdini CSS paint feature did with its "PaintRenderingContext2D" interface that is a subset of CRC2D. In the spec, the IDL for CRC2D was split into sub-interfaces specifically so that we could do this sort of mix and match approach. The alternative, I suppose, would be to throw InvalideStateError when invoking APIs that do not work with OffscreenCanvas. I realize we're kind of shooting in the dark by implementing before spec'ing... I intend to fix that soon, but I first wanted to gather your thoughts on this particular issue. Cheers!

(In reply to Milan Sreckovic [:milan] from comment #141) > > Wait - if these patches land, and I set gfx.offscreencanvas.enabled to true, > I will get Moz2D calls off main thread? Yes, you can see some Moz2D calls off main thread when gfx.offscreencanvas.enabled is true. > We don't want to do that. > Are you talking about the same limitation from Comment 139? What is the root cause that we couldn’t use draw target in off main thread? > If the above meant "gfx.offscreencanvas.enabled is set to true or false, and > we never get Moz2D calls off main thread", then no complaints. Do we have any work item to support off-main thread usage for Moz2D? It seems that off-main thread painting on content process will encounter it.

Bug 1264023 is a start.

(In reply to Vincent Liu[:vliu] from comment #143) > (In reply to Milan Sreckovic [:milan] from comment #141) > > > > Wait - if these patches land, and I set gfx.offscreencanvas.enabled to true, > > I will get Moz2D calls off main thread? > > Yes, you can see some Moz2D calls off main thread when > gfx.offscreencanvas.enabled is true. > > > We don't want to do that. > > > > Are you talking about the same limitation from Comment 139? What is the root > cause that we couldn’t use draw target in off main thread? Caches which aren't threadsafe and singletons and other such things. > > > If the above meant "gfx.offscreencanvas.enabled is set to true or false, and > > we never get Moz2D calls off main thread", then no complaints. > > Do we have any work item to support off-main thread usage for Moz2D? It > seems that off-main thread painting on content process will encounter it. That's a little different considering how off-main thread painting is implemented. But yes, some work would need to be done for that as well, most certainly.

Just tested Chrome OffscreenCanavas: it's about 3-4 times faster which is really promising. Please make this available in Firefox too.

Hey guys, I don't know if I should file a bug or what but when an OffscreenCanvas can't make a certain kind of context it's just suppose to return 'null' and continue but currently in Firefox if you call `OffscreenCanvas.getContext("2d")` instead of just returning null and continuing it instead just stops executing JavaScript completely. This makes it kind of hard to write code that checks for 2D support (which is currently working in Chrome) and that fails gracefully in firefox. Here's a repo https://jsfiddle.net/greggman/wcgcdh5h/ you'll see it just stops at `getContext` and never makes it to the next 2 `console.log` statements. It would be nice if you had time to make it turn NULL. Note: I recently trying getting TWGL running offscreen. https://twgljs.org/examples/offscreencanvas.html It's running in both Firefox (stable) and Chrome (stable) but in Firefox it is unable to load images. (using Fetch->Blob->ImageBitmap) There are visible no errors, the network tab shows the images were loaded but for some reason they don't make it into the textures. Should I file a bug?

As Jeff suggested, I'm adding my usecase that this would help: DevTools a11y panel has a colour contrast analyzer tool that uses canvas. To improve performance, it would be nice to use offscreen canvas to do as much canvas work as possible in the worker thread since it is only used to calculate contrast values based on the image data for a drawn window.

[removed]