Bug 53953 – ImageDecoderSkia doesn't notify Skia's bitmap when the pixels have changed

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Images

[George Wright]

Even though the image decoder supports incremental loading of images it never notifies Skia when the pixels in the image buffer have changed.

[George Wright]

Created attachment 81538 [details]
Notify skia

Notify skia that the bitmap's changed when we update the status of an image decode.

[Eric Seidel (no email)]

Looks sane to me.

[Eric Seidel (no email)]

How do we test this?

[George Wright]

(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.

[Peter Kasting]

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.

[George Wright]

(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 :)

[Peter Kasting]

(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().

[George Wright]

(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.

[Peter Kasting]

(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).

[George Wright]

(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.

[David Levin]

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.