From 65d3f5dee52920b06e13f825f9b406a54ef48bdd Mon Sep 17 00:00:00 2001 From: Jackson Gardner Date: Fri, 6 Oct 2023 09:49:22 -0700 Subject: [PATCH 1/3] Fix async image loading issues in skwasm. There were a few different issues here: * We need to do our own message passing for rendering pictures. The async methods provided by emscripten have their own queue that can drain synchronously, so basically it's not guaranteed to be FIFO with other messages sent to the web worker or main thread. * When we drop frames, we should only drop intermediate frames, so that when the rendering flurry stops that the frame that is displayed is the last one that was actually requested. * We need to reset the GL context after lazy image creation, otherwise skia's renderer gets into a bad state for that frame. --- lib/web_ui/lib/src/engine/scene_view.dart | 51 +++++++++++++++++---- lib/web_ui/skwasm/image.cpp | 4 ++ lib/web_ui/skwasm/library_skwasm_support.js | 13 ++++++ lib/web_ui/skwasm/skwasm_support.h | 5 ++ lib/web_ui/skwasm/surface.cpp | 20 ++++---- lib/web_ui/skwasm/surface.h | 7 ++- lib/web_ui/test/engine/scene_view_test.dart | 29 +++++++++++- 7 files changed, 103 insertions(+), 26 deletions(-) diff --git a/lib/web_ui/lib/src/engine/scene_view.dart b/lib/web_ui/lib/src/engine/scene_view.dart index f1c75c0939d25..ae932ef4ed894 100644 --- a/lib/web_ui/lib/src/engine/scene_view.dart +++ b/lib/web_ui/lib/src/engine/scene_view.dart @@ -16,6 +16,20 @@ abstract class PictureRenderer { FutureOr renderPicture(ScenePicture picture); } +class SceneRender { + SceneRender(this.scene, this.completer) { + scene.beginRender(); + } + + final EngineScene scene; + final Completer completer; + + void done() { + scene.endRender(); + completer.complete(); + } +} + // This class builds a DOM tree that composites an `EngineScene`. class EngineSceneView { factory EngineSceneView(PictureRenderer pictureRenderer) { @@ -30,16 +44,38 @@ class EngineSceneView { List containers = []; - int queuedRenders = 0; - static const int kMaxQueuedRenders = 3; + SceneRender? currentRender; + SceneRender? nextRender; + + Future renderScene(EngineScene scene) { + if (currentRender != null) { + // If a scene is already queued up, drop it and queue this one up instead + // so that the scene view always displays the most recently requested scene. + nextRender?.done(); + final Completer completer = Completer(); + nextRender = SceneRender(scene, completer); + return completer.future; + } + final Completer completer = Completer(); + currentRender = SceneRender(scene, completer); + _kickRenderLoop(); + return completer.future; + } - Future renderScene(EngineScene scene) async { - if (queuedRenders >= kMaxQueuedRenders) { + Future _kickRenderLoop() async { + final SceneRender current = currentRender!; + await _renderScene(current.scene); + current.done(); + currentRender = nextRender; + nextRender = null; + if (currentRender == null) { return; + } else { + return _kickRenderLoop(); } - queuedRenders += 1; + } - scene.beginRender(); + Future _renderScene(EngineScene scene) async { final List slices = scene.rootLayer.slices; final Iterable> renderFutures = slices.map( (LayerSlice slice) async => switch (slice) { @@ -113,9 +149,6 @@ class EngineSceneView { sceneElement.removeChild(currentElement); currentElement = sibling; } - scene.endRender(); - - queuedRenders -= 1; } } diff --git a/lib/web_ui/skwasm/image.cpp b/lib/web_ui/skwasm/image.cpp index 0e7665d0722d2..def216398afa5 100644 --- a/lib/web_ui/skwasm/image.cpp +++ b/lib/web_ui/skwasm/image.cpp @@ -101,6 +101,10 @@ class TextureSourceImageGenerator : public GrExternalTextureGenerator { auto backendTexture = GrBackendTextures::MakeGL( fInfo.width(), fInfo.height(), mipmapped, glInfo); + + // In order to bind the image source to the texture, makeTexture has changed + // which texture is "in focus" for the WebGL context. + GrAsDirectContext(context)->resetContext(kTextureBinding_GrGLBackendState); return std::make_unique( backendTexture, glInfo.fID, emscripten_webgl_get_current_context()); } diff --git a/lib/web_ui/skwasm/library_skwasm_support.js b/lib/web_ui/skwasm/library_skwasm_support.js index 46467e507b60b..5e62614e009c4 100644 --- a/lib/web_ui/skwasm/library_skwasm_support.js +++ b/lib/web_ui/skwasm/library_skwasm_support.js @@ -27,6 +27,9 @@ mergeInto(LibraryManager.library, { return; } switch (skwasmMessage) { + case 'renderPicture': + _surface_renderPictureOnWorker(data.surface, data.picture, data.callbackId); + return; case 'onRenderComplete': _surface_onRenderComplete(data.surface, data.callbackId, data.imageBitmap); return; @@ -51,6 +54,14 @@ mergeInto(LibraryManager.library, { PThread.pthreads[threadId].addEventListener("message", eventListener); } }; + _skwasm_dispatchRenderPicture = function(threadId, surfaceHandle, pictureHandle, callbackId) { + PThread.pthreads[threadId].postMessage({ + skwasmMessage: 'renderPicture', + surface: surfaceHandle, + picture: pictureHandle, + callbackId, + }); + }; _skwasm_createOffscreenCanvas = function(width, height) { const canvas = new OffscreenCanvas(width, height); var contextAttributes = { @@ -114,6 +125,8 @@ mergeInto(LibraryManager.library, { skwasm_disposeAssociatedObjectOnThread__deps: ['$skwasm_support_setup'], skwasm_registerMessageListener: function() {}, skwasm_registerMessageListener__deps: ['$skwasm_support_setup'], + skwasm_dispatchRenderPicture: function() {}, + skwasm_dispatchRenderPicture__deps: ['$skwasm_support_setup'], skwasm_createOffscreenCanvas: function () {}, skwasm_createOffscreenCanvas__deps: ['$skwasm_support_setup'], skwasm_resizeCanvas: function () {}, diff --git a/lib/web_ui/skwasm/skwasm_support.h b/lib/web_ui/skwasm/skwasm_support.h index 21c790d6507eb..0c9cbaaa845c8 100644 --- a/lib/web_ui/skwasm/skwasm_support.h +++ b/lib/web_ui/skwasm/skwasm_support.h @@ -4,6 +4,7 @@ #include #include +#include "third_party/skia/include/core/SkPicture.h" namespace Skwasm { class Surface; @@ -19,6 +20,10 @@ extern SkwasmObject skwasm_getAssociatedObject(void* pointer); extern void skwasm_disposeAssociatedObjectOnThread(unsigned long threadId, void* pointer); extern void skwasm_registerMessageListener(pthread_t threadId); +extern void skwasm_dispatchRenderPicture(unsigned long threadId, + Skwasm::Surface* surface, + SkPicture* picture, + uint32_t callbackId); extern uint32_t skwasm_createOffscreenCanvas(int width, int height); extern void skwasm_resizeCanvas(uint32_t contextHandle, int width, int height); extern void skwasm_captureImageBitmap(Skwasm::Surface* surfaceHandle, diff --git a/lib/web_ui/skwasm/surface.cpp b/lib/web_ui/skwasm/surface.cpp index cebb4889faf2b..28b64bd25a328 100644 --- a/lib/web_ui/skwasm/surface.cpp +++ b/lib/web_ui/skwasm/surface.cpp @@ -43,9 +43,7 @@ uint32_t Surface::renderPicture(SkPicture* picture) { assert(emscripten_is_main_browser_thread()); uint32_t callbackId = ++_currentCallbackId; picture->ref(); - emscripten_dispatch_to_thread(_thread, EM_FUNC_SIG_VIII, - reinterpret_cast(fRenderPicture), - nullptr, this, picture, callbackId); + skwasm_dispatchRenderPicture(_thread, this, picture, callbackId); return callbackId; } @@ -138,7 +136,7 @@ void Surface::_recreateSurface() { } // Worker thread only -void Surface::_renderPicture(const SkPicture* picture, uint32_t callbackId) { +void Surface::renderPictureOnWorker(SkPicture* picture, uint32_t callbackId) { SkRect pictureRect = picture->cullRect(); SkIRect roundedOutRect; pictureRect.roundOut(&roundedOutRect); @@ -195,13 +193,6 @@ void Surface::fDispose(Surface* surface) { surface->_dispose(); } -void Surface::fRenderPicture(Surface* surface, - SkPicture* picture, - uint32_t callbackId) { - surface->_renderPicture(picture, callbackId); - picture->unref(); -} - void Surface::fOnRasterizeComplete(Surface* surface, SkData* imageData, uint32_t callbackId) { @@ -239,6 +230,13 @@ SKWASM_EXPORT uint32_t surface_renderPicture(Surface* surface, return surface->renderPicture(picture); } +SKWASM_EXPORT void surface_renderPictureOnWorker(Surface* surface, + SkPicture* picture, + uint32_t callbackId) { + surface->renderPictureOnWorker(picture, callbackId); + picture->unref(); +} + SKWASM_EXPORT uint32_t surface_rasterizeImage(Surface* surface, SkImage* image, ImageByteFormat format) { diff --git a/lib/web_ui/skwasm/surface.h b/lib/web_ui/skwasm/surface.h index c7cd137a00fb5..d99164831e523 100644 --- a/lib/web_ui/skwasm/surface.h +++ b/lib/web_ui/skwasm/surface.h @@ -70,13 +70,15 @@ class Surface { std::unique_ptr createTextureSourceWrapper( SkwasmObject textureSource); + // Worker thread + void renderPictureOnWorker(SkPicture* picture, uint32_t callbackId); + private: void _runWorker(); void _init(); void _dispose(); void _resizeCanvasToFit(int width, int height); void _recreateSurface(); - void _renderPicture(const SkPicture* picture, uint32_t callbackId); void _rasterizeImage(SkImage* image, ImageByteFormat format, uint32_t callbackId); @@ -99,9 +101,6 @@ class Surface { pthread_t _thread; static void fDispose(Surface* surface); - static void fRenderPicture(Surface* surface, - SkPicture* picture, - uint32_t callbackId); static void fOnRenderComplete(Surface* surface, uint32_t callbackId, SkwasmObject imageBitmap); diff --git a/lib/web_ui/test/engine/scene_view_test.dart b/lib/web_ui/test/engine/scene_view_test.dart index cea07e15294ab..62d5c744d610c 100644 --- a/lib/web_ui/test/engine/scene_view_test.dart +++ b/lib/web_ui/test/engine/scene_view_test.dart @@ -25,6 +25,7 @@ class StubPictureRenderer implements PictureRenderer { @override Future renderPicture(ScenePicture picture) async { + lastRenderedPicture = picture; final ui.Rect cullRect = picture.cullRect; final DomImageBitmap bitmap = (await createImageBitmap( scratchCanvasElement, @@ -32,12 +33,16 @@ class StubPictureRenderer implements PictureRenderer { ).toDart)! as DomImageBitmap; return bitmap; } + + ScenePicture? lastRenderedPicture; } void testMain() { late EngineSceneView sceneView; + late StubPictureRenderer stubPictureRenderer; setUp(() { - sceneView = EngineSceneView(StubPictureRenderer()); + stubPictureRenderer = StubPictureRenderer(); + sceneView = EngineSceneView(stubPictureRenderer); }); test('SceneView places canvas according to device-pixel ratio', () async { @@ -72,7 +77,7 @@ void testMain() { debugOverrideDevicePixelRatio(null); }); - test('SceneView places canvas according to device-pixel ratio', () async { + test('SceneView places platform view according to device-pixel ratio', () async { debugOverrideDevicePixelRatio(2.0); final PlatformView platformView = PlatformView( @@ -101,4 +106,24 @@ void testMain() { debugOverrideDevicePixelRatio(null); }); + + test('SceneView always renders most recent picture', () async { + final List pictures = []; + final List> renderFutures = >[]; + for (int i = 1; i < 20; i++) { + final StubPicture picture = StubPicture(const ui.Rect.fromLTWH( + 50, + 80, + 100, + 120, + )); + pictures.add(picture); + final EngineRootLayer rootLayer = EngineRootLayer(); + rootLayer.slices.add(PictureSlice(picture)); + final EngineScene scene = EngineScene(rootLayer); + renderFutures.add(sceneView.renderScene(scene)); + } + await Future.wait(renderFutures); + expect(stubPictureRenderer.lastRenderedPicture, pictures.last); + }); } From b2f82dcb828cf2ca7f0f27a495aa90ed1d80feeb Mon Sep 17 00:00:00 2001 From: Jackson Gardner Date: Thu, 19 Oct 2023 12:47:08 -0700 Subject: [PATCH 2/3] Make SceneRender more private. --- lib/web_ui/lib/src/engine/scene_view.dart | 28 +++++++++++------------ 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/lib/web_ui/lib/src/engine/scene_view.dart b/lib/web_ui/lib/src/engine/scene_view.dart index ae932ef4ed894..41ac3fab96cd0 100644 --- a/lib/web_ui/lib/src/engine/scene_view.dart +++ b/lib/web_ui/lib/src/engine/scene_view.dart @@ -16,17 +16,17 @@ abstract class PictureRenderer { FutureOr renderPicture(ScenePicture picture); } -class SceneRender { - SceneRender(this.scene, this.completer) { +class _SceneRender { + _SceneRender(this.scene, this._completer) { scene.beginRender(); } final EngineScene scene; - final Completer completer; + final Completer _completer; void done() { scene.endRender(); - completer.complete(); + _completer.complete(); } } @@ -44,31 +44,31 @@ class EngineSceneView { List containers = []; - SceneRender? currentRender; - SceneRender? nextRender; + _SceneRender? _currentRender; + _SceneRender? _nextRender; Future renderScene(EngineScene scene) { - if (currentRender != null) { + if (_currentRender != null) { // If a scene is already queued up, drop it and queue this one up instead // so that the scene view always displays the most recently requested scene. - nextRender?.done(); + _nextRender?.done(); final Completer completer = Completer(); - nextRender = SceneRender(scene, completer); + _nextRender = _SceneRender(scene, completer); return completer.future; } final Completer completer = Completer(); - currentRender = SceneRender(scene, completer); + _currentRender = _SceneRender(scene, completer); _kickRenderLoop(); return completer.future; } Future _kickRenderLoop() async { - final SceneRender current = currentRender!; + final _SceneRender current = _currentRender!; await _renderScene(current.scene); current.done(); - currentRender = nextRender; - nextRender = null; - if (currentRender == null) { + _currentRender = _nextRender; + _nextRender = null; + if (_currentRender == null) { return; } else { return _kickRenderLoop(); From 9a26b0b398c50318237d7fe79507599ceb9f2df6 Mon Sep 17 00:00:00 2001 From: Jackson Gardner Date: Fri, 20 Oct 2023 10:29:41 -0700 Subject: [PATCH 3/3] Make the unit test check explicitly for cancelation of intermediate frames. --- lib/web_ui/test/engine/scene_view_test.dart | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/web_ui/test/engine/scene_view_test.dart b/lib/web_ui/test/engine/scene_view_test.dart index 62d5c744d610c..2b83d1e53d40d 100644 --- a/lib/web_ui/test/engine/scene_view_test.dart +++ b/lib/web_ui/test/engine/scene_view_test.dart @@ -25,7 +25,7 @@ class StubPictureRenderer implements PictureRenderer { @override Future renderPicture(ScenePicture picture) async { - lastRenderedPicture = picture; + renderedPictures.add(picture); final ui.Rect cullRect = picture.cullRect; final DomImageBitmap bitmap = (await createImageBitmap( scratchCanvasElement, @@ -34,7 +34,7 @@ class StubPictureRenderer implements PictureRenderer { return bitmap; } - ScenePicture? lastRenderedPicture; + List renderedPictures = []; } void testMain() { @@ -107,7 +107,7 @@ void testMain() { debugOverrideDevicePixelRatio(null); }); - test('SceneView always renders most recent picture', () async { + test('SceneView always renders most recent picture and skips intermediate pictures', () async { final List pictures = []; final List> renderFutures = >[]; for (int i = 1; i < 20; i++) { @@ -124,6 +124,10 @@ void testMain() { renderFutures.add(sceneView.renderScene(scene)); } await Future.wait(renderFutures); - expect(stubPictureRenderer.lastRenderedPicture, pictures.last); + + // Should just render the first and last pictures and skip the one inbetween. + expect(stubPictureRenderer.renderedPictures.length, 2); + expect(stubPictureRenderer.renderedPictures.first, pictures.first); + expect(stubPictureRenderer.renderedPictures.last, pictures.last); }); }