Rasterize to RGBA, then copy to NRGBA

[redawl]

While trying to speed up fyne solitaire, I discovered that golang.org/x/image has a fast path for RGBA dst images, which results in much faster raster times compared to NRGBA. The difference is so great that even the extra overhead of copying an RGBA image to a new NRGBA image after the raster results in ~65% faster svg rendering speeds.

The downside is that there are numerous off-by-1 errors that cause all of the SVG snapshot tests to fail, as well as any snapshot tests that contain an SVG (There's a lot), though I do not see any difference visually between the snapshots. Still, I think it's worth considering, given how massive of a speed boost it gives.

Another option would be to contribute a fast path to golang.org/x/image for NRGBA, but I haven't looked into how feasible that is.

Checklist:

• Tests included.
• Lint and formatter run with no errors.
• Tests all pass.
• Any breaking changes have a deprecation path or have been discussed.

[andydotxyz]

I agree this sounds worth it. Probably with a comment in the code to make sure folk don't accidentally remove the seemingly wasteful draw/copy process.

Contributing upstream is a nice idea too, a longer term fix perhaps - if there is an issue then leave that in the comment as well so we can find it later.

[dweymouth]

Probably makes sense to add a benchmark test to make it easier to verify (now and in the future) the performance improvements

[redawl]

Probably makes sense to add a benchmark test to make it easier to verify (now and in the future) the performance improvements

Absolutely. Just wanted to confirm it was worth moving forward before making a benchmark :)

[coveralls]

coverage: 60.437% (+0.04%) from 60.396%
when pulling 1d4a5fa on redawl:svg_optimization
into d9f3573 on fyne-io:develop.

[redawl]

Ready for review. I have added a benchmark, fixed the snapshots, and added an explanatory comment so the copy doesn't get removed accidentally in the future.

Here is the benchmark results on my machine:

redawl@workstation ~/projects/fyne $ benchstat /tmp/{old,new}.txt
goos: linux
goarch: amd64
pkg: fyne.io/fyne/v2/internal/svg
cpu: AMD Ryzen 7 7800X3D 8-Core Processor
                           │ /tmp/old.txt │            /tmp/new.txt             │
                           │    sec/op    │   sec/op     vs base                │
Svg/group_of_rects-16        20.254m ± 1%   6.041m ± 1%  -70.17% (p=0.000 n=10)
Svg/transluscent_RGBA-16     15.857m ± 1%   4.618m ± 1%  -70.88% (p=0.000 n=10)
Svg/Alpha-16                 15.804m ± 1%   4.644m ± 1%  -70.62% (p=0.000 n=10)
Svg/rects-16                 13.619m ± 2%   4.531m ± 2%  -66.73% (p=0.000 n=10)
Svg/NRGBA64-16               16.268m ± 2%   4.615m ± 1%  -71.63% (p=0.000 n=10)
Svg/transluscent_RGBA64-16   15.796m ± 2%   4.604m ± 1%  -70.86% (p=0.000 n=10)
Svg/Alpha16-16               15.790m ± 1%   4.611m ± 1%  -70.80% (p=0.000 n=10)
Svg/Gray-16                  16.297m ± 1%   4.584m ± 1%  -71.87% (p=0.000 n=10)
Svg/Gray16-16                16.474m ± 2%   4.682m ± 1%  -71.58% (p=0.000 n=10)
Svg/circles-16               16.160m ± 1%   4.612m ± 2%  -71.46% (p=0.000 n=10)
Svg/polygons-16              14.867m ± 1%   4.567m ± 1%  -69.28% (p=0.000 n=10)
Svg/negative_rects-16        13.648m ± 1%   4.568m ± 1%  -66.53% (p=0.000 n=10)
Svg/group_of_polygons-16     20.930m ± 1%   6.009m ± 1%  -71.29% (p=0.000 n=10)
Svg/RGBA64-16                16.382m ± 1%   4.668m ± 1%  -71.51% (p=0.000 n=10)
Svg/group_of_circles-16      30.989m ± 1%   7.527m ± 1%  -75.71% (p=0.000 n=10)
Svg/translucent_NRGBA64-16   15.797m ± 1%   4.658m ± 1%  -70.51% (p=0.000 n=10)
Svg/RGBA-16                  16.265m ± 2%   4.596m ± 2%  -71.74% (p=0.000 n=10)
Svg/paths-16                  8.455m ± 1%   3.151m ± 2%  -62.73% (p=0.000 n=10)
Svg/group_of_paths-16         7.551m ± 4%   3.135m ± 2%  -58.48% (p=0.000 n=10)
geomean                       15.53m        4.674m       -69.91%

[redawl]

One thing I'd like to discuss:

This change will break existing snapshot tests for user applications. I don't think there is a way to avoid this. How do we let users know that snapshot test breakage is expected once this lands?

[dweymouth]

Would be interesting to run the benchmark for different sizes of output image as well, especially small sizes. 16x16, 32x32, 64x64, etc. The old approach could be more efficient for small images (or maybe not)

[redawl]

Would be interesting to run the benchmark for different sizes of output image as well, especially small sizes. 16x16, 32x32, 64x64, etc. The old approach could be more efficient for small images (or maybe not)

This is for 16x16:

redawl@workstation ~/projects/fyne/internal/svg $ benchstat /tmp/{old,new}.txt
goos: linux
goarch: amd64
pkg: fyne.io/fyne/v2/internal/svg
cpu: AMD Ryzen 7 7800X3D 8-Core Processor
                           │ /tmp/old.txt │            /tmp/new.txt             │
                           │    sec/op    │   sec/op     vs base                │
Svg/negative_rects-16        14.676µ ± 1%   5.732µ ± 0%  -60.94% (p=0.000 n=10)
Svg/group_of_paths-16         8.099µ ± 1%   4.298µ ± 0%  -46.93% (p=0.000 n=10)
Svg/group_of_circles-16       38.43µ ± 1%   16.89µ ± 1%  -56.06% (p=0.000 n=10)
Svg/Alpha-16                 19.076µ ± 0%   9.755µ ± 1%  -48.87% (p=0.000 n=10)
Svg/rects-16                 14.647µ ± 1%   5.738µ ± 1%  -60.83% (p=0.000 n=10)
Svg/group_of_polygons-16     23.429µ ± 1%   7.586µ ± 1%  -67.62% (p=0.000 n=10)
Svg/NRGBA64-16               19.089µ ± 1%   9.771µ ± 1%  -48.82% (p=0.000 n=10)
Svg/translucent_NRGBA64-16   19.039µ ± 0%   9.796µ ± 1%  -48.55% (p=0.000 n=10)
Svg/RGBA-16                  19.014µ ± 1%   9.785µ ± 1%  -48.54% (p=0.000 n=10)
Svg/transluscent_RGBA-16     19.085µ ± 1%   9.790µ ± 1%  -48.70% (p=0.000 n=10)
Svg/transluscent_RGBA64-16   19.133µ ± 1%   9.763µ ± 1%  -48.97% (p=0.000 n=10)
Svg/Gray-16                  19.071µ ± 0%   9.811µ ± 1%  -48.56% (p=0.000 n=10)
Svg/paths-16                  8.963µ ± 1%   4.474µ ± 1%  -50.09% (p=0.000 n=10)
Svg/polygons-16              14.962µ ± 0%   5.961µ ± 0%  -60.16% (p=0.000 n=10)
Svg/group_of_rects-16         25.15µ ± 1%   10.69µ ± 1%  -57.51% (p=0.000 n=10)
Svg/Alpha16-16               19.047µ ± 1%   9.768µ ± 1%  -48.72% (p=0.000 n=10)
Svg/circles-16               19.063µ ± 0%   9.788µ ± 1%  -48.66% (p=0.000 n=10)
Svg/RGBA64-16                19.022µ ± 1%   9.761µ ± 1%  -48.69% (p=0.000 n=10)
Svg/Gray16-16                19.114µ ± 1%   9.771µ ± 0%  -48.88% (p=0.000 n=10)
geomean                       17.91µ        8.445µ       -52.84%

Looks like it's still way faster, but not quite as fast as for 500x500

[dweymouth]

One thing I'd like to discuss:

This change will break existing snapshot tests for user applications. I don't think there is a way to avoid this. How do we let users know that snapshot test breakage is expected once this lands?

This is an interesting question @andydotxyz. Given that we've made changes before that necessitate replacing all the golden .pngs for tests, I think it's a non-issue and we don't promise that pixel perfect rendering remains the same across even patch versions?

[andydotxyz]

Yes that's about the size of it. In general in my tutorials etc I encourage using unit testing on behaviour not image capture.