Bug 53953 - ImageDecoderSkia doesn't notify Skia's bitmap when the pixels have changed
Summary: ImageDecoderSkia doesn't notify Skia's bitmap when the pixels have changed
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-07 15:12 PST by George Wright
Modified: 2013-04-09 12:57 PDT (History)
4 users (show)

See Also:


Attachments
Notify skia (1.68 KB, patch)
2011-02-07 15:16 PST, George Wright
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description George Wright 2011-02-07 15:12:54 PST
Even though the image decoder supports incremental loading of images it never notifies Skia when the pixels in the image buffer have changed.
Comment 1 George Wright 2011-02-07 15:16:05 PST
Created attachment 81538 [details]
Notify skia

Notify skia that the bitmap's changed when we update the status of an image decode.
Comment 2 Eric Seidel (no email) 2011-02-07 23:43:50 PST
Looks sane to me.
Comment 3 Eric Seidel (no email) 2011-02-07 23:43:59 PST
How do we test this?
Comment 4 George Wright 2011-02-08 08:47:42 PST
(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.
Comment 5 Peter Kasting 2011-02-08 11:02:24 PST
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.
Comment 6 George Wright 2011-02-08 11:12:26 PST
(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 :)
Comment 7 Peter Kasting 2011-02-08 11:22:01 PST
(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().
Comment 8 George Wright 2011-02-08 11:58:01 PST
(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.
Comment 9 Peter Kasting 2011-02-08 12:06:45 PST
(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).
Comment 10 George Wright 2011-02-09 07:54:54 PST
(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 11 David Levin 2011-02-09 12:42:34 PST
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.