From 82538455f95aaf584730faaf843708829773eee2 Mon Sep 17 00:00:00 2001 From: William Candillon Date: Tue, 2 Apr 2024 11:46:15 +0200 Subject: [PATCH] =?UTF-8?q?fix(=F0=9F=90=9B):=20memory=20leak=20with=20CPU?= =?UTF-8?q?=20surfaces=20on=20Node/Web=20(#2327)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/docs/getting-started/headless.md | 5 +- package/src/__tests__/setup.ts | 4 +- package/src/__tests__/snapshots/leak.png | Bin 0 -> 8423 bytes package/src/headless/index.ts | 12 ++++- .../src/renderer/__tests__/SkiaDOM.spec.tsx | 8 ++-- .../src/renderer/__tests__/Surfaces.spec.tsx | 43 ++++++++++++++++++ package/src/renderer/__tests__/setup.tsx | 7 +-- package/src/skia/web/JsiSkImage.ts | 26 +++++++++-- package/src/skia/web/JsiSkSurface.ts | 9 +++- package/src/skia/web/JsiSkSurfaceFactory.ts | 19 +++++++- 10 files changed, 116 insertions(+), 17 deletions(-) create mode 100644 package/src/__tests__/snapshots/leak.png create mode 100644 package/src/renderer/__tests__/Surfaces.spec.tsx diff --git a/docs/docs/getting-started/headless.md b/docs/docs/getting-started/headless.md index b88740e77c..6ff74ae96c 100644 --- a/docs/docs/getting-started/headless.md +++ b/docs/docs/getting-started/headless.md @@ -15,13 +15,16 @@ You will notice in the example below that the import URL looks different than th ```tsx import { LoadSkiaWeb } from "@shopify/react-native-skia/lib/commonjs/web/LoadSkiaWeb"; -import { Fill, makeOffscreenSurface, drawOffscreen } from "@shopify/react-native-skia/lib/commonjs/headless"; +import { Fill, makeOffscreenSurface, drawOffscreen, getSkiaExports } from "@shopify/react-native-skia/lib/commonjs/headless"; (async () => { const width = 256; const height = 256; const r = size * 0.33; await LoadSkiaWeb(); + // Once that CanvasKit is loaded, you can access Skia via getSkiaExports() + // Alternatively you can do const {Skia} = require("@shopify/react-native-skia") + const {Skia} = getSkiaExports(); const surface = makeOffscreenSurface(width, height); const image = drawOffscreen(surface, diff --git a/package/src/__tests__/setup.ts b/package/src/__tests__/setup.ts index 17060c5a0a..0995bbb7c2 100644 --- a/package/src/__tests__/setup.ts +++ b/package/src/__tests__/setup.ts @@ -25,7 +25,9 @@ export const processResult = ( surface.flush(); const image = surface.makeImageSnapshot(); surface.getCanvas().clear(Float32Array.of(0, 0, 0, 0)); - return checkImage(image, relPath, { overwrite }); + const result = checkImage(image, relPath, { overwrite }); + image.dispose(); + return result; }; interface CheckImageOptions { diff --git a/package/src/__tests__/snapshots/leak.png b/package/src/__tests__/snapshots/leak.png new file mode 100644 index 0000000000000000000000000000000000000000..ae8234bbb596c27454cbed8c760bcccfae44788b GIT binary patch literal 8423 zcmeHN`9GBF-@ok2dX7qwt&*c`QBBd{P)7*Wp&@&eGT91*ulq7=L1$(N`7uNUMy@y#h&-vJ7Pm_}C8WioeYMBM0b zTnCnhP9hoVHqONaR`ky-G#K;p7Zw#U56H;$sP*NIG+202*J;CadRAmsR-YkmhAx|j zU!s4^%F4P$r_)_A7|a_o7IUp5%ig&oE9+C1J>?^Fy2nsZ!P3(5PIGhf>GS6!)l^kQ z6ctm@ii(QkLqkIek&%(C&!0cz!@|OtTeohVR#sCRPtTHF{8;xSDP*NB+acI!m=gp5 z>GUopU^J#%ka7p1gzL_Idg-iQUZ9|)7yTtlDMgQ&+trh57Rzky%yo$_kq4lzEUx$RB}!1& zEJ0k77l4@PXz8X?`GYeu$S2|RnVFe#CF5^n6B3U4(%9k#=Z&b}VzibBYd!sJ4=i@l zXmx%E0J9&U<`31_$0b_69*HlA7 zGriPHFAp|$YH@*xS}7^1tn#6U`wv@TFh-Twgh_Z!&3-@h&`d-`q=U(p;0DAbMpRy&DfLLhYRJu};%`zq}Cr2ar^H$7NcMp$v=<+(TwAP8&{cL7& zqSS#4#~C7NLkBN>y3c4p^{>0`E4*D!Ck%8Cj$o(m9rVensD zm8@L;Ec4*_uGP1Hc3JIua?&wiP_nqVpdd~bfbG5AJ%5h$uhUS8*Ku-o#Y7v7-HQM? z4HOij4_0Jld3x2|GJ9o-!4wx~PQRXmUSoG4Udyn_XWmokTsHPXVcRoUs;a7~&>5>u z54Y}biaiz>>9uHe?r`9I@`h!S035l2L`-Wt>2bxEC{c{Zbf=%8VYLXHuY#PNU0Il( zzHgOgK4E5MWi`IKx@rrZXGCw*nF)HAk)g8s)?7QXzP`Ts>vL7I7yt}vq@t;;nu^M! zikPU?vuDnnx$z}ZQ4AWO+li`Nc|6J@5k#23J~+Pm+}1Pj7y#avF%B!m+0fuJV^-T@ zM}LkQlB=lxpoJ&}p~F|t5^ybMD6|X!%V9b*WecxeHFpFj?G-r=W3Jq9TQf)VbQ3GZw3*EFmI&Sf%g#cW)0En?`14 zj|p1WhU>;+HI<_t@6g)Y*V+00HyGDZG}<#?CG)uN5_wLBdoPw6N9|><4=ZJ+!o&;u z-d;5EUc&sy2byDGAL8ob!c1*w@^Bt3{4cc{kGQ~N5qYtNQd+9DR8*W=&ja+;Oj6E? zu^6eN*Woq;jJuloB<*r`KRb^;2+LQ_$5WgtN6sn+D;KgXc_M~g644N{+LHJ@To!%9 zL(n;B$_AdES#U5bvtrCtI{@z6-qDd$4nTy^d0X4maA_3Pw6fV*N-xB~0;zV~pb9A0 zwTg9@Fno4nz{2tM4Iln+!HT<=^yG`#_4 zkJ`KR^oJEbk=I@iR^LmFg~g2@5JNQNjfmPhIQH5?6Gy?N^1_yDHaHw^Y^pbq8m6AL zr05LvnKq?<<9c^h-edQToEK={21hPqZLizG&>fRjRF))X%EWpi12DKZe_^jz%i^bU zjbJ{Fu-A9rdA6)fTo^DupDE{^`s{eW7J29oRa^fhw1N=*mdOhBW}zVXzCSpG z`+HZ(f}1qT({D*`*CgEWe9dENuQ0R#h^VHQj3)FLZtGon$`}>RFa|`i)>I`h?zg-m zq@J;VYQ~vZVVqiB>k!}^kJ~5(ot*(TyPn(+sr|Fr5h)=8azV56FtxZy%bWCleCAx5 z&`Wkd^o4mC_V82OXab&2>huNRXmW1uT;9izAIESyishB?#?N=5zJv=5XpqK=m)GZ` zE6cF`6~R1*guJ&vv?3`R%*buuc=HXcnNtv-jNy(d%9KV%{%Qd&sh(JrAR5U9Cufq$ zSa~-t(pd<50@mkhF`p86!1iGzhAC_6BBW3;xdcxS57tR|YWE$`Tmj53H%!Tz*zM}} zOxx=GCfy$7yySHjCpw7q&fK2~X0eAA`EP80MVRW!A2rM_XLe`ij>1rVEg%~(P#?ip zE462_;`pv7o>#Bh?F4}OlP4UY7rtqsE?y9S#Yt`%pIz(cP$d8us7r=bfA2=d50Ks_ z@5as#8>34JcIqHpC*zPA2z2ab@tt z&?ymaMrA*Xj&po~<^dWf3yM?sk;&vFIhTmmC8WYnPp8$xnpdTe2+A1Ep>G>vQEu0^ zy-M`xE>JF8o!I}?rHKo;h8sG7W9;RhiH*bZRaMscml{SdG{L@8QdDFm@%8K1Bss?j zKgLEeL#hz~;-YiBPSf4j_xsa(W5#WWTsbbaq#Xb0Qh|;6?%j3(88!>UQ>VnZg?D15 zKtRu$8aSKQX4sJt_~y47jjF1E`7eh~Al(}A;1Ir_uc|%Mt8cd5*6Z^NzU}h1MOQc3 z7FS$1oH`1K%Tyd!4#a&Y4)AmFKQ9ykcf2Enh;9ji{w9xpC*Hj1!2=Y^M!Bn22zB;U z*ewhKn)rwdoCr!8tkrjBAqaH4qfl(~6I4;uK-@2(y_X*IfEhb*gdS^@%CiybBxM7do<3q`@obi5D| zLeZ@?J9+Dbu`*!ZR`ZTH5?)@=yG#{*{;Tj>P*h|d4l_#J2!07`A`_J>z@_On+8CdR zoY}@On~|y)yo)KXPlIh_Br4Ypy5wZ2_{Zm}wl$w+dF$d()PSGMov>i~>@gG^9Eo<6 z-HI2N_ss|db&g-e4FcvqONP0Kal4WZiGh{q6KC zc|*tjPD2pmHqE0SVnXjOBx^Fo^=US1AR;l6;(z^ltt@w487%+XJ|}QN_&l6|#^3w& z_n0+&>Ru!Sy~>Y}8Fg^}&p-Y3RAao33=i>`>tD0>mQ+iCI>WzKG^%CZ`e#LdKfgUL zVqC6NdLKms@s6hY6$!ACMxpd{3lNVzoubo8Xddz3jg0OI!A@)V04b^PSs0A2))1t* z=tC3YI0ArpEHVT30Ql_9Lg*=F8;E%E{PNsbIWYfc-n)0d$>0U$7czoDoy8XjG7|nV z;oP}%Q{n&h$Z&)LiA!I1pSoO;I#U?v(-gxVhL#Em23{UJjm&i>udJ-MnE1P_l(yeH zCD?xMMUISIw9mZrYiwReN^)vzCHk=n-z-z;^`&1-t*6@1F3E<_%*u3kk_^ zal=jS)D2peyMTBh$GQ6O-!7aTtmfeUIk&I%^&t+YPmAM4F8U3uc;k?cqCHcj)rG%rz#SqwJ)Kq z&x953h!Fx9LjQXtc+8Naj)biB0*I)DZ=JTXT0A}VJMmjCjZ2=ONM(mX5Bk9{`({HF zewUWEDfY03NziQNCNTVFxQB4yV*QrNZQ}D)PnBz@!;nm9f@vtR_f+RrUvYV@Kf+~O zw&M9Ao~^6^o#H;C@`nFO$((&OdHqZONy+WHL7(&FJxR?}aZrBV|M7 z0D%kQbzxdyIk3>QI0%9cmm zzs(rpCmDo6xNMGGt@Qa>lfnZcmZ*!oPJ?~j$W$k0r%B%tyTd;O+zYuQIOrk$51KE)s9h=H={U<&}NQ%UU#0StJ|P=KJq zovwVnqP;D*0ncB90_EU^=E(bOVTd7H@&V7Qva(~RVy^W@Z)KA~-Xp(;lxD&u3N zvmb?uVQ}+^uJX#?5jTer?4H(9K@E*dO$jVL7o?IS75{H{e)mOk)B`&v&yh3YAp6aE|9UddTnS=}9PNmYLc%d1A+iaEIA?YG#5DVG93^fkr;^2xz(T9ayX zTR@(zv9a-y#a0VDDWu*RK@O?t1@T<9^7w#^brJygS|^ntJ@sz4aSKk$$?<-5iHl_5 z?CiWetfaTHxB)gUeYnLJ4~_LRxq%aC-wVYFnI94m&AothxB2+41LFB0`02?#AIkz4 zV1eTSb!XfXS!sRnjR2EX--S`aTOCMOKK`P|1<$%EgjpuAiWh%1(}-Tu!fljIpA8p4 z+^U-rH1kNyydT2g8)+6B`?w{fwroQXB%>x)G*(15u6n&rj{ zQ~7#}54yW^SqFr^n-&(jRCl=%CM1w^oo_`b7JSFZvOZcg1I`jqCa!XEuR5C>82x+~ z4$3`#yfLjfXKC5$NZ9Eue&WOlhjl)XSKEehTy(FyrIUO%%irlS7hu>_qLIy)mX_v2 zgvygtw4YHWwNQMDWydISSizw$kYy_P+vOZ1yHF*R+0{D(t;U905N8fgBYlXDn=YvD5j z5!CqpB{AS?QbXlh#xBr;Oa0mFD-@UYt96GLMw|N4Uer`9QVBF(b$54TCng$urSk^p zlIZ3?VQajnK~8M>`~fwf0`jLrCXARCXK; zy7@DZj~DJXB0Z{5j?(;5_hfg=`PlrJa(hg9Nx`adEL2%B2R@r3?Xb z)E8FLXlST(I)uVr6Uo$r?yc__8%z^`S2I%5(vlVl z$I_rr_${<8VGo>|Q*?w;mYk9@!m!D|G%aS6^CODd^F=AKjH9G?<)|uLiqPFHE9D5q z50vYNP>UNB-^QbEI9uDbPN>3g@MEu;XN0x|Wofy7sIk!ItS{hF&Zb4e?N`B8B56lp zEvj$XvgLR`*r&Jux8F`WLgj^U3Vs9iL`|2Kc}lb#WQ;#hx1lbBt$iD<&Y25`qEJjn zcenOJoNr^7OrDd=yWR~^2m(yKUjevZ`$S&E&oETjxIf_M<0kESSQ1X`+|;aZX{&QM zc&2=*4?a|Z*yZDt3yZU(xC=8_z zp&+S9xcK#XAJlm2>`8oSZ{F-;WkNY}M{O_+s{1a#uXL@9CiWu`6v0G^JB8}QcP>Gq zCzEEGK3vq}xz+(=^~+s-()DIzrN!PGlOCMsCMhP+6J;IoIO@vxwA7Fw$GTe$15X1CBjFfZ7lWrHccn8rraozFbW$oZeTQHP9@^$CH6~wOW_3EyQZzMEs%3_ zedbO`+P5BXt&4y%rc4=xUdfVbcAY<<7_;o12&0N|9^s!D$KWpXgT^HAylGH+S^;yfv0y zzEojLVk|(`*!OF$K%EtGWYD-2Hk7v-BxOUE2lE;m8#{N4?1NT1!y?h6$DRw%mer!Y z;8b`I-o;;N;)T+U9dOmRwZ%6;y=0x$T{y{}Y_ZTo)5cQ?wmDw?6`d{@pF!%a;Y<6* z(sFAI3a)>u%sFe%8k~Vj#EIAoO?)5%YX^=)Im&$qn_bTCE5vdZ;tpXG(nBC`jzpu; zxwZB0pjh_SBJsyUZg~7Wtu^gWPScA6NgVc2m}L2Oyc!_djdIHiK}yuv3zhoJCOyu2 zn#HlX<84CfWf4({U*~G&5+QeB+P1tkTf6SzG3|lKy)C(J+@KD1wB*OmsdHI&f%H%j zX-{N=f@sH(l54EhMMM6c6CZ}ZM71FJQOAF(le9@hO1D>-wVa#whCn+y)!VzL^cE`uNLKIDH*dEG%^>K3>{Vts?-oPGeU_(W@nMg9bX!Q4X-Gi;xwZ!UDQ zovZgTA&r|>-oJlR4gRVqD|el-v&+j&P9AQEj2yz;6Om?iMpv$n5J>BZP$vHO=U)%} z>w$kg@UI8{Up+uR?Y { if (!Skia) { @@ -24,12 +24,20 @@ export const makeOffscreenSurface = (width: number, height: number) => { return surface; }; +export const getSkiaExports = () => { + if (!Skia) { + Skia = JsiSkApi(CanvasKit); + } + return { Skia }; +}; + export const drawOffscreen = (surface: SkSurface, element: ReactNode) => { const root = new SkiaRoot(Skia, false); root.render(element); const canvas = surface.getCanvas(); const ctx = new JsiDrawingContext(Skia, canvas); root.dom.render(ctx); + root.unmount(); surface.flush(); return surface.makeImageSnapshot(); }; diff --git a/package/src/renderer/__tests__/SkiaDOM.spec.tsx b/package/src/renderer/__tests__/SkiaDOM.spec.tsx index e00f6a2791..56620bc45e 100644 --- a/package/src/renderer/__tests__/SkiaDOM.spec.tsx +++ b/package/src/renderer/__tests__/SkiaDOM.spec.tsx @@ -15,8 +15,8 @@ describe("Test introductionary examples from our documentation", () => { ); - expect(root.children().length).toBe(1); - const child = root.children()[0]!; + expect(root.dom.children().length).toBe(1); + const child = root.dom.children()[0]!; expect(child).toBeDefined(); expect(child.type).toBe(NodeType.Group); expect(child.children().length).toBe(3); @@ -31,8 +31,8 @@ describe("Test introductionary examples from our documentation", () => { ); - expect(root.children().length).toBe(1); - const child = root.children()[0]!; + expect(root.dom.children().length).toBe(1); + const child = root.dom.children()[0]!; expect(child).toBeDefined(); expect(child.type).toBe(NodeType.Circle); const circle = child; diff --git a/package/src/renderer/__tests__/Surfaces.spec.tsx b/package/src/renderer/__tests__/Surfaces.spec.tsx new file mode 100644 index 0000000000..89cfa5692e --- /dev/null +++ b/package/src/renderer/__tests__/Surfaces.spec.tsx @@ -0,0 +1,43 @@ +import React from "react"; + +import { Circle, Group } from "../components"; +import { processResult } from "../../__tests__/setup"; +import { setupSkia } from "../../skia/__tests__/setup"; + +import { drawOnNode } from "./setup"; + +describe("Surface", () => { + it("A raster surface shouldn't leak (1)", () => { + const { Skia } = setupSkia(); + // When leaking, the WASM memory limit will be reached quite quickly + // causing the test to fail + for (let i = 0; i < 500; i++) { + const surface = Skia.Surface.Make(1920, 1080)!; + const canvas = surface.getCanvas(); + canvas.clear(Skia.Color("cyan")); + surface.flush(); + const image = surface.makeImageSnapshot(); + image.dispose(); + surface.dispose(); + } + }); + it("A raster surface shouldn't leak (2)", () => { + for (let i = 0; i < 1000; i++) { + //const t = performance.now(); + const r = 128; + const surface = drawOnNode( + <> + + + + + + + ); + surface.flush(); + //console.log(`Iteration ${i} took ${Math.floor(performance.now() - t)}ms`); + processResult(surface, "snapshots/leak.png"); + surface.dispose(); + } + }); +}); diff --git a/package/src/renderer/__tests__/setup.tsx b/package/src/renderer/__tests__/setup.tsx index 52ecbf361c..089799ec74 100644 --- a/package/src/renderer/__tests__/setup.tsx +++ b/package/src/renderer/__tests__/setup.tsx @@ -185,8 +185,9 @@ export const height = 256 * PIXEL_RATIO; export const center = { x: width / 2, y: height / 2 }; export const drawOnNode = (element: ReactNode) => { - const { surface: ckSurface, draw } = mountCanvas(element); + const { surface: ckSurface, draw, root } = mountCanvas(element); draw(); + root.unmount(); return ckSurface; }; @@ -201,7 +202,7 @@ export const mountCanvas = (element: ReactNode) => { root.render(element); return { surface: ckSurface, - root: root.dom, + root, draw: () => { const ctx = new JsiDrawingContext(Skia, canvas); root.dom.render(ctx); @@ -211,7 +212,7 @@ export const mountCanvas = (element: ReactNode) => { export const serialize = (element: ReactNode) => { const { root } = mountCanvas(element); - const serialized = serializeNode(root); + const serialized = serializeNode(root.dom); return JSON.stringify(serialized); }; diff --git a/package/src/skia/web/JsiSkImage.ts b/package/src/skia/web/JsiSkImage.ts index 76c307aed1..b3a522c196 100644 --- a/package/src/skia/web/JsiSkImage.ts +++ b/package/src/skia/web/JsiSkImage.ts @@ -45,7 +45,11 @@ export const toBase64String = (bytes: Uint8Array) => { }; export class JsiSkImage extends HostObject implements SkImage { - constructor(CanvasKit: CanvasKit, ref: Image) { + constructor( + CanvasKit: CanvasKit, + ref: Image, + private releaseCtx?: () => void + ) { super(CanvasKit, ref, "Image"); } @@ -128,6 +132,8 @@ export class JsiSkImage extends HostObject implements SkImage { return toBase64String(bytes); } + // TODO: this is leaking on Web + // Add signature with allocated buffer readPixels(srcX?: number, srcY?: number, imageInfo?: ImageInfo) { const info = this.getImageInfo(); const pxInfo: CKImageInfo = { @@ -148,6 +154,9 @@ export class JsiSkImage extends HostObject implements SkImage { dispose = () => { this.ref.delete(); + if (this.releaseCtx) { + this.releaseCtx(); + } }; makeNonTextureImage(): SkImage { @@ -157,7 +166,16 @@ export class JsiSkImage extends HostObject implements SkImage { ...partialInfo, colorSpace, }; - const pixels = this.ref.readPixels(0, 0, info) as Uint8Array | null; + + var pixelLen = info.width * info.height * 4; + const pixelPtr = this.CanvasKit.Malloc(Uint8Array, pixelLen); + const pixels = this.ref.readPixels( + 0, + 0, + info, + pixelPtr, + info.width * 4 + ) as Uint8Array | null; if (!pixels) { throw new Error("Could not create image from bytes"); } @@ -165,6 +183,8 @@ export class JsiSkImage extends HostObject implements SkImage { if (!img) { throw new Error("Could not create image from bytes"); } - return new JsiSkImage(this.CanvasKit, img); + return new JsiSkImage(this.CanvasKit, img, () => { + this.CanvasKit.Free(pixelPtr); + }); } } diff --git a/package/src/skia/web/JsiSkSurface.ts b/package/src/skia/web/JsiSkSurface.ts index 884678476c..a5e2c5c0ad 100644 --- a/package/src/skia/web/JsiSkSurface.ts +++ b/package/src/skia/web/JsiSkSurface.ts @@ -11,12 +11,19 @@ export class JsiSkSurface extends HostObject implements SkSurface { - constructor(CanvasKit: CanvasKit, ref: Surface) { + constructor( + CanvasKit: CanvasKit, + ref: Surface, + private releaseCtx?: () => void + ) { super(CanvasKit, ref, "Surface"); } dispose = () => { this.ref.delete(); + if (this.releaseCtx) { + this.releaseCtx(); + } }; flush() { diff --git a/package/src/skia/web/JsiSkSurfaceFactory.ts b/package/src/skia/web/JsiSkSurfaceFactory.ts index 57e3813c3b..32395aab28 100644 --- a/package/src/skia/web/JsiSkSurfaceFactory.ts +++ b/package/src/skia/web/JsiSkSurfaceFactory.ts @@ -11,11 +11,26 @@ export class JsiSkSurfaceFactory extends Host implements SurfaceFactory { } Make(width: number, height: number) { - const surface = this.CanvasKit.MakeSurface(width, height); + var pixelLen = width * height * 4; + const pixelPtr = this.CanvasKit.Malloc(Uint8Array, pixelLen); + const surface = this.CanvasKit.MakeRasterDirectSurface( + { + width: width, + height: height, + colorType: this.CanvasKit.ColorType.RGBA_8888, + alphaType: this.CanvasKit.AlphaType.Unpremul, + colorSpace: this.CanvasKit.ColorSpace.SRGB, + }, + pixelPtr, + width * 4 + ); if (!surface) { return null; } - return new JsiSkSurface(this.CanvasKit, surface); + surface.getCanvas().clear(this.CanvasKit.TRANSPARENT); + return new JsiSkSurface(this.CanvasKit, surface, () => { + this.CanvasKit.Free(pixelPtr); + }); } MakeOffscreen(width: number, height: number) {