Wrap ImageShader::sk_image_ with a SkiaGPUObject #28698
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| @@ -45,7 +45,7 @@ void ImageShader::initWithImage(CanvasImage* image, | |||
| ToDart("ImageShader constructor called with non-genuine Image.")); | |||
| return; | |||
| } | |||
| sk_image_ = image->image(); | |||
| sk_image_ = UIDartState::CreateGPUObject(image->image()); | |||
There was a problem hiding this comment.
This is an example of the multiple guard objects that I was referring to in a recent discussion. The SkiaGPUObject held by CanvasImage is a unique pointer and so we have to construct a new guard object here. If we made it shared with ref counts then we could use the same object here and elsewhere. The downside of the unique guard objects is that we have to enqueue both for an unref on the raster thread and only one of those unrefs will actually delete the object.
That's not a problem for this PR, but it's an open issue to consider. Hopefully if Skia makes SkImage thread-safe we can ignore all of this...
|
Can we write a Dart test that creates an image, creates a shader from it, then drops the image and makes sure that it goes through the queue, then drops the shader and watch it crash (or not with the fix)? |
SkImages must be dereferenced on the raster thread
jason-simmons
force-pushed
the
image_shader_wrapper
branch
from
14e04dd to
4747fc3
Compare
|
I added a Dart test to at least ensure that this API is getting some coverage (AFAICT the API is not currently used anywhere within the engine or framework trees) The Dart environment doesn't have a way to verify which thread was used to delete the SkImage though. |
| final ImageShader shader = ImageShader(image, TileMode.clamp, TileMode.clamp, Float64List(16)); | ||
| final Paint paint = Paint()..shader=shader; | ||
| final Rect rect = Rect.fromLTRB(0, 0, 100, 100); | ||
| testCanvas((Canvas canvas) => canvas.drawRect(rect, paint)); |
There was a problem hiding this comment.
Actually, this is testing the safe path. When you use the shader in a paint in a draw call - that is when the image gets wrapped in a GPUObject. The path that might cause a problem would be:
- create an Image
- create an ImageShader from that image
- Don't use that shader in a rendering call
- drop the Dart reference to the image (so that the flutter::Image reference goes away)
- wait for the unref thread to unref the image that was held from the Image object
- now drop the Shader reference as well
- wait for GC which should try to reclaim the flutter::ImageShader which will unref the SkImage without protection.
There was a problem hiding this comment.
yes - this test is just an attempt to invoke ImageShader in order to verify that a PR like this has not broken the Dart API.
Dart does not offer the fine grained control over GC and engine threads needed for the above scenario. And the unit test environment will be using a software rasterizer, so it would not show the potential resource leaks that might be seen when deleting the SkImage on the UI thread on the Android/OpenGL platform.
jason-simmons
added
the
waiting for tree to go green
SkImages must be dereferenced on the raster thread