Last Comment Bug 709490 - Run WebGL on Web Worker with commit()
: Run WebGL on Web Worker with commit()
Status: RESOLVED FIXED
webgl-next [games:p1][platform-rel-G...
: dev-doc-complete, devrel-needed
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: -- normal with 8 votes (vote)
: FxOS-S8 (02Oct)
Assigned To: Morris Tseng [:mtseng] [:Morris]
:
Mentors:
Depends on: omtc 924647 845545 1010371 1027073 1054576 1144906 1172405 1174043 1197713 1207887 1210321 1212724 1214571 1217191
Blocks: gecko-games 907637 B2G-Multicore foxeye 1187812 1203382 1172796 1180705
  Show dependency treegraph
 
Reported: 2011-12-10 10:37 PST by Benoit Girard (:BenWa)
Modified: 2016-08-18 11:06 PDT (History)
93 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
2.5+
fixed


Attachments
(WIP)WebGLinWorkers-step0 (14.68 KB, patch)
2013-10-21 03:46 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
(WIP)WebGLinWorkers-step1 (7.05 KB, patch)
2013-10-21 03:47 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
(WIP)WebGLinWorkers-step2 (33.87 KB, patch)
2013-10-21 03:48 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
workerCanvas-test.zip (1.43 KB, application/zip)
2013-10-21 03:49 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details
workerCanvas-complex.zip (12.08 KB, application/zip)
2013-10-21 06:23 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details
(WIP)WebGLinWorkers-step0 (39.79 KB, patch)
2013-11-06 01:46 PST, Morris Tseng [:mtseng] [:Morris]
nical.bugzilla: feedback+
Details | Diff | Splinter Review
(WIP)WebGLinWorkers-step1 (7.58 KB, patch)
2013-11-06 01:47 PST, Morris Tseng [:mtseng] [:Morris]
khuey: feedback+
Details | Diff | Splinter Review
(WIP)WebGLinWorkers-step2 (44.08 KB, patch)
2013-11-06 01:47 PST, Morris Tseng [:mtseng] [:Morris]
khuey: feedback+
Details | Diff | Splinter Review
workerCanvas-test.tar.bz2 (10.51 KB, application/x-bzip)
2013-11-06 01:47 PST, Morris Tseng [:mtseng] [:Morris]
no flags Details
(WIP)WebGLinWorkers-step0 (41.89 KB, patch)
2013-11-19 22:58 PST, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
(WIP)WebGLinWorkers-step1 (8.56 KB, patch)
2013-11-19 22:58 PST, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
(WIP)WebGLinWorkers-step1.5 (29.16 KB, patch)
2013-11-19 23:02 PST, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
(WIP)WebGLinWorkers-step2 (39.15 KB, patch)
2013-11-19 23:03 PST, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
WebGLinWorkers-step0 (41.59 KB, patch)
2013-11-26 18:03 PST, Morris Tseng [:mtseng] [:Morris]
nical.bugzilla: review+
Details | Diff | Splinter Review
WebGLinWorkers-step1.5 (29.17 KB, patch)
2013-11-26 18:05 PST, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
WebGLinWorkers-step2 (39.15 KB, patch)
2013-11-26 18:06 PST, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
WorkerCanvas-WIP-Part-1-OMT-canvas-with-ImageBridge.patch (24.58 KB, patch)
2014-06-12 17:58 PDT, Jon Morton [:jmorton]
no flags Details | Diff | Splinter Review
WorkerCanvas-WIP-Part-1-OMT-canvas-with-ImageBridge.patch (29.02 KB, patch)
2014-06-12 23:53 PDT, Jon Morton [:jmorton]
nical.bugzilla: review+
nical.bugzilla: feedback+
Details | Diff | Splinter Review
WorkerCanvas-WIP-Part-2-WebIDL.patch (9.34 KB, patch)
2014-06-13 00:19 PDT, Jon Morton [:jmorton]
no flags Details | Diff | Splinter Review
0001-WorkerCanvas-WIP-Part-1-OMT-canvas-with-ImageBridge.patch (32.11 KB, patch)
2014-06-24 18:33 PDT, Jon Morton [:jmorton]
no flags Details | Diff | Splinter Review
0001-WorkerCanvas-WIP-Part-1-OMT-canvas-with-ImageBridge.patch (32.11 KB, patch)
2014-06-24 18:38 PDT, Jon Morton [:jmorton]
no flags Details | Diff | Splinter Review
0002-WorkerCanvas-WIP-Part-2-WebIDL.patch (9.34 KB, patch)
2014-06-24 18:43 PDT, Jon Morton [:jmorton]
khuey: review+
Details | Diff | Splinter Review
0003-WorkerCanvas-WIP-Part-3.patch (60.59 KB, patch)
2014-06-24 18:49 PDT, Jon Morton [:jmorton]
no flags Details | Diff | Splinter Review
0001-WorkerCanvas-WIP-Part-1-OMT-canvas-with-ImageBridge.patch (34.64 KB, patch)
2014-08-05 16:18 PDT, Jon Morton [:jmorton]
nical.bugzilla: review-
Details | Diff | Splinter Review
0002-WorkerCanvas-WIP-Part-2-Implementation.patch (84.79 KB, patch)
2014-08-05 16:19 PDT, Jon Morton [:jmorton]
ehsan: review-
Details | Diff | Splinter Review
0003-WorkerCanvas-WIP-Part-3-Tests.patch (18.43 KB, patch)
2014-08-05 16:20 PDT, Jon Morton [:jmorton]
ehsan: review+
Details | Diff | Splinter Review
Interdiff (23.51 KB, patch)
2014-08-05 16:26 PDT, Jon Morton [:jmorton]
no flags Details | Diff | Splinter Review
1.patch (44.16 KB, patch)
2015-03-31 10:15 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
2.patch (95.24 KB, patch)
2015-03-31 10:22 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
WorkerCanvas: Part 1. (50.35 KB, patch)
2015-05-07 03:19 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
WorkerCanvas: Part 2. (99.93 KB, patch)
2015-05-07 03:19 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
WorkerCanvas: Part 1. (50.30 KB, patch)
2015-05-07 03:27 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
WorkerCanvas: Part 2. (99.99 KB, patch)
2015-05-07 21:02 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
WorkerCanvas: Part 2. (100.30 KB, patch)
2015-05-08 00:17 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
WorkerCanvas: WIP Part 3 - Tests (19.31 KB, patch)
2015-05-08 00:17 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 1: Let ImageBridge transfer CanvasClient async. (52.33 KB, patch)
2015-05-13 00:27 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 2: Introduce WorkerCanvas and let WebGL context work on workers. (91.40 KB, patch)
2015-05-13 00:28 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 3: Transfer WorkerCanvas from mainthread to workers. (11.00 KB, patch)
2015-05-13 00:28 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 4: Mochitests for workercanvas. (19.39 KB, patch)
2015-05-13 00:28 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Append WorkerCanvas to clonedObjects to prevent WorkerCanvas get CCed. (1.07 KB, patch)
2015-05-13 00:46 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 1: Let ImageBridge transfer CanvasClient async v2. (52.33 KB, patch)
2015-05-14 02:08 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 2: Introduce WorkerCanvas and let WebGL context work on workers v2. (92.59 KB, patch)
2015-05-14 02:09 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 3: Transfer WorkerCanvas from mainthread to workers v2. (17.46 KB, patch)
2015-05-14 02:10 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 4: Mochitests for workercanvas v2. (19.34 KB, patch)
2015-05-14 02:11 PDT, Morris Tseng [:mtseng] [:Morris]
ehsan: review-
Details | Diff | Splinter Review
Part 1: Let ImageBridge transfer CanvasClient async v3. (57.10 KB, patch)
2015-05-21 01:38 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 2: Introduce WorkerCanvas and let WebGL context work on workers v3. (93.05 KB, patch)
2015-05-21 01:38 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 3: Transfer WorkerCanvas from mainthread to workers v3. (17.38 KB, patch)
2015-05-21 01:39 PDT, Morris Tseng [:mtseng] [:Morris]
amarchesini: review+
sphink: review+
Details | Diff | Splinter Review
Part 3: Transfer WorkerCanvas from mainthread to workers v4. (carry r+: baku, sfink) (17.35 KB, patch)
2015-05-26 20:57 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 1: Let ImageBridge transfer CanvasClient async v4. (58.28 KB, patch)
2015-06-05 02:06 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 2: Introduce WorkerCanvas and let WebGL context work on workers v4. (94.67 KB, patch)
2015-06-05 02:06 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 1: Let ImageBridge transfer CanvasClient async v5. (57.47 KB, patch)
2015-06-08 02:37 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v5. (94.28 KB, patch)
2015-06-08 02:45 PDT, Morris Tseng [:mtseng] [:Morris]
ehsan: superreview-
Details | Diff | Splinter Review
Part 3: Transfer OffscreenCanvas from mainthread to workers v5. (carry r+: baku, sfink) (17.83 KB, patch)
2015-06-08 02:46 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 5: Add interfaces test. (4.07 KB, patch)
2015-06-08 23:32 PDT, Morris Tseng [:mtseng] [:Morris]
ehsan: review-
Details | Diff | Splinter Review
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v6. (94.31 KB, patch)
2015-06-08 23:35 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v7. (96.07 KB, patch)
2015-06-09 06:48 PDT, Morris Tseng [:mtseng] [:Morris]
ehsan: superreview-
Details | Diff | Splinter Review
Part 5: Add interfaces test v2. (4.37 KB, patch)
2015-06-10 01:00 PDT, Morris Tseng [:mtseng] [:Morris]
ehsan: review+
Details | Diff | Splinter Review
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v8. (97.38 KB, patch)
2015-06-10 01:01 PDT, Morris Tseng [:mtseng] [:Morris]
jgilbert: review-
Details | Diff | Splinter Review
Part 1: Let ImageBridge transfer CanvasClient async v6. (carry r+: nical) (56.89 KB, patch)
2015-06-30 02:02 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v9. (119.58 KB, patch)
2015-06-30 02:05 PDT, Morris Tseng [:mtseng] [:Morris]
jgilbert: review-
ehsan: superreview+
Details | Diff | Splinter Review
Part 3: Transfer OffscreenCanvas from mainthread to workers v6. (carry r+: sfink) (13.06 KB, patch)
2015-06-30 02:15 PDT, Morris Tseng [:mtseng] [:Morris]
amarchesini: review+
Details | Diff | Splinter Review
Part 1: Let ImageBridge transfer CanvasClient async v7. (56.02 KB, patch)
2015-07-15 02:02 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v10. (carry sr+: ehsan) (127.31 KB, patch)
2015-07-15 02:08 PDT, Morris Tseng [:mtseng] [:Morris]
jgilbert: review+
Details | Diff | Splinter Review
Part 1: Let ImageBridge transfer CanvasClient async v8. (55.03 KB, patch)
2015-07-20 02:45 PDT, Morris Tseng [:mtseng] [:Morris]
nical.bugzilla: review+
Details | Diff | Splinter Review
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v11. (carry r+: jgilbert) (125.18 KB, patch)
2015-07-20 02:49 PDT, Morris Tseng [:mtseng] [:Morris]
jmuizelaar: review-
nical.bugzilla: review+
jmuizelaar: review-
Details | Diff | Splinter Review
Part 6: Add frame ID to CanvasClient so compositor could update frame correctly. (4.37 KB, patch)
2015-07-20 23:43 PDT, Morris Tseng [:mtseng] [:Morris]
roc: review-
Details | Diff | Splinter Review
Part 6: Add frame ID to CanvasClient so compositor could update frame correctly v2. (6.02 KB, patch)
2015-07-21 20:14 PDT, Morris Tseng [:mtseng] [:Morris]
roc: review+
Details | Diff | Splinter Review
Part 6: Add frame ID to CanvasClient so compositor could update frame correctly v3. (carry r+: roc) (6.01 KB, patch)
2015-07-21 22:58 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 7: Always create a layer for offscreen canvas. (1.66 KB, patch)
2015-07-27 02:51 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 7: If layer is not available, fallback to BasicCanvasLayer. (19.08 KB, patch)
2015-07-28 02:47 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v12. (carry r+: jgilbert, nical) (123.19 KB, patch)
2015-07-28 20:45 PDT, Morris Tseng [:mtseng] [:Morris]
jmuizelaar: review-
Details | Diff | Splinter Review
Part 7: If layer is not available, fallback to BasicCanvasLayer v2. (22.60 KB, patch)
2015-07-29 02:04 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 8: Copy to a temp texture when readback from IOSurface. (2.27 KB, patch)
2015-08-18 02:45 PDT, Morris Tseng [:mtseng] [:Morris]
jgilbert: review+
Details | Diff | Splinter Review
Part 7: If layer is not available, fallback to BasicCanvasLayer v3. (41.52 KB, patch)
2015-08-19 00:48 PDT, Morris Tseng [:mtseng] [:Morris]
roc: review-
Details | Diff | Splinter Review
Part 7: If layer is not available, fallback to BasicCanvasLayer v4. (52.01 KB, patch)
2015-08-24 01:00 PDT, Morris Tseng [:mtseng] [:Morris]
roc: review-
Details | Diff | Splinter Review
Part 4 interdiff. (25.76 KB, patch)
2015-08-24 01:02 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 4: Mochitests for offscreencanvas. (25.17 KB, patch)
2015-08-24 01:09 PDT, Morris Tseng [:mtseng] [:Morris]
amarchesini: review+
Details | Diff | Splinter Review
part 7 interdiff. (26.96 KB, patch)
2015-08-26 02:54 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 7: If layer is not available, fallback to BasicCanvasLayer v5. (57.12 KB, patch)
2015-08-26 02:56 PDT, Morris Tseng [:mtseng] [:Morris]
roc: review-
Details | Diff | Splinter Review
Part 4: Mochitests for offscreencanvas v4. (carry r+: baku) (24.41 KB, patch)
2015-08-26 02:59 PDT, Morris Tseng [:mtseng] [:Morris]
jgilbert: review+
Details | Diff | Splinter Review
(WIP)Readback without blocking main thread. (4.03 KB, patch)
2015-08-27 03:16 PDT, Morris Tseng [:mtseng] [:Morris]
jgilbert: feedback-
Details | Diff | Splinter Review
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v13. (carry r+: jgilbert, nical) (122.49 KB, patch)
2015-09-01 20:34 PDT, Morris Tseng [:mtseng] [:Morris]
jmuizelaar: review+
Details | Diff | Splinter Review
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v14. (carry r+: jgilbert, nical, sr+: ehsan) (126.79 KB, patch)
2015-09-17 01:37 PDT, Morris Tseng [:mtseng] [:Morris]
jmuizelaar: review+
Details | Diff | Splinter Review
Part 3: Transfer OffscreenCanvas from mainthread to workers v7. (carry r+: sfink) (9.67 KB, patch)
2015-09-17 01:42 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 7: If layer is not available, fallback to BasicCanvasLayer v6. (61.99 KB, patch)
2015-09-17 01:48 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 9: Readback without blocking main thread. (24.30 KB, patch)
2015-09-17 01:51 PDT, Morris Tseng [:mtseng] [:Morris]
jgilbert: review-
Details | Diff | Splinter Review
Part 10: Using mechanism in RuntimeService to get pref in worker thread instead of gfxPref. (6.59 KB, patch)
2015-09-17 01:54 PDT, Morris Tseng [:mtseng] [:Morris]
amarchesini: review-
Details | Diff | Splinter Review
Part 10 interdiff. (2.25 KB, patch)
2015-09-17 02:13 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 3 interdiff. (3.70 KB, patch)
2015-09-17 02:13 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 3: Transfer OffscreenCanvas from mainthread to workers v8. (carry r+: sfink) (10.10 KB, patch)
2015-09-17 02:16 PDT, Morris Tseng [:mtseng] [:Morris]
amarchesini: review+
Details | Diff | Splinter Review
Part 10: Using mechanism in RuntimeService to get pref in worker thread instead of gfxPref v2. (7.83 KB, patch)
2015-09-17 02:18 PDT, Morris Tseng [:mtseng] [:Morris]
amarchesini: review+
Details | Diff | Splinter Review
Part 3: Transfer OffscreenCanvas from mainthread to workers v9. (carry r+: baku, sfink) (10.12 KB, patch)
2015-09-17 02:41 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 10: Using mechanism in RuntimeService to get pref in worker thread instead of gfxPref v3. (carry r+: baku) (8.40 KB, patch)
2015-09-17 02:43 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 4: Mochitests for offscreencanvas v5. (24.11 KB, patch)
2015-09-20 23:06 PDT, Morris Tseng [:mtseng] [:Morris]
amarchesini: review+
jgilbert: review+
Details | Diff | Splinter Review
Part 7: If layer is not available, fallback to BasicCanvasLayer v7. (60.55 KB, patch)
2015-09-20 23:27 PDT, Morris Tseng [:mtseng] [:Morris]
roc: review+
Details | Diff | Splinter Review
Part 9: Readback without blocking main thread v2. (35.99 KB, patch)
2015-09-20 23:28 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 9: Readback without blocking main thread v3. (36.12 KB, patch)
2015-09-21 01:40 PDT, Morris Tseng [:mtseng] [:Morris]
jgilbert: review-
Details | Diff | Splinter Review
Part 4: Mochitests for offscreencanvas v6. (carry r+: baku, jgilbert) (24.14 KB, patch)
2015-09-21 22:41 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 9: Readback without blocking main thread v4. (37.90 KB, patch)
2015-09-21 22:42 PDT, Morris Tseng [:mtseng] [:Morris]
jgilbert: review+
Details | Diff | Splinter Review
Part 11: Diabled test_offscreencanvas_many.html on gonk, android, windows and linux. (991 bytes, patch)
2015-09-22 03:12 PDT, Morris Tseng [:mtseng] [:Morris]
jgilbert: review+
Details | Diff | Splinter Review
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v15. (carry r+ jgilbert, nical, jrmuizel, sr+: ehsan) (127.84 KB, patch)
2015-09-23 19:33 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 9: Readback without blocking main thread v5. (carry r+: jgilbert) (37.96 KB, patch)
2015-09-23 19:34 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 1: Let ImageBridge transfer CanvasClient async v9. (carry r+: nical) (57.03 KB, patch)
2015-09-23 19:54 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 5: Add interfaces test v3. (carry r+: ehsan) (4.37 KB, patch)
2015-09-23 19:56 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 6: Add frame ID to CanvasClient so compositor could update frame correctly v4. (carry r+: roc) (6.06 KB, patch)
2015-09-23 19:58 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 7: If layer is not available, fallback to BasicCanvasLayer v8. (carry r+: roc) (60.59 KB, patch)
2015-09-23 20:00 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 1: Let ImageBridge transfer CanvasClient async v10. (carry r+: nical) (57.13 KB, patch)
2015-09-24 19:33 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 1: Let ImageBridge transfer CanvasClient async v11. (carry r+: nical) (57.14 KB, patch)
2015-09-24 20:30 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 1: Let ImageBridge transfer CanvasClient async v12. (carry r+: nical) (57.13 KB, patch)
2015-09-27 08:04 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v16. (carry r+: jgilbert, nical, jrmuizel, sr+: ehsan) (126.63 KB, patch)
2015-09-27 08:05 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v17. (carry r+: jgilbert, nical, jrmuizel, sr+: ehsan) (126.80 KB, patch)
2015-09-27 18:51 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review
Part 1: Let ImageBridge transfer CanvasClient async v13. (carry r+: nical) (57.13 KB, patch)
2015-09-28 02:18 PDT, Morris Tseng [:mtseng] [:Morris]
no flags Details | Diff | Splinter Review

Description Benoit Girard (:BenWa) 2011-12-10 10:37:30 PST
Currently we can only run WebGL from the main thread. This is bad for responsiveness.

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

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

== index.js ==

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

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

== async_webgl.js ==

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

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

My questions:
- Are there any fundamental limitations that I missed?
- What do you think?
Comment 1 Benoit Jacob [:bjacob] (mostly away) 2011-12-10 11:14:07 PST
The ability to run WebGL in a Web Worker has definitely been discussed before, but I can't remember the conclusions. Maybe search the public_webgl archives...
Comment 2 Benoit Girard (:BenWa) 2011-12-10 11:45:30 PST
Here is what I found from searching:
http://groups.google.com/group/webgl-dev-list/browse_thread/thread/05b5a68d31bd3d36?pli=1

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

Not really related:
http://www.khronos.org/webgl/public-mailing-list/archives/1104/msg00051.html
Comment 3 Benoit Girard (:BenWa) 2011-12-12 14:23:08 PST
Benoit Jacob told me that Vivien might have an opinion on this.

I'd like to discuss how complicated this is and what we think the win would be.
Comment 4 Joe Drew (not getting mail) 2011-12-12 15:35:41 PST
We currently don't support canvas in web workers, but we could (if we used threadsafe Cairo or our Azure canvas implemented on top of Skia). This is very analogous to that.
Comment 5 Benoit Girard (:BenWa) 2011-12-12 15:43:51 PST
Here's a bit of the low level explanation of how this could be implemented with async compositor. Note: We would need a fall back if async composition isn't supported.

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

var gl = glWorker.bind();
^== Opens an IPC connection with the compositor, creates a GLContext for that specific web worker thread, invalidates glWorker so that it can't be used again.
Comment 6 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-12-12 21:46:56 PST
We could allow something like this yeah. However:

1. The magic should happen in the postMessage. As soon as a window postMessages a
   context to a worker it should lose any ability to interact with it.
2. By "any ability" I also include the ability to read data from it, including
   calling .getImageData on the context itself, calling .toDataURL or calling
   .getContext. In theory asynchronous functions like toBlob could be ok though seems
   unadvisable since you could get a half-painted picture.
3. Changes to the canvas elements width/height wouldn't clear the canvas or change
   it's actual size.
4. The page would have no ability to synchronize main-thread animations happening
   through .requestAnimationFrame
5. We'd need some API to tell the context that the picture is "fully painted" and
   is ready to be displayed on the screen. Currently we implicitly do that by
   returning to the event loop.
Comment 7 Benoit Girard (:BenWa) 2011-12-12 22:27:47 PST
#1 Are there any reason why you think this would be better then explicitly binding?
#2 Umm, that's an interesting point. The other threads should never see a context so calling a method like getImageData should be impossible. I'm not sure how the other methods should fail (throw exceptions?)
#3 AFAIK the glViewport doesn't have to match the pixels width/height so this isn't a deal breaker on it own, but it should be easy for the implementer to handle this case so perhaps something extra is needed here.
#4 I don't think this is a problem. If you request an async context that's just the price you have to pay. This is also a problem with resize. This is a problem we face in general with async composition.
#5 I think this will also happen when the web worker returns to it's IPC event loop, but it's hard to say without having written any code for that.
Comment 8 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-12-12 23:22:23 PST
(In reply to Benoit Girard (:BenWa) from comment #7)
> #1 Are there any reason why you think this would be better then explicitly
> binding?

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

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

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

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

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

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

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

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

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

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

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

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

You make a good point here. Game likes to run in a 'game loop'. Although I don't think this is possible with web workers because they need to yield to get new messages?
Comment 10 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-12-13 00:30:24 PST
Yes, you currently need to yield to get message, however there are proposals to change that so I wouldn't rely on that being the case forever.
Comment 11 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-12-13 09:23:36 PST
(In reply to Benoit Girard (:BenWa) from comment #3)
> Benoit Jacob told me that Vivien might have an opinion on this.
> 
> I'd like to discuss how complicated this is and what we think the win would
> be.

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

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

On Pdf.js every page is a canvas, and to save some memory pages are rendered only when they are visible. This doesn't fit well with smooth panning since when the canvas is draw, panning stops.
Comment 12 Benoit Jacob [:bjacob] (mostly away) 2011-12-13 10:22:33 PST
(In reply to Benoit Girard (:BenWa) from comment #3)
> Benoit Jacob told me that Vivien might have an opinion on this.
> 
> I'd like to discuss how complicated this is and what we think the win would
> be.

I meant Cedric Vivier !!!
Comment 13 Benoit Girard (:BenWa) 2011-12-13 10:51:37 PST
I was confused, I double checked on IRC. Clearly a miss communication.
Comment 14 Benoit Jacob [:bjacob] (mostly away) 2011-12-13 11:08:53 PST
(In reply to Benoit Girard (:BenWa) from comment #13)
> I was confused, I double checked on IRC. Clearly a miss communication.

Yeah I remember now when you asked but I thought you only wanted to check the spelling in Vivien's name, I didn't make the connection with this bug.
Comment 15 Cedric Vivier [:cedricv] 2011-12-15 08:18:51 PST
I'm not sure this is really a win for Javascript developers. Makes WebGL development even harder than it already is imho.

Could we implement out-of-process WebGL rendering on our side only without JS developers having to render in workers to get better performance.
That's one thing Chromium is doing great afaik (WebGL rendering is done in GPU processes).
Comment 16 Jeff Gilbert [:jgilbert] 2011-12-15 08:55:26 PST
I'll add that to keep the web workers from running wild with rendering, we should (at some point) also give them access to a SwapBuffers() like function.

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

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

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

Coupled with a zero-copy mechanism for transferring pixel buffers from worker to worker, this would allow users to implement double (or triple) buffering.
Comment 18 Benoit Girard (:BenWa) 2011-12-24 17:24:08 PST
(In reply to Patrick Walton (:pcwalton) from comment #17)

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

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

Don't we already have these problems without off main thread compositing regardless?
Comment 19 Patrick Walton (:pcwalton) 2011-12-24 20:12:55 PST
(In reply to Benoit Girard (:BenWa) from comment #18)
> (In reply to Patrick Walton (:pcwalton) from comment #17)
> 
> If I understand your suggestion then you need to coordinate with the main
> thread for rendering. The key idea of my proposal is to allow rendering
> without hitting the main thread (after setup). This is important for
> responsiveness because rendering doesn't block the main thread and the main
> thread doesn't block rendering. Perhaps you can elaborate more on the
> advantages.

Well, you would have to synchronize with the main thread, but the part on the main thread could be very small: just a pointer swap to swap buffers.
Comment 20 Benoit Girard (:BenWa) 2011-12-24 20:20:43 PST
(In reply to Patrick Walton (:pcwalton) from comment #19)
> Well, you would have to synchronize with the main thread, but the part on
> the main thread could be very small: just a pointer swap to swap buffers.

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

Hmm, I see, I guess it doesn't work if other tabs are executing long-running scripts. Ignore me then.
Comment 22 Benoit Girard (:BenWa) 2011-12-24 20:42:28 PST
I think being able to pass pixel buffers between workers and the main thread could be useful so I'm not trying to say it's a bad idea. But I think its use case is independent from my suggestions.
Comment 23 Jeff Gilbert [:jgilbert] 2011-12-27 16:58:16 PST
It seems that for most cases, this doesn't win us much, since we're still resolving on the compositing thread (which will eventually be non-main-thread, but not yet). However, this should help with more complicated rendering, specifically any multi-pass rendering that has dependent passes.

Ideally, we could just do all the graphics stuff on the main JS thread (and put all the real computation in webworkers. Really, similar to how it's done in Java), but since that's also main-thread for us, it's not really viable. (At least that's my understanding) While getting off-main-thread JS (Electrolysis?) would obviate this need, it would still be nice to have the choice, so it's probably a good idea even in the long run.
Comment 24 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-27 18:55:30 PST
I think the ability to use WebGL (and 2D-canvas) from Workers would be really useful. Apart from GPU flushing issues, it's really the only way for Web developers to get reliable frame rates in the face of unpredictable main-thread delays. It would also be useful for real-time video processing with ProcessedMediaStream.

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

Unfortunately adding APIs to Workers currently requires writing raw JSAPI. So making more APIs like WebGL available from Workers is prohibitively painful right now. Need to wait until we have a WebIDL bindings framework in place that can work both on the main thread and in Workers, and is fast.
Comment 25 Benoit Girard (:BenWa) 2011-12-27 19:02:46 PST
(In reply to Jeff Gilbert [:jgilbert] from comment #23)
> It seems that for most cases, this doesn't win us much, since we're still
> resolving on the compositing thread (which will eventually be
> non-main-thread, but not yet). However, this should help with more
> complicated rendering, specifically any multi-pass rendering that has
> dependent passes.
> 

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

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

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

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

That's sad. Does someone have the bug number for this change so we can make it block this?
Comment 26 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-27 20:27:04 PST
Although I think this feature would be useful, I don't expect many developers would use it since it would be quite hard to write code to do all rendering from a Worker. So I don't think you should see this as any kind of performance solution for OMTC. You should probably have a separate bug tracking optimizations of GPU flushing for main-thread WebGL and OMTC.
Comment 27 Dan Mosedale (:dmose) 2012-03-21 10:04:59 PDT
At GDC, one of the (non-Google) speakers claimed that the Chrome team was working this, though they hadn't announced it yet.  I have no insight into whether that's actually true.
Comment 28 Jeff Gilbert [:jgilbert] 2012-03-21 10:28:59 PDT
(In reply to Dan Mosedale (:dmose) from comment #27)
> At GDC, one of the (non-Google) speakers claimed that the Chrome team was
> working this, though they hadn't announced it yet.  I have no insight into
> whether that's actually true.

WebGL in workers was an important TODO at the last Khronos F2F, so all WebGL implementors are working towards WebGL in workers.
Comment 29 Benoit Girard (:BenWa) 2012-03-21 10:39:54 PDT
I think it's important to have a solution to lets WebGL post directly to the compositor rather then simply have the worker offload computation.
Comment 30 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-21 15:39:34 PDT
Yes.

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

There are a couple of standards-related issues here. Due to the fact that we can't use DOM elements in a worker, the "canvas" attribute can't work (it could just return null I guess), and texImage2D_dom and texSubImage2D_dom would not be usable. Also we need an API to actually pass control of a WebGL canvas to a worker. I'm not sure what that should look like.
Comment 31 Benoit Girard (:BenWa) 2012-03-21 15:46:34 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30)
I'm not sure what that should look like.

What do you think of my proposal in Comment 0?
Comment 32 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-21 16:22:15 PDT
Oops, sorry.

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

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

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

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

For frame rate control, we will need some new API to tell the worker when composition has happened and to allow the worker to swap buffers.
Comment 33 Jeff Gilbert [:jgilbert] 2012-03-21 16:50:13 PDT
I don't see any real reason resizing couldn't be async. We're already talking about lazy buffer allocation in another bug, as right now changing the width, then the height yields two resize operations, likely with large buffers on the GPU.
Comment 34 Boris Zbarsky [:bz] 2012-03-21 18:12:15 PDT
> The new-DOM-bindings team's next project is going to be canvas-2d but probably could be
> WebGL instead.

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

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

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

That said, the memory management in workers is somewhat different from main-thread.  I do think we want to make it possible to use the same object in both, though...
Comment 35 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-21 18:45:02 PDT
How are you handling that with XHR?
Comment 36 Boris Zbarsky [:bz] 2012-03-21 19:10:25 PDT
The worker XHR impl is completely different (since it has to proxy all the necko access to the main thread, for example)...
Comment 37 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-21 20:01:29 PDT
To be clear, the worker XHR implementation creates an nsXMLHttpRequest on the main thread, and proxies calls and events between the threads.
Comment 38 :Ms2ger (⌚ UTC+1/+2) 2012-03-22 12:02:23 PDT
I'd appreciate it if this was brought up on whatwg@whatwg.org...
Comment 39 (dormant account) 2012-04-30 15:12:36 PDT
pwalton suggested that we need an OMTC tab strip implemented in canvas if we are to have a responsive tab strip. I'm told that even though this this bug is about WebGL, it applies equally to 2d canvas.
Comment 40 Patrick Walton (:pcwalton) 2012-04-30 15:13:31 PDT
I don't think it's the only way to have a responsive tab strip, but it's one I think is worth investigating.
Comment 41 Joe Drew (not getting mail) 2012-04-30 15:40:01 PDT
This is off-topic for this bug, but please CC me on the bug where we have gathered the data showing where we're spending our time while painting the tab strip. It seems like it'd be better to make the tab strip cheap to paint.
Comment 42 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-04-30 15:54:17 PDT
(In reply to Joe Drew (:JOEDREW!) from comment #41)

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

bug 593680 covers general tab strip slowness.

We also need ability to make the tab strip async(ie to make the throbber, tab animations not jank during page load), which seems on topic for this bug
Comment 44 Benoit Girard (:BenWa) 2012-04-30 17:19:31 PDT
(In reply to Taras Glek (:taras) from comment #43)
> 
> We also need ability to make the tab strip async(ie to make the throbber,
> tab animations not jank during page load), which seems on topic for this bug

It's not, please don't scope creep this bug otherwise it wont get done. This bug is about a suggestion to extended WebGL. If this feature is useful for something else feel free to make it block this bug.
Comment 45 Benoit Jacob [:bjacob] (mostly away) 2012-04-30 18:22:02 PDT
Talking of getting this done: BenWa, when you get a chance, could you draft a spec proposal? I don't see any other next step to make progress here.
Comment 46 Benoit Girard (:BenWa) 2012-04-30 18:52:06 PDT
(In reply to Benoit Jacob [:bjacob] from comment #45)
> Talking of getting this done: BenWa, when you get a chance, could you draft
> a spec proposal? I don't see any other next step to make progress here.

I wont have time until Fennec ships, after that I'll see what my priorities are. Earliest for me would be Q3.
Comment 47 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-08-04 09:12:35 PDT
I will probably be driving this.
Comment 48 David Bruant 2013-08-05 04:15:21 PDT
I don't see it mentioned in this bug, so I'll share it.
CanvasProxy http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#canvasproxy
Something very close to what is described here. Note that it's a transferable.

On a side note, there is more and more discussion about making process-isolated iframes (Servo, Blink, different standard mailing-lists). If iframes could be process-isolated, I begin to wonder what the benefits of WebGL in worker really are (since you can do WebGL *and lots of other things* in an iframe).
Maybe a bug isn't the right place to discuss this. I'm happy to join if someone starts a thread on the topic (I'm not sure which ML would be mosy appropriate)
Comment 49 Benoit Jacob [:bjacob] (mostly away) 2013-08-05 06:36:22 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #47)
> I will probably be driving this.

\o/

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

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

The other aspect of this work is going to be hooking up with the compositor; let us know when you get there.
Comment 50 Benoit Jacob [:bjacob] (mostly away) 2013-08-05 06:38:31 PDT
Oh, and the big documentation comments about WebGL reference counting (why objects have *2* refcounts) in WebGLObjectModel.h is recommended reading.
Comment 51 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-08-05 06:45:42 PDT
(In reply to David Bruant from comment #48)
> I don't see it mentioned in this bug, so I'll share it.
> CanvasProxy
> http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-
> element.html#canvasproxy
> Something very close to what is described here. Note that it's a
> transferable.
> 
> On a side note, there is more and more discussion about making
> process-isolated iframes (Servo, Blink, different standard mailing-lists).
> If iframes could be process-isolated, I begin to wonder what the benefits of
> WebGL in worker really are (since you can do WebGL *and lots of other
> things* in an iframe).
> Maybe a bug isn't the right place to discuss this. I'm happy to join if
> someone starts a thread on the topic (I'm not sure which ML would be mosy
> appropriate)

CanvasProxy is not a very good design.  We're going to have an alternate proposal.
Comment 52 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-08-05 06:56:00 PDT
(In reply to David Bruant from comment #48)
> On a side note, there is more and more discussion about making
> process-isolated iframes (Servo, Blink, different standard mailing-lists).
> If iframes could be process-isolated, I begin to wonder what the benefits of
> WebGL in worker really are (since you can do WebGL *and lots of other
> things* in an iframe).
> Maybe a bug isn't the right place to discuss this. I'm happy to join if
> someone starts a thread on the topic (I'm not sure which ML would be mosy
> appropriate)

The benefit is that we can do off main thread WebGL in the next few months.  Process isolated iframes in Gecko are likely years away.
Comment 53 Boris Zbarsky [:bz] 2013-08-05 08:28:19 PDT
> On a side note, there is more and more discussion about making process-isolated iframes

Those are an implementation detail, not a spec requirement.
Comment 54 David Bruant 2013-08-05 08:32:22 PDT
(In reply to Boris Zbarsky (:bz) from comment #53)
> > On a side note, there is more and more discussion about making process-isolated iframes
> 
> Those are an implementation detail, not a spec requirement.
I know, I know, but I was wondering whether the work of this bug wasn't largely overlapping with the work of process-isolating iframes.
Kyle answered my concern (few months VS years).
Comment 55 Jeff Gilbert [:jgilbert] 2013-08-05 12:28:25 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #51)
> CanvasProxy is not a very good design.  We're going to have an alternate
> proposal.

I didn't catch this first time around, but there's already a draft for this kicking around the WebGL WG. IIRC, we had a pretty good design for this already, though it naturally diverges from the CanvasProxy spec.
Comment 56 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-08-05 13:11:01 PDT
Yeah, I know.  I've been talking to vlad about all of this ;-)
Comment 57 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-08-06 22:09:40 PDT
Is this new design written down anywhere?
Comment 58 Morris Tseng [:mtseng] [:Morris] 2013-10-21 03:46:15 PDT
Created attachment 819664 [details] [diff] [review]
(WIP)WebGLinWorkers-step0

This patch add function to allow off-main-thread canvas send ipc message to composition thread by using ImageBridge
Comment 59 Morris Tseng [:mtseng] [:Morris] 2013-10-21 03:47:34 PDT
Created attachment 819665 [details] [diff] [review]
(WIP)WebGLinWorkers-step1

this patch create webidl interfaces for WorkerCanvas
Comment 60 Morris Tseng [:mtseng] [:Morris] 2013-10-21 03:48:31 PDT
Created attachment 819666 [details] [diff] [review]
(WIP)WebGLinWorkers-step2

glue up all codes and let WebGL in worker works
Comment 61 Morris Tseng [:mtseng] [:Morris] 2013-10-21 03:49:21 PDT
Created attachment 819667 [details]
workerCanvas-test.zip

example for webgl in workers
Comment 62 Nicolas Silva [:nical] 2013-10-21 04:25:39 PDT
(In reply to Morris Tseng [:mtseng] from comment #58)
> Created attachment 819664 [details] [diff] [review]
> (WIP)WebGLinWorkers-step0
> 
> This patch add function to allow off-main-thread canvas send ipc message to
> composition thread by using ImageBridge

Awesome, I gave it a look and it looks good to me.
Comment 63 Jeff Gilbert [:jgilbert] 2013-10-21 05:11:54 PDT
Comment on attachment 819666 [details] [diff] [review]
(WIP)WebGLinWorkers-step2

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

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

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

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

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

Why do we not call this a CanvasProxy, as the current spec does?
Comment 64 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-10-21 05:16:47 PDT
There's a big thread on whatwg about this with a new spec proposal, summarized at https://wiki.mozilla.org/User:Roc/WorkerCanvasProposal.
Comment 65 Morris Tseng [:mtseng] [:Morris] 2013-10-21 05:46:42 PDT
(In reply to Jeff Gilbert [:jgilbert] from comment #63)
> Comment on attachment 819666 [details] [diff] [review]
> (WIP)WebGLinWorkers-step2
> 
> Review of attachment 819666 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Before finishing this up, we should have a spec document for how this is
> supposed to work. It's great to see a WIP, though.
> 
> ::: content/canvas/src/WebGLContext.cpp
> @@ +862,5 @@
> > +        WorkerCanvas* canvas = userdata->mContent;
> > +        WebGLContext* context = static_cast<WebGLContext*>(canvas->GetContext());
> > +
> > +        // Present our screenbuffer, if needed.
> > +        context->PresentScreenBuffer();
> 
> We don't want to (and, I think, can't) do this for workers. Workers will
> need to explicitly call `commit` in order to Present frames.

Yes, the commmit would call Present frames. Above code will be removed.
Comment 66 Morris Tseng [:mtseng] [:Morris] 2013-10-21 06:23:47 PDT
Created attachment 819719 [details]
workerCanvas-complex.zip

more complex demo use webgl in workers. the code are modified version of below link's example: http://learningwebgl.com/blog/?p=370. So the result should be as same as the link provided
Comment 67 Morris Tseng [:mtseng] [:Morris] 2013-11-06 01:46:38 PST
Created attachment 827917 [details] [diff] [review]
(WIP)WebGLinWorkers-step0
Comment 68 Morris Tseng [:mtseng] [:Morris] 2013-11-06 01:47:03 PST
Created attachment 827919 [details] [diff] [review]
(WIP)WebGLinWorkers-step1
Comment 69 Morris Tseng [:mtseng] [:Morris] 2013-11-06 01:47:20 PST
Created attachment 827920 [details] [diff] [review]
(WIP)WebGLinWorkers-step2
Comment 70 Morris Tseng [:mtseng] [:Morris] 2013-11-06 01:47:47 PST
Created attachment 827921 [details]
workerCanvas-test.tar.bz2
Comment 71 Morris Tseng [:mtseng] [:Morris] 2013-11-06 01:52:12 PST
Rebase patches to latest mozilla-central codebase and most of features are completed.
Comment 72 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-11-11 23:24:29 PST
Comment on attachment 827917 [details] [diff] [review]
(WIP)WebGLinWorkers-step0

nical, can you take a look at this for us?
Comment 73 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-11-11 23:25:21 PST
Comment on attachment 827919 [details] [diff] [review]
(WIP)WebGLinWorkers-step1

Setting f?me so I don't forget to look at these.
Comment 74 Nicolas Silva [:nical] 2013-11-12 01:54:11 PST
Comment on attachment 827917 [details] [diff] [review]
(WIP)WebGLinWorkers-step0

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

Overall looks good.

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

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

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

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

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

I'd like to see some class documentation.

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

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

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

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

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

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

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

> ::: gfx/layers/ipc/ImageBridgeParent.cpp
> @@ +95,5 @@
> >  ImageBridgeParent::RecvUpdateNoSwap(const EditArray& aEdits)
> >  {
> >    InfallibleTArray<EditReply> noReplies;
> >    bool success = RecvUpdate(aEdits, &noReplies);
> > +  //NS_ABORT_IF_FALSE(noReplies.Length() == 0, "RecvUpdateNoSwap requires a sync Update to carry Edits");
> 
> The final patch should not have this commented out.
Comment 76 Nicolas Silva [:nical] 2013-11-13 02:53:40 PST
(In reply to Morris Tseng [:mtseng] from comment #75)
> OffMainThread case use async version of SurfaceStream which is waiting for
> compositor taking the produced frame. In the WorkerThread case, sometimes
> Compositor thread will go away but not worker thread (e.g. If we switch to
> another tab and switch back). If we use async version of SurfaceStream in
> this case, WorkerThread is going to enter a deadlock because it will wait
> for compositor forever. In order to avoid deadlock, I use non-wait version
> of SurfaceStream.

Ok, makes sense. Please add a comment in SurfaceStream::ChooseGLStreamType to explain this. Because this function reads like this: "if we are doing OMTC do A, else do B" and the worker thread scenario can also be doing OMTC so we need to make it apparent here that we consciously choose the "non-OMTC" branch even though we may be dong off-main-thread compositing.
Comment 77 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-11-13 23:52:49 PST
Some general comments:

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

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

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

80 character lines everywhere please.
Comment 78 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-11-13 23:59:49 PST
Comment on attachment 827919 [details] [diff] [review]
(WIP)WebGLinWorkers-step1

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

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

nit: extra newline

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

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

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

aRv.

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

Here too.

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

And in the header.

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

You can remove the comment from the code generator.

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

We decided to remove this.
Comment 79 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-11-14 00:01:17 PST
Comment on attachment 827920 [details] [diff] [review]
(WIP)WebGLinWorkers-step2

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

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

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

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

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

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

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

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

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

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

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

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

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

80 character lines please.

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

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

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

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

I duplicate lots of code from HTMLCanvasElemnt class(e.g GetContext(), ToBlob() and other utility functions). I think I should move those codes to CanvasUtils.h as well. Any opinions?
Comment 81 Vladimir Vukicevic [:vlad] [:vladv] 2013-11-18 09:18:02 PST
CanvasUtils or a base class?  It seems like you really just want an abstract base class here for both of these, even if it just has static helpers for now if that's simpler.  I'd support doing a base class shared by both of the canvas element-ish impls.
Comment 82 Morris Tseng [:mtseng] [:Morris] 2013-11-19 22:58:02 PST
Created attachment 8335128 [details] [diff] [review]
(WIP)WebGLinWorkers-step0

update patch to address comment
Comment 83 Morris Tseng [:mtseng] [:Morris] 2013-11-19 22:58:31 PST
Created attachment 8335129 [details] [diff] [review]
(WIP)WebGLinWorkers-step1

update patch to address comment
Comment 84 Morris Tseng [:mtseng] [:Morris] 2013-11-19 23:02:56 PST
Created attachment 8335131 [details] [diff] [review]
(WIP)WebGLinWorkers-step1.5

Move common functions which are related to CanvasRenderingContext to a base class.
Comment 85 Morris Tseng [:mtseng] [:Morris] 2013-11-19 23:03:34 PST
Created attachment 8335132 [details] [diff] [review]
(WIP)WebGLinWorkers-step2

update patch to address comment
Comment 86 Nicolas Silva [:nical] 2013-11-25 02:59:04 PST
Comment on attachment 8335128 [details] [diff] [review]
(WIP)WebGLinWorkers-step0

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

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

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

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

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

nit: MOZ_COUNT_CTOR here and MOZ_COUNT_DTOR in the destructor below.

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

In what cases would we hit this assertion? Looks like we should keep it and fix what's causing us to generate replies in a async transactions.
Comment 87 Morris Tseng [:mtseng] [:Morris] 2013-11-26 18:03:10 PST
Created attachment 8338983 [details] [diff] [review]
WebGLinWorkers-step0

I updated patch to address comment. And also put the assertion code in ImageBridgeParent back.
Comment 88 Morris Tseng [:mtseng] [:Morris] 2013-11-26 18:05:35 PST
Created attachment 8338984 [details] [diff] [review]
WebGLinWorkers-step1.5

Because patch step0 change, I have to change some dependency codes.
Comment 89 Morris Tseng [:mtseng] [:Morris] 2013-11-26 18:06:00 PST
Created attachment 8338985 [details] [diff] [review]
WebGLinWorkers-step2

Because patch step0 change, I have to change some dependency codes.
Comment 90 Till Schneidereit [:till] 2014-02-13 12:21:40 PST
Morris, what's the status on this? The Shumway project would be very interested in having this.
Comment 91 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2014-02-13 14:07:27 PST
The patch needs to be unbitrotted and reviewed.  Unfortunately reviewer bandwidth (i.e. me) has been very constrained :(
Comment 92 Jon Morton [:jmorton] 2014-06-12 17:58:46 PDT
Created attachment 8439616 [details] [diff] [review]
WorkerCanvas-WIP-Part-1-OMT-canvas-with-ImageBridge.patch

Unbitrotted version of mtseng's part0.
Most of it is essentially same, except for CanvasClientSurfaceStream::Update*.
Comment 93 Jon Morton [:jmorton] 2014-06-12 23:53:43 PDT
Created attachment 8439738 [details] [diff] [review]
WorkerCanvas-WIP-Part-1-OMT-canvas-with-ImageBridge.patch

Oops! Patch was missing new AsyncCanvasRenderer files, here's a new version
Comment 94 Jon Morton [:jmorton] 2014-06-13 00:19:57 PDT
Created attachment 8439740 [details] [diff] [review]
WorkerCanvas-WIP-Part-2-WebIDL.patch

Unbitrotted version of mtseng's part 1 - WebIDL skeleton.
Mostly unchanged except for some binding updates.
Comment 95 Nicolas Silva [:nical] 2014-06-16 05:06:09 PDT
Comment on attachment 8439738 [details] [diff] [review]
WorkerCanvas-WIP-Part-1-OMT-canvas-with-ImageBridge.patch

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

This is a big patch so It'll take time for me to review it. So far it looks good.
Comment 96 Nicolas Silva [:nical] 2014-06-18 07:40:52 PDT
Comment on attachment 8439738 [details] [diff] [review]
WorkerCanvas-WIP-Part-1-OMT-canvas-with-ImageBridge.patch

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

Note that this needs to block on ImageBridge being activated by default on all platforms. This should be the case soon (depends on whether linux OMTC will have async video by default or not).
Comment 97 Jeff Gilbert [:jgilbert] 2014-06-18 16:10:55 PDT
Comment on attachment 8439738 [details] [diff] [review]
WorkerCanvas-WIP-Part-1-OMT-canvas-with-ImageBridge.patch

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

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

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

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

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

All in all, ouch. Try something closer to:
static void
CreateCanvasClientSync(ReentrantMonitor* const barrier,
                       CanvasClient::CanvasClientType type,
                       TextureFlags flag,
                       RefPtr<CanvasClient>* const out_result,
                       bool* const out_done)
Comment 98 Jon Morton [:jmorton] 2014-06-23 14:48:04 PDT
(In reply to Jeff Gilbert [:jgilbert] from comment #97)
> Comment on attachment 8439738 [details] [diff] [review]
> WorkerCanvas-WIP-Part-1-OMT-canvas-with-ImageBridge.patch
> 
> Review of attachment 8439738 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm going the cherry-pick this function to illustrate some style issues.
> I'm not going to r- it, since it's not my files, but I'd like to point this
> out. :)
> 

Ouch indeed... this part was copied from the original patch, which appears to be copied from the existing CreateImageClientSync. I should have noticed and fixed this before, though. I'll update this and the original (and any other bad style I can find) for the new patch -- thanks!
Comment 99 Jon Morton [:jmorton] 2014-06-24 18:33:45 PDT
Created attachment 8445609 [details] [diff] [review]
0001-WorkerCanvas-WIP-Part-1-OMT-canvas-with-ImageBridge.patch
Comment 100 Jon Morton [:jmorton] 2014-06-24 18:38:07 PDT
Created attachment 8445610 [details] [diff] [review]
0001-WorkerCanvas-WIP-Part-1-OMT-canvas-with-ImageBridge.patch

Updated Part 1
 * Fix code style issues
 * Fix crash on canvas cleanup
 * Resolve conflicts in changes on m-c to CanvasClientSurfaceStream
Comment 101 Jon Morton [:jmorton] 2014-06-24 18:43:04 PDT
Created attachment 8445611 [details] [diff] [review]
0002-WorkerCanvas-WIP-Part-2-WebIDL.patch

Updated Part 2 - FileCallback? -> FileCallback in WorkerCanvas.webidl so it's the same as HTMLCanvasElement
Comment 102 Jon Morton [:jmorton] 2014-06-24 18:49:12 PDT
Created attachment 8445617 [details] [diff] [review]
0003-WorkerCanvas-WIP-Part-3.patch

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

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

Tests and examples I've been using are at https://github.com/Jonanin/WorkerCanvasTests
Comment 103 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2014-07-01 16:57:19 PDT
Comment on attachment 8445611 [details] [diff] [review]
0002-WorkerCanvas-WIP-Part-2-WebIDL.patch

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

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

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

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

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

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

toBlob is unimplemented for now, so just leave that off and we can add it back later.
Comment 104 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2014-07-03 10:16:21 PDT
Comment on attachment 8445617 [details] [diff] [review]
0003-WorkerCanvas-WIP-Part-3.patch

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

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

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

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

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

mCanvasClient should be a smart pointer

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

whitespace at EOL
Comment 105 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2014-07-03 10:30:51 PDT
Also, you should add something in all of the commit messages here crediting Morris Tseng for writing the original patches.
Comment 106 Jon Morton [:jmorton] 2014-08-05 16:18:25 PDT
Created attachment 8468112 [details] [diff] [review]
0001-WorkerCanvas-WIP-Part-1-OMT-canvas-with-ImageBridge.patch

Updated Part 1 - Notable change is in ImageBridgeChild::FlushCanvasClientTexture
Comment 107 Jon Morton [:jmorton] 2014-08-05 16:19:46 PDT
Created attachment 8468113 [details] [diff] [review]
0002-WorkerCanvas-WIP-Part-2-Implementation.patch
Comment 108 Jon Morton [:jmorton] 2014-08-05 16:20:10 PDT
Created attachment 8468114 [details] [diff] [review]
0003-WorkerCanvas-WIP-Part-3-Tests.patch
Comment 109 Jon Morton [:jmorton] 2014-08-05 16:26:05 PDT
Created attachment 8468115 [details] [diff] [review]
Interdiff

Most changes since last patch set, which includes:
 - Several fixes for crashes with WorkerCanvas and ImageBridgeChild shutdown
 - Support for WebGLContext.canvas returning WorkerCanvas
 - Removed lots of Main-thread-only stubs in WebGLContext
 - (Not in this interdiff, oops) move all WebGLContext pref access to gfxPrefs to support OMT access
 - (Not in this interdiff, oops) Add WorkerCanvasObserver (nsIThreadObserver) to manage committing invalidated canvas frames
 - Misc changes/fixes during rebase to m-c.
Comment 110 Jon Morton [:jmorton] 2014-08-05 16:31:04 PDT
I have also been working to port the WebGL conformance suite to work in Web Workers, which I will be attaching soon. At this point, almost all of the tests pass, with these exceptions:
 - HTMLCanvasElement features not implemented in WorkerCanvas (toDataURL, toBlob, texImage2D)
 - Tests that depend on certain DOM manipulations
 - Extra tests in the more/ directory that depend on an entirely different testing library that I haven't ported
Comment 111 Nicolas Silva [:nical] 2014-08-06 04:58:36 PDT
Comment on attachment 8468112 [details] [diff] [review]
0001-WorkerCanvas-WIP-Part-1-OMT-canvas-with-ImageBridge.patch

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

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

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

MOZ_OVERRIDE

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

MOZ_OVERRIDE

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

nit: trailing space

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

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

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

nit: trailing spaces
Comment 112 :Ehsan Akhgari 2014-08-12 14:57:08 PDT
I'm still struggling to do these reviews today...
Comment 113 :Ehsan Akhgari 2014-08-13 14:15:24 PDT
Comment on attachment 8468113 [details] [diff] [review]
0002-WorkerCanvas-WIP-Part-2-Implementation.patch

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

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

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

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

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

Here too.

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

Here too.

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

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

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

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

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

Nit: please remove this.

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

What makes these silent early returns OK?

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

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

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

Shouldn't you do this after the mInvalidated check?

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

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

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

Does this not break losing the WebGL context on workers?

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

Ditto.

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

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

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

This too.

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

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

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

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

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

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

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

The destructor doesn't need to be virtual.

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

Nit: explicit.

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

Nit: make this non-virtual please.

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

Shouldn't this return the worker global or something?

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

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

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

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

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

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

Nit: const.

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

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

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

Nit: const.

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

Nit: please reorder the boolean members for better packing.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Nit: please use nsRefPtr.
Comment 114 :Ehsan Akhgari 2014-08-13 14:27:27 PDT
Comment on attachment 8468114 [details] [diff] [review]
0003-WorkerCanvas-WIP-Part-3-Tests.patch

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

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

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

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

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

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

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

How can this happen?

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

/me pretends he knows WebGL...

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

Please define functions like one normally would.

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

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

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

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

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

Ditto.
Comment 115 Till Schneidereit [:till] 2014-09-18 11:14:58 PDT
Not relevant to shumway anymore
Comment 116 Nicolas Silva [:nical] 2014-09-23 15:13:11 PDT
Comment on attachment 8468112 [details] [diff] [review]
0001-WorkerCanvas-WIP-Part-1-OMT-canvas-with-ImageBridge.patch

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

Marking r- because there are a few details to change and I want bugzilla to stop nagging me. This is not far from being landable though.
Comment 117 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2015-02-27 11:01:40 PST
Comment on attachment 8468113 [details] [diff] [review]
0002-WorkerCanvas-WIP-Part-2-Implementation.patch

These are pretty bitrotted at this point, unfortunately.  I think baku is going to try to dust them off.
Comment 118 Andrea Marchesini [:baku] 2015-03-31 10:15:11 PDT
Created attachment 8586204 [details] [diff] [review]
1.patch
Comment 119 Andrea Marchesini [:baku] 2015-03-31 10:22:15 PDT
Created attachment 8586208 [details] [diff] [review]
2.patch

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

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

A feedback about how to proceed would be great! Thanks.
Comment 120 Jukka Jylänki 2015-04-09 07:21:15 PDT
Here's some thoughts from Emscripten perspective. In Emscripten, moving execution to workers is forced mainly by these:
   a. main thread can't run a blocking main loop, and it may not be possible to port the codebase to run an async main loop. (king.com as an example)
   b. synchronous file IO from an IndexedDB (or sync XHR for that matter) is not accessible in main thread, and it may not be possible to port the codebase to run async file IO (UE 4 as an example).
   c. SharedArrayBuffer won't most likely be accessible to main thread, but only to workers (any native applications that uses multiple threads).
   d. some native applications use GL in a multithreaded manner, e.g. have a dedicated GL rendering thread, and/or streaming GL resource loading thread.

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

Application A: fully single-threaded application:

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


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

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

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

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

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

Application B: multithreaded GL usage:

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

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

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

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

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

How do these requirements 1.-3. look to you guys from the perspective of the current patches?
Comment 121 Kenneth Russell 2015-04-09 16:28:14 PDT
May I comment on the feasibility of these requirements from the API standpoint rather than just Firefox's implementation and patches in progress?

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

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

In the meantime, if we just consider the problem of a single thread in the worker group pushing OpenGL frames to the browser's compositor, https://wiki.whatwg.org/wiki/OffscreenCanvas is a proposal to enable that. Please comment on the ongoing discussion "Canvas image to blob/dataurl within Worker" on whatwg@whatwg.org.
Comment 122 Luke Wagner [:luke] 2015-04-09 19:55:53 PDT
(In reply to Jukka Jylänki from comment #120)
Whoaa now, I don't think we need to be allowing rendering from-multiple threads for this feature to be massively useful.  Other than the issue of async resource creation (which it seems like we can optimize separately w/in current API of WebGL to a degree), aren't most C++ apps keeping all their GL activity on a single thread anyway?  Also, as Kenneth says, we have a much bigger issue with sharing Web API resources between threads, one that we'll likely need to address directly in the future --- but only  after spending some time to better understand the problem and collect use cases.  For now, it seems like we have a quite workable solution if we can only allow a single worker to access a given webgl canvas.

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

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

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

That's right. I was merely asking what how the current patches look in terms of items 1. - 3. . Indeed item 2. is the most important one, since the prospect of being able to have applications run their own blocking main loop would enable porting much more applications. Multithreaded GL context access is much more rare.
Comment 124 Kenneth Russell 2015-04-10 11:49:21 PDT
(In reply to Luke Wagner [:luke] from comment #122)
> 
> The only only tweak I was hoping to discuss on this bug (and if it's not
> crazy, take it to the broader WHATWG discussion) which prompted me to poke
> Jukka to comment is: rather than requiring workers to return to their event
> loop to render the next frame, could we provide a blocking (worker-only) API
> to present() so that the app can maintain its existing event-loop structure.
> This has shown up repeatedly over the years as one of the big reasons apps
> have trouble porting with Emscripten (vim being another example (one near to
> my heart ;) having apparently many event loops scattered around).

Hopefully that need has been captured in one API proposal being discussed on the WHATWG mailing list: please see https://wiki.whatwg.org/wiki/OffscreenCanvas . Both the 2D and WebGL contexts will add a "commit" method which will allow explicit presentation of a frame into an HTMLCanvasElement, and that method can be used both from workers and from the main thread. However, your point is a good one that it needs to block so that if the worker doesn't return control to its event loop, that it doesn't run uncontrollably quickly. I can see that that behavior could be implemented as a heuristic inside the browser but should it be made explicit in the proposal? I'm guessing it should.
Comment 125 Luke Wagner [:luke] 2015-04-10 15:48:01 PDT
(In reply to Kenneth Russell from comment #124)
Ah hah, I had been looking at https://wiki.whatwg.org/wiki/WorkerCanvas.

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

Making it explicit would also address the limitation bullet: "A known good way to drive an animation loop from a worker is needed. requestAnimationFrame or a similar API needs to be defined on worker threads."
Comment 126 Morris Tseng [:mtseng] [:Morris] 2015-04-30 06:28:25 PDT
I rebased those patches today and hope we can run it normally. But I failed :( I always hit this crash [1] when I transfer canvas to worker. Kyle, do you have any idea about this?

[1]: https://dxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.cpp#920
Comment 127 Morris Tseng [:mtseng] [:Morris] 2015-04-30 06:32:28 PDT
BTW, here is callstack.

  * frame #0: 0x0000000104b1f595 XUL`(anonymous namespace)::Wrap(cx=0x0000000139aee340, existing=JS::HandleObject at 0x000000013dacb2a8, obj=JS::HandleObject at 0x000000013dacb2a0) + 117 at RuntimeService.cpp:920
    frame #1: 0x00000001076443af XUL`JSCompartment::wrap(this=0x000000013c2c4800, cx=0x0000000139aee340, obj=JS::MutableHandleObject at 0x000000013dacb5f0, existingArg=JS::HandleObject at 0x000000013dacb5e8) + 3119 at jscompartment.cpp:447
    frame #2: 0x0000000106fbee43 XUL`JSCompartment::wrap(this=0x000000013c2c4800, cx=0x0000000139aee340, vp=JS::MutableHandleValue at 0x000000013dacb7a0, existing=JS::HandleObject at 0x000000013dacb798) + 1011 at jscompartmentinlines.h:117
    frame #3: 0x0000000107644595 XUL`JS_WrapValue(cx=0x0000000139aee340, vp=JS::MutableHandleValue at 0x000000013dacb808) + 117 at jsapi.cpp:1014
    frame #4: 0x0000000103b75cfa XUL`bool mozilla::dom::binding_detail::DoGetOrCreateDOMReflector<mozilla::dom::WorkerCanvas, (mozilla::dom::binding_detail::GetOrCreateReflectorWrapBehavior)0>(cx=0x0000000139aee340, value=0x000000013c234dc0, rval=MutableHandle<JS::Value> at 0x000000013dacb8a0) + 666 at BindingUtils.h:978
    frame #5: 0x0000000103b7592d XUL`bool mozilla::dom::GetOrCreateDOMReflector<mozilla::dom::WorkerCanvas>(cx=0x0000000139aee340, value=0x000000013c234dc0, rval=MutableHandle<JS::Value> at 0x000000013dacb8d8) + 45 at BindingUtils.h:997
    frame #6: 0x0000000104b80b6a XUL`(anonymous namespace)::WorkerStructuredCloneCallbacks::ReadTransfer(aCx=0x0000000139aee340, aReader=0x000000013dacbd10, aTag=4294934544, aData=0x000000013c234dc0, aExtraData=0, aClosure=0x0000000000000000, aReturnObject=MutableHandle<JSObject *> at 0x000000013dacb980) + 170 at WorkerPrivate.cpp:699
    frame #7: 0x0000000107148c05 XUL`JSStructuredCloneReader::readTransferMap(this=0x000000013dacbd10) + 1285 at StructuredClone.cpp:1783
    frame #8: 0x0000000107141a06 XUL`JSStructuredCloneReader::read(this=0x000000013dacbd10, vp=JS::MutableHandleValue at 0x000000013dacbc90) + 38 at StructuredClone.cpp:1818
    frame #9: 0x0000000107141949 XUL`ReadStructuredClone(cx=0x0000000139aee340, data=0x00000001399ecd00, nbytes=112, vp=JS::MutableHandleValue at 0x000000013dacbd08, cb=0x000000010a5bf030, cbClosure=0x0000000000000000) + 169 at StructuredClone.cpp:377
    frame #10: 0x0000000107149091 XUL`JS_ReadStructuredClone(cx=0x0000000139aee340, buf=0x00000001399ecd00, nbytes=112, version=5, vp=JS::MutableHandleValue at 0x000000013dacbec0, optionalCallbacks=0x000000010a5bf030, closure=0x0000000000000000) + 209 at StructuredClone.cpp:1895
    frame #11: 0x00000001071496cc XUL`JSAutoStructuredCloneBuffer::read(this=0x00000001399ecdb0, cx=0x0000000139aee340, vp=JS::MutableHandleValue at 0x000000013dacbf28, optionalCallbacks=0x000000010a5bf030, closure=0x0000000000000000) + 268 at StructuredClone.cpp:2053
    frame #12: 0x0000000104b85001 XUL`(anonymous namespace)::MessageEventRunnable::DispatchDOMEvent(this=0x00000001399ecd80, aCx=0x0000000139aee340, aWorkerPrivate=0x0000000136aaa800, aTarget=0x0000000130579a00, aIsMainThread=false) + 225 at WorkerPrivate.cpp:1257
    frame #13: 0x0000000104b84f00 XUL`(anonymous namespace)::MessageEventRunnable::WorkerRun(this=0x00000001399ecd80, aCx=0x0000000139aee340, aWorkerPrivate=0x0000000136aaa800) + 768 at WorkerPrivate.cpp:1336
    frame #14: 0x0000000104b66069 XUL`mozilla::dom::workers::WorkerRunnable::Run(this=0x00000001399ecd80) + 1849 at WorkerRunnable.cpp:356
    frame #15: 0x0000000101708a2a XUL`nsThread::ProcessNextEvent(this=0x0000000130882960, aMayWait=false, aResult=0x000000013dacc5fe) + 2042 at nsThread.cpp:868
    frame #16: 0x0000000101763ec7 XUL`NS_ProcessNextEvent(aThread=0x0000000130882960, aMayWait=false) + 151 at nsThreadUtils.cpp:265
    frame #17: 0x0000000104b591b1 XUL`mozilla::dom::workers::WorkerPrivate::DoRunLoop(this=0x0000000136aaa800, aCx=0x0000000139aee340) + 2161 at WorkerPrivate.cpp:5271
Comment 128 Jeff Gilbert [:jgilbert] 2015-04-30 09:13:34 PDT
We're retargeting this bug for just the commit() component of WebGL+Workers.
There will be a follow-up bug for the explicit buffer management part.
Comment 129 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2015-05-04 14:16:21 PDT
(In reply to Morris Tseng [:mtseng] from comment #126)
> I rebased those patches today and hope we can run it normally. But I failed
> :( I always hit this crash [1] when I transfer canvas to worker. Kyle, do
> you have any idea about this?
> 
> [1]:
> https://dxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.
> cpp#920

Not offhand.  Do you see this with Andrea's patches?  Or something else?
Comment 130 Morris Tseng [:mtseng] [:Morris] 2015-05-04 19:10:59 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #129)
> Not offhand.  Do you see this with Andrea's patches?  Or something else?
I compare with both Andrea and Jon's patches. But there is no big difference in the worker stuff. The MOZ_CRASH code added in bug 1092102 which is related to worker debug things. So I guess bug 1092102 is assume Wrap function is only called from debugger compartment to debuggee compartment or vice versa. But in the worker canvas case, we actually want to wrap a WorkerCanvas object from main thread to worker thread. So it hits this MOZ_CRASH. I try to ignore the MOZ_CRASH for testing, but it will crash elsewhere. I'll keep digging into this!
Comment 131 Morris Tseng [:mtseng] [:Morris] 2015-05-05 22:47:03 PDT
Ah, I see there is tiny difference causes this issue. Andrea's version shares the same WorkerCanvas on main thread and worker but Jon's version uses cloned WorkerCanvas on worker. So, it would crash if we share the same WorkerCanvas. With this modification, WorkerCanvas is able to transfer to worker without error. Next, I'll fix graphics related code to make it works again!
Comment 132 Andrea Marchesini [:baku] 2015-05-06 10:40:05 PDT
My patches are not ready at all. I just tried to rebase the existing code but I think I'll go for a full rewrite.
Comment 133 Morris Tseng [:mtseng] [:Morris] 2015-05-06 18:44:43 PDT
(In reply to Andrea Marchesini (:baku) from comment #132)
> My patches are not ready at all. I just tried to rebase the existing code
> but I think I'll go for a full rewrite.
I have a workable patches now. I'll upload it asap.
Comment 134 Morris Tseng [:mtseng] [:Morris] 2015-05-07 03:19:21 PDT
Created attachment 8602661 [details] [diff] [review]
WorkerCanvas: Part 1.
Comment 135 Morris Tseng [:mtseng] [:Morris] 2015-05-07 03:19:41 PDT
Created attachment 8602662 [details] [diff] [review]
WorkerCanvas: Part 2.
Comment 136 Morris Tseng [:mtseng] [:Morris] 2015-05-07 03:23:34 PDT
Those patches are rebased to latest m-c and can run the demo. I also addressed most reviewer's comment above. But in this patch, the result image would be upside down. I'll fix this problem.
Comment 137 Morris Tseng [:mtseng] [:Morris] 2015-05-07 03:27:01 PDT
Created attachment 8602669 [details] [diff] [review]
WorkerCanvas: Part 1.

Remove debug printf.
Comment 138 Andrea Marchesini [:baku] 2015-05-07 03:31:48 PDT
Comment on attachment 8602662 [details] [diff] [review]
WorkerCanvas: Part 2.

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

First of all, thanks! Just a comment about WorkerPrivate.

::: dom/workers/WorkerPrivate.cpp
@@ +711,5 @@
> +           void** aContent, uint64_t* aExtraData)
> +  {
> +    WorkerCanvas* canvas = nullptr;
> +    nsresult rv = UNWRAP_OBJECT(WorkerCanvas, aObj, canvas);
> +    if (NS_SUCCEEDED(rv) && canvas && !canvas->IsNeutered()) {

What does it keep alive the canvas obj in the transfer? WorkerCanvas is a CCed obj.
What about this:

1. you call CloneForWorker() in the Transfer and you store the pointer: *aContent = clonedCanvas.forget();
2. in the read you use the aData directly and you wrap it.
3. Add a freeTransfer() method and unref *aData.
Comment 139 Morris Tseng [:mtseng] [:Morris] 2015-05-07 20:36:33 PDT
(In reply to Andrea Marchesini (:baku) from comment #138)
> Comment on attachment 8602662 [details] [diff] [review]
> WorkerCanvas: Part 2.
> 
> Review of attachment 8602662 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> First of all, thanks! Just a comment about WorkerPrivate.
> 
> ::: dom/workers/WorkerPrivate.cpp
> @@ +711,5 @@
> > +           void** aContent, uint64_t* aExtraData)
> > +  {
> > +    WorkerCanvas* canvas = nullptr;
> > +    nsresult rv = UNWRAP_OBJECT(WorkerCanvas, aObj, canvas);
> > +    if (NS_SUCCEEDED(rv) && canvas && !canvas->IsNeutered()) {
> 
> What does it keep alive the canvas obj in the transfer? WorkerCanvas is a
> CCed obj.
> What about this:
> 
> 1. you call CloneForWorker() in the Transfer and you store the pointer:
> *aContent = clonedCanvas.forget();
WorkerCanvas is inherited from EventTarget. So it is not thread-safe. If I clone it in mainthread and send it to worker, then I'll hit crash in worker thread. Therefore, I have to clone it in worker thread.

> 2. in the read you use the aData directly and you wrap it.
> 3. Add a freeTransfer() method and unref *aData.
Comment 140 Morris Tseng [:mtseng] [:Morris] 2015-05-07 21:02:02 PDT
Created attachment 8603142 [details] [diff] [review]
WorkerCanvas: Part 2.

Fix upside down problem.
Comment 141 Andrea Marchesini [:baku] 2015-05-07 23:38:15 PDT
> WorkerCanvas is inherited from EventTarget. So it is not thread-safe. If I
> clone it in mainthread and send it to worker, then I'll hit crash in worker
> thread. Therefore, I have to clone it in worker thread.

Correct.
The probably the easier solution is to add the WorkerCanvas into the cloneObjects array in the aClosure.
What I'm trying to point out is that WorkerCanvas has to kept alive for the duration of the transfer.
CC/GC can run in the main-thread, dropping the reference to the WorkerCanvas object before we do the read.
Comment 142 Morris Tseng [:mtseng] [:Morris] 2015-05-08 00:17:27 PDT
Created attachment 8603196 [details] [diff] [review]
WorkerCanvas: Part 2.

Setting attributes throw more exception when workercanvas is transferred to worker.
Comment 143 Morris Tseng [:mtseng] [:Morris] 2015-05-08 00:17:56 PDT
Created attachment 8603197 [details] [diff] [review]
WorkerCanvas: WIP Part 3 - Tests

Addressed reviewer's comment.
Comment 144 Morris Tseng [:mtseng] [:Morris] 2015-05-08 00:23:39 PDT
> Correct.
> The probably the easier solution is to add the WorkerCanvas into the
> cloneObjects array in the aClosure.
> What I'm trying to point out is that WorkerCanvas has to kept alive for the
> duration of the transfer.
> CC/GC can run in the main-thread, dropping the reference to the WorkerCanvas
> object before we do the read.
Ok! I'll address this problem in my next patch. Thanks for your advice :)
Comment 145 Morris Tseng [:mtseng] [:Morris] 2015-05-08 00:26:14 PDT
Comment on attachment 8603197 [details] [diff] [review]
WorkerCanvas: WIP Part 3 - Tests

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

::: dom/canvas/test/mochitest.ini
@@ +255,5 @@
> +tags = workercanvas
> +[test_workercanvas_sizechange.html]
> +tags = workercanvas
> +[test_workercanvas_subworker.html]
> +tags = workercanvas

I add "workercanvas" tag to those tests. So we can test all workercanvas tests by typing following command.

./mach mochitest-plain --tag workercanvas

Right now we'll leak many things when tests are over. I'll fix this problem later as well.
Comment 146 Andrea Marchesini [:baku] 2015-05-11 03:35:23 PDT
:mtseng, let me know if I can help you in any way.
I can review the DOM part and/or to fight against some memory leak.
Comment 147 Morris Tseng [:mtseng] [:Morris] 2015-05-13 00:27:03 PDT
Created attachment 8605091 [details] [diff] [review]
Part 1: Let ImageBridge transfer CanvasClient async.
Comment 148 Morris Tseng [:mtseng] [:Morris] 2015-05-13 00:28:09 PDT
Created attachment 8605092 [details] [diff] [review]
Part 2: Introduce WorkerCanvas and let WebGL context work on workers.
Comment 149 Morris Tseng [:mtseng] [:Morris] 2015-05-13 00:28:33 PDT
Created attachment 8605093 [details] [diff] [review]
Part 3: Transfer WorkerCanvas from mainthread to workers.
Comment 150 Morris Tseng [:mtseng] [:Morris] 2015-05-13 00:28:54 PDT
Created attachment 8605094 [details] [diff] [review]
Part 4: Mochitests for workercanvas.
Comment 151 Morris Tseng [:mtseng] [:Morris] 2015-05-13 00:35:17 PDT
I split patch to 4 parts so that it should be easier to review.

:baku, I tried to append WorkerCanvas into cloneObjects. But I hit crash as well because worker trying to clone "cloneObjects". Like previous solution I tried, WorkerCanvas is not thread-safe object. So it crashes again. I'll append call-stack and patch later.
Comment 152 Morris Tseng [:mtseng] [:Morris] 2015-05-13 00:46:51 PDT
Created attachment 8605102 [details] [diff] [review]
Append WorkerCanvas to clonedObjects to prevent WorkerCanvas get CCed.
Comment 153 Morris Tseng [:mtseng] [:Morris] 2015-05-13 00:47:36 PDT
Hit MOZ_CRASH(DOMEventTargetHelper not thread-safe) at /Users/Morris/mozilla/gecko-dev/dom/events/DOMEventTargetHelper.cpp:77
  * frame #0: 0x000000010420746a XUL`mozilla::DOMEventTargetHelper::Release(this=0x0000000133def2c0) + 218 at DOMEventTargetHelper.cpp:76
    frame #1: 0x00000001041667ef XUL`mozilla::dom::WorkerCanvas::Release(this=0x0000000133def2c0) + 31 at WorkerCanvas.cpp:28
    frame #2: 0x0000000101654fe5 XUL`nsCOMPtr_base::~nsCOMPtr_base(this=0x000000012a6b4e28) + 85 at nsCOMPtr.h:296
    frame #3: 0x0000000101654f85 XUL`nsCOMPtr<nsISupports>::~nsCOMPtr(this=0x000000012a6b4e28) + 21 at nsCOMPtr.h:744
    frame #4: 0x000000010163c695 XUL`nsCOMPtr<nsISupports>::~nsCOMPtr(this=0x000000012a6b4e28) + 21 at nsCOMPtr.h:744
    frame #5: 0x0000000101649055 XUL`nsTArrayElementTraits<nsCOMPtr<nsISupports> >::Destruct(aE=0x000000012a6b4e28) + 21 at nsTArray.h:518
    frame #6: 0x0000000101648ff6 XUL`nsTArray_Impl<nsCOMPtr<nsISupports>, nsTArrayInfallibleAllocator>::DestructRange(this=0x0000000145a72038, aStart=0, aCount=1) + 86 at nsTArray.h:1756
    frame #7: 0x0000000101648f5a XUL`nsTArray_Impl<nsCOMPtr<nsISupports>, nsTArrayInfallibleAllocator>::RemoveElementsAt(this=0x0000000145a72038, aStart=0, aCount=1) + 378 at nsTArray.h:1449
    frame #8: 0x000000010165827f XUL`nsTArray_Impl<nsCOMPtr<nsISupports>, nsTArrayInfallibleAllocator>::Clear(this=0x0000000145a72038) + 47 at nsTArray.h:1458
    frame #9: 0x0000000101658239 XUL`nsTArray_Impl<nsCOMPtr<nsISupports>, nsTArrayInfallibleAllocator>::~nsTArray_Impl(this=0x0000000145a72038) + 25 at nsTArray.h:819
    frame #10: 0x0000000101658215 XUL`nsTArray<nsCOMPtr<nsISupports> >::~nsTArray(this=0x0000000145a72038) + 21 at nsTArray.h:1830
    frame #11: 0x000000010163a815 XUL`nsTArray<nsCOMPtr<nsISupports> >::~nsTArray(this=0x0000000145a72038) + 21 at nsTArray.h:1830
    frame #12: 0x0000000104be3cc6 XUL`(anonymous namespace)::MessageEventRunnable::DispatchDOMEvent(this=0x0000000126e8f680, aCx=0x00000001415e6ba0, aWorkerPrivate=0x000000011c533000, aTarget=0x000000010033ab60, aIsMainThread=false) + 1030 at WorkerPrivate.cpp:1276
    frame #13: 0x0000000104be38a0 XUL`(anonymous namespace)::MessageEventRunnable::WorkerRun(this=0x0000000126e8f680, aCx=0x00000001415e6ba0, aWorkerPrivate=0x000000011c533000) + 768 at WorkerPrivate.cpp:1323
    frame #14: 0x0000000104bc63b9 XUL`mozilla::dom::workers::WorkerRunnable::Run(this=0x0000000126e8f680) + 1849 at WorkerRunnable.cpp:357
Comment 154 Nicolas Silva [:nical] 2015-05-13 05:26:11 PDT
Comment on attachment 8605091 [details] [diff] [review]
Part 1: Let ImageBridge transfer CanvasClient async.

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

::: gfx/layers/CopyableCanvasLayer.cpp
@@ +80,5 @@
>  
>  void
>  CopyableCanvasLayer::UpdateTarget(DrawTarget* aDestTarget)
>  {
> +  if (!mDrawTarget || !mGLContext) {

I assume you meant &&?

Otherwise everything below the branch "if (mDrawTarget) {" a few lines later becomes dead code.
Comment 155 Andrea Marchesini [:baku] 2015-05-13 06:49:57 PDT
Comment on attachment 8605092 [details] [diff] [review]
Part 2: Introduce WorkerCanvas and let WebGL context work on workers.

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

::: dom/canvas/WorkerCanvas.cpp
@@ +173,5 @@
> +  }
> +  return;
> +}
> +
> +already_AddRefed<WorkerCanvas>

Just to avoid thread refcounting issues and live-time problems, what about this:

struct WorkerCanvasCloneData
{
  WorkerCanvasCloneData(AsyncWorkerCanvasRenderer* aRenderer, uint32_t aWidth, int32_t aHeight,) // opaque too
    : mRenderer(aRenderer)
    , mWidth(aWidth)
    , mHeight(aHeight)
    , mOpaque(aOpaque)
  {}

  nsRefPtr<AsyncCanvasREnderer> mCanvasRenderer;
  uint32_t mWidth;
  uint32_t mHeight;
  foo mOpaque;
};

WorkerCanvasCloneData*
WorkerCanvas::ToCloneData()
{
  return new WorkerCanvasCloneData(mCanvasRednerer, mWidth, mHeight, mOpaque);
}

/* static */ already_AddRefed<WorkerCanvas>
WorkerCanvas::CreateFromCloneData(WorkercanvasCloneData* aData)
{
  MOZ_ASSERT(aData);
  nsRefPtr<WorkerCanvas> wc = new WorkerCanvas(aData->mWidth, aData->mHeigth, aData->mOpaque, aData->mRenderer);
  return wc.forget();
}

This comment was needed for the review of patch 3.
Comment 156 Andrea Marchesini [:baku] 2015-05-13 07:39:58 PDT
Comment on attachment 8605093 [details] [diff] [review]
Part 3: Transfer WorkerCanvas from mainthread to workers.

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

::: dom/workers/WorkerCanvasObserver.cpp
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/dom/WorkerCanvasObserver.h"

Usually, if the header file is in the same directory, we just do: #include "WorkerCanvasObserver.h"

@@ +21,5 @@
> +}
> +
> +void
> +WorkerCanvasObserver::InvalidateWorkerCanvas(WorkerCanvas* aWorkerCanvas)
> +{

MOZ_ASSERT(aWorkerCanvas);

::: dom/workers/WorkerCanvasObserver.h
@@ +6,5 @@
> +
> +#ifndef mozilla_dom_workers_WorkerCanvasObserver_h
> +#define mozilla_dom_workers_WorkerCanvasObserver_h
> +
> +#include "Workers.h"

do you need this include?

@@ +8,5 @@
> +#define mozilla_dom_workers_WorkerCanvasObserver_h
> +
> +#include "Workers.h"
> +#include "nsIThreadInternal.h"
> +#include "nsTArray.h"

alphabetic order.

::: dom/workers/WorkerPrivate.cpp
@@ +711,5 @@
> +           void** aContent, uint64_t* aExtraData)
> +  {
> +    WorkerCanvas* canvas = nullptr;
> +    nsresult rv = UNWRAP_OBJECT(WorkerCanvas, aObj, canvas);
> +    if (NS_SUCCEEDED(rv) && canvas && !canvas->IsNeutered()) {

wait... we should transfer neutered workercanvas. and make them disabled in the worker. I think we want to support this:

for (var i = 0; i < 10; ++i)worker.postMessage(workerCanvas, [ workerCanvas ]);

@@ +714,5 @@
> +    nsresult rv = UNWRAP_OBJECT(WorkerCanvas, aObj, canvas);
> +    if (NS_SUCCEEDED(rv) && canvas && !canvas->IsNeutered()) {
> +      canvas->SetNeutered();
> +      *aTag = DOMWORKER_SCTAG_CANVAS;
> +      *aContent = canvas;

If you like my approach, we can simple do:

*aContent = canvas->ToCloneData();
canvas->SetNeutered() // if already neutered, it's nop.

Then, implemente a freeTransfer() where you just do |delete aContent;|
and in ReadTransfer do:

nsRefPtr<WorkerCanvas> canvas = WorkerCanvas::CreateFromCloneData(aData);
delete aData;

::: dom/workers/WorkerPrivate.h
@@ +1266,5 @@
> +  WorkerCanvasObserver*
> +  GetWorkerCanvasObserver();
> +
> +  void
> +  InvalidateWorkerCanvas(WorkerCanvas* aWorkerCanvas);

This is not actually used/implemented by this patch or others.

@@ +1395,5 @@
>  
>  enum WorkerStructuredDataType
>  {
>    DOMWORKER_SCTAG_BLOB = SCTAG_DOM_MAX,
>    DOMWORKER_SCTAG_FORMDATA = SCTAG_DOM_MAX + 1,

Nt your code, but ocan you remove this SCTAG_DOM_MAX + 1?

::: dom/workers/moz.build
@@ +10,5 @@
>      'ServiceWorkerContainer.h',
>      'ServiceWorkerEvents.h',
>      'ServiceWorkerRegistrar.h',
>      'ServiceWorkerRegistration.h',
> +    'WorkerCanvasObserver.h',

I don't think we need to export it. Is it required by other patches?
Comment 157 Milan Sreckovic [:milan] 2015-05-13 10:20:28 PDT
If this gets close to working - it should not land before bug 1144906 lands.  It will probably also need some reworking once that bug lands.
Comment 158 Morris Tseng [:mtseng] [:Morris] 2015-05-13 19:09:19 PDT
Let's wait bug 1144906 lands.
Comment 159 Morris Tseng [:mtseng] [:Morris] 2015-05-14 01:57:37 PDT
(In reply to Andrea Marchesini (:baku) from comment #156)
> ::: dom/workers/WorkerPrivate.h
> @@ +1266,5 @@
> > +  WorkerCanvasObserver*
> > +  GetWorkerCanvasObserver();
> > +
> > +  void
> > +  InvalidateWorkerCanvas(WorkerCanvas* aWorkerCanvas);
> 
> This is not actually used/implemented by this patch or others.
This is used by WorkerCanvas::Invalidate().
> 
> 
> ::: dom/workers/moz.build
> @@ +10,5 @@
> >      'ServiceWorkerContainer.h',
> >      'ServiceWorkerEvents.h',
> >      'ServiceWorkerRegistrar.h',
> >      'ServiceWorkerRegistration.h',
> > +    'WorkerCanvasObserver.h',
> 
> I don't think we need to export it. Is it required by other patches?
It is required by WokerCanvas::Invalidate() as well. Please see WorkerCanvas.cpp at patch part 2.
Comment 160 Morris Tseng [:mtseng] [:Morris] 2015-05-14 02:08:32 PDT
Created attachment 8605731 [details] [diff] [review]
Part 1: Let ImageBridge transfer CanvasClient async v2.

Addressed :nical's comment.
Comment 161 Morris Tseng [:mtseng] [:Morris] 2015-05-14 02:09:44 PDT
Created attachment 8605733 [details] [diff] [review]
Part 2: Introduce WorkerCanvas and let WebGL context work on workers v2.

Addressed :baku's comment.
Comment 162 Morris Tseng [:mtseng] [:Morris] 2015-05-14 02:10:29 PDT
Created attachment 8605734 [details] [diff] [review]
Part 3: Transfer WorkerCanvas from mainthread to workers v2.

Addressed :baku's comment.
Comment 163 Morris Tseng [:mtseng] [:Morris] 2015-05-14 02:11:32 PDT
Created attachment 8605735 [details] [diff] [review]
Part 4: Mochitests for workercanvas v2.

Neutered workercanvas can be transferred again. So modify our test case to fit this change.
Comment 164 Morris Tseng [:mtseng] [:Morris] 2015-05-14 02:55:52 PDT
Comment on attachment 8605731 [details] [diff] [review]
Part 1: Let ImageBridge transfer CanvasClient async v2.

Deferred review until bug 1144906 is ready.
Comment 165 Morris Tseng [:mtseng] [:Morris] 2015-05-14 02:56:19 PDT
Comment on attachment 8605733 [details] [diff] [review]
Part 2: Introduce WorkerCanvas and let WebGL context work on workers v2.

Deferred review until bug 1144906 is ready.
Comment 166 :Ehsan Akhgari 2015-05-19 11:41:22 PDT
Comment on attachment 8605735 [details] [diff] [review]
Part 4: Mochitests for workercanvas v2.

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

I would appreciate if you would submit an interdiff addressing only the comments below for the next round of reviews.

Also, how does this API behave with shared workers?  What about service workers?  Those two worker types are significant here because they can have no associated window/document.  (Or more than one, for that matter.)

Whatever the desired behavior is, we need to have tests for it.

::: dom/canvas/test/test_workercanvas_basic_webgl.html
@@ +23,5 @@
> +    var msg = evt.data || {};
> +    if (msg.type == "test") {
> +      ok(msg.result, msg.name);
> +    }
> +    if (msg.type == "todo") {

This branch can be removed once you address my other comments.

::: dom/canvas/test/test_workercanvas_many.html
@@ +12,5 @@
> +<div><canvas id="c4" width="64" height="64"></canvas></div>
> +<div><canvas id="c5" width="64" height="64"></canvas></div>
> +<div><canvas id="c6" width="64" height="64"></canvas></div>
> +<div><canvas id="c7" width="64" height="64"></canvas></div>
> +<div><canvas id="c8" width="64" height="64"></canvas></div>

Can you please add a comment explaining the goal behind testing 8 canveses?  Ideally your comment would explain why you picked 8, and not let's say 7 or 137.  ;-)

@@ +31,5 @@
> +      var msg = evt.data || {};
> +      if (msg.type == "test") {
> +        ok(msg.result, msg.name);
> +      }
> +      if (msg.type == "todo") {

Ditto.

::: dom/canvas/test/test_workercanvas_neuter.html
@@ +33,5 @@
> +  SimpleTest.doesThrow(
> +    function() { htmlCanvas.height = 128; },
> +    "Can't change html canvas' height after transferControlToWorker");
> +
> +  ok(!htmlCanvas.getContext("2d"), "Can't getContext after transferControlToWorker");

Please also test what happens when getting a webgl context here.

@@ +47,5 @@
> +    function() { workerCanvas.height = 128; },
> +    "Can't change transfered worker canvas height");
> +
> +  SimpleTest.doesThrow(
> +    function() { workerCanvas.getContext("webgl") },

Please test what happens when getting a 2d context here as well.

::: dom/canvas/test/test_workercanvas_sizechange.html
@@ +20,5 @@
> +    var msg = evt.data || {};
> +    if (msg.type == "test") {
> +      ok(msg.result, msg.name);
> +    }
> +    if (msg.type == "todo") {

Ditto.

::: dom/canvas/test/test_workercanvas_subworker.html
@@ +14,5 @@
> +<div><canvas id="c6" width="64" height="64"></canvas></div>
> +<div><canvas id="c7" width="64" height="64"></canvas></div>
> +<div><canvas id="c8" width="64" height="64"></canvas></div>
> +<div><canvas id="c9" width="64" height="64"></canvas></div>
> +<div><canvas id="c10" width="64" height="64"></canvas></div>

Same comment should be added to this test as well.

@@ +29,5 @@
> +    var msg = evt.data || {};
> +    if (msg.type == "test") {
> +      ok(msg.result, msg.name);
> +    }
> +    if (msg.type == "todo") {

Ditto.

::: dom/canvas/test/workercanvas.js
@@ +4,5 @@
> +function ok(expect, msg) {
> +    postMessage({type: "test", result: !!expect, name: msg});
> +}
> +
> +function todo(msg) {

Please remove todo() once you addressed the comment below.

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

How is this not a test failure?!

@@ +29,5 @@
> +        todo("WebGL is unavailable");
> +        return null;
> +    }
> +
> +    var vertSrc = "attribute vec2 position; \

Please have someone who is familiar with the WebGL API review this part of the test as well.

::: dom/canvas/test/workercanvas_neuter.js
@@ +1,1 @@
> +

Nit: please remove this empty line.
Comment 167 Morris Tseng [:mtseng] [:Morris] 2015-05-21 01:38:17 PDT
Created attachment 8608565 [details] [diff] [review]
Part 1: Let ImageBridge transfer CanvasClient async v3.

Bug 1144906 use shared surface for all webgl context. So this version use shared surface for worker canvas as well. You need apply bug 1144906 before apply this patch.
Comment 168 Morris Tseng [:mtseng] [:Morris] 2015-05-21 01:38:50 PDT
Created attachment 8608566 [details] [diff] [review]
Part 2: Introduce WorkerCanvas and let WebGL context work on workers v3.

Use shared surface.
Comment 169 Morris Tseng [:mtseng] [:Morris] 2015-05-21 01:39:26 PDT
Created attachment 8608567 [details] [diff] [review]
Part 3: Transfer WorkerCanvas from mainthread to workers v3.

Rebased.
Comment 170 Andrea Marchesini [:baku] 2015-05-26 02:51:43 PDT
Comment on attachment 8608567 [details] [diff] [review]
Part 3: Transfer WorkerCanvas from mainthread to workers v3.

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

sfink, can you review the StructuredClone changes?

::: dom/workers/WorkerCanvasObserver.cpp
@@ +13,5 @@
> +NS_IMPL_ISUPPORTS(WorkerCanvasObserver, nsIThreadObserver)
> +
> +WorkerCanvasObserver::WorkerCanvasObserver()
> +{
> +}

move CTOR and DTOR to the header.

::: dom/workers/WorkerPrivate.cpp
@@ +697,5 @@
> +      WorkerCanvasCloneData* data =
> +        static_cast<WorkerCanvasCloneData*>(aData);
> +      nsRefPtr<WorkerCanvas> canvas = WorkerCanvas::CreateFromCloneData(data);
> +      delete data;
> +      JS::Rooted<JSObject*> obj(aCx, canvas->WrapObject(aCx, nullptr));

JS::Rooted<JS::Value> value(aCx);
if (!GetOrCreateDOMReflector(aCx, canvas, &value)) {
  JS_ClearPendingException(aCx);
  return false;
}

aReturnObject.set(&value.toObject());
return true;

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

if (NS_FAILED(rv)) {
  return false;
}

MOZ_ASSERT(canvas);
...

return true;
Comment 171 Steve Fink [:sfink] [:s:] 2015-05-26 13:16:53 PDT
Comment on attachment 8608567 [details] [diff] [review]
Part 3: Transfer WorkerCanvas from mainthread to workers v3.

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

Yuck. The JSAutoStructureCloneBuffer callbacks and closure are weird, in that they're only relevant for the freeing. But this change completes the current setup, so r=me. We really ought to fix up the callback registration mess.
Comment 172 Morris Tseng [:mtseng] [:Morris] 2015-05-26 20:57:08 PDT
Created attachment 8610999 [details] [diff] [review]
Part 3: Transfer WorkerCanvas from mainthread to workers v4. (carry r+: baku, sfink)

Addressed baku's comment. Thanks for review.
Comment 173 Morris Tseng [:mtseng] [:Morris] 2015-06-05 02:06:00 PDT
Created attachment 8615892 [details] [diff] [review]
Part 1: Let ImageBridge transfer CanvasClient async v4.

Thanks Jon Morton [:jmorton] (jonanin@gmail.com) for polishing patches.
Comment 174 Morris Tseng [:mtseng] [:Morris] 2015-06-05 02:06:09 PDT
Created attachment 8615893 [details] [diff] [review]
Part 2: Introduce WorkerCanvas and let WebGL context work on workers v4.

Thanks Jon Morton [:jmorton] (jonanin@gmail.com) for polishing patches.
Comment 175 Kenneth Russell 2015-06-05 08:26:25 PDT
Great that this is getting close to landing.

There's been more discussion and a naming change since the original WorkerCanvas proposal. The current proposed naming convention is OffscreenCanvas and the spec draft is here: https://wiki.whatwg.org/wiki/OffscreenCanvas . This is the one Chromium plans to prototype. To keep Firefox and Chromium in sync, would you consider changing the naming convention in this patch?
Comment 176 Martin Best (:mbest) 2015-06-05 08:42:47 PDT
Getting a common naming convention would definitely make the dev community a happier.  Is there a concern with doing this past some potential delay?
Comment 177 Morris Tseng [:mtseng] [:Morris] 2015-06-08 02:37:14 PDT
Created attachment 8616598 [details] [diff] [review]
Part 1: Let ImageBridge transfer CanvasClient async v5.

Rename WorkerCanvas to OffscreenCanvas.
Comment 178 Morris Tseng [:mtseng] [:Morris] 2015-06-08 02:45:44 PDT
Created attachment 8616603 [details] [diff] [review]
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v5.

Rename WorkerCanvas to OffscreenCanvas
Comment 179 Morris Tseng [:mtseng] [:Morris] 2015-06-08 02:46:58 PDT
Created attachment 8616604 [details] [diff] [review]
Part 3: Transfer OffscreenCanvas from mainthread to workers v5. (carry r+: baku, sfink)

Rename WorkerCanvas to OffscreenCanvas
Comment 180 Morris Tseng [:mtseng] [:Morris] 2015-06-08 02:52:29 PDT
Part 4 should rename to OffscreenCanvas, too. But I didn't fully address reviewer's comment yet. So I'll upload it when I'm ready.
Comment 181 :Ehsan Akhgari 2015-06-08 06:37:59 PDT
Comment on attachment 8616603 [details] [diff] [review]
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v5.

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

r- on the WebIDL changes because of two reasons:

* The changes are vastly different than the ones in the spec draft.  Please either fix that to make sure we follow the spec precisely, or explain why there are differences.
* You should modify dom/workers/test/test_worker_interfaces.js for all of the new interfaces you are exposing to workers (and also dom/tests/mochitest/general/test_interfaces.html for the ones you end up exposing to Window.)

::: dom/webidl/HTMLCanvasElement.webidl
@@ +47,5 @@
>    [Throws, UnsafeInPrerendering, Pref="canvas.capturestream.enabled"]
>    CanvasCaptureMediaStream captureStream(optional double frameRate);
>  };
>  
> +// For OffscreenCanvas

Nit: here and elsewhere, please link to <https://wiki.whatwg.org/wiki/OffscreenCanvas> as the spec draft.  When this finally ends up in the HTML spec, those links need to be updated.

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

Nit: please make this interface exposure dependent on the gfx.offscreencanvas.enabled pref.

Also, this lacks the [Constructor].

@@ +7,5 @@
> + * http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2013-October/041074.html
> + */
> +
> +[Exposed=(Window,Worker)]
> +interface OffscreenCanvas : EventTarget {

This should also implement Transferable according to the spec draft.

@@ +15,5 @@
> +  attribute unsigned long height;
> +
> +  [Throws]
> +  nsISupports? getContext(DOMString contextId,
> +                          optional any contextOptions = null);

The syntax in the spec draft for contextOptions uses a variadic any argument.  Why is there a difference here?

@@ +16,5 @@
> +
> +  [Throws]
> +  nsISupports? getContext(DOMString contextId,
> +                          optional any contextOptions = null);
> +};

What about transferToImageBitmap and toBlob?

::: dom/webidl/WebGLRenderingContext.webidl
@@ +44,5 @@
>      boolean preserveDrawingBuffer = false;
>      boolean failIfMajorPerformanceCaveat = false;
>  };
>  
> +[Exposed=(Window,Worker)]

You should probably add some note about these to the spec draft.

@@ +513,5 @@
>      const GLenum UNPACK_COLORSPACE_CONVERSION_WEBGL = 0x9243;
>      const GLenum BROWSER_DEFAULT_WEBGL          = 0x9244;
>  
>      // The canvas might actually be null in some cases, apparently.
> +    readonly attribute (HTMLCanvasElement or OffscreenCanvas)? canvas;

Why is this nullable, and the same attribute in the spec draft is non-nullable?
Comment 182 Morris Tseng [:mtseng] [:Morris] 2015-06-08 21:03:56 PDT
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #181)
> Comment on attachment 8616603 [details] [diff] [review]
> Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v5.
> 
> Review of attachment 8616603 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- on the WebIDL changes because of two reasons:
> 
> * The changes are vastly different than the ones in the spec draft.  Please
> either fix that to make sure we follow the spec precisely, or explain why
> there are differences.
This bug focus on transfer canvas from main thread to worker. So there are some spec doesn't implement in this bug, such as [Constructor], toBlob() and transferToImageBitmap in OffscreenCanvas. I'll create a followup bug for implementing remaining spec.

> * You should modify dom/workers/test/test_worker_interfaces.js for all of
> the new interfaces you are exposing to workers (and also
> dom/tests/mochitest/general/test_interfaces.html for the ones you end up
> exposing to Window.)
Ok, will do.
> 
> @@ +7,5 @@
> > + * http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2013-October/041074.html
> > + */
> > +
> > +[Exposed=(Window,Worker)]
> > +interface OffscreenCanvas : EventTarget {
> 
> This should also implement Transferable according to the spec draft.
It looks like I don't need actually implement Transferable to make OffscreenCanvas transferable
> 
> @@ +15,5 @@
> > +  attribute unsigned long height;
> > +
> > +  [Throws]
> > +  nsISupports? getContext(DOMString contextId,
> > +                          optional any contextOptions = null);
> 
> The syntax in the spec draft for contextOptions uses a variadic any
> argument.  Why is there a difference here?
I just copied getContext of HTMLCanvasElement.webidl and it didn't have use a variadic any, either. I read spec and found that although spec use variadic any, we actually only process one parameter for now. So I think we are good here.
> 
> 
> ::: dom/webidl/WebGLRenderingContext.webidl
> @@ +44,5 @@
> >      boolean preserveDrawingBuffer = false;
> >      boolean failIfMajorPerformanceCaveat = false;
> >  };
> >  
> > +[Exposed=(Window,Worker)]
> 
> You should probably add some note about these to the spec draft.
OK. will do.
> 
> @@ +513,5 @@
> >      const GLenum UNPACK_COLORSPACE_CONVERSION_WEBGL = 0x9243;
> >      const GLenum BROWSER_DEFAULT_WEBGL          = 0x9244;
> >  
> >      // The canvas might actually be null in some cases, apparently.
> > +    readonly attribute (HTMLCanvasElement or OffscreenCanvas)? canvas;
> 
> Why is this nullable, and the same attribute in the spec draft is
> non-nullable?
Because we can new a context without binding any HTMLCanvasElement or OffscreenCanvas. So this attribute might be null. I'm not sure why spec draft is non-nullable.
Comment 183 Morris Tseng [:mtseng] [:Morris] 2015-06-08 23:29:56 PDT
> ::: dom/webidl/OffscreenCanvas.webidl
> @@ +6,5 @@
> > + * For more information on this interface, please see
> > + * http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2013-October/041074.html
> > + */
> > +
> > +[Exposed=(Window,Worker)]
> 
> Nit: please make this interface exposure dependent on the
> gfx.offscreencanvas.enabled pref.
I found that we cannot use [Pref=gfx.offscreencanvas.enabled] with [Exposed=(Window,Worker)]. We will hit this error. https://dxr.mozilla.org/mozilla-central/source/dom/bindings/parser/WebIDL.py#1110
Comment 184 Morris Tseng [:mtseng] [:Morris] 2015-06-08 23:32:34 PDT
Created attachment 8617156 [details] [diff] [review]
Part 5: Add interfaces test.
Comment 185 Morris Tseng [:mtseng] [:Morris] 2015-06-08 23:35:31 PDT
Created attachment 8617158 [details] [diff] [review]
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v6.

Ehsan, how about comment 182 and 183?
Comment 186 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2015-06-09 06:11:17 PDT
(In reply to Morris Tseng [:mtseng] from comment #183)
> > ::: dom/webidl/OffscreenCanvas.webidl
> > @@ +6,5 @@
> > > + * For more information on this interface, please see
> > > + * http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2013-October/041074.html
> > > + */
> > > +
> > > +[Exposed=(Window,Worker)]
> > 
> > Nit: please make this interface exposure dependent on the
> > gfx.offscreencanvas.enabled pref.
> I found that we cannot use [Pref=gfx.offscreencanvas.enabled] with
> [Exposed=(Window,Worker)]. We will hit this error.
> https://dxr.mozilla.org/mozilla-central/source/dom/bindings/parser/WebIDL.
> py#1110

Look at Cache::PrefEnabled for an example of what you need to do here.
Comment 187 Morris Tseng [:mtseng] [:Morris] 2015-06-09 06:16:38 PDT
Comment on attachment 8617158 [details] [diff] [review]
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v6.

khuey, thanks for info. I'll request review once new patch is ready.
Comment 188 Morris Tseng [:mtseng] [:Morris] 2015-06-09 06:48:57 PDT
Created attachment 8617316 [details] [diff] [review]
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v7.

Using khuey's advice for detecting preference in webidl.
Comment 189 :Ehsan Akhgari 2015-06-09 07:27:57 PDT
Comment on attachment 8617156 [details] [diff] [review]
Part 5: Add interfaces test.

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

::: dom/tests/mochitest/general/test_interfaces.html
@@ +816,5 @@
>      "Notification",
>  // IMPORTANT: Do not change this list without review from a DOM peer!
>      "NotifyPaintEvent",
>  // IMPORTANT: Do not change this list without review from a DOM peer!
> +    "OffscreenCanvas",

This means that the test checks OffscreenCanvas to be exposed to the Window object unconditionally, which contradicts what your patch is doing.  You need to express conditions for what platforms/releases this is expected to show up in.

Some examples: you would add |release: false| if this feature is expected to be hidden behind #ifdef RELEASE_BUILD.  You would add |b2g: false| if this feature is expected to be disabled for b2g for now.

The basic idea is that this test needs to pass as your patch travels from Nightly to Aurora, Beta and Release.  There are other examples in this file that show you how to code in various conditions.

::: dom/workers/test/test_worker_interfaces.js
@@ +142,5 @@
>      "MessageEvent",
>  // IMPORTANT: Do not change this list without review from a DOM peer!
>      "MessagePort",
>  // IMPORTANT: Do not change this list without review from a DOM peer!
> +    "OffscreenCanvas",

Ditto.

@@ +192,5 @@
> +    "WebGLShaderPrecisionFormat",
> +// IMPORTANT: Do not change this list without review from a DOM peer!
> +    "WebGLTexture",
> +// IMPORTANT: Do not change this list without review from a DOM peer!
> +    "WebGLUniformLocation",

They all also need to be exposed conditionally.
Comment 190 :Ehsan Akhgari 2015-06-09 07:34:21 PDT
(In reply to Morris Tseng [:mtseng] from comment #182)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #181)
> > Comment on attachment 8616603 [details] [diff] [review]
> > Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v5.
> > 
> > Review of attachment 8616603 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r- on the WebIDL changes because of two reasons:
> > 
> > * The changes are vastly different than the ones in the spec draft.  Please
> > either fix that to make sure we follow the spec precisely, or explain why
> > there are differences.
> This bug focus on transfer canvas from main thread to worker. So there are
> some spec doesn't implement in this bug, such as [Constructor], toBlob() and
> transferToImageBitmap in OffscreenCanvas. I'll create a followup bug for
> implementing remaining spec.

OK.  It would be nice to add a comment in the WebIDL file pointing to the follow-up bug.

> > @@ +7,5 @@
> > > + * http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2013-October/041074.html
> > > + */
> > > +
> > > +[Exposed=(Window,Worker)]
> > > +interface OffscreenCanvas : EventTarget {
> > 
> > This should also implement Transferable according to the spec draft.
> It looks like I don't need actually implement Transferable to make
> OffscreenCanvas transferable

I'm not sure what you mean here.  I was talking about this line in the IDL: |OffscreenCanvas implements Transferable;|.  Are you saying this is not needed?  Why?

> > @@ +15,5 @@
> > > +  attribute unsigned long height;
> > > +
> > > +  [Throws]
> > > +  nsISupports? getContext(DOMString contextId,
> > > +                          optional any contextOptions = null);
> > 
> > The syntax in the spec draft for contextOptions uses a variadic any
> > argument.  Why is there a difference here?
> I just copied getContext of HTMLCanvasElement.webidl and it didn't have use
> a variadic any, either. I read spec and found that although spec use
> variadic any, we actually only process one parameter for now. So I think we
> are good here.

Hmm.  I'm curious to know why this inconsistency exists, but it's probably fine for now...

> > @@ +513,5 @@
> > >      const GLenum UNPACK_COLORSPACE_CONVERSION_WEBGL = 0x9243;
> > >      const GLenum BROWSER_DEFAULT_WEBGL          = 0x9244;
> > >  
> > >      // The canvas might actually be null in some cases, apparently.
> > > +    readonly attribute (HTMLCanvasElement or OffscreenCanvas)? canvas;
> > 
> > Why is this nullable, and the same attribute in the spec draft is
> > non-nullable?
> Because we can new a context without binding any HTMLCanvasElement or
> OffscreenCanvas. So this attribute might be null. I'm not sure why spec
> draft is non-nullable.

What you're saying makes sense.  Please raise this with the maintainer of that wiki page.  Thanks!  :-)
Comment 191 :Ehsan Akhgari 2015-06-09 07:39:23 PDT
Comment on attachment 8617316 [details] [diff] [review]
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v7.

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

This is close, but the WebGLRenderingContext.webidl changes are still not right, sorry if I was not more clear before.

::: dom/canvas/OffscreenCanvas.cpp
@@ +225,5 @@
> +
> +/* static */ bool
> +OffscreenCanvas::PrefEnabled(JSContext* aCx, JSObject* aObj)
> +{
> +  return gfxPrefs::OffscreenCanvasEnabled();

I expect this works on workers too?

::: dom/webidl/WebGLRenderingContext.webidl
@@ +44,5 @@
>      boolean preserveDrawingBuffer = false;
>      boolean failIfMajorPerformanceCaveat = false;
>  };
>  
> +[Exposed=(Window,Worker)]

These are still exposed to workers unconditionally.  You probably want a similar [Func] attribute that points to a function which returns true on the main thread unconditionally, and looks at a pref of some kind on a worker.

::: modules/libpref/init/all.js
@@ +4108,5 @@
>  pref("webgl.angle.try-d3d11", true);
>  pref("webgl.angle.force-d3d11", false);
>  #endif
>  
> +pref("gfx.offscreencanvas.enabled", false);

Hmm, do your test_interfaces changes even pass locally with this?
Comment 192 Morris Tseng [:mtseng] [:Morris] 2015-06-10 00:09:35 PDT
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #190)
> > > @@ +7,5 @@
> > > > + * http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2013-October/041074.html
> > > > + */
> > > > +
> > > > +[Exposed=(Window,Worker)]
> > > > +interface OffscreenCanvas : EventTarget {
> > > 
> > > This should also implement Transferable according to the spec draft.
> > It looks like I don't need actually implement Transferable to make
> > OffscreenCanvas transferable
> 
> I'm not sure what you mean here.  I was talking about this line in the IDL:
> |OffscreenCanvas implements Transferable;|.  Are you saying this is not
> needed?  Why?
> 
Yes, I mean we don't need this line in the idl. In Window.webidl, we typedef any to Transferable. That is, all object should be transferable if we handle it properly in gecko without this line in the idl. I'll add this line to idl eventually but comment it out just like MessagePort.webidl.
Comment 193 Morris Tseng [:mtseng] [:Morris] 2015-06-10 00:14:21 PDT
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #191)
> Comment on attachment 8617316 [details] [diff] [review]
> Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v7.
> 
> Review of attachment 8617316 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is close, but the WebGLRenderingContext.webidl changes are still not
> right, sorry if I was not more clear before.
> 
> ::: dom/canvas/OffscreenCanvas.cpp
> @@ +225,5 @@
> > +
> > +/* static */ bool
> > +OffscreenCanvas::PrefEnabled(JSContext* aCx, JSObject* aObj)
> > +{
> > +  return gfxPrefs::OffscreenCanvasEnabled();
> 
> I expect this works on workers too?
Yes, this works on any thread.
> 
> ::: dom/webidl/WebGLRenderingContext.webidl
> @@ +44,5 @@
> >      boolean preserveDrawingBuffer = false;
> >      boolean failIfMajorPerformanceCaveat = false;
> >  };
> >  
> > +[Exposed=(Window,Worker)]
> 
> These are still exposed to workers unconditionally.  You probably want a
> similar [Func] attribute that points to a function which returns true on the
> main thread unconditionally, and looks at a pref of some kind on a worker.
Got it.
> 
> ::: modules/libpref/init/all.js
> @@ +4108,5 @@
> >  pref("webgl.angle.try-d3d11", true);
> >  pref("webgl.angle.force-d3d11", false);
> >  #endif
> >  
> > +pref("gfx.offscreencanvas.enabled", false);
> 
> Hmm, do your test_interfaces changes even pass locally with this?
I test it locally with this pref enabled. I'll test it with default setting. Thanks.
Comment 194 Morris Tseng [:mtseng] [:Morris] 2015-06-10 01:00:26 PDT
Created attachment 8617779 [details] [diff] [review]
Part 5: Add interfaces test v2.

Address ehsan's comment.
Comment 195 Morris Tseng [:mtseng] [:Morris] 2015-06-10 01:01:37 PDT
Created attachment 8617781 [details] [diff] [review]
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v8.

Address ehsan's comment.
Comment 196 :Ehsan Akhgari 2015-06-10 14:23:22 PDT
Comment on attachment 8617781 [details] [diff] [review]
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v8.

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

::: dom/canvas/OffscreenCanvas.h
@@ +117,5 @@
> +  static bool PrefEnabled(JSContext* aCx, JSObject* aObj);
> +
> +  // Return true on main-thread, and return gfx.offscreencanvas.enabled
> +  // on other thread.
> +  static bool PrefEnabledOnOtherThread(JSContext* aCx, JSObject* aObj);

Nit: please rename this to PrefEnabledOnWorkerThread.
Comment 197 Jeff Gilbert [:jgilbert] 2015-06-10 19:23:44 PDT
Comment on attachment 8616604 [details] [diff] [review]
Part 3: Transfer OffscreenCanvas from mainthread to workers v5. (carry r+: baku, sfink)

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

::: dom/workers/OffscreenCanvasObserver.cpp
@@ +35,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +OffscreenCanvasObserver::AfterProcessNextEvent(nsIThreadInternal *aThread,

State why this is what we want, in a comment.

::: dom/workers/OffscreenCanvasObserver.h
@@ +16,5 @@
> +class OffscreenCanvas;
> +
> +/**
> + * Keeps track of invalidated worker canvases and will commit them after the
> + * next dom worker event is processed. Lives on WorkerPrivate.

"after the next dom worker event is processed"
Ideally:
"after the current event"

It seems like this is the effect we get, but stating the intent in the comment is useful.
Comment 198 Jeff Gilbert [:jgilbert] 2015-06-11 18:24:23 PDT
Comment on attachment 8616598 [details] [diff] [review]
Part 1: Let ImageBridge transfer CanvasClient async v5.

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

::: gfx/gl/GLScreenBuffer.cpp
@@ +63,5 @@
> +    if (!gfxPrefs::WebGLForceLayersReadback()) {
> +        switch (forwarder->GetCompositorBackendType()) {
> +            case mozilla::layers::LayersBackend::LAYERS_OPENGL: {
> +#if defined(XP_MACOSX)
> +                factory = SurfaceFactory_IOSurface::Create(gl, caps, forwarder, flags);

This is no longer around, please rebase.

::: gfx/layers/ipc/ImageBridgeChild.cpp
@@ +806,5 @@
> +ImageBridgeChild::CreateCanvasClient(CanvasClient::CanvasClientType aType,
> +                                     TextureFlags aFlag)
> +{
> +  if (InImageBridgeChildThread()) {
> +    return CreateCanvasClientNow(aType, aFlag);

Surely this infinitely recurses?
Comment 199 Jeff Gilbert [:jgilbert] 2015-06-11 18:38:03 PDT
Comment on attachment 8617781 [details] [diff] [review]
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v8.

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

I have a bunch of questions. We really need more commenting here, particularly since this code tends to be at the intersection of peoples' areas of expertise.

::: dom/canvas/CanvasRenderingContextHelper.cpp
@@ +37,5 @@
> +
> +#ifdef DEBUG
> +  if (mCurrentContext) {
> +    // We disallow canvases of width or height zero, and set them to 1, so
> +    // we will have a discrepancy with the sizes of the canvas and the context.

How can this happen? The width and height should always be >0 by my understanding.

@@ +54,5 @@
> +    mCurrentContext->GetImageBuffer(&imageBuffer, &format);
> +  }
> +
> +  // Encoder callback when encoding is complete.
> +  class EncodeCallback : public EncodeCompleteCallback

Why is this inside the function? It doesn't seem more clear this way.

@@ +141,5 @@
> +    }
> +  } else {
> +    // We already have a context of some type.
> +    if (contextType != mCurrentContextType)
> +      return nullptr;

This is a really short branch. Why not do this before the !mCurrentContext case?

::: dom/canvas/OffscreenCanvas.cpp
@@ +20,5 @@
> +#ifdef XP_MACOSX
> +#include "SharedSurfaceIO.h"
> +#endif
> +
> +using namespace mozilla::layers;

Avoid using a namespace. Use a specific couple things if it improves clarity.

@@ +47,5 @@
> +NS_IMPL_RELEASE_INHERITED(OffscreenCanvas, DOMEventTargetHelper)
> +
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(OffscreenCanvas)
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +NS_INTERFACE_MAP_END_INHERITING(DOMEventTargetHelper)

Put this goop at the end of the file.

@@ +106,5 @@
> +    return nullptr;
> +  }
> +
> +  if (!(contextType == CanvasContextType::WebGL1 ||
> +        contextType == CanvasContextType::WebGL2)) {

Brace on new line for multi-line conditionals.

@@ +179,5 @@
> +void
> +OffscreenCanvas::CommitFrameToCompositor()
> +{
> +  // The attributes has changed, we have to notify main
> +  // thread to change canvas size and update GLContext size.

Update GLContext's size on the main thread? I hope not!

@@ +231,5 @@
> +
> +/* static */ bool
> +OffscreenCanvas::PrefEnabledOnOtherThread(JSContext* aCx, JSObject* aObj)
> +{
> +  if (NS_IsMainThread()) {

Why is this here?
If this should only be called from the worker thread ('worker' not 'other'), MOZ_RELEASE_ASSERT that it's not on the main thread.

@@ +242,5 @@
> +void
> +OffscreenCanvas::Invalidate()
> +{
> +  workers::GetCurrentThreadWorkerPrivate()
> +           ->GetOffscreenCanvasObserver()->InvalidateOffscreenCanvas(this);

foo::bar->bat
        ->baz

not
foo::bar
   ->bat->baz

::: dom/canvas/OffscreenCanvas.h
@@ +28,5 @@
> +
> +struct OffscreenCanvasCloneData final
> +{
> +  OffscreenCanvasCloneData(layers::AsyncCanvasRenderer* aRenderer,
> +                        uint32_t aWidth, uint32_t aHeight,

Align this.

@@ +40,5 @@
> +  bool mNeutered;
> +};
> +
> +class OffscreenCanvas final : public DOMEventTargetHelper,
> +                              public CanvasRenderingContextHelper

foo : bar
    , bat

not
foo : bar,
      bat

@@ +53,5 @@
> +                  layers::AsyncCanvasRenderer* aRenderer);
> +
> +  OffscreenCanvas* GetParentObject() const;
> +
> +  virtual JSObject* WrapObject(JSContext *cx,

Star to left.

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

[1]

@@ +111,1 @@
>          return;

[1]

@@ +128,1 @@
>          return;

[1]

@@ +144,1 @@
>          return;

[1]

@@ +161,1 @@
>          return NS_OK;

Why does it return OK if it's not on the main thread? [1]

@@ +179,5 @@
>  WebGLObserver::HandleEvent(nsIDOMEvent* event)
>  {
>      nsAutoString type;
>      event->GetType(type);
> +    if (!mWebGL || !NS_IsMainThread() ||

[1]

@@ +545,5 @@
>  static already_AddRefed<GLContext>
>  CreateHeadlessNativeGL(bool forceEnabled, const nsCOMPtr<nsIGfxInfo>& gfxInfo,
>                         bool requireCompatProfile, WebGLContext* webgl)
>  {
> +    if (NS_IsMainThread() && !forceEnabled &&

No way! Skipping blacklist checks if we're not on the main thread?

@@ +715,5 @@
>  
>      // Done with baseCaps construction.
>  
> +    bool forceAllowAA = gfxPrefs::WebGLForceMSAA();
> +    if (NS_IsMainThread() && !forceAllowAA &&

Again, only checking the blacklist on the main thread?

@@ +911,5 @@
>      }
>  
>      nsCOMPtr<nsIGfxInfo> gfxInfo = do_GetService("@mozilla.org/gfx/info;1");
>      bool failIfMajorPerformanceCaveat =
> +                    NS_IsMainThread() &&

no

@@ +1032,5 @@
>      const size_t kMaxWebGLContexts             = 32;
>  #endif
>      MOZ_ASSERT(kMaxWebGLContextsPerPrincipal < kMaxWebGLContexts);
>  
> +    if (!NS_IsMainThread()) {

no

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

no

@@ +1305,5 @@
> +    if (mCanvasElement) {
> +        aRetVal.SetValue().SetAsHTMLCanvasElement() = mCanvasElement;
> +    }
> +    else {
> +        MOZ_ASSERT(mOffscreenCanvas);

Default to MOZ_RELEASE_ASSERT where overhead is negligible.

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

Todo...?

@@ +1679,5 @@
>          bool useDefaultHandler;
> +
> +        if (mCanvasElement) {
> +            nsContentUtils::DispatchTrustedEvent(mCanvasElement->OwnerDoc(),
> +                static_cast<nsIDOMHTMLCanvasElement*>(mCanvasElement),

Align spilled arg lists.

@@ +1752,5 @@
> +                NS_LITERAL_STRING("webglcontextrestored"),
> +                true,
> +                true);
> +        } else {
> +            bool unused;

Decls should go near their use.

::: dom/canvas/WebGLContext.h
@@ +352,4 @@
>      dom::HTMLCanvasElement* GetCanvas() const { return mCanvasElement; }
> +
> +    // WebIDL WebGLRenderingContext API
> +    void GetCanvas(Nullable<dom::OwningHTMLCanvasElementOrOffscreenCanvas>& aRetVal);

No a-prefix for args in WebGL code.

::: dom/canvas/WebGLContextExtensions.cpp
@@ +82,3 @@
>  
> +    if (NS_IsMainThread() &&
> +        Preferences::GetBool("webgl.enable-privileged-extensions", false)) {

How hard is it to fix this? It seems easy to make this work off the main thread.

@@ +182,5 @@
>      }
>  
> +    if (NS_IsMainThread() &&
> +        (Preferences::GetBool("webgl.enable-draft-extensions", false) ||
> +         IsWebGL2()))

This *must* work off the main thread for WebGL 2.

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

Brace on next line, since the conditional is multi-line now.

::: dom/canvas/WebGLContextLossHandler.cpp
@@ +80,5 @@
> +
> +NS_IMETHODIMP
> +ContextLossWorkerRunnable::Cancel()
> +{
> +    return NS_OK;

What does cancelling mean and why are we ignoring it?

@@ +188,5 @@
>  
>  void
>  WebGLContextLossHandler::DisableTimer()
>  {
> +    if (mIsDisabled)

Oh cool, so this never worked. :(

I filed bug 1174043 so we can backport this fix.

@@ +195,5 @@
>      mIsDisabled = true;
>  
> +    if (!NS_IsMainThread()) {
> +        dom::workers::WorkerPrivate* workerPrivate =
> +            dom::workers::GetCurrentThreadWorkerPrivate();

Why do we do this outside the conditional.

::: dom/canvas/WebGLContextValidate.cpp
@@ +29,5 @@
>  #if defined(MOZ_WIDGET_COCOA)
>  #include "nsCocoaFeatures.h"
>  #endif
>  
> +#include "gfxPrefs.h"

Put this alphabetically in with the rest of the includes.

@@ +1908,5 @@
>      }
>  #endif
>  
>      // Check the shader validator pref
> +    NS_ENSURE_TRUE(!NS_IsMainThread() || Preferences::GetRootBranch(), false);

This can be removed.

::: dom/html/HTMLCanvasElement.cpp
@@ +41,5 @@
>  #include "ActiveLayerTracker.h"
>  #include "WebGL1Context.h"
>  #include "WebGL2Context.h"
>  
> +#include <map>

Why is this included here?

@@ +586,5 @@
> +    renderer->SetHeight(sz.height);
> +    renderer->SetIsOpaque(GetIsOpaque());
> +
> +    mOffscreenCanvas = new OffscreenCanvas(sz.width, sz.height, GetIsOpaque(),
> +                                           renderer);

This must check if the GL context attached to this canvas is current. If it is, we need to un-MakeCurrent it.

Note that I'm pretty sure that multiplexed canvas2d via skiagl is incompatible with OffscreenCanvas without not-trivial changes.

@@ +837,5 @@
>  HTMLCanvasElement::GetCanvasLayer(nsDisplayListBuilder* aBuilder,
>                                    CanvasLayer *aOldLayer,
>                                    LayerManager *aManager)
>  {
> +  static uint8_t gOffscreenCanvasLayerUserData;

It looks like you're trying to use this as a key.
static const uint8_t* kLayerUserDataKey = &gDummy;?

@@ +855,5 @@
> +      NS_WARNING("CreateCanvasLayer failed!");
> +      return nullptr;
> +    }
> +
> +    LayerUserData *userData = nullptr;

star to left.

@@ +873,5 @@
> +    return mCurrentContext->ShouldForceInactiveLayer(aManager);
> +  }
> +
> +  if (mOffscreenCanvas) {
> +    return false;

This looks like it should be TODO.

::: dom/html/HTMLCanvasElement.h
@@ +77,5 @@
> +    if (!mOffscreenCanvas) {
> +      SetUnsignedIntAttr(nsGkAtoms::height, aHeight, aRv);
> +    } else {
> +      aRv.Throw(NS_ERROR_FAILURE);
> +    }

This is clearer as:
{
  if (mOffscreenCanvas) {
    rv.Throw(NS_ERROR_FAILURE);
    return;
  }

  SetUnsignedIntAttr(...)
}

::: dom/webidl/OffscreenCanvas.webidl
@@ +13,5 @@
> + */
> +
> +[Exposed=(Window,Worker),
> + Func="mozilla::dom::OffscreenCanvas::PrefEnabled"]
> +interface OffscreenCanvas : EventTarget {

Where is `commit()`? It's still in the proposal at https://wiki.whatwg.org/wiki/OffscreenCanvas . Is that proposal out of date?

@@ +20,5 @@
> +  [Pure, SetterThrows]
> +  attribute unsigned long height;
> +
> +  [Throws]
> +  nsISupports? getContext(DOMString contextId,

Why is this nsISupports instead of RenderingContext?

::: gfx/thebes/gfxPrefs.h
@@ +376,5 @@
>  
>    DECL_GFX_PREF(Live, "ui.click_hold_context_menus.delay",     UiClickHoldContextMenusDelay, int32_t, 500);
> +
> +  // WebGL (for pref access from Worker threads)
> +  DECL_GFX_PREF(Once, "webgl.all-angle-options",               WebGLAllANGLEOptions, bool, false);

All of these Prefs should default to Live (to match existing functionality).

We can talk about making some of them Once in a different bug.
Comment 200 Morris Tseng [:mtseng] [:Morris] 2015-06-12 00:10:27 PDT
(In reply to Jeff Gilbert [:jgilbert] from comment #198)
> Comment on attachment 8616598 [details] [diff] [review]
> Part 1: Let ImageBridge transfer CanvasClient async v5.
> 
> Review of attachment 8616598 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLScreenBuffer.cpp
> @@ +63,5 @@
> > +    if (!gfxPrefs::WebGLForceLayersReadback()) {
> > +        switch (forwarder->GetCompositorBackendType()) {
> > +            case mozilla::layers::LayersBackend::LAYERS_OPENGL: {
> > +#if defined(XP_MACOSX)
> > +                factory = SurfaceFactory_IOSurface::Create(gl, caps, forwarder, flags);
> 
> This is no longer around, please rebase.
Those code is not old code. I move it from ClientCanvasLayer::Initialize. Because in the part 2, OffscreenCanvas::GetContext also need this function. So I moved it to a common place.

> 
> ::: gfx/layers/ipc/ImageBridgeChild.cpp
> @@ +806,5 @@
> > +ImageBridgeChild::CreateCanvasClient(CanvasClient::CanvasClientType aType,
> > +                                     TextureFlags aFlag)
> > +{
> > +  if (InImageBridgeChildThread()) {
> > +    return CreateCanvasClientNow(aType, aFlag);
> 
> Surely this infinitely recurses?
Why this infinitely recurses? This logic just like CreateImageClient.
Comment 201 Jeff Gilbert [:jgilbert] 2015-06-12 12:45:57 PDT
Comment on attachment 8616598 [details] [diff] [review]
Part 1: Let ImageBridge transfer CanvasClient async v5.

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

::: gfx/gl/GLScreenBuffer.cpp
@@ +63,5 @@
> +    if (!gfxPrefs::WebGLForceLayersReadback()) {
> +        switch (forwarder->GetCompositorBackendType()) {
> +            case mozilla::layers::LayersBackend::LAYERS_OPENGL: {
> +#if defined(XP_MACOSX)
> +                factory = SurfaceFactory_IOSurface::Create(gl, caps, forwarder, flags);

Huh, so it is. I was thinking of a different source for this.

::: gfx/layers/ipc/ImageBridgeChild.cpp
@@ +806,5 @@
> +ImageBridgeChild::CreateCanvasClient(CanvasClient::CanvasClientType aType,
> +                                     TextureFlags aFlag)
> +{
> +  if (InImageBridgeChildThread()) {
> +    return CreateCanvasClientNow(aType, aFlag);

Ah, this calls ImageBridgeChild::CreateCanvasClientNow, which then calls CanvasClient::CreateCanvasClient, not ImageBridgeChild::CreateCanvasClient.
Comment 202 Morris Tseng [:mtseng] [:Morris] 2015-06-17 00:47:53 PDT
Progress update: I'm working on webgl context loss/restore mechanism for OffscreenCanvas currently. After this is done, I'll upload new patch for reviewing.
Comment 203 Morris Tseng [:mtseng] [:Morris] 2015-06-30 02:00:08 PDT
(In reply to Jeff Gilbert [:jgilbert] from comment #199)
> Comment on attachment 8617781 [details] [diff] [review]
> Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v8.
> 
> Review of attachment 8617781 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/canvas/CanvasRenderingContextHelper.cpp
> @@ +37,5 @@
> > +
> > +#ifdef DEBUG
> > +  if (mCurrentContext) {
> > +    // We disallow canvases of width or height zero, and set them to 1, so
> > +    // we will have a discrepancy with the sizes of the canvas and the context.
> 
> How can this happen? The width and height should always be >0 by my
> understanding.
> 
> @@ +54,5 @@
> > +    mCurrentContext->GetImageBuffer(&imageBuffer, &format);
> > +  }
> > +
> > +  // Encoder callback when encoding is complete.
> > +  class EncodeCallback : public EncodeCompleteCallback
> 
> Why is this inside the function? It doesn't seem more clear this way.
> 
> @@ +141,5 @@
> > +    }
> > +  } else {
> > +    // We already have a context of some type.
> > +    if (contextType != mCurrentContextType)
> > +      return nullptr;
> 
> This is a really short branch. Why not do this before the !mCurrentContext
> case?
CanvasRendeingContextHelper moved some code from HTMLCanvasElement to a common place. So above comments are existing problems in existing code. Maybe we should fix it in other bug, not this one.

> 
> ::: dom/canvas/OffscreenCanvas.cpp
> @@ +20,5 @@
> > +#ifdef XP_MACOSX
> > +#include "SharedSurfaceIO.h"
> > +#endif
> > +
> > +using namespace mozilla::layers;
> 
> Avoid using a namespace. Use a specific couple things if it improves clarity.
Done. And all coding style issues are fixed. I'll remove following comments about coding style.
> 
> @@ +179,5 @@
> > +void
> > +OffscreenCanvas::CommitFrameToCompositor()
> > +{
> > +  // The attributes has changed, we have to notify main
> > +  // thread to change canvas size and update GLContext size.
> 
> Update GLContext's size on the main thread? I hope not!
Hmm, the comment is wrong. Fixed!

> 
> ::: dom/canvas/WebGLContext.cpp
> @@ +179,5 @@
> >  WebGLObserver::HandleEvent(nsIDOMEvent* event)
> >  {
> >      nsAutoString type;
> >      event->GetType(type);
> > +    if (!mWebGL || !NS_IsMainThread() ||
> 
> [1]
This is about context loss handling on workers and already resolved in next patch.
> 
> @@ +545,5 @@
> >  static already_AddRefed<GLContext>
> >  CreateHeadlessNativeGL(bool forceEnabled, const nsCOMPtr<nsIGfxInfo>& gfxInfo,
> >                         bool requireCompatProfile, WebGLContext* webgl)
> >  {
> > +    if (NS_IsMainThread() && !forceEnabled &&
> 
> No way! Skipping blacklist checks if we're not on the main thread?
Will support blacklist checking in next patch!
> 
> @@ +1180,5 @@
> >  void
> >  WebGLContext::UpdateLastUseIndex()
> >  {
> > +    if (!NS_IsMainThread()) {
> > +        return;
> 
> no
This is related to WebGLMemoryReporter. Place a todo here.
> 
> @@ +1620,5 @@
> >          return NS_OK;
> >      }
> > +
> > +    NS_IMETHOD Cancel() {
> > +        return NS_OK;
> 
> Todo...?
I'll reset the "mWebGL" member variable.
> 
> ::: dom/canvas/WebGLContextExtensions.cpp
> @@ +82,3 @@
> >  
> > +    if (NS_IsMainThread() &&
> > +        Preferences::GetBool("webgl.enable-privileged-extensions", false)) {
> 
> How hard is it to fix this? It seems easy to make this work off the main
> thread.
Yap, fixed.
> 
> @@ +182,5 @@
> >      }
> >  
> > +    if (NS_IsMainThread() &&
> > +        (Preferences::GetBool("webgl.enable-draft-extensions", false) ||
> > +         IsWebGL2()))
> 
> This *must* work off the main thread for WebGL 2.
Done.
> 
> ::: dom/canvas/WebGLContextGL.cpp
> 
> ::: dom/canvas/WebGLContextLossHandler.cpp
> 
> @@ +195,5 @@
> >      mIsDisabled = true;
> >  
> > +    if (!NS_IsMainThread()) {
> > +        dom::workers::WorkerPrivate* workerPrivate =
> > +            dom::workers::GetCurrentThreadWorkerPrivate();
> 
> Why do we do this outside the conditional.
Done.
> 
> @@ +1908,5 @@
> >      }
> >  #endif
> >  
> >      // Check the shader validator pref
> > +    NS_ENSURE_TRUE(!NS_IsMainThread() || Preferences::GetRootBranch(), false);
> 
> This can be removed.
Done.
> 
> ::: dom/html/HTMLCanvasElement.cpp
> @@ +41,5 @@
> >  #include "ActiveLayerTracker.h"
> >  #include "WebGL1Context.h"
> >  #include "WebGL2Context.h"
> >  
> > +#include <map>
> 
> Why is this included here?
Removed it.
> 
> @@ +586,5 @@
> > +    renderer->SetHeight(sz.height);
> > +    renderer->SetIsOpaque(GetIsOpaque());
> > +
> > +    mOffscreenCanvas = new OffscreenCanvas(sz.width, sz.height, GetIsOpaque(),
> > +                                           renderer);
> 
> This must check if the GL context attached to this canvas is current. If it
> is, we need to un-MakeCurrent it.
I change the logic that if current canvas attached to a GL context, then we cannot transfer it to worker. So we can avoid this un-MakeCurrent stuff.
> 
> Note that I'm pretty sure that multiplexed canvas2d via skiagl is
> incompatible with OffscreenCanvas without not-trivial changes.
> 
> 
> @@ +873,5 @@
> > +    return mCurrentContext->ShouldForceInactiveLayer(aManager);
> > +  }
> > +
> > +  if (mOffscreenCanvas) {
> > +    return false;
> 
> This looks like it should be TODO.
Mark it as TODO.
> 
> 
> ::: dom/webidl/OffscreenCanvas.webidl
> @@ +13,5 @@
> > + */
> > +
> > +[Exposed=(Window,Worker),
> > + Func="mozilla::dom::OffscreenCanvas::PrefEnabled"]
> > +interface OffscreenCanvas : EventTarget {
> 
> Where is `commit()`? It's still in the proposal at
> https://wiki.whatwg.org/wiki/OffscreenCanvas . Is that proposal out of date?
No, I missed it. Add it in next patch.
> 
> @@ +20,5 @@
> > +  [Pure, SetterThrows]
> > +  attribute unsigned long height;
> > +
> > +  [Throws]
> > +  nsISupports? getContext(DOMString contextId,
> 
> Why is this nsISupports instead of RenderingContext?
I just followed "getContext" in HTMLCanvasElement.webidl. I'm not sure why we use nsISupports here. But if I change it to RenderingContext it will compile error.
> 
> ::: gfx/thebes/gfxPrefs.h
> @@ +376,5 @@
> >  
> >    DECL_GFX_PREF(Live, "ui.click_hold_context_menus.delay",     UiClickHoldContextMenusDelay, int32_t, 500);
> > +
> > +  // WebGL (for pref access from Worker threads)
> > +  DECL_GFX_PREF(Once, "webgl.all-angle-options",               WebGLAllANGLEOptions, bool, false);
> 
> All of these Prefs should default to Live (to match existing functionality).
> 
> We can talk about making some of them Once in a different bug.
Done.
Comment 204 Morris Tseng [:mtseng] [:Morris] 2015-06-30 02:02:05 PDT
Created attachment 8627590 [details] [diff] [review]
Part 1: Let ImageBridge transfer CanvasClient async v6. (carry r+: nical)

Rebased and fix some nit.
Comment 205 Morris Tseng [:mtseng] [:Morris] 2015-06-30 02:05:29 PDT
Created attachment 8627591 [details] [diff] [review]
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v9.

Adressed jgilbert's comment.

Ehsan, I add "commit()" function to the WebGLRenderingContext.webidl which is missing in previous patch. Would you review it again? Thanks very much.
Comment 206 Morris Tseng [:mtseng] [:Morris] 2015-06-30 02:15:41 PDT
Created attachment 8627595 [details] [diff] [review]
Part 3: Transfer OffscreenCanvas from mainthread to workers v6. (carry r+: sfink)

Because latest part 2 add commit function. So we don't need OffscreenCanvasObserver to track down which OffscreenCanvas is invalidate. This patch remove OffscreenCanvasObserver. Also, I rebased it. :baku, Would you review it again? Thanks. The structure clone thing remains unchange. So it doesn not need to review again.
Comment 207 Andrea Marchesini [:baku] 2015-07-01 07:22:43 PDT
Comment on attachment 8627595 [details] [diff] [review]
Part 3: Transfer OffscreenCanvas from mainthread to workers v6. (carry r+: sfink)

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

do we have MOZ_COUNT_CTOR and MOZ_COUNT_DTOR in the OffscreenCanvasCloneData?
Comment 208 Jeff Gilbert [:jgilbert] 2015-07-14 15:50:48 PDT
Comment on attachment 8627591 [details] [diff] [review]
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v9.

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

Alright, I think that's everything for this round, and I have a better idea of how this works.

Subsequent reviews will be much quicker!

I marked the major things with (r-) to differentiate from the nits and questions.

::: dom/canvas/CanvasRenderingContextHelper.cpp
@@ +181,5 @@
> +nsresult
> +CanvasRenderingContextHelper::ParseParams(JSContext* aCx,
> +                                          const nsAString& aType,
> +                                          const JS::Value& aEncoderOptions,
> +                                          nsAString& aParams,

aParams should be marked as an outparam.

::: dom/canvas/CanvasRenderingContextHelper.h
@@ +5,5 @@
> +
> +#ifndef mozilla_dom_CanvasRenderingContextHelper_h
> +#define mozilla_dom_CanvasRenderingContextHelper_h
> +
> +#include "mozilla/ErrorResult.h"

This include isn't needed here.

::: dom/canvas/OffscreenCanvas.cpp
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/dom/OffscreenCanvas.h"

The parent header should be first in the child cpp, and should be included locally, not via namespaces.

@@ +52,5 @@
> +{}
> +
> +OffscreenCanvas::~OffscreenCanvas()
> +{
> +  if (mCanvasRenderer) {

Is mCanvasRenderer ever null? It would be much nicer if we could guarantee that mCanvasRenderer is non-null. I don't think OffscreenCanvas makes sense with a null renderer, does it?

@@ +110,5 @@
> +                                             aRv);
> +
> +  if (mCanvasRenderer && mCurrentContext && ImageBridgeChild::IsCreated()) {
> +    TextureFlags flags = TextureFlags::IMMEDIATE_UPLOAD |
> +                         TextureFlags::DEALLOCATE_CLIENT |

Why IMMEDIATE_UPLOAD?
Why DEALLOCATE_CLIENT?

@@ +114,5 @@
> +                         TextureFlags::DEALLOCATE_CLIENT |
> +                         TextureFlags::ORIGIN_BOTTOM_LEFT;
> +
> +    mCanvasClient = ImageBridgeChild::GetSingleton()->
> +      CreateCanvasClient(CanvasClient::CanvasClientTypeShSurf, flags).take();

Why do we `take()` this? It looks like we're avoiding using a RefPtr here, which isn't good. (r-)

@@ +136,5 @@
> +  return result;
> +}
> +
> +already_AddRefed<nsICanvasRenderingContextInternal>
> +OffscreenCanvas::CreateContext(CanvasContextType aContextType)

Can't most of this be shared with the similar func for HTMLCanvas?

@@ +181,5 @@
> +      mCanvasRenderer->SetIsOpaque(mOpaque);
> +      mCanvasRenderer->NotifyElementAboutAttributesChanged();
> +    }
> +    mAttrDirty = false;
> +    return;

Woah, we don't actually Present if we changed attribs? I'm pretty sure we still should! (r-)

@@ +225,5 @@
> +/* static */ bool
> +OffscreenCanvas::PrefEnabledOnWorkerThread(JSContext* aCx, JSObject* aObj)
> +{
> +  if (NS_IsMainThread()) {
> +    return true;

Why does this return true?

@@ +226,5 @@
> +OffscreenCanvas::PrefEnabledOnWorkerThread(JSContext* aCx, JSObject* aObj)
> +{
> +  if (NS_IsMainThread()) {
> +    return true;
> +  } else {

No `else` after `return`.

::: dom/canvas/OffscreenCanvas.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_OffscreenCanvas_h

These should be ALL_CAPS.

@@ +8,5 @@
> +#define mozilla_dom_OffscreenCanvas_h
> +
> +#include "mozilla/Attributes.h"
> +#include "mozilla/DOMEventTargetHelper.h"
> +#include "mozilla/ErrorResult.h"

Move uses of ErrorResult to the cpp file, and remove this include from the header.

@@ +10,5 @@
> +#include "mozilla/Attributes.h"
> +#include "mozilla/DOMEventTargetHelper.h"
> +#include "mozilla/ErrorResult.h"
> +#include "mozilla/RefPtr.h"
> +#include "mozilla/dom/CanvasRenderingContextHelper.h"

Don't specify namespaces for local (same directory) includes.

@@ +12,5 @@
> +#include "mozilla/ErrorResult.h"
> +#include "mozilla/RefPtr.h"
> +#include "mozilla/dom/CanvasRenderingContextHelper.h"
> +#include "nsCycleCollectionParticipant.h"
> +#include "nsWrapperCache.h"

Why is this included?

@@ +25,5 @@
> +}
> +
> +namespace dom {
> +
> +struct OffscreenCanvasCloneData final

Leave a comment saying what this is for.

@@ +123,5 @@
> +  OffscreenCanvasCloneData* ToCloneData();
> +
> +  void CommitFrameToCompositor();
> +
> +  virtual bool GetOpaqueAttr() override

MozOpaque or CSSOpaque.

@@ +162,5 @@
> +    UpdateContext(nullptr, JS::NullHandleValue);
> +  }
> +
> +  // Mapped to HTMLCanvasElement's moz_opaque
> +  bool mOpaque;

Why track this? moz_opaque is not part of the OffscreenCanvas spec.

Also, even if we keep it, it should be mIsMozOpaque, or mIsCSSOpaque. We don't want people checking mOpaque when they mean to ask if the element is actually opaque. (which is really usually `!WebGLContext::mOptions.alpha` for webgl)

@@ +170,5 @@
> +
> +  uint32_t mWidth;
> +  uint32_t mHeight;
> +
> +  layers::CanvasClient* mCanvasClient;

Who owns this object, or at least keeps it alive?

@@ +171,5 @@
> +  uint32_t mWidth;
> +  uint32_t mHeight;
> +
> +  layers::CanvasClient* mCanvasClient;
> +  nsRefPtr<layers::AsyncCanvasRenderer> mCanvasRenderer;

Prefer RefPtr to nsRefPtr.

::: dom/canvas/WebGLContext.cpp
@@ +311,5 @@
> +
> +void
> +WebGLContext::OnMemoryPressure()
> +{
> +    bool wantToLoseContext = mLoseContextOnMemoryPressure;

'shouldLoseContext'

@@ +1197,5 @@
>  
>  void
> +WebGLContext::Commit()
> +{
> +    if (mOffscreenCanvas) {

Surely we should just MOZ_RELEASE_ASSERT this? Otherwise, shouldn't this error if there's no mOffscreenCanvas? (NOT_IMPLEMENTED, or similar?)

@@ +1206,5 @@
> +void
> +WebGLContext::GetCanvas(Nullable<dom::OwningHTMLCanvasElementOrOffscreenCanvas>& retval)
> +{
> +    if (mCanvasElement) {
> +        retval.SetValue().SetAsHTMLCanvasElement() = mCanvasElement;

Assert mOffscreenCanvas is null.

::: dom/canvas/WebGLContext.h
@@ -1702,5 @@
>      return ValidateObjectAssumeNonNull(info, object);
>  }
>  
> -// Listen visibilitychange and memory-pressure event for context lose/restore
> -class WebGLObserver final

Why did you move this away?

::: dom/canvas/WebGLContextLossHandler.cpp
@@ +217,5 @@
>  
> +bool
> +WebGLContextLossHandler::Notify(JSContext* aCx, dom::workers::Status aStatus)
> +{
> +    if (aStatus >= dom::workers::Closing && mIsTimerRunning) {

What does `aStatus >= dom::workers::Closing` mean? Pull it out into a boolean with a descriptive name.

::: dom/html/HTMLCanvasElement.cpp
@@ +705,5 @@
> +
> +    mOffscreenCanvas = new OffscreenCanvas(sz.width, sz.height, GetIsOpaque(),
> +                                           renderer);
> +    mContextObserver = new WebGLObserver(this);
> +    MOZ_RELEASE_ASSERT(mContextObserver, "Can't alloc WebGLContextObserver");

`new` is infallible, so this assert will never be hit.

@@ +969,3 @@
>  
> +  if (mOffscreenCanvas) {
> +    if (aOldLayer && aOldLayer->HasUserData(&gOffscreenCanvasLayerUserData)) {

Move the declaration of `gOffscreenCanvasLayerUserData` closer to this function.

Actually, it's a static, so you can just put it inside this function.

Also mark it as a dummy key, since that's how it's used. From the name, it sounds like it actually stores data.

::: dom/html/HTMLCanvasElement.h
@@ +44,4 @@
>  
> +// Listen visibilitychange and memory-pressure event for context lose/restore
> +class WebGLObserver final : public nsIObserver
> +                          , public nsIDOMEventListener

Why is this class defined in this header? The header doesn't directly have to do with WebGL. Shouldn't this go over in WebGLContext.h?

::: widget/GfxInfoBase.cpp
@@ +156,5 @@
>  
>    return name;
>  }
>  
> +typedef int32_t (*BLACKLIST_PREF_FUNCTION)();

This this is a type, so likeThisT please.

@@ +214,5 @@
>  // If the pref doesn't exist, aValue is not touched, and returns false.
>  static bool
>  GetPrefValueForFeature(int32_t aFeature, int32_t& aValue)
>  {
> +  BLACKLIST_PREF_FUNCTION fun = GetPrefFunction(aFeature);

`func` not `fun`
Comment 209 Morris Tseng [:mtseng] [:Morris] 2015-07-15 02:00:14 PDT
(In reply to Jeff Gilbert [:jgilbert] from comment #208)
> Comment on attachment 8627591 [details] [diff] [review]
> Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v9.
> 
> Review of attachment 8627591 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Alright, I think that's everything for this round, and I have a better idea
> of how this works.
> 
> Subsequent reviews will be much quicker!
> 
> I marked the major things with (r-) to differentiate from the nits and
> questions.
> 
> @@ +52,5 @@
> > +{}
> > +
> > +OffscreenCanvas::~OffscreenCanvas()
> > +{
> > +  if (mCanvasRenderer) {
> 
> Is mCanvasRenderer ever null? It would be much nicer if we could guarantee
> that mCanvasRenderer is non-null. I don't think OffscreenCanvas makes sense
> with a null renderer, does it?

In this implementation, no. We always transfer OffscreenCanvas from canvas on main-thread in current implementation.
But OffscreenCanvas also can be created on workers directly. If OffscreenCanvas is created on worker thread, mCanvasRenderer
would be null.

OffscreenCanvas draft spec describes this use case. Please see https://wiki.whatwg.org/wiki/OffscreenCanvas#Processing_Model
BTW, I'm implementing it in bug 1172796.
> 
> @@ +110,5 @@
> > +                                             aRv);
> > +
> > +  if (mCanvasRenderer && mCurrentContext && ImageBridgeChild::IsCreated()) {
> > +    TextureFlags flags = TextureFlags::IMMEDIATE_UPLOAD |
> > +                         TextureFlags::DEALLOCATE_CLIENT |
> 
> Why IMMEDIATE_UPLOAD?
> Why DEALLOCATE_CLIENT?
Right, we don't need those two flags. Removed.

> 
> @@ +114,5 @@
> > +                         TextureFlags::DEALLOCATE_CLIENT |
> > +                         TextureFlags::ORIGIN_BOTTOM_LEFT;
> > +
> > +    mCanvasClient = ImageBridgeChild::GetSingleton()->
> > +      CreateCanvasClient(CanvasClient::CanvasClientTypeShSurf, flags).take();
> 
> Why do we `take()` this? It looks like we're avoiding using a RefPtr here,
> which isn't good. (r-)
Just like ImageClient create by ImageBridgeChild. We cannot maintain lifetime rely on RefPtr here because those client created by ImageBridge should be released on ImageBridge thread. If lifetime is controlled by RefPtr, we may release it on wrong thread. Please see ImageBridgeChild::DispatchReleaseImageClient for more detail. We release this mCanvasClient in the destructor of OffscreenCanvas by calling ImageBridgeChild::DispatchReleaseCanvasClient. I know it looks weird here. But I just follow the mechanism of ImageBridgeChild. 

> 
> @@ +136,5 @@
> > +  return result;
> > +}
> > +
> > +already_AddRefed<nsICanvasRenderingContextInternal>
> > +OffscreenCanvas::CreateContext(CanvasContextType aContextType)
> 
> Can't most of this be shared with the similar func for HTMLCanvas?
Yes, move it to CanvasRenderingContextHelper.h

> 
> @@ +181,5 @@
> > +      mCanvasRenderer->SetIsOpaque(mOpaque);
> > +      mCanvasRenderer->NotifyElementAboutAttributesChanged();
> > +    }
> > +    mAttrDirty = false;
> > +    return;
> 
> Woah, we don't actually Present if we changed attribs? I'm pretty sure we
> still should! (r-)
Hmm, I'll always present no matter attribs is change or not.

> 
> @@ +225,5 @@
> > +/* static */ bool
> > +OffscreenCanvas::PrefEnabledOnWorkerThread(JSContext* aCx, JSObject* aObj)
> > +{
> > +  if (NS_IsMainThread()) {
> > +    return true;
> 
> Why does this return true?
We want to always expose webgl webidl interface to main-thread, but only expose it to worker thread only if offscreen canvas is enabled. So this pref always return true on main-thread.
> 
> @@ +12,5 @@
> > +#include "mozilla/ErrorResult.h"
> > +#include "mozilla/RefPtr.h"
> > +#include "mozilla/dom/CanvasRenderingContextHelper.h"
> > +#include "nsCycleCollectionParticipant.h"
> > +#include "nsWrapperCache.h"
> 
> Why is this included?
No need, removed.
> 
> 
> @@ +162,5 @@
> > +    UpdateContext(nullptr, JS::NullHandleValue);
> > +  }
> > +
> > +  // Mapped to HTMLCanvasElement's moz_opaque
> > +  bool mOpaque;
> 
> Why track this? moz_opaque is not part of the OffscreenCanvas spec.
> 
> Also, even if we keep it, it should be mIsMozOpaque, or mIsCSSOpaque. We
> don't want people checking mOpaque when they mean to ask if the element is
> actually opaque. (which is really usually `!WebGLContext::mOptions.alpha`
> for webgl)
I think we don't need track this. Removed all code related to mOpaque.
> 
> @@ +170,5 @@
> > +
> > +  uint32_t mWidth;
> > +  uint32_t mHeight;
> > +
> > +  layers::CanvasClient* mCanvasClient;
> 
> Who owns this object, or at least keeps it alive?
OffscreenCanvas owns this and release it when OffscrrenCanvas destructs as I mentioned above.

> 
> @@ +1197,5 @@
> >  
> >  void
> > +WebGLContext::Commit()
> > +{
> > +    if (mOffscreenCanvas) {
> 
> Surely we should just MOZ_RELEASE_ASSERT this? Otherwise, shouldn't this
> error if there's no mOffscreenCanvas? (NOT_IMPLEMENTED, or similar?)
https://wiki.whatwg.org/wiki/OffscreenCanvas#Web_IDL
draft spec says we do nothing if there is no OffscreenCanvas. Should I throw exception for this case?

> 
> ::: dom/canvas/WebGLContext.h
> @@ -1702,5 @@
> >      return ValidateObjectAssumeNonNull(info, object);
> >  }
> >  
> > -// Listen visibilitychange and memory-pressure event for context lose/restore
> > -class WebGLObserver final
> 
> Why did you move this away?
Because it associate to HTMLCanvasElement now, not to WebGLContext anymore.
> 
> ::: dom/canvas/WebGLContextLossHandler.cpp
> @@ +217,5 @@
> >  
> > +bool
> > +WebGLContextLossHandler::Notify(JSContext* aCx, dom::workers::Status aStatus)
> > +{
> > +    if (aStatus >= dom::workers::Closing && mIsTimerRunning) {
> 
> What does `aStatus >= dom::workers::Closing` mean? Pull it out into a
> boolean with a descriptive name.
That's mean the worker thread is not running anymore.
> 
> ::: dom/html/HTMLCanvasElement.h
> @@ +44,4 @@
> >  
> > +// Listen visibilitychange and memory-pressure event for context lose/restore
> > +class WebGLObserver final : public nsIObserver
> > +                          , public nsIDOMEventListener
> 
> Why is this class defined in this header? The header doesn't directly have
> to do with WebGL. Shouldn't this go over in WebGLContext.h?
As mentioned above, This class associate to HTMLCanvasElement now. I rename it to HTMLCanvasElementObserver to avoid confusing.
Comment 210 Morris Tseng [:mtseng] [:Morris] 2015-07-15 02:02:47 PDT
Created attachment 8633970 [details] [diff] [review]
Part 1: Let ImageBridge transfer CanvasClient async v7.

Rebased.
Comment 211 Morris Tseng [:mtseng] [:Morris] 2015-07-15 02:08:52 PDT
Created attachment 8633977 [details] [diff] [review]
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v10. (carry sr+: ehsan)

Addressed reviewer's comment.

This patch fix two problems which found by the try run. Two problems are related to thread issue.
1. CrashReporter::AppendAppNotesToCrashReport only can be run on main-thread. So I create a runnable to dispatch it to main-thread. Please see changes in gfxCrashReporterUtils.cpp.
2. ContentChild::SendGetGraphicsFeatureStatus only run on main-thread as well. Please see changes in GfxInfoBase.cpp. I added GetGraphicsFeatureStatusRunnable to handle this problem.
Comment 212 Morris Tseng [:mtseng] [:Morris] 2015-07-15 19:35:57 PDT
Comment on attachment 8633970 [details] [diff] [review]
Part 1: Let ImageBridge transfer CanvasClient async v7.

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

I forgot that this patch isn't just rebase. There is some modification describe below. Nical, can you review those changes?

::: gfx/layers/client/CanvasClient.cpp
@@ +422,5 @@
>      gfxCriticalError() << "Failed to allocate a TextureClient for SharedSurface Canvas. Size: " << aSize;
>      return;
>    }
>  
> +  mNewFront = newFront;

So, UpdateAsync focus on preparing texture for Compositor only. It doesn't send ipc.

@@ +428,5 @@
> +
> +void
> +CanvasClientSharedSurface::Updated()
> +{
> +  auto forwarder = GetForwarder();

Updated() focus on sending ipc to compositor.

::: gfx/layers/ipc/ImageBridgeChild.cpp
@@ +490,5 @@
> +
> +// static
> +void ImageBridgeChild::UpdateAsyncCanvasRenderer(AsyncCanvasRenderer* aWrapper)
> +{
> +  aWrapper->GetCanvasClient()->UpdateAsync(aWrapper);

This function is called by worker thread and UpdateAsync() might use GLContext and GLContext must use with its creation thread or it will crash. So this function have to call on worker thread. So I moved it to here. This function call were in the UpdateAsyncCanvasRendererSync in previous patch which will crash program since UpdateAsyncCanvasRendererSync happened on ImageBridgeThread.

@@ +517,5 @@
> +void ImageBridgeChild::UpdateAsyncCanvasRendererNow(AsyncCanvasRenderer* aWrapper)
> +{
> +  MOZ_ASSERT(aWrapper);
> +  sImageBridgeChildSingleton->BeginTransaction();
> +  aWrapper->GetCanvasClient()->Updated();

Call Updated() instead of UpdateAsync() to send texture to compositor by IPC.
Comment 213 Jeff Gilbert [:jgilbert] 2015-07-16 18:54:38 PDT
Comment on attachment 8627590 [details] [diff] [review]
Part 1: Let ImageBridge transfer CanvasClient async v6. (carry r+: nical)

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

::: dom/html/HTMLCanvasElement.cpp
@@ +52,5 @@
>  HTMLImageOrCanvasOrVideoElement;
>  
> +typedef std::map<mozilla::layers::AsyncCanvasRenderer*,
> +                 mozilla::dom::HTMLCanvasElement*> AsyncCanvasMap;
> +AsyncCanvasMap gAsyncCanvasMap;

What?! Why not just add back-refs?

::: gfx/layers/AsyncCanvasRenderer.cpp
@@ +79,5 @@
> +
> +void
> +AsyncCanvasRenderer::SetGLContext(gl::GLContext* aContext)
> +{
> +  mGLContext = aContext;

There's no use making this a separate function if the setter is trivial. Just make the member public.

::: gfx/layers/AsyncCanvasRenderer.h
@@ +56,5 @@
> +  }
> +
> +  void SetIsOpaque(uint32_t aIsOpaque)
> +  {
> +    mIsOpaque = aIsOpaque;

Trivial getter+setter means we can just make the member public.
Comment 214 Jeff Gilbert [:jgilbert] 2015-07-16 18:59:13 PDT
Comment on attachment 8633977 [details] [diff] [review]
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v10. (carry sr+: ehsan)

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

Over to :nical to review the lifetimes of the ImageBridge stuff, and over to :jrmuizel to either review the blacklisting stuff, or find the right person.

::: dom/canvas/CanvasRenderingContextHelper.h
@@ +57,5 @@
> +
> +  virtual already_AddRefed<nsICanvasRenderingContextInternal>
> +  CreateContext(CanvasContextType aContextType);
> +
> +  virtual nsIntSize GetWidthHeight() = 0;

Why isn't this GetSize?

::: dom/canvas/OffscreenCanvas.cpp
@@ +109,5 @@
> +  if (mCanvasRenderer && mCurrentContext && ImageBridgeChild::IsCreated()) {
> +    TextureFlags flags = TextureFlags::ORIGIN_BOTTOM_LEFT;
> +
> +    mCanvasClient = ImageBridgeChild::GetSingleton()->
> +      CreateCanvasClient(CanvasClient::CanvasClientTypeShSurf, flags).take();

I'm making :nical sign-off on this bit.

::: dom/html/HTMLCanvasElement.cpp
@@ +678,5 @@
> +  }
> +
> +  if (!mOffscreenCanvas) {
> +    nsIntSize sz = GetWidthHeight();
> +    AsyncCanvasRenderer* renderer = GetAsyncCanvasRenderer();

Generally, when dealing with cross-thread refcounted objects, always take a strong reference.

@@ +944,5 @@
> +  // The address of gOffscreenCanvasLayerUserData is used as the user
> +  // data key for retained LayerManagers managed by FrameLayerBuilder.
> +  // We don't much care about what value in it, so just assign a dummy
> +  // value for it.
> +  static uint8_t gOffscreenCanvasLayerUserData = 0;

Again, please add 'dummy' to the name of the variable, since it's a dummy variable.

's' is the prefix for static, not 'g'.

::: gfx/thebes/gfxPrefs.h
@@ +213,5 @@
> +  DECL_GFX_PREF(Live, "gfx.blacklist.stagefright",             BlackListStagefright, int32_t, 2);
> +  DECL_GFX_PREF(Live, "gfx.blacklist.webgl.angle",             BlackListWebGLANGLE, int32_t, 2);
> +  DECL_GFX_PREF(Live, "gfx.blacklist.webgl.msaa",              BlackListWebGLMSAA, int32_t, 2);
> +  DECL_GFX_PREF(Live, "gfx.blacklist.webgl.opengl",            BlackListWebGLOpenGL, int32_t, 2);
> +  DECL_GFX_PREF(Live, "gfx.blacklist.webrtc.hw.acceleration",  BlackListWebRTCHWAccel, int32_t, 2);

I have no experiences with these, and so can't review them.

::: widget/GfxInfoBase.cpp
@@ +209,5 @@
>  
>    return name;
>  }
>  
> +typedef int32_t (*blacklistPrefFunction)();

Typedefs should end in 'T'.
Also pfnBlacklistPrefGetterT is probably a better name.

@@ +217,5 @@
> +{
> +  blacklistPrefFunction result = nullptr;
> +  switch(aFeature) {
> +    case nsIGfxInfo::FEATURE_DIRECT2D:
> +      result = &gfxPrefs::BlackListDirect2D;

Why not just return the pref directly?

@@ +796,5 @@
> +      new GetGraphicsFeatureStatusRunnable(aFeature, aStatus, &barrier, &done);
> +    if (!NS_IsMainThread()) {
> +      NS_DispatchToMainThread(runnable);
> +      while (!done) {
> +        barrier.Wait();

Noooooo, we really can't synchronously wait on a event round-trip to the main thread every time we query this.

Instead we need a threadsafe wrapper around this.

That said, since this doesn't affect normal WebGL, and only makes startup for WebGL-in-Workers bad, so we can wait to do a follow-up here. (Please file a bug for this, though!)
Comment 215 Morris Tseng [:mtseng] [:Morris] 2015-07-20 02:45:16 PDT
Created attachment 8635949 [details] [diff] [review]
Part 1: Let ImageBridge transfer CanvasClient async v8.

Addressed jgilbert's comment. Include:
1. Use public members for trivial getter/setter.
2. Use back ref for HTMLCanvasElement instead of a weird map.
3. In PImageBridge.ipdl, let second parameter of PCompositable to be nullable to resovled a crash during runtime.

:nical, Can you review this patch based on this comment and comment 212? Thanks.
Comment 216 Morris Tseng [:mtseng] [:Morris] 2015-07-20 02:49:37 PDT
Created attachment 8635952 [details] [diff] [review]
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v11. (carry r+: jgilbert)

Addressed jgilbert's comment.

1. Using nsRefPtr to hold cross-thread refcounted objects.
2. Rename gOffscreenCanvasLayerUserData. Now is sOffscreenCanvasLayerUserDataDummy.
3. Don't use function pointer in GfxInfoBase.cpp. Return pref directly now.

:jrmuizel, :nical, Can you review this patch per comment 214? Thanks.
Comment 217 Morris Tseng [:mtseng] [:Morris] 2015-07-20 23:43:33 PDT
Created attachment 8636434 [details] [diff] [review]
Part 6: Add frame ID to CanvasClient so compositor could update frame correctly.

ImageHost rely on mFrameID to determine this frame would be rendered by compositor or not. I added it so that webgl on worker will render correctly.
Comment 218 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2015-07-21 03:29:33 PDT
Comment on attachment 8636434 [details] [diff] [review]
Part 6: Add frame ID to CanvasClient so compositor could update frame correctly.

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

::: gfx/layers/LayerTreeInvalidation.cpp
@@ +386,5 @@
>    IntRect mBounds;
>  };
>  
> +template<class T>
> +static ImageHost* GetImageHost(T* aLayer)

Can't this just be Layer* aLayer?

::: gfx/layers/client/CanvasClient.cpp
@@ +125,5 @@
>      CompositableForwarder::TimedTextureClient* t = textures.AppendElement();
>      t->mTextureClient = mBuffer;
>      t->mPictureRect = nsIntRect(nsIntPoint(0, 0), mBuffer->GetSize());
> +    // Use address of buffer as frame ID
> +    t->mFrameID = reinterpret_cast<uintptr_t>(mBuffer.get());

See the comments around frameID in ImageContainer.h. Frame IDs need to increase monotonically, so you can't just use a cast like this.

Store a uint32 mFrameID somewhere and increment it every time you need one here.
Comment 219 Morris Tseng [:mtseng] [:Morris] 2015-07-21 20:14:30 PDT
Created attachment 8636928 [details] [diff] [review]
Part 6: Add frame ID to CanvasClient so compositor could update frame correctly v2.

Addressed roc's comment.
Comment 220 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2015-07-21 21:54:50 PDT
Comment on attachment 8636928 [details] [diff] [review]
Part 6: Add frame ID to CanvasClient so compositor could update frame correctly v2.

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

::: gfx/layers/client/CanvasClient.h
@@ +78,5 @@
>  
>    virtual void Updated() { }
> +
> +protected:
> +  int32_t mTextureID;

Call this mFrameID to avoid confusion.
Comment 221 Morris Tseng [:mtseng] [:Morris] 2015-07-21 22:58:36 PDT
Created attachment 8636976 [details] [diff] [review]
Part 6: Add frame ID to CanvasClient so compositor could update frame correctly v3. (carry r+: roc)

Change mTextureID to mFrameID.
Comment 222 Nicolas Silva [:nical] 2015-07-24 02:39:49 PDT
Comment on attachment 8635952 [details] [diff] [review]
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v11. (carry r+: jgilbert)

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

::: dom/canvas/WebGLContext.cpp
@@ +1526,5 @@
>      {
>      }
>  
>      NS_IMETHOD Run() {
> +        if (mWebGL)

nit: I cringe when I see ifs that are not braced (I know the Webgl/gl context code is full of these nasty things).
Comment 223 Morris Tseng [:mtseng] [:Morris] 2015-07-27 02:51:05 PDT
Created attachment 8639208 [details] [diff] [review]
Part 7: Always create a layer for offscreen canvas.

Offscreen canvas only works with an individual layer. So when canvas is async, force creating a layer for it.
Comment 224 Jeff Gilbert [:jgilbert] 2015-07-27 12:28:07 PDT
(In reply to Nicolas Silva [:nical] from comment #222)
> Comment on attachment 8635952 [details] [diff] [review]
> Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v11.
> (carry r+: jgilbert)
> 
> Review of attachment 8635952 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLContext.cpp
> @@ +1526,5 @@
> >      {
> >      }
> >  
> >      NS_IMETHOD Run() {
> > +        if (mWebGL)
> 
> nit: I cringe when I see ifs that are not braced (I know the Webgl/gl
> context code is full of these nasty things).

It is industry-standard C++, and we used it in Gecko all the time before early 2014. (before the "goto fail" scare, which was a code review failure more than anything)
Comment 225 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2015-07-27 19:20:18 PDT
(In reply to Jeff Gilbert [:jgilbert] from comment #224)
> It is industry-standard C++, and we used it in Gecko all the time before
> early 2014. (before the "goto fail" scare, which was a code review failure
> more than anything)

Actually our C++ style guide has required bracing for all conditional statements since August 2009:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style$compare?locale=en-US&to=7315&from=7314
I've been enforcing it in reviews since around then, if not earlier. Even before then we required braces for conditional statements other than "break", "return" and "continue", though I can't be bothered to track down a reference for that.

"Industry-standard C++" is irrelevant; the industry standard for C++ is for every large organization to come up with their own style rules and enforce them :-).
Comment 226 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2015-07-27 19:25:14 PDT
Comment on attachment 8639208 [details] [diff] [review]
Part 7: Always create a layer for offscreen canvas.

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

::: layout/generic/nsHTMLCanvasFrame.cpp
@@ +130,5 @@
>  
> +    HTMLCanvasElement* elem =
> +      static_cast<HTMLCanvasElement*>(mFrame->GetContent());
> +    if (elem->IsAsync()) {
> +      return mozilla::LAYER_ACTIVE_FORCE;

So this is a bit of a problem :-(. This means that in certain situations rendering will be broken. It would be really good if we can have fallback rendering for the case where the layer is not being sent to the compositor, i.e. it's a BasicCanvasLayer instead of a ClientCanvasLayer.

If that's impossible or very difficult for some reason, let's discuss that and maybe we should land this as-is, but I'd like to investigate the problem first.
Comment 227 Morris Tseng [:mtseng] [:Morris] 2015-07-28 02:46:57 PDT
Ah, right. I'll upload a new version which supports BasicCanvasLayer.
Comment 228 Morris Tseng [:mtseng] [:Morris] 2015-07-28 02:47:12 PDT
Created attachment 8639764 [details] [diff] [review]
Part 7: If layer is not available, fallback to BasicCanvasLayer.
Comment 229 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2015-07-28 04:30:23 PDT
Comment on attachment 8639764 [details] [diff] [review]
Part 7: If layer is not available, fallback to BasicCanvasLayer.

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

This needs more comments to explain how it works.

I'm not actually sure I know how to make this fallback work well. The problem is that whether the layer is a BasicLayer or not can change on the main thread at any time, e.g. due to CSS changes, and I don't think this code handles that. I think perhaps the best way to make this work is to have the worker send frames to the compositor and also notify the main thread that the canvas element has been updated. On the main thread, those updates would check if a BasicLayer is being used, and if it is, we'd invalidate the canvas to do our normal painting. Every time BasicCanvasLayer needs to render, it would have to read back the current image from the async renderer (which we need to support for toDataURL and stuff like that, anyway).

::: gfx/layers/Layers.h
@@ +2190,5 @@
>        , mHasAlpha(false)
>        , mIsGLAlphaPremult(true)
>      { }
>  
>      // One of these two must be specified for Canvas2D, but never both

"One of these three must be specified for Canvas2D, but never more than one"
Comment 230 Jeff Muizelaar [:jrmuizel] 2015-07-28 07:23:26 PDT
Comment on attachment 8635952 [details] [diff] [review]
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v11. (carry r+: jgilbert)

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

::: gfx/src/gfxCrashReporterUtils.cpp
@@ +91,5 @@
> +  {
> +  }
> +
> +  NS_IMETHOD Run() override {
> +    CrashReporter::AppendAppNotesToCrashReport(mFeatureString);

AppendAppNotesToCrashReport is thread safe so you shouldn't need this Runnable.

::: widget/GfxInfoBase.cpp
@@ +780,1 @@
>      *aStatus = FEATURE_STATUS_OK;

Can we just dispatch the whole call to GetFeatureStatus to the main thread instead of trying to make it thread safe?
Comment 231 Morris Tseng [:mtseng] [:Morris] 2015-07-28 20:14:40 PDT
(In reply to Jeff Muizelaar [:jrmuizel] from comment #230)
> Comment on attachment 8635952 [details] [diff] [review]
> Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v11.
> (carry r+: jgilbert)
> 
> Review of attachment 8635952 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/src/gfxCrashReporterUtils.cpp
> @@ +91,5 @@
> > +  {
> > +  }
> > +
> > +  NS_IMETHOD Run() override {
> > +    CrashReporter::AppendAppNotesToCrashReport(mFeatureString);
> 
> AppendAppNotesToCrashReport is thread safe so you shouldn't need this
> Runnable.
https://dxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/nsExceptionHandler.cpp#1834

This code says "Cannot call AnnotateCrashReport in child processes from non-main thread.". Without this runnable, e10s will crash because this assertion.
> 
> ::: widget/GfxInfoBase.cpp
> @@ +780,1 @@
> >      *aStatus = FEATURE_STATUS_OK;
> 
> Can we just dispatch the whole call to GetFeatureStatus to the main thread
> instead of trying to make it thread safe?
OK, I'll try this.
Comment 232 Morris Tseng [:mtseng] [:Morris] 2015-07-28 20:45:38 PDT
Created attachment 8640290 [details] [diff] [review]
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v12. (carry r+: jgilbert, nical)

Addressed :jrmuizel's comment.
Comment 233 Morris Tseng [:mtseng] [:Morris] 2015-07-29 02:01:32 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #229)
> Comment on attachment 8639764 [details] [diff] [review]
> Part 7: If layer is not available, fallback to BasicCanvasLayer.
> 
> Review of attachment 8639764 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This needs more comments to explain how it works.
> 
> I'm not actually sure I know how to make this fallback work well. The
> problem is that whether the layer is a BasicLayer or not can change on the
> main thread at any time, e.g. due to CSS changes, and I don't think this
> code handles that. I think perhaps the best way to make this work is to have
> the worker send frames to the compositor and also notify the main thread
> that the canvas element has been updated. On the main thread, those updates
> would check if a BasicLayer is being used, and if it is, we'd invalidate the
> canvas to do our normal painting. Every time BasicCanvasLayer needs to
> render, it would have to read back the current image from the async renderer
> (which we need to support for toDataURL and stuff like that, anyway).
Here is detail of this patch,
When context.commit() is called in worker, worker do following things:
1. If attributes are changed (e.g. width or height), notify main thread to update layer size. AsyncCanvasRenderer::NotifyElementAboutAttributesChanged is for doing this.
2. Notify main thread the content is dirty as well. So the layer will repaint. Please refer to AsyncCanvasRenderer::NotifyElementAboutInvalidation();
3. If layers backend is LAYER_BASIC, we read back webgl's result to AsyncCanvasRenderer::mSurface.

And when BasicCanvasLayer::Paint is called in main thread, we do:
1. Get the surface from AsyncCanvasRenderer::mSurface
2. Paint this surface to DrawTarget as old code.

The problem is, step 3 in worker and step 1 in main thread access same member(e.g. AsyncCanvasRenderer::mSurface). To avoid race condition, I create a mutex(AsyncCanvasRenderer::mSurfaceMutex) to protect it.

Does it make sense to make the fallback work? I'll upload a patch with more detailed comments. Thanks.
Comment 234 Morris Tseng [:mtseng] [:Morris] 2015-07-29 02:04:09 PDT
Created attachment 8640371 [details] [diff] [review]
Part 7: If layer is not available, fallback to BasicCanvasLayer v2.

Add more detailed comments.
Comment 235 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2015-07-29 02:55:12 PDT
(In reply to Morris Tseng [:mtseng] from comment #233)
> When context.commit() is called in worker, worker do following things:
> 1. If attributes are changed (e.g. width or height), notify main thread to
> update layer size. AsyncCanvasRenderer::NotifyElementAboutAttributesChanged
> is for doing this.
> 2. Notify main thread the content is dirty as well. So the layer will
> repaint. Please refer to
> AsyncCanvasRenderer::NotifyElementAboutInvalidation();
> 3. If layers backend is LAYER_BASIC, we read back webgl's result to
> AsyncCanvasRenderer::mSurface.
> 
> And when BasicCanvasLayer::Paint is called in main thread, we do:
> 1. Get the surface from AsyncCanvasRenderer::mSurface
> 2. Paint this surface to DrawTarget as old code.
> 
> The problem is, step 3 in worker and step 1 in main thread access same
> member(e.g. AsyncCanvasRenderer::mSurface). To avoid race condition, I
> create a mutex(AsyncCanvasRenderer::mSurfaceMutex) to protect it.

OK, but what if the layers backend is LAYERS_CLIENT when commit() is called, but then the content changes the layer to LAYERS_BASIC (e.g. because CSS changes so that the layer must be rendered using content-side fallback), before commit() is called again? That's why I think the readback needs to be on the content main thread, not in the commit() call.

Also, does canvas.toDataURL() work when the backend is LAYERS_CLIENT? There should be a test for this.
Comment 236 Morris Tseng [:mtseng] [:Morris] 2015-07-29 03:13:15 PDT
Comment on attachment 8640371 [details] [diff] [review]
Part 7: If layer is not available, fallback to BasicCanvasLayer v2.

I didn't aware those situations. So clear the review flags first. Thanks for the comments.
Comment 237 Jeff Muizelaar [:jrmuizel] 2015-07-29 11:39:13 PDT
Comment on attachment 8640290 [details] [diff] [review]
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v12. (carry r+: jgilbert, nical)

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

::: widget/GfxInfoBase.cpp
@@ +750,5 @@
> +      barrier.Wait();
> +    }
> +
> +    return runnable->GetNSResult();
> +  }

I'd rather this be in the caller instead of in GfxInfoBase
Comment 238 Morris Tseng [:mtseng] [:Morris] 2015-08-18 02:45:00 PDT
Created attachment 8649178 [details] [diff] [review]
Part 8: Copy to a temp texture when readback from IOSurface.
Comment 239 Morris Tseng [:mtseng] [:Morris] 2015-08-19 00:48:07 PDT
Created attachment 8649718 [details] [diff] [review]
Part 7: If layer is not available, fallback to BasicCanvasLayer v3.

Addressed reviewer's comment.
Comment 240 Morris Tseng [:mtseng] [:Morris] 2015-08-19 00:52:24 PDT
(In reply to Jeff Muizelaar [:jrmuizel] from comment #237)
> Comment on attachment 8640290 [details] [diff] [review]
> Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v12.
> (carry r+: jgilbert, nical)
> 
> Review of attachment 8640290 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/GfxInfoBase.cpp
> @@ +750,5 @@
> > +      barrier.Wait();
> > +    }
> > +
> > +    return runnable->GetNSResult();
> > +  }
> 
> I'd rather this be in the caller instead of in GfxInfoBase
GetNSResult returns the result of GetFeatureStatus. So I have to return it in GfxInfoBase. How do I return it in the caller?
Comment 241 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2015-08-19 14:27:06 PDT
Comment on attachment 8649718 [details] [diff] [review]
Part 7: If layer is not available, fallback to BasicCanvasLayer v3.

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

::: gfx/layers/AsyncCanvasRenderer.h
@@ +51,5 @@
> + * fallback to BasicLayerManager or calling toDataURL in Javascript. Simply call
> + * UpdateTarget() will readback the result to mSurface.
> + *
> + * If layers backend is LAYERS_CLIENT, this object will pass to ImageBridgeChild
> + * for submitting frames to Compositor.

I think this is looking good, but we need a lot more documentation and assertions about the behavior of AsyncCanvasRenderer.

In particular, I want to see comments explaining which thread(s) each method can be called on, and which thread(s) each member of AsyncCanvasRenderer can be accessed on, and which locks must be held when accessing each member. Each method that should be called on particular thread(s) should assert that.

We also need tests --- reftests I think --- to ensure that the BasicCanvasLayer fallback works and keeps working. One way to do that that works for now would be to wrap the canvas in an SVG clip-path and filter.
Comment 242 Morris Tseng [:mtseng] [:Morris] 2015-08-24 01:00:11 PDT
Created attachment 8651643 [details] [diff] [review]
Part 7: If layer is not available, fallback to BasicCanvasLayer v4.

Add more comments and assertions.
Add reftest to test fallback case.
Comment 243 Morris Tseng [:mtseng] [:Morris] 2015-08-24 01:02:13 PDT
Created attachment 8651645 [details] [diff] [review]
Part 4 interdiff.
Comment 244 Morris Tseng [:mtseng] [:Morris] 2015-08-24 01:09:04 PDT
Created attachment 8651649 [details] [diff] [review]
Part 4: Mochitests for offscreencanvas.

Fix :ehsan's comment. Interdiff is provided in comment 243.
:jgilbert, can you review the code about webgl? Thanks.
Comment 245 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2015-08-24 02:50:49 PDT
Comment on attachment 8651643 [details] [diff] [review]
Part 7: If layer is not available, fallback to BasicCanvasLayer v4.

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

Closer, but we still need more work :-)

::: dom/canvas/OffscreenCanvas.cpp
@@ +74,5 @@
> +
> +    if (mCanvasRenderer) {
> +      mCanvasRenderer->SetCanvasClient(nullptr);
> +      mCanvasRenderer->mContext = nullptr;
> +      mCanvasRenderer->mGLContext = nullptr;

What thread is this called on and how do we know that no other thread can be accessing these values?

::: dom/canvas/WebGLContext.cpp
@@ +1218,5 @@
> +            layers::LayerManager* layerManager = docWidget->GetLayerManager();
> +            return layerManager->GetCompositorBackendType();
> +        }
> +    } else if (mOffscreenCanvas) {
> +        return mOffscreenCanvas->GetLayersBackend();

Seems to me that the main thread updates mLayersBackend and then the worker thread can read it here with no synchronization. So this could be called on the worker, and then whatever it returns could immediately become wrong because the main thread changes it. In particular we could return CLIENT here and then the main thread decides to change it to BASIC. Likewise we could return BASIC here while the main thread changes it to CLIENT. So whatever this gets used for, it will be wrong.

Actually it seems to me that we're computing the wrong backend type in this code. This is supposed to return the backend used by the compositor, which does *not* depend on the layer type chosen by the content process. So what we should do is compute the compositor backend type once, from the element using the code that's already there, and store that backend type in the OffscreenCanvas and make sure the WebGLContext always has it.

We'll want to have some reftests that test a dynamic transition from normal rendering to fallback rendering, and vice versa, while the worker is continuously producing frames.

::: gfx/layers/AsyncCanvasRenderer.cpp
@@ +325,5 @@
> +
> +  format = imgIEncoder::INPUT_FORMAT_HOSTARGB;
> +
> +  return dom::ImageEncoder::GetInputStream(mWidth, mHeight, imageBuffer, format,
> +                                           encoder, aEncoderOptions, aStream);

Can't we share more of this code with the rest of the canvas code? Seems to me the existing canvas code could just get the SourceSurface using a thread-safe method (see below) and proceed from there.

::: gfx/layers/AsyncCanvasRenderer.h
@@ +123,5 @@
> +    return mActiveThread;
> +  }
> +
> +  // Indicate the backend type of layer which belong to this renderer
> +  LayersBackend mBackend;

Which thread(s) can access this?

@@ +129,2 @@
>    // The lifetime is controllered by HTMLCanvasElement.
>    dom::HTMLCanvasElement* mHTMLCanvasElement;

Comment that this can only be accessed from the main thread.

@@ +129,4 @@
>    // The lifetime is controllered by HTMLCanvasElement.
>    dom::HTMLCanvasElement* mHTMLCanvasElement;
>  
>    nsICanvasRenderingContextInternal* mContext;

Which thread(s) can this be accessed on?

@@ +132,5 @@
>    nsICanvasRenderingContextInternal* mContext;
>  
>    // We need to keep a reference to the context around here, otherwise the
>    // canvas' surface texture destructor will deref and destroy it too early
>    RefPtr<gl::GLContext> mGLContext;

Can only the active thread access this?

::: gfx/layers/CopyableCanvasLayer.cpp
@@ +84,5 @@
>  CopyableCanvasLayer::UpdateTarget(DrawTarget* aDestTarget)
>  {
> +  if (mAsyncRenderer) {
> +    mAsyncRenderer->UpdateTarget();
> +    mSurface = mAsyncRenderer->GetSurface();

So this method must only be called when mAsyncRenderer::mIsSurfaceMutexLocked is true?

Seems like it would be simpler to give mAsyncRenderer a method that a) gets the lock b) does UpdateTarget and GetSurface() c) releases the lock and d) returns the surface, and just call that from here. Then we wouldn't need the GetSurfaceHelper stuff in BasicCanvasLayer. Or is there a reason why we need to lock the AsyncRenderer all the way through BasicCanvasLayer::Paint?
Comment 246 :Ehsan Akhgari 2015-08-24 06:50:33 PDT
Comment on attachment 8651649 [details] [diff] [review]
Part 4: Mochitests for offscreencanvas.

I'm sorry, but as my bugzilla name suggests, I don't have the bandwidth to do reviews these days.  I'd appreciate if you asked someone else please.
Comment 247 Andrea Marchesini [:baku] 2015-08-24 07:35:28 PDT
Comment on attachment 8651649 [details] [diff] [review]
Part 4: Mochitests for offscreencanvas.

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

::: dom/canvas/test/offscreencanvas.js
@@ +1,1 @@
> +

no extra line.

@@ +2,5 @@
> +/* WebWorker for test_offscreencanvas_*.html */
> +var port = null;
> +
> +function ok(expect, msg) {
> +    if (port) {

usually tests are indented with 2 spaces.
Comment 248 Morris Tseng [:mtseng] [:Morris] 2015-08-25 00:00:52 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #245)
> ::: gfx/layers/CopyableCanvasLayer.cpp
> @@ +84,5 @@
> >  CopyableCanvasLayer::UpdateTarget(DrawTarget* aDestTarget)
> >  {
> > +  if (mAsyncRenderer) {
> > +    mAsyncRenderer->UpdateTarget();
> > +    mSurface = mAsyncRenderer->GetSurface();
> 
> So this method must only be called when
> mAsyncRenderer::mIsSurfaceMutexLocked is true?
> 
> Seems like it would be simpler to give mAsyncRenderer a method that a) gets
> the lock b) does UpdateTarget and GetSurface() c) releases the lock and d)
> returns the surface, and just call that from here. Then we wouldn't need the
> GetSurfaceHelper stuff in BasicCanvasLayer. Or is there a reason why we need
> to lock the AsyncRenderer all the way through BasicCanvasLayer::Paint?
Yap, [1] need use mSurface as source to draw to a drawtarget. So at least we need to lock the surface until drawing is over. So if we only lock the AsyncRenderer in UpdateTarget which is described above, we need another lock when BasicCanvasLayer::Paint() using the mSurface. Does it make sense?

[1]: https://dxr.mozilla.org/mozilla-central/source/gfx/layers/basic/BasicCanvasLayer.cpp?#53
Comment 249 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2015-08-25 15:32:40 PDT
(In reply to Morris Tseng [:mtseng] from comment #248)
> Yap, [1] need use mSurface as source to draw to a drawtarget. So at least we
> need to lock the surface until drawing is over. So if we only lock the
> AsyncRenderer in UpdateTarget which is described above, we need another lock
> when BasicCanvasLayer::Paint() using the mSurface. Does it make sense?

No :-). Are you saying that if we return the mSurface to BasicCanvasLayer, and while BasicCanvasLayer is using it, the worker commit()s a new frame, the contents of mSurface will be modified while BasicCanvasLayer is using it? Because that should not happen; SourceSurfaces are supposed to be immutable.
Comment 250 Morris Tseng [:mtseng] [:Morris] 2015-08-26 02:54:28 PDT
Created attachment 8652767 [details] [diff] [review]
part 7 interdiff.
Comment 251 Morris Tseng [:mtseng] [:Morris] 2015-08-26 02:56:38 PDT
Created attachment 8652769 [details] [diff] [review]
Part 7: If layer is not available, fallback to BasicCanvasLayer v5.

:roc, thanks for your comments. Here is updated version. Interdiff is provided at previous comment.
Comment 252 Morris Tseng [:mtseng] [:Morris] 2015-08-26 02:59:48 PDT
Created attachment 8652774 [details] [diff] [review]
Part 4: Mochitests for offscreencanvas v4. (carry r+: baku)

Fix indent and remove extra line.
Comment 253 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2015-08-26 04:05:50 PDT
Comment on attachment 8652769 [details] [diff] [review]
Part 7: If layer is not available, fallback to BasicCanvasLayer v5.

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

::: gfx/layers/AsyncCanvasRenderer.h
@@ +84,5 @@
> +  // Active thread means the thread which spawns GLContext.
> +  void SetActiveThread();
> +  void ResetActiveThread();
> +
> +  already_AddRefed<gfx::DataSourceSurface> GetAndUpdateSurface();

Document this.
Comment 254 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2015-08-26 04:07:41 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #245)
> ::: gfx/layers/AsyncCanvasRenderer.cpp
> @@ +325,5 @@
> > +
> > +  format = imgIEncoder::INPUT_FORMAT_HOSTARGB;
> > +
> > +  return dom::ImageEncoder::GetInputStream(mWidth, mHeight, imageBuffer, format,
> > +                                           encoder, aEncoderOptions, aStream);
> 
> Can't we share more of this code with the rest of the canvas code? Seems to
> me the existing canvas code could just get the SourceSurface using a
> thread-safe method (see below) and proceed from there.

It looks like you didn't address this comment.
Comment 255 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2015-08-26 04:23:01 PDT
Comment on attachment 8652769 [details] [diff] [review]
Part 7: If layer is not available, fallback to BasicCanvasLayer v5.

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

::: dom/canvas/test/reftest/offscreencanvas-fallback.html
@@ +7,5 @@
> +<canvas id="c" width="64" height="64"></canvas>
> +<script>
> +function doTest() {
> +  var htmlCanvas = document.getElementById("c");
> +  htmlCanvas.style.mask = "url('offscreencanvas_mask.svg#fade_mask_both')";

If this test is intended to test dynamic fallback, it doesn't really, because we transfer the canvas to the worker at the same time as we start fallback.

We should transfer control to the worker, render a few frames in the worker, then trigger fallback, render a few more frames, then end the test.

::: gfx/layers/AsyncCanvasRenderer.cpp
@@ +200,5 @@
> +      return;
> +    }
> +
> +    while (!done) {
> +      barrier.Wait();

Having the main thread wait for the worker like this seems to me like it will introduce a risk of deadlock :-(.

@@ +254,5 @@
> +{
> +  MutexAutoLock lock(mMutex);
> +  UpdateTarget();
> +  RefPtr<gfx::DataSourceSurface> result = mSurface;
> +  return result.forget();

There's a problem here, which is that SourceSurfaces aren't thread-safe. Their refcounts aren't even thread-safe, so we can't allow the active thread and the main thread to both a a reference to the same surface.

It's unfortunate, I know, but I think that means we need to copy the surface here, or something like that.

::: gfx/layers/AsyncCanvasRenderer.h
@@ +48,5 @@
>   * OffscreenCanvas will keep reference pointer of this object.
> + *
> + * Sometimes main thread needs AsyncCanvasRenderer's result, such as layers
> + * fallback to BasicLayerManager or calling toDataURL in Javascript. Simply call
> + * UpdateTarget() will readback the result to mSurface.

I think you need to fix this comment to refer to GetAndUpdateSurface?

@@ +84,5 @@
> +  // Active thread means the thread which spawns GLContext.
> +  void SetActiveThread();
> +  void ResetActiveThread();
> +
> +  already_AddRefed<gfx::DataSourceSurface> GetAndUpdateSurface();

Probably should just call this GetSurface(). And document that it can be called on any thread, or at least the main thread.
Comment 256 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2015-08-26 04:30:21 PDT
This potential deadlock is a tough problem. I'm a bit too tired to figure it out right now, but here are the constraints as I understand them:
1) The GL context and its objects can only be accessed on the worker's thread. Or at least, acquiring the AsyncRenderer mutex is not enough to access them safely since they can be accessed by the worker in other ways.
2) When the main thread paints we may need to get some DataSourceSurface representing the read-back state of the canvas. It doesn't have to be the most up-to-date state, actually, as long as we don't render any data that's older than a previous frame we rendered, and we don't get too far behind.
3) The main thread should not block on the worker thread to get the surface, since the worker thread might block on the main thread. Even if the worker thread can't block on the main thread, we don't want to delay main thread painting for arbitrarily long.
4) We don't want to do any readbacks if the main thread never requires fallback.

Kyle, is #3 correct?
Comment 257 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2015-08-26 04:49:30 PDT
Would it be possible for the main thread to do a sync IPC to the ImageBridge thread and do a readback to a SourceSurface there?
Comment 258 Nicolas Silva [:nical] 2015-08-26 07:22:55 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #257)
> Would it be possible for the main thread to do a sync IPC to the ImageBridge
> thread and do a readback to a SourceSurface there?

Yes, you can do that sort of things as long as you never make the ImageBridge thread wait for the main thread. We don't have "IPDL" kind of IPC between the main thread and ImageBridge but we can do synchronous proxies with the message loop and a monitor like this: https://dxr.mozilla.org/mozilla-central/rev/f61c3cc0eb8b7533818e7379ccc063b611015d9d/gfx/layers/ipc/ImageBridgeChild.cpp#681

I assume the question was focused on the sync IPC rather than the readback part. I don't know for sure what doing the readback on the ImageBridge thread vs the worker thread implies in term the webgl implementation.
Comment 259 Morris Tseng [:mtseng] [:Morris] 2015-08-26 08:24:24 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #256)
> This potential deadlock is a tough problem. I'm a bit too tired to figure it
> out right now, but here are the constraints as I understand them:
> 1) The GL context and its objects can only be accessed on the worker's
> thread. Or at least, acquiring the AsyncRenderer mutex is not enough to
> access them safely since they can be accessed by the worker in other ways.
> 2) When the main thread paints we may need to get some DataSourceSurface
> representing the read-back state of the canvas. It doesn't have to be the
> most up-to-date state, actually, as long as we don't render any data that's
> older than a previous frame we rendered, and we don't get too far behind.
> 3) The main thread should not block on the worker thread to get the surface,
> since the worker thread might block on the main thread. Even if the worker
> thread can't block on the main thread, we don't want to delay main thread
> painting for arbitrarily long.
> 4) We don't want to do any readbacks if the main thread never requires
> fallback.
> 
> Kyle, is #3 correct?
I have a idea about this. Since we can share buffer between webgl and compositor through some shared handle (such as: gralloc, MACIOSurface, EGLImage...etc) with different GLContext and different thread. So maybe we can send this shared handle to main thread and readback using this shared handle just like sharing buffer with compositor. The whole idea is:
1) When OffscreenCanvas create a GLContext in worker thread, also create a GLContext in main thread.
2) When frame is produced by calling commit(), we just save the shared handle in AsyncCanvasRenderer.
3) When fallback happened, readback this shared handle by using GLContext of main thread which is created by step 1.

I'm not sure this idea is doable. :jgilbert, :nical, how do you think?
Comment 260 Morris Tseng [:mtseng] [:Morris] 2015-08-26 08:30:33 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #254)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment
> #245)
> > ::: gfx/layers/AsyncCanvasRenderer.cpp
> > @@ +325,5 @@
> > > +
> > > +  format = imgIEncoder::INPUT_FORMAT_HOSTARGB;
> > > +
> > > +  return dom::ImageEncoder::GetInputStream(mWidth, mHeight, imageBuffer, format,
> > > +                                           encoder, aEncoderOptions, aStream);
> > 
> > Can't we share more of this code with the rest of the canvas code? Seems to
> > me the existing canvas code could just get the SourceSurface using a
> > thread-safe method (see below) and proceed from there.
> 
> It looks like you didn't address this comment.
Ah, right. Sorry about it. I have a question about this.

Does "the rest of the canvas code" means the code at BasicCanvasLayer::Paint() which also readback from AsyncCanvasRenderer and do y-flip?
Comment 261 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2015-08-26 15:06:57 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #256)
> 3) The main thread should not block on the worker thread to get the surface,
> since the worker thread might block on the main thread. Even if the worker
> thread can't block on the main thread, we don't want to delay main thread
> painting for arbitrarily long.

> Kyle, is #3 correct?

Yes.  The worker may block on the main thread (e.g. to implement something like XHR) any time it is running script.
Comment 262 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2015-08-26 17:14:26 PDT
(In reply to Morris Tseng [:mtseng] from comment #260)
> Does "the rest of the canvas code" means the code at
> BasicCanvasLayer::Paint() which also readback from AsyncCanvasRenderer and
> do y-flip?

Presumably when we call toDataURL on a regular WebGL canvas we have code somewhere to do the readback and Y-flip. Looks like that's in WebGLContext::GetImageBuffer or thereabouts. Maybe we can use that?
Comment 263 Morris Tseng [:mtseng] [:Morris] 2015-08-27 03:16:47 PDT
Created attachment 8653387 [details] [diff] [review]
(WIP)Readback without blocking main thread.

This is prototype of comment 259. Looks like it works on my MAC. So I think this basically work on EGL, GLX, MacIOSurface and gralloc. The biggest problem is ANGLE. The SurfaceDescriptor for ANGLE is only for DirectX surface. Can we share ANGLE buffer to another GLContext?
Comment 264 Morris Tseng [:mtseng] [:Morris] 2015-08-27 04:55:10 PDT
Looks like ANGLE support eglImage. So maybe ANGLE is ok...
Comment 265 Jeff Muizelaar [:jrmuizel] 2015-09-01 15:41:48 PDT
(In reply to Morris Tseng [:mtseng] from comment #240)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #237)
> > Comment on attachment 8640290 [details] [diff] [review]
> > Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v12.
> > (carry r+: jgilbert, nical)
> > 
> > Review of attachment 8640290 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: widget/GfxInfoBase.cpp
> > @@ +750,5 @@
> > > +      barrier.Wait();
> > > +    }
> > > +
> > > +    return runnable->GetNSResult();
> > > +  }
> > 
> > I'd rather this be in the caller instead of in GfxInfoBase
> GetNSResult returns the result of GetFeatureStatus. So I have to return it
> in GfxInfoBase. How do I return it in the caller?

Have the caller dispatch the runnable and wait for it instead of GetFeatureStatus()
Comment 266 Jeff Gilbert [:jgilbert] 2015-09-01 17:01:15 PDT
Comment on attachment 8653387 [details] [diff] [review]
(WIP)Readback without blocking main thread.

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

::: gfx/layers/AsyncCanvasRenderer.cpp
@@ +181,5 @@
> +  SurfaceDescriptor desc;
> +  frontbuffer->ToSurfaceDescriptor(&desc);
> +
> +  const SurfaceDescriptorMacIOSurface& descIO =
> +    desc.get_SurfaceDescriptorMacIOSurface();

What? This is OSX-specific. Isn't this function non-OS-specific?

@@ +192,5 @@
> +  // Re-create SharedSurface with new GLContext and existing MacIOSurface.
> +  UniquePtr<gl::SharedSurface> newSurf =
> +    gl::SharedSurface_IOSurface::Create(surface, gl, !descIO.isOpaque());
> +
> +  gl->Readback(newSurf.get(), mSurface);

I believe you'd better off trying to lock the IOSurface and just memcpy out of it.
Comment 267 Morris Tseng [:mtseng] [:Morris] 2015-09-01 19:27:54 PDT
(In reply to Jeff Gilbert [:jgilbert] from comment #266)
> Comment on attachment 8653387 [details] [diff] [review]
> (WIP)Readback without blocking main thread.
> 
> Review of attachment 8653387 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/AsyncCanvasRenderer.cpp
> @@ +181,5 @@
> > +  SurfaceDescriptor desc;
> > +  frontbuffer->ToSurfaceDescriptor(&desc);
> > +
> > +  const SurfaceDescriptorMacIOSurface& descIO =
> > +    desc.get_SurfaceDescriptorMacIOSurface();
> 
> What? This is OSX-specific. Isn't this function non-OS-specific?
Yes, I test the proof of concept only on OSX. Eventually this function is non-OS-specific.
> 
> @@ +192,5 @@
> > +  // Re-create SharedSurface with new GLContext and existing MacIOSurface.
> > +  UniquePtr<gl::SharedSurface> newSurf =
> > +    gl::SharedSurface_IOSurface::Create(surface, gl, !descIO.isOpaque());
> > +
> > +  gl->Readback(newSurf.get(), mSurface);
> 
> I believe you'd better off trying to lock the IOSurface and just memcpy out
> of it.
I think this might work for IOSurface and gralloc. How about GLXPixmap and ANGLE? I'm not familiar with GLXPixmap. ANGLE provided a shared handle which is d3d texture. How can I lock it and copy?
Comment 268 Morris Tseng [:mtseng] [:Morris] 2015-09-01 20:34:06 PDT
Created attachment 8655786 [details] [diff] [review]
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v13. (carry r+: jgilbert, nical)

Move dispatch stuff to caller. Please check GetFeatureStatus in WebGLContext.cpp
Comment 269 Morris Tseng [:mtseng] [:Morris] 2015-09-02 01:37:51 PDT
> > I believe you'd better off trying to lock the IOSurface and just memcpy out
> > of it.
> I think this might work for IOSurface and gralloc. How about GLXPixmap and
> ANGLE? I'm not familiar with GLXPixmap. ANGLE provided a shared handle which
> is d3d texture. How can I lock it and copy?
I found that maybe I can use d3d api to lock shared handle. I'll try it first. clear ni.
Comment 270 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2015-09-02 04:45:49 PDT
I guess the platform-specific code to read back from a TextureClient should be part of the implementation of each TextureClient.
Comment 271 Nicolas Silva [:nical] 2015-09-02 05:09:24 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #270)
> I guess the platform-specific code to read back from a TextureClient should
> be part of the implementation of each TextureClient.

Yes. We already have a thing doing some backend specific readback in TextureClients with a ReadbackSink thingy, but it only does stuff in Unlock(). We can look into separating it and making it useful in more places. Or do we want something more like texture->GetAsSourceSurface()->GetDataSurface() kind of API?
Comment 272 Morris Tseng [:mtseng] [:Morris] 2015-09-17 01:37:49 PDT
Created attachment 8662255 [details] [diff] [review]
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v14. (carry r+: jgilbert, nical, sr+: ehsan)

Not only WebGLContext call GetFeatureStatus on worker thread, GLLibraryEGL.cpp call GetFeatureStatus on worker thread as well. So I move GetFeatureStatus runnable stuff to gfxUtils and let WebGLContext and GLLibraryEGL use those stuff. Please check WebGLContext.cpp, GLLibraryEGL.cpp, gfxUtils.h and gfxUtils.cpp for changes. Thanks.
Comment 273 Morris Tseng [:mtseng] [:Morris] 2015-09-17 01:42:29 PDT
Created attachment 8662256 [details] [diff] [review]
Part 3: Transfer OffscreenCanvas from mainthread to workers v7. (carry r+: sfink)

This patch became more cleaner with :baku's refactoring on StructuredClone stuff. :)

:baku, can you review it again? Thanks.
Comment 274 Morris Tseng [:mtseng] [:Morris] 2015-09-17 01:48:52 PDT
Created attachment 8662259 [details] [diff] [review]
Part 7: If layer is not available, fallback to BasicCanvasLayer v6.

The main change is:
1. Address comment in comment 255.
2. This patch just leave AsyncCanvasRenderer::UpdateTarget as blank. I'll implement readback without blocking main thread in patch part 9.
3. Move reftest to mochitest so that I don't need to copy offscreencanvas.js to reftest again, please check test_offscreencanvas_dynamic_fallback.html.
Comment 275 Morris Tseng [:mtseng] [:Morris] 2015-09-17 01:51:33 PDT
Created attachment 8662263 [details] [diff] [review]
Part 9: Readback without blocking main thread.

Readback content by locking shared handle directly.
Comment 276 Morris Tseng [:mtseng] [:Morris] 2015-09-17 01:54:36 PDT
Created attachment 8662267 [details] [diff] [review]
Part 10: Using mechanism in RuntimeService to get pref in worker thread instead of gfxPref.

The instance of gfxPrefs may be created too late when our code calling OffscreenCanvas::PrefEnabled() and causing crash. As :baku suggests, I using the mechanism in RuntimeServices.cpp to avoid this kind of situation.
Comment 277 Andrea Marchesini [:baku] 2015-09-17 01:57:33 PDT
Comment on attachment 8662256 [details] [diff] [review]
Part 3: Transfer OffscreenCanvas from mainthread to workers v7. (carry r+: sfink)

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

I didn't check the js/src/StructuredClone.cpp because I suspect you didn't touch it.
Can you send me the patch again with these comments applied? Thanks!

::: dom/base/StructuredCloneHelper.cpp
@@ +21,5 @@
>  #include "mozilla/dom/StructuredClone.h"
>  #include "mozilla/dom/MessagePort.h"
>  #include "mozilla/dom/MessagePortBinding.h"
>  #include "mozilla/dom/PMessagePort.h"
> +#include "mozilla/dom/OffscreenCanvas.h"

alphabetic order. Move it before 'P'MessagePort.

@@ +1055,5 @@
>      }
>  
>      aReturnObject.set(&value.toObject());
>      return true;
> +  } else if (aTag == SCTAG_DOM_CANVAS) {

no "else" after a return. Just do:

if (aTag == ..

@@ +1056,5 @@
>  
>      aReturnObject.set(&value.toObject());
>      return true;
> +  } else if (aTag == SCTAG_DOM_CANVAS) {
> +    MOZ_ASSERT(aContent);

This would work only on the same process, right?

Then do:

 MOZ_ASSERT(mContext == SameProcessSameThread ||
            mContext == SameProcessDifferentThread);

@@ +1102,5 @@
>        *aOwnership = JS::SCTAG_TMO_CUSTOM;
>        *aContent = nullptr;
>  
>        return true;
>      }

}

if (mContext == SameProcessSameThread ||
    mContext == SameProcessDifferentThread) {

@@ +1112,5 @@
> +
> +      *aExtraData = 0;
> +      *aTag = SCTAG_DOM_CANVAS;
> +      *aOwnership = JS::SCTAG_TMO_CUSTOM;
> +      *aContent = canvas->ToCloneData();

MOZ_ASSERT(*aContent);

@@ +1134,5 @@
>    if (aTag == SCTAG_DOM_MAP_MESSAGEPORT) {
>      MOZ_ASSERT(!aContent);
>      MOZ_ASSERT(aExtraData < mPortIdentifiers.Length());
>      MessagePort::ForceClose(mPortIdentifiers[aExtraData]);
> +  } else if (aTag == SCTAG_DOM_CANVAS) {

return;
}

if (aTag == SCTAG_DOM_CANVAS) {
 MOZ_ASSERT(mContext == SameProcessSameThread ||
            mContext == SameProcessDifferentThread);
Comment 278 Andrea Marchesini [:baku] 2015-09-17 02:04:13 PDT
Comment on attachment 8662267 [details] [diff] [review]
Part 10: Using mechanism in RuntimeService to get pref in worker thread instead of gfxPref.

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

Send me an interdiff with the RuntimeService UnregisterCallback, thanks.

::: dom/canvas/OffscreenCanvas.cpp
@@ +211,5 @@
>  OffscreenCanvas::PrefEnabled(JSContext* aCx, JSObject* aObj)
>  {
> +  if (NS_IsMainThread()) {
> +    return Preferences::GetBool("gfx.offscreencanvas.enabled");
> +  } else {

WorkerPrivate* workerPrivate = GetWorkerPrivateFromContext(aCx);
MOZ_ASSERT(workerPrivate);
return workerPrivate->OffscreenCanvasEnabled();

::: dom/workers/RuntimeService.cpp
@@ +1969,5 @@
>        NS_FAILED(Preferences::RegisterCallbackAndCall(
>                                    WorkerPrefChanged,
>                                    PREF_REQUESTCONTEXT_ENABLED,
>                                    reinterpret_cast<void *>(WORKERPREF_REQUESTCONTEXT))) ||
> +      NS_FAILED(Preferences::RegisterCallbackAndCall(

You have to unregisterCallback too.
Comment 279 Morris Tseng [:mtseng] [:Morris] 2015-09-17 02:13:25 PDT
Created attachment 8662279 [details] [diff] [review]
Part 10 interdiff.
Comment 280 Morris Tseng [:mtseng] [:Morris] 2015-09-17 02:13:57 PDT
Created attachment 8662280 [details] [diff] [review]
Part 3 interdiff.
Comment 281 Morris Tseng [:mtseng] [:Morris] 2015-09-17 02:16:57 PDT
Created attachment 8662283 [details] [diff] [review]
Part 3: Transfer OffscreenCanvas from mainthread to workers v8. (carry r+: sfink)

Interdiff provided in comment 280.
Comment 282 Morris Tseng [:mtseng] [:Morris] 2015-09-17 02:18:01 PDT
Created attachment 8662284 [details] [diff] [review]
Part 10: Using mechanism in RuntimeService to get pref in worker thread instead of gfxPref v2.

Interdiff provided in comment 279.
Comment 283 Andrea Marchesini [:baku] 2015-09-17 02:33:15 PDT
Comment on attachment 8662283 [details] [diff] [review]
Part 3: Transfer OffscreenCanvas from mainthread to workers v8. (carry r+: sfink)

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

::: dom/base/StructuredCloneHelper.cpp
@@ +1114,5 @@
> +      OffscreenCanvas* canvas = nullptr;
> +      rv = UNWRAP_OBJECT(OffscreenCanvas, aObj, canvas);
> +      if (NS_SUCCEEDED(rv)) {
> +        MOZ_ASSERT(canvas);
> +        MOZ_ASSERT(*aContent);

wait... no this must be check after canvas->ToCloneData(). Move it to line 1124. You must check it only after setting it :)

@@ +1142,5 @@
>    if (aTag == SCTAG_DOM_MAP_MESSAGEPORT) {
>      MOZ_ASSERT(!aContent);
>      MOZ_ASSERT(aExtraData < mPortIdentifiers.Length());
>      MessagePort::ForceClose(mPortIdentifiers[aExtraData]);
> +  } else if (aTag == SCTAG_DOM_CANVAS) {

return;
}

if (aTag ==SCTAG_DOM_CANVAS) {
 ...
Comment 284 Morris Tseng [:mtseng] [:Morris] 2015-09-17 02:41:27 PDT
Created attachment 8662296 [details] [diff] [review]
Part 3: Transfer OffscreenCanvas from mainthread to workers v9. (carry r+: baku, sfink)

Fix reviewer's comment.
Comment 285 Morris Tseng [:mtseng] [:Morris] 2015-09-17 02:43:41 PDT
Created attachment 8662298 [details] [diff] [review]
Part 10: Using mechanism in RuntimeService to get pref in worker thread instead of gfxPref v3. (carry r+: baku)

Forget to add WORKERPREF_OFFSCREENCANVAS in switch case at RuntimeService::WorkerPrefChanged().
Comment 286 Morris Tseng [:mtseng] [:Morris] 2015-09-17 03:09:54 PDT
Comment on attachment 8662296 [details] [diff] [review]
Part 3: Transfer OffscreenCanvas from mainthread to workers v9. (carry r+: baku, sfink)

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

::: dom/base/StructuredCloneHelper.cpp
@@ +1109,5 @@
>        return true;
>      }
> +
> +    if (mContext == SameProcessSameThread ||
> +        mContext == SameProcessDifferentThread) {

I found that if I call postMessage on shared worker. I always get DifferentProcess here. Does it mean we cannot transfer offscreencanvas to shared worker? If so, I should rewrite my test case about shared worker.
Comment 287 Morris Tseng [:mtseng] [:Morris] 2015-09-17 03:11:32 PDT
:baku, do you have idea about comment 286? Thanks.
Comment 288 Andrea Marchesini [:baku] 2015-09-17 06:10:00 PDT
(In reply to Morris Tseng [:mtseng] from comment #287)
> :baku, do you have idea about comment 286? Thanks.

Right. MessagePort by default doesn't allow the transferring of WorkerCanvas because we don't know the destination. In theory you can send the MessagePort data via IPC or other crazy stuff between processes.

Can you rewrite the test without having SharedWorkers involved? Then we can fix this issue in a follow up.
Is it ok for you?
Comment 289 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2015-09-17 16:28:45 PDT
I think we should not support transferring canvases to SharedWorkers. AFAIK we don't really need that and it is going to be very hard to spec and implement. One reason is that a DedicatedWorker is always associated with a single DOM Window but a SharedWorker is not.
Comment 290 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2015-09-17 16:34:06 PDT
Comment on attachment 8662259 [details] [diff] [review]
Part 7: If layer is not available, fallback to BasicCanvasLayer v6.

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

::: gfx/layers/AsyncCanvasRenderer.cpp
@@ +143,5 @@
> +}
> +
> +void
> +AsyncCanvasRenderer::UpdateTarget()
> +{

Add a comment saying that this will be fleshed out in a later patch.

@@ +149,5 @@
> +
> +void
> +AsyncCanvasRenderer::EnsureSurface(gfx::IntSize aSize, gfx::SurfaceFormat aFormat)
> +{
> +  if (!mSurface ||

Assert that we're onthe main thread here.

@@ +164,5 @@
> +already_AddRefed<gfx::DataSourceSurface>
> +AsyncCanvasRenderer::GetSurface()
> +{
> +  MutexAutoLock lock(mMutex);
> +  UpdateTarget();

Assert that we're on the main thread here, and document in the header that it can only be called on the main thread. At least I assume that's the plan?

@@ +186,5 @@
> +  }
> +
> +  memcpy(dstMap.GetData(),
> +         srcMap.GetData(),
> +         srcMap.GetStride() * mSurface->GetSize().height);

Assuming mSurface can only be called on the main thread, I don't think we need to (or should) copy it here, as long as we allocate a new surface every time we update mSurface.
Comment 291 Jeff Gilbert [:jgilbert] 2015-09-17 17:34:09 PDT
Comment on attachment 8652774 [details] [diff] [review]
Part 4: Mochitests for offscreencanvas v4. (carry r+: baku)

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

::: dom/canvas/test/offscreencanvas.js
@@ +54,5 @@
> +
> +  var createProgram = function(vsSrc, fsSrc) {
> +    var vs = createShader(vsSrc, gl.VERTEX_SHADER);
> +    var fs = createShader(fsSrc, gl.FRAGMENT_SHADER);
> +    if (!vs || !fs) {

This isn't possible, since createShader doesn't check for errors. (nor should it!)
You can just remove this check.

@@ +244,5 @@
> +      };
> +
> +      var findTransferables = function(t) {
> +        if (t.test == "subworker") {
> +          return [].concat.apply([], t.subtests.map(findTransferables));

This is needlessly terse. Expand it so that it's readable.

Especially here, it doesn't matter if this is optimal. It matters that it's readable.

@@ +260,5 @@
> +  port = evt.ports[0];
> +  entryFunction(evt.data.test, evt.data.subtests, evt.data.canvas);
> +};
> +
> +onconnect = function(e) {

Why `e` here bug `evt` above?

::: dom/canvas/test/test_offscreencanvas_many.html
@@ +50,5 @@
> +
> +  /* create 4 workers that do the regular drawing test and 4 workers
> +     that do the size change test */
> +  for (var i = 1; i < 9; i++) {
> +    var htmlCanvas = document.getElementById("c" + i);

It would really be cleaner to just document.createElement() the canvases. That way it's strictly programmatic, and you don't need to mess with id-matching at all.

@@ +51,5 @@
> +  /* create 4 workers that do the regular drawing test and 4 workers
> +     that do the size change test */
> +  for (var i = 1; i < 9; i++) {
> +    var htmlCanvas = document.getElementById("c" + i);
> +    startWorker(htmlCanvas, i % 2 == 0 ? 'webgl' : 'webgl_changesize');

Unless you need to interleave these for testing reasons, it's better to just `for (0..3) A()` and `for (0..3) B()`. Do things explicitly instead of trying to save code.

::: dom/canvas/test/test_offscreencanvas_neuter.html
@@ +60,5 @@
> +  SimpleTest.doesThrow(
> +    function() { offscreenCanvas.getContext("webgl2") },
> +    "Can't getContext on transfered worker canvas");
> +
> +  // Transfer a neutered offscreencanvas should be ok.

This is surprising to me.

::: dom/canvas/test/test_offscreencanvas_serviceworker.html
@@ +26,5 @@
> +    // Wait until the service worker is active.
> +    .then(navigator.serviceWorker.ready)
> +    // ...and then show the interface for the commands once it's ready.
> +    .then(function() {
> +      content = document.getElementById("content");

Couldn't you just append it to the body?

::: dom/canvas/test/test_offscreencanvas_subworker.html
@@ +42,5 @@
> +  }
> +
> +  var c = [1,2,3,4,5,6,7,8,9,10].map(function(i) {
> +    return document.getElementById("c" + i)
> +      .transferControlToOffscreen();

Just create a transfered canvas on-demand.

Below, instead of c[N], do newTransferredCanvas().

@@ +62,5 @@
> +        {test: 'subworker', subtests: [
> +          {test: 'webgl_changesize', canvas: c[8]},
> +          {test: 'subworker', subtests: [
> +            {test: 'subworker', subtests: [
> +              {test: 'webgl_changesize', canvas: c[9]}]}]}]},

{test: 'webgl_changesize', canvas: c[9]}]}]}]},
TO:
              {test: 'webgl_changesize', canvas: c[9]}
            ]}
          ]}
        ]},

Really though, this whole blob is basically unreadable. Break it out into readable components.
Comment 292 Jeff Gilbert [:jgilbert] 2015-09-17 18:42:03 PDT
Comment on attachment 8662263 [details] [diff] [review]
Part 9: Readback without blocking main thread.

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

Sorry for the delay. Next round will be faster.

::: gfx/gl/SharedSurfaceANGLE.cpp
@@ +340,5 @@
> +    bool mIsLocked;
> +    RefPtr<ID3D11Texture2D> mTexture;
> +    RefPtr<ID3D11Texture2D> mCopiedTexture;
> +    RefPtr<IDXGIKeyedMutex> mMutex;
> +    nsRefPtr<ID3D11DeviceContext> mDeviceContext;

Don't mix nsRefPtr and RefPtr. Prefer RefPtr.

@@ +360,5 @@
> +        return false;
> +    }
> +
> +    ScopedLockTexture scopedLock(tex);
> +    if (!scopedLock.mIsLocked) {

I think it's more explicit to do scopedLock(tex, &succeeded)\n if (!succeeded)

@@ +364,5 @@
> +    if (!scopedLock.mIsLocked) {
> +        return false;
> +    }
> +
> +    uint8_t* data = reinterpret_cast<uint8_t*>(scopedLock.mSubresource.pData);

const uint8_t*

@@ +365,5 @@
> +        return false;
> +    }
> +
> +    uint8_t* data = reinterpret_cast<uint8_t*>(scopedLock.mSubresource.pData);
> +    uint32_t stride = scopedLock.mSubresource.RowPitch;

There are two strides here. This is the source stride, so `srcStride` would be better.

@@ +374,5 @@
> +    }
> +
> +    for (int32_t i = 0; i < out_surface->GetSize().height; i++) {
> +        memcpy(map.GetData() + i * map.GetStride(),
> +               data + i * stride, stride);

Don't copy `stride` bytes, as `stride` might be wider than the stride for `out_surface`.

If the strides match, just memcpy the whole thing, instead of by-row.
If the strides don't match, memcpy bytesPerPixel*width bytes per row.

@@ +377,5 @@
> +        memcpy(map.GetData() + i * map.GetStride(),
> +               data + i * stride, stride);
> +    }
> +
> +    SwapRAndBComponents(out_surface);

Don't do this unconditionally. Programmatically match and/or assert the formats. (r-)

::: gfx/gl/SharedSurfaceEGL.cpp
@@ +221,5 @@
> +    MOZ_ASSERT(out_surface);
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +    if (!mReadbackGL) {
> +        mReadbackGL = gl::GLContextProvider::CreateHeadless(gl::CreateContextFlags::NONE);

A context *per ShSurf*? No, add ReadbackEGLImage to the EGL library object and mutex it. (r-)

@@ +226,5 @@
> +    }
> +
> +    ScopedTexture destTex(mReadbackGL);
> +    ScopedBindTexture autoTex(mReadbackGL, destTex.Texture());
> +    mReadbackGL->fEGLImageTargetTexture2D(LOCAL_GL_TEXTURE_2D, mImage);

This can result in filling the EGLImage with undefined data. (r-) We either need to use PRESERVE (which we don't have in all cases), or support a draw-to-blit path with TEXTURE_EXTERNAL. (I believe we already have helpers for this) We should probably just return false here if we didn't get PRESERVE, since draw-to-blit is really wasteful.

::: gfx/gl/SharedSurfaceGLX.cpp
@@ +102,5 @@
> +    }
> +
> +    for (int32_t i = 0; i < dataSurf->GetSize().height; i++) {
> +        memcpy(map.GetData() + i * map.GetStride(),
> +               mapSrc.GetData() + i * mapSrc.GetStride(), mapSrc.GetStride());

memcpy(map.GetData() + i * map.GetStride(),
       mapSrc.GetData() + i * mapSrc.GetStride(),
       min(mapSrc.GetStride(), map.GetStride()));

::: gfx/gl/SharedSurfaceGralloc.cpp
@@ +287,5 @@
> +SharedSurface_Gralloc::ReadbackBySharedHandle(gfx::DataSourceSurface* out_surface)
> +{
> +    MOZ_ASSERT(out_surface);
> +    sp<GraphicBuffer> buffer = mTextureClient->GetGraphicBuffer();
> +    int32_t stride = buffer->getStride() * 4;

Woah, why is `stride` `buffer->getStride() * 4`?

@@ +289,5 @@
> +    MOZ_ASSERT(out_surface);
> +    sp<GraphicBuffer> buffer = mTextureClient->GetGraphicBuffer();
> +    int32_t stride = buffer->getStride() * 4;
> +    int32_t height = buffer->getHeight();
> +    int32_t width = buffer->getWidth();

Move these vars closer to where they're used. They're waaaaay up here for some reason.

Also, why are these signed ints? Prefer uint32_t or size_t.

@@ +291,5 @@
> +    int32_t stride = buffer->getStride() * 4;
> +    int32_t height = buffer->getHeight();
> +    int32_t width = buffer->getWidth();
> +
> +    uint8_t* grallocData = nullptr;

`const uint8_t*`

@@ +292,5 @@
> +    int32_t height = buffer->getHeight();
> +    int32_t width = buffer->getWidth();
> +
> +    uint8_t* grallocData = nullptr;
> +    if (BAD_VALUE == buffer->lock(GRALLOC_USAGE_SW_READ_OFTEN |

Don't use yoda-conditionals.

Do:
auto result = buffer->lock(...)
if (result == BAD_VALUE) {...}

@@ +311,5 @@
> +    }
> +
> +    buffer->unlock();
> +
> +    SwapRAndBComponents(out_surface);

Don't do this unconditionally.

::: gfx/gl/SharedSurfaceIO.cpp
@@ +190,5 @@
> +    size_t bytesPerRow = mIOSurf->GetBytesPerRow();
> +    size_t ioWidth = mIOSurf->GetDevicePixelWidth();
> +    size_t ioHeight = mIOSurf->GetDevicePixelHeight();
> +
> +    unsigned char* ioData = (unsigned char*)mIOSurf->GetBaseAddress();

const uint8_t*

::: gfx/layers/AsyncCanvasRenderer.cpp
@@ +149,5 @@
> +  MutexAutoLock lock(mMutex);
> +  RefPtr<BufferTextureClient> buffer = static_cast<BufferTextureClient*>(aTextureClient);
> +  buffer->Lock(layers::OpenMode::OPEN_READ);
> +  EnsureSurface(aTextureClient->GetSize(), gfx::SurfaceFormat::B8G8R8A8);
> +  uint8_t* lockedBytes = buffer->GetLockedData();

const uint8_t*

@@ +152,5 @@
> +  EnsureSurface(aTextureClient->GetSize(), gfx::SurfaceFormat::B8G8R8A8);
> +  uint8_t* lockedBytes = buffer->GetLockedData();
> +  gfx::DataSourceSurface::ScopedMap map(mSurface, gfx::DataSourceSurface::MapType::WRITE);
> +  if (!map.IsMapped()) {
> +    return;

Don't we have to unlock `buffer` here?

@@ +156,5 @@
> +    return;
> +  }
> +
> +  memcpy(map.GetData(), lockedBytes, map.GetStride() * mSurface->GetSize().height);
> +  gl::SwapRAndBComponents(mSurface);

Don't swap unconditionally.

@@ +189,5 @@
> +    return;
> +  }
> +
> +  if (!frontbuffer->ReadbackBySharedHandle(mSurface)) {
> +    return;

Wait, so it just silently fails?

@@ +213,5 @@
>  
>  already_AddRefed<gfx::DataSourceSurface>
>  AsyncCanvasRenderer::GetSurface()
>  {
> +  MOZ_ASSERT(NS_IsMainThread());

I don't think this is a good long-term assert. What are you trying to assert here?

::: gfx/layers/client/CanvasClient.cpp
@@ +411,5 @@
>      auto layersBackend = shadowForwarder->GetCompositorBackendType();
>      mReadbackClient = TexClientFromReadback(surf, forwarder, flags, layersBackend);
>  
> +    if (asyncRenderer) {
> +      asyncRenderer->CopyFromTextureClient(mReadbackClient);

It would be more efficient to readback directly into mSurface where viable, I think.
Comment 293 Morris Tseng [:mtseng] [:Morris] 2015-09-17 18:43:35 PDT
(In reply to Andrea Marchesini (:baku) from comment #288)
> (In reply to Morris Tseng [:mtseng] from comment #287)
> > :baku, do you have idea about comment 286? Thanks.
> 
> Right. MessagePort by default doesn't allow the transferring of WorkerCanvas
> because we don't know the destination. In theory you can send the
> MessagePort data via IPC or other crazy stuff between processes.
> 
> Can you rewrite the test without having SharedWorkers involved? Then we can
> fix this issue in a follow up.
> Is it ok for you?
Sure, I'll update my patch later. Thanks.
Comment 294 Morris Tseng [:mtseng] [:Morris] 2015-09-17 19:23:34 PDT
(In reply to Jeff Gilbert [:jgilbert] from comment #292) 
> ::: gfx/layers/client/CanvasClient.cpp
> @@ +411,5 @@
> >      auto layersBackend = shadowForwarder->GetCompositorBackendType();
> >      mReadbackClient = TexClientFromReadback(surf, forwarder, flags, layersBackend);
> >  
> > +    if (asyncRenderer) {
> > +      asyncRenderer->CopyFromTextureClient(mReadbackClient);
> 
> It would be more efficient to readback directly into mSurface where viable,
> I think.
mReadbackClient is for compositing, mSurface is for the fallback and GetDataURL case in main thread. So we should need to keep both copies here. My idea is, readback from GLContext is less efficient. So I copy it from TextureClient directly. Does it make sense here?
Comment 295 Jeff Gilbert [:jgilbert] 2015-09-18 13:23:08 PDT
(In reply to Morris Tseng [:mtseng] from comment #294)
> (In reply to Jeff Gilbert [:jgilbert] from comment #292) 
> > ::: gfx/layers/client/CanvasClient.cpp
> > @@ +411,5 @@
> > >      auto layersBackend = shadowForwarder->GetCompositorBackendType();
> > >      mReadbackClient = TexClientFromReadback(surf, forwarder, flags, layersBackend);
> > >  
> > > +    if (asyncRenderer) {
> > > +      asyncRenderer->CopyFromTextureClient(mReadbackClient);
> > 
> > It would be more efficient to readback directly into mSurface where viable,
> > I think.
> mReadbackClient is for compositing, mSurface is for the fallback and
> GetDataURL case in main thread. So we should need to keep both copies here.
> My idea is, readback from GLContext is less efficient. So I copy it from
> TextureClient directly. Does it make sense here?

Yes.
Comment 296 Morris Tseng [:mtseng] [:Morris] 2015-09-20 23:06:07 PDT
Created attachment 8663513 [details] [diff] [review]
Part 4: Mochitests for offscreencanvas v5.

Rewrite test in shared worker to test the offscreen canvas cannot transfer to shared worker. :baku, could you review this part? It's in test_offscreencanvas_sharedworker.html. Thanks.

Addressed :jgilbert's comment in comment 291.
Comment 297 Morris Tseng [:mtseng] [:Morris] 2015-09-20 23:27:15 PDT
Created attachment 8663521 [details] [diff] [review]
Part 7: If layer is not available, fallback to BasicCanvasLayer v7.

Fix roc's comment in comment 290. I removed EnsureTarget() because we create new DataSourceSurface each time when UpdateTarget() get called(not in this patch, this change will be in part 9).
Comment 298 Morris Tseng [:mtseng] [:Morris] 2015-09-20 23:28:44 PDT
Created attachment 8663522 [details] [diff] [review]
Part 9: Readback without blocking main thread v2.

Fix jgilbert's comment.
Comment 299 Morris Tseng [:mtseng] [:Morris] 2015-09-20 23:33:40 PDT
(In reply to Jeff Gilbert [:jgilbert] from comment #292)
> @@ +213,5 @@
> >  
> >  already_AddRefed<gfx::DataSourceSurface>
> >  AsyncCanvasRenderer::GetSurface()
> >  {
> > +  MOZ_ASSERT(NS_IsMainThread());
> 
> I don't think this is a good long-term assert. What are you trying to assert
> here?
> 
Because this function only be called on the fallback case or GetDataURL() which are only happened in the main thread. So I assert here.
Comment 300 Morris Tseng [:mtseng] [:Morris] 2015-09-21 01:40:58 PDT
Created attachment 8663539 [details] [diff] [review]
Part 9: Readback without blocking main thread v3.

Fix swap rb condition in SharedSurface_Gralloc.
Comment 301 Jeff Gilbert [:jgilbert] 2015-09-21 17:09:02 PDT
Comment on attachment 8663539 [details] [diff] [review]
Part 9: Readback without blocking main thread v3.

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

::: gfx/gl/GLReadTexImageHelper.h
@@ +78,2 @@
>  
> +    bool ReadTexImage(gfx::DataSourceSurface* outSurface,

`dest` instead of `outSurface`. It's not an out-var.

::: gfx/gl/SharedSurfaceANGLE.cpp
@@ +389,5 @@
> +        }
> +    }
> +
> +    if (out_surface->GetFormat() == SurfaceFormat::R8G8B8A8 ||
> +        out_surface->GetFormat() == SurfaceFormat::R8G8B8X8)

This isn't enough. You need to match the formats. The ANGLE surface may be either BGRA or RGBA. If it can only be one of the two, we *must* assert here.

::: gfx/gl/SharedSurfaceGLX.cpp
@@ +95,5 @@
> +    if (!mapSrc.IsMapped()) {
> +        return false;
> +    }
> +
> +    gfx::DataSourceSurface::ScopedMap map(out_surface, gfx::DataSourceSurface::WRITE);

`mapDest`.

::: gfx/gl/SharedSurfaceGralloc.cpp
@@ +315,5 @@
> +
> +    buffer->unlock();
> +
> +    if (out_surface->GetFormat() == SurfaceFormat::B8G8R8A8 ||
> +        out_surface->GetFormat() == SurfaceFormat::B8G8R8X8)

Is it guaranteed that Gralloc buffers are RGBA/X?

::: gfx/gl/SharedSurfaceIO.cpp
@@ +185,5 @@
> +bool
> +SharedSurface_IOSurface::ReadbackBySharedHandle(gfx::DataSourceSurface* out_surface)
> +{
> +    MOZ_ASSERT(out_surface);
> +    mIOSurf->Lock();

I'm assuming this is infallible? (returns void?) If not, we really need to check this.

::: gfx/layers/AsyncCanvasRenderer.cpp
@@ +147,5 @@
> +AsyncCanvasRenderer::CopyFromTextureClient(TextureClient* aTextureClient)
> +{
> +  MutexAutoLock lock(mMutex);
> +  RefPtr<BufferTextureClient> buffer = static_cast<BufferTextureClient*>(aTextureClient);
> +  buffer->Lock(layers::OpenMode::OPEN_READ);

Is this an infallible lock? If it's non-null but should always work, use MOZ_ALWAYS_TRUE.

@@ +150,5 @@
> +  RefPtr<BufferTextureClient> buffer = static_cast<BufferTextureClient*>(aTextureClient);
> +  buffer->Lock(layers::OpenMode::OPEN_READ);
> +
> +  const gfx::IntSize& size = aTextureClient->GetSize();
> +  const gfx::SurfaceFormat format = gfx::SurfaceFormat::B8G8R8A8;

Why always BGRA? Comment!

@@ +171,5 @@
> +
> +  memcpy(map.GetData(), lockedBytes, map.GetStride() * mSurfaceForBasic->GetSize().height);
> +  if (mSurfaceForBasic->GetFormat() == gfx::SurfaceFormat::R8G8B8A8 ||
> +      mSurfaceForBasic->GetFormat() == gfx::SurfaceFormat::R8G8B8X8) {
> +    gl::SwapRAndBComponents(mSurfaceForBasic);

This could have issues because mSurfaceForBasic is still locked for WRITE, and Swap...() is going to require re-mapping with read+write.

@@ +199,5 @@
> +    return nullptr;
> +  }
> +
> +  const gfx::IntSize& size = frontbuffer->mSize;
> +  const gfx::SurfaceFormat format = gfx::SurfaceFormat::B8G8R8A8;

Why always BGRA? Comment!

::: gfx/layers/client/CanvasClient.cpp
@@ +411,5 @@
>      auto layersBackend = shadowForwarder->GetCompositorBackendType();
>      mReadbackClient = TexClientFromReadback(surf, forwarder, flags, layersBackend);
>  
> +    if (asyncRenderer) {
> +      asyncRenderer->CopyFromTextureClient(mReadbackClient);

Leave a short comment here saying what this is for.
Comment 302 Morris Tseng [:mtseng] [:Morris] 2015-09-21 22:41:11 PDT
Created attachment 8664057 [details] [diff] [review]
Part 4: Mochitests for offscreencanvas v6. (carry r+: baku, jgilbert)

Disable service worker on b2g. See bug 1056702.
Comment 303 Morris Tseng [:mtseng] [:Morris] 2015-09-21 22:42:13 PDT
Created attachment 8664058 [details] [diff] [review]
Part 9: Readback without blocking main thread v4.

Fix reviewer's comment.
Comment 304 Morris Tseng [:mtseng] [:Morris] 2015-09-22 03:12:57 PDT
Created attachment 8664151 [details] [diff] [review]
Part 11: Diabled test_offscreencanvas_many.html on gonk, android, windows and linux.

We need disabled test on those platforms because:
1. On gonk, we hit crash on emulator. Looks like the emulator gl driver bug. On my real firefox os devices, it works well.
2. Android looks like have same issue with gonk. On my device it works ok, too.
3. On windows, ANGLE isn't thread-safe now. So we hit crash.
4. On linux, we hit crash on libLLVM ( see https://treeherder.mozilla.org/logviewer.html#?job_id=11753429&repo=try ) which is mesa OpenGL driver. On my local machine which doesn't use mesa driver works ok as well.
I'll create follow-up bugs to track those issues. Does it make sense to disabled test on those platforms?

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6aaef4d24a40
Comment 305 Milan Sreckovic [:milan] 2015-09-22 07:40:12 PDT
I don't think I'm comfortable with us landing the changes where we have to disable the tests on so many platforms.  Can we change the test?  Can we fix this problems?
Comment 306 Jeff Gilbert [:jgilbert] 2015-09-22 13:03:59 PDT
Comment on attachment 8664058 [details] [diff] [review]
Part 9: Readback without blocking main thread v4.

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

::: gfx/gl/SharedSurfaceANGLE.cpp
@@ +390,5 @@
> +    }
> +
> +    DXGI_FORMAT srcFormat = scopedLock.mDesc.Format;
> +    MOZ_ASSERT(srcFormat == DXGI_FORMAT_B8G8R8A8_UNORM ||
> +               srcFormat == DXGI_FORMAT_R8G8B8A8_UNORM);

Add the RGBX and BGRX formats, too. We don't use them right now, but we will.
Comment 307 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2015-09-22 19:03:34 PDT
(In reply to Milan Sreckovic [:milan] from comment #305)
> I don't think I'm comfortable with us landing the changes where we have to
> disable the tests on so many platforms.  Can we change the test?  Can we fix
> this problems?

Personally I think we should definitely not *ship* this code enabled with testing on these platforms disabled, but we should still *land* the code with OSX-only test coverage. That will avoid code rot and make it easier for people to fix the platform-specific bugs.
Comment 308 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2015-09-22 19:04:30 PDT
In particular having this code landed would make it much easier to test my work in bug 1203382.
Comment 309 Morris Tseng [:mtseng] [:Morris] 2015-09-22 19:23:58 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #307)
> (In reply to Milan Sreckovic [:milan] from comment #305)
> > I don't think I'm comfortable with us landing the changes where we have to
> > disable the tests on so many platforms.  Can we change the test?  Can we fix
> > this problems?
> 
> Personally I think we should definitely not *ship* this code enabled with
> testing on these platforms disabled, but we should still *land* the code
> with OSX-only test coverage. That will avoid code rot and make it easier for
> people to fix the platform-specific bugs.

This feature also behind the pref "gfx.offscreencanvas.enabled" now. So land the code should be ok. Once the platform-specific bugs are all resolved. We can remove the pref and ship the feature.
Comment 310 Morris Tseng [:mtseng] [:Morris] 2015-09-22 19:33:13 PDT
(In reply to Jeff Gilbert [:jgilbert] from comment #306)
> Comment on attachment 8664058 [details] [diff] [review]
> Part 9: Readback without blocking main thread v4.
> 
> Review of attachment 8664058 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/SharedSurfaceANGLE.cpp
> @@ +390,5 @@
> > +    }
> > +
> > +    DXGI_FORMAT srcFormat = scopedLock.mDesc.Format;
> > +    MOZ_ASSERT(srcFormat == DXGI_FORMAT_B8G8R8A8_UNORM ||
> > +               srcFormat == DXGI_FORMAT_R8G8B8A8_UNORM);
> 
> Add the RGBX and BGRX formats, too. We don't use them right now, but we will.
DXGI_FORMAT only include BGRX, not include RGBX.
Reference: https://msdn.microsoft.com/en-us/library/windows/desktop/bb173059%28v=vs.85%29.aspx
So I only add the BGRX format, is it sufficient?
Comment 311 Morris Tseng [:mtseng] [:Morris] 2015-09-22 20:12:32 PDT
:jrmuizel, review ping about comment 272. Thanks.
Comment 312 Jeff Muizelaar [:jrmuizel] 2015-09-23 08:18:15 PDT
Comment on attachment 8662255 [details] [diff] [review]
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v14. (carry r+: jgilbert, nical, sr+: ehsan)

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

It seems like this part could easily be split out into a separate patch that you can land ahead of time.

::: gfx/thebes/gfxUtils.cpp
@@ +1596,5 @@
> +};
> +
> +/* static */ nsresult
> +gfxUtils::GetFeatureStatus(const nsCOMPtr<nsIGfxInfo>& gfxInfo,
> +                           int32_t feature, int32_t* status)

Can you call this ThreadSafeGetFeatureStatus?
Comment 313 Morris Tseng [:mtseng] [:Morris] 2015-09-23 19:33:26 PDT
Created attachment 8665225 [details] [diff] [review]
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v15. (carry r+ jgilbert, nical, jrmuizel, sr+: ehsan)

Fix reviewer's comment.
Comment 314 Morris Tseng [:mtseng] [:Morris] 2015-09-23 19:34:21 PDT
Created attachment 8665227 [details] [diff] [review]
Part 9: Readback without blocking main thread v5. (carry r+: jgilbert)

Add check for BGRX format.
Comment 315 Morris Tseng [:mtseng] [:Morris] 2015-09-23 19:54:35 PDT
Created attachment 8665231 [details] [diff] [review]
Part 1: Let ImageBridge transfer CanvasClient async v9. (carry r+: nical)

Rebased.
Comment 316 Morris Tseng [:mtseng] [:Morris] 2015-09-23 19:56:36 PDT
Created attachment 8665232 [details] [diff] [review]
Part 5: Add interfaces test v3. (carry r+: ehsan)

Rebased.
Comment 317 Morris Tseng [:mtseng] [:Morris] 2015-09-23 19:58:14 PDT
Created attachment 8665233 [details] [diff] [review]
Part 6: Add frame ID to CanvasClient so compositor could update frame correctly v4. (carry r+: roc)

Rebased.
Comment 318 Morris Tseng [:mtseng] [:Morris] 2015-09-23 20:00:04 PDT
Created attachment 8665235 [details] [diff] [review]
Part 7: If layer is not available, fallback to BasicCanvasLayer v8. (carry r+: roc)

Rebased.
Comment 319 Morris Tseng [:mtseng] [:Morris] 2015-09-24 18:45:15 PDT
latest try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec135187e4db
Comment 320 Morris Tseng [:mtseng] [:Morris] 2015-09-24 19:33:16 PDT
Created attachment 8665753 [details] [diff] [review]
Part 1: Let ImageBridge transfer CanvasClient async v10. (carry r+: nical)

Fix build failure.
Comment 321 Morris Tseng [:mtseng] [:Morris] 2015-09-24 19:36:50 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a36fd9216c6
Comment 322 Morris Tseng [:mtseng] [:Morris] 2015-09-24 20:30:24 PDT
Created attachment 8665766 [details] [diff] [review]
Part 1: Let ImageBridge transfer CanvasClient async v11. (carry r+: nical)

Fix try failure again.
Comment 323 Morris Tseng [:mtseng] [:Morris] 2015-09-24 20:32:44 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7a22e4f5e55
Comment 324 Morris Tseng [:mtseng] [:Morris] 2015-09-27 08:04:29 PDT
Created attachment 8666468 [details] [diff] [review]
Part 1: Let ImageBridge transfer CanvasClient async v12. (carry r+: nical)

Rebased again.
Comment 325 Morris Tseng [:mtseng] [:Morris] 2015-09-27 08:05:32 PDT
Created attachment 8666469 [details] [diff] [review]
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v16. (carry r+: jgilbert, nical, jrmuizel, sr+: ehsan)

Rebased again.
Comment 326 Morris Tseng [:mtseng] [:Morris] 2015-09-27 18:51:46 PDT
Created attachment 8666563 [details] [diff] [review]
Part 2: Introduce OffscreenCanvas and let WebGL context work on workers v17. (carry r+: jgilbert, nical, jrmuizel, sr+: ehsan)

Fix try failure.
latest try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b403693dcede
Comment 327 Morris Tseng [:mtseng] [:Morris] 2015-09-27 18:53:52 PDT
Try looks good. Ready for check-in.
Comment 328 Carsten Book [:Tomcat] - PTO-back Sept 4th 2015-09-28 01:53:38 PDT
Hi Morris, part 1 failed to apply :

renamed 709490 -> Bug-709490---Part-1-Let-ImageBridge-transfer-Canva.patch
applying Bug-709490---Part-1-Let-ImageBridge-transfer-Canva.patch
patching file gfx/layers/Layers.cpp
Hunk #2 FAILED at 2103
1 out of 2 hunks FAILED -- saving rejects to file gfx/layers/Layers.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh Bug-709490---Part-1-Let-ImageBridge-transfer-Canva.patch
can you take a look, thanks!
Comment 329 Morris Tseng [:mtseng] [:Morris] 2015-09-28 02:18:29 PDT
Created attachment 8666615 [details] [diff] [review]
Part 1: Let ImageBridge transfer CanvasClient async v13. (carry r+: nical)

Rebased to latest inbound.
Comment 330 Morris Tseng [:mtseng] [:Morris] 2015-09-28 02:20:50 PDT
Rebased. Can you check-in it again? Thanks.
Comment 331 Carsten Book [:Tomcat] - PTO-back Sept 4th 2015-09-28 23:18:57 PDT
Hi Morris, now part 8 failed to apply:

renamed 709490 -> Bug-709490---Part-6-Add-frame-ID-to-CanvasClient-s.patch
applying Bug-709490---Part-6-Add-frame-ID-to-CanvasClient-s.patch
patching file gfx/layers/LayerTreeInvalidation.cpp
Hunk #1 FAILED at 380
1 out of 3 hunks FAILED -- saving rejects to file gfx/layers/LayerTreeInvalidation.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh Bug-709490---Part-6-Add-frame-ID-to-CanvasClient-s.patch
Comment 332 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2015-09-28 23:21:46 PDT
Do you not have commit access?  Why can't you land this yourself?
Comment 333 Morris Tseng [:mtseng] [:Morris] 2015-09-29 03:18:34 PDT
My commit access have some problem so I cannot checkin code by myself. I'll find someone have commit access  to help me land this code. Thanks.
Comment 335 Andrea Marchesini [:baku] 2015-09-29 04:01:43 PDT
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=00c0e85dd8cd

patches landed.
Comment 336 Morris Tseng [:mtseng] [:Morris] 2015-09-29 04:18:25 PDT
That's awesome. Thank you, baku. I'll find that why my commit access broken.
Comment 340 Morris Tseng [:mtseng] [:Morris] 2015-10-01 20:09:50 PDT
Bug 1210321 resolved the android failure. Once it got landed. We can re-land this bug.
Comment 341 Morris Tseng [:mtseng] [:Morris] 2015-10-01 20:11:19 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fcfec12d46b

Try run with bug 1210321 applied.
Comment 342 Morris Tseng [:mtseng] [:Morris] 2015-10-04 20:11:24 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb4e0992056905143120e4a869dfe84fdfe46b96
Bug 709490 - Part 1: Let ImageBridge transfer CanvasClient async. r=nical

https://hg.mozilla.org/integration/mozilla-inbound/rev/2285ce1596b828513b98a755cc251ee311e8a15a
Bug 709490 - Part 2: Introduce OffscreenCanvas and let WebGL context work on workers. r=nical, r=jgilbert, r=jrmuizel, sr=ehsan

https://hg.mozilla.org/integration/mozilla-inbound/rev/37fba20111e240a407a454b93cd3ca70d4b7ac94
Bug 709490 - Part 3: Transfer OffscreenCanvas from mainthread to workers. r=baku, r=sfink

https://hg.mozilla.org/integration/mozilla-inbound/rev/9bceaf9137919047eb67f347f103be0c7e313f00
Bug 709490 - Part 4: Mochitests for offscreencanvas. r=baku, r=jgilbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/acf6220b431a91b2e201362b86083a983039e4b1
Bug 709490 - Part 5: Add interfaces test. r=ehsan

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc513b4109496554352cab341f37a75048dc3556
Bug 709490 - Part 6: Add frame ID to CanvasClient so compositor could update frame correctly. r=roc

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9d130aea88e7d8d88eae87c2e3e483696709e39
Bug 709490 - Part 7: If layer is not available, fallback to BasicCanvasLayer. r=roc

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b20f2c833c468d24115f8fcc32bb46ffcd99cc1
Bug 709490 - Part 8: Copy to a temp texture when readback from IOSurface. r=jgilbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e687c9143c1b4686be35da7ddac010b76e81a90
Bug 709490 - Part 9: Readback without blocking main thread. r=jgilbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd8f9410d335d118d6ab7be48c7808e30cf850ef
Bug 709490 - Part 10: Using mechanism in RuntimeService to get pref in worker thread instead of gfxPref. r=baku

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc04c5d43550ead4306f210dfa23e306a9fcd2bb
Bug 709490 - Part 11: Diabled test_offscreencanvas_many.html on gonk, android, windows and linux. r=jgilbert
Comment 345 Morris Tseng [:mtseng] [:Morris] 2015-10-05 22:55:46 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a54d5f481b39
Comment 346 Morris Tseng [:mtseng] [:Morris] 2015-10-06 19:29:09 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cce7c4c9cfb
Comment 350 Florian Scholz [:fscholz] (MDN) 2016-01-20 06:14:08 PST
Documentation updates:

HTMLCanvasElement.transferControlToOffscreen (behind pref)
https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement/transferControlToOffscreen

OffscreenCanvas interface (behind pref)
https://developer.mozilla.org/en-US/docs/Web/API/OffscreenCanvas
https://developer.mozilla.org/en-US/docs/Web/API/OffscreenCanvas/height
https://developer.mozilla.org/en-US/docs/Web/API/OffscreenCanvas/width
https://developer.mozilla.org/en-US/docs/Web/API/OffscreenCanvas/getContext

WebGLRenderingContext.commit (behind pref)
https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/commit

OffscreenCanvas added to WebGLRenderingContext.canvas return value
https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/canvas

Interfaces now marked as available to workers (behind pref)
https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext
https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext
https://developer.mozilla.org/en-US/docs/Web/API/WebGLShaderPrecisionFormat 
https://developer.mozilla.org/en-US/docs/Web/API/WebGLActiveInfo
https://developer.mozilla.org/en-US/docs/Web/API/WebGLUniformLocation
https://developer.mozilla.org/en-US/docs/Web/API/WebGLTexture
https://developer.mozilla.org/en-US/docs/Web/API/WebGLShader
https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderbuffer
https://developer.mozilla.org/en-US/docs/Web/API/WebGLProgram
https://developer.mozilla.org/en-US/docs/Web/API/WebGLFramebuffer
https://developer.mozilla.org/en-US/docs/Web/API/WebGLBuffer

WebGL API added to overview of available APIs in workers (behind pref)
https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Functions_and_classes_available_to_workers#APIs_available_in_workers

Developer release notes
https://developer.mozilla.org/en-US/Firefox/Releases/44#Canvas

Any review to the MDN wiki pages is appreciated (as always).
Comment 351 Luis Garcia 2016-02-07 09:08:42 PST
When I write code like this:
---
tranferControl
draw rect1
commit
draw rect2
commit
---

In result I see only rect2. Before each commit it clears the canvas. Is this correct behavior? I just want to paint yet another object without repainting entire canvas.
Comment 352 Milan Sreckovic [:milan] 2016-08-16 14:15:22 PDT
Morris, is there a reason the offscreen canvas dimensions are uint32_t?  Other places where we have canvas (HTMLCanvasElement and CanvasRenderingContext2D) are using int32_t.  Some places in off-screen canvas class also create nsIntSize, which is using int32_t under the covers.
Comment 353 Jeff Gilbert [:jgilbert] 2016-08-16 16:08:09 PDT
If it can't be negative, it should be unsigned. (as long as range isn't a concern, which it isn't here)
Comment 354 Morris Tseng [:mtseng] [:Morris] 2016-08-17 00:03:16 PDT
Yap, I think the dimensions should be positive. So I use unsigned to store the dimensions here.
Comment 355 Milan Sreckovic [:milan] 2016-08-18 10:11:36 PDT
Note that we cast to int32_t value without checking, and end up with a negative value.
Comment 356 Jeff Gilbert [:jgilbert] 2016-08-18 11:06:11 PDT
(In reply to Milan Sreckovic [:milan] from comment #355)
> Note that we cast to int32_t value without checking, and end up with a
> negative value.

This is only possible if the canvas dimensions are >INT32_MAX, which shouldn't be possible now, and will likely cause other problems if it is allowed.

Note You need to log in before you can comment on or make changes to this bug.