Bug 232779 – [CG] When drawing a large sub-rect of a CGImage always create a sub-image

• Component: Images
• Status: NEW

[Said Abou-Hallawa]

1. Launch mini-browser from the debugger
2. Open the attached test case
3. Refresh the page multiple times.

Result: GPUProcess jetsams when the memory allocation reaches the 200MB limit.

[Said Abou-Hallawa]

Created attachment 443466 [details]
green-4000x4000.png

[Said Abou-Hallawa]

Created attachment 443467 [details]
test case

[Said Abou-Hallawa]

Created attachment 443470 [details]
Patch

[Said Abou-Hallawa]

Created attachment 443471 [details]
Patch

[Said Abou-Hallawa]

rdar://84238598

[Tim Horton]

Comment on attachment 443471 [details]
Patch

At first I thought this was a partial revert of http://trac.webkit.org/changeset/278863/webkit, but then the old code A) had a much smaller threshold and B) looked at the destination size instead of the source? Very confusing. Also confusing that we would make decisions in GraphicsContextCG based on GPUP behavior. But ok.

[Kimmo Kinnunen]

If cutting the subimage is a correctness approach, then it's not really correct to disregard this correctness aspect for big images. If the minification is wrong for 10x10 images, it's also wrong for 2000x2000 images?
To test this, your 4000x4000 should have 1px red frame and the draw of 3999x3999 srcRect should test that the destination doesn't contain red in pixels.

The test-case here doesn't trigger the bug, it creates 375x375 bitmaps which do not cause the jetsams and do not take the if-path added in the patch.

After running the test, you can inspect the GPUP size by running footprint.

To write a better test, maybe creating 7 distinct images (of same png) and cut big areas only, e.g. 3999 px * 3999 px in size.

Mostly the real problem here isn't that QuartzCore takes the copy.
This only counts against peak memory use. Jetsam isn't mostly synchronous.

The problem is that we store the ref to that copy in our subimage cache.

To make the copy in subimage cache account against WP, we should not use CGImageCreateWithImageInRect. We should instead make the subimage a iosurface image, and attribute that to WP. This would mean that subimage cache should be per-RRB, e.g. this should be propagated from RRB -> GraphicsContext.

If subimage cache is only about minification correctness, it should have a flag to trigger this and GPUP canvas should use it only for that. Maxification with gpu acceleration seems to be a waste of memory.

Also subimage cache should then be capped by memory limit, not entry limit.

[Kimmo Kinnunen]

And the minimal fix would probably be to separate "should use subimage" to two cases:
- "should use subimage" --> Trigger this for correctness
- "should cache subimage" --> Trigger this for memory use using heuristics based on src size

[Kimmo Kinnunen]

> we should not use CGImageCreateWithImageInRect. We should instead make the subimage a iosurface image, and attribute that to WP

And to clarify this:
Based on my brief test, CGImageCreateWithImageInRect is visible as CG raster data, e.g. it creates a malloc-buffer bitmap image. This gets Canvas Context2D GPUP the worst of all worlds, as the source is always IOSurface bitmap image and the destination is mostly always a IOSurface bitmap image. So we might already create the resources as IOSurface. This has the added benefit that we can attribute it to WP.