Even though the image decoder supports incremental loading of images it never notifies Skia when the pixels in the image buffer have changed.
Created attachment 81538 [details] Notify skia Notify skia that the bitmap's changed when we update the status of an image decode.
Looks sane to me.
How do we test this?
(In reply to comment #3) > How do we test this? This would need a Skia backend that doesn't update the pixels on-screen for the bitmap unless it gets the notifyPixelsChanged() call.
How often do you need to notify the backend? setStatus() is called rarely. You'll get a setStatus() call before any pixels are decoded and another one after all the pixels are decoded, but not any time in between. If you need to be called whenever pixels changed, you'd need to either piggyback on ImageFrame::setRGBA() or write a new function that the decoders can call more frequently.
(In reply to comment #5) > How often do you need to notify the backend? setStatus() is called rarely. You'll get a setStatus() call before any pixels are decoded and another one after all the pixels are decoded, but not any time in between. If you need to be called whenever pixels changed, you'd need to either piggyback on ImageFrame::setRGBA() or write a new function that the decoders can call more frequently. Well, the main one is to be called after the entire image has loaded to make sure that we don't get stuck in a half-loaded state where Skia just keeps a half-downloaded image on the screen and never updates it. Anything in between is a bonus. This seemed like the least invasive place to put it, although I'm open to suggestions on better places to put it :)
(In reply to comment #6) > (In reply to comment #5) > > How often do you need to notify the backend? > > Well, the main one is to be called after the entire image has loaded to make sure that we don't get stuck in a half-loaded state where Skia just keeps a half-downloaded image on the screen and never updates it. If that's all you need, this probably works. It does seem a bit confusing though as setStatus() doesn't necessarily imply anything about the immediate pixel state, so a "notify pixels changed" call feels wrong semantically. A better fix might be to instead make this call inside NativeImageSkia::setDataComplete().
(In reply to comment #7) > A better fix might be to instead make this call inside NativeImageSkia::setDataComplete(). Ideally we'd want to notify the pixels have changed every time the pixel data changes, but I will give this a try and see if it works.
(In reply to comment #8) > (In reply to comment #7) > > A better fix might be to instead make this call inside NativeImageSkia::setDataComplete(). > > Ideally we'd want to notify the pixels have changed every time the pixel data changes, but I will give this a try and see if it works. If you really want that, then you need to modify ImageFrame::setRGBA(), and perf-test your change to see if it impacts decoding speed (because this is called once for every pixel in every image).
(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > A better fix might be to instead make this call inside NativeImageSkia::setDataComplete(). > > > > Ideally we'd want to notify the pixels have changed every time the pixel data changes, but I will give this a try and see if it works. > > If you really want that, then you need to modify ImageFrame::setRGBA(), and perf-test your change to see if it impacts decoding speed (because this is called once for every pixel in every image). That sounds like it'd have a huge perf hit. I think once at the end of everything should do the trick; I'll give setDataComplete a go.
Comment on attachment 81538 [details] Notify skia Clearing the r? flag as it sounds like a new approach is being attempted. Feel free to add back the r? if appropriate.