Closed Bug 1163856 Opened 5 years ago Closed 4 years ago
Considering delaying the image load event until the size is available instead of performing a sync size decode
Right now we have a problem: it's not unusual for RasterImage::OnImageDataComplete to get called before RasterImage::SetSize. Generally the size decoder that would call SetSize has already finished running, but in order to actually call RasterImage::SetSize it needs to dispatch a runnable to the main thread, as SetSize is a main-thread-only method. The result is that we do a synchronous size decode in OnImageDataComplete - in other words, we frequently end up doing *two* size decodes for an image. This hurts us, particularly on lower-end devices. Bug 1160423 explores one approach to solving this problem: just allow RasterImage::SetSize to be called off-main-thread. I'm not necessarily convinced this is the ideal solution though, because it forces us to add a lot of locking since we touch RasterImage::mSize everywhere, and because unless we're willing to block the main thread for a potentially unlimited amount of time, we *still* may end up having to do multiple size decodes for each image. Before going in that direction, I'd prefer to try just waiting to send the image's load event until the size is available. I've experimented with this approach before, and AFAIK it works fine; the only real issues are: (1) We have a test that specifically checks for this and fails if we do it; we need to change that test. (2) We need to still do a sync size decode for certain types of images. In particular, it's necessary to do this for imgLoader::DecodeImageData unless we want to just synchronously decode the entire image in that method, which I'd prefer to avoid. So we'll need a new Image initialization flag that keeps the current behavior in this case. (At least until we come up with a better approach.)
Here's the patch. I think we need to see how this looks on try before requesting review.
Looks to me like the issue with those failures is that we're not blocking the document load event until we deliver the image load event in FinalizeDecoder. As long as the Necko is loading the image, the document load event will be blocked because the load group will be nonempty. But if in OnImageDataComplete we decide to delay firing the image load event until FinalizeDecoder, we can't rely on that because the Necko transfer is complete, so we have to manually block the document load event. This version of the patch makes us do that by blocking onload in OnImageDataComplete if we're going to delay the image load event, and unblocking onload in FinalizeDecoder when we fire the delayed image load event.
Attachment #8604373 - Attachment is obsolete: true
I made one other change in the new version of the patch: if we're doing a sync load, we never create the async size decoder, as it's just useless.
Attachment #8622770 - Attachment is obsolete: true
This is the same patch as before, just renamed to "part 1" since a part 2 is getting added.
This part fixes tests busted by part 1.
Attachment #8626344 - Flags: review?(tnikkel)
Attachment #8626344 - Flags: review?(tnikkel) → review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/cf2d7e67dab0 - basically every Win8 and Win7 debug suite asserted like https://treeherder.mozilla.org/logviewer.html#?job_id=11115761&repo=mozilla-inbound
And a few random other places, like WinXP m-oth a couple of times and Linux64 m-oth once.
This updated version of part 1 (which I've just pushed) resolves the oranges that caused the previous backout by ensuring that even when mSyncLoad is set, RasterImage::Init still fails when a RasterImage is created with an invalid MIME type. The old version of part 1 broke this by not creating a size decoder in RasterImage::Init when mSyncLoad was set. The new version still doesn't create a size decoder for sync loaded images, but it checks that there's a decoder type available for the requested MIME type and makes RasterImage::Init fail if there isn't one.
Attachment #8626343 - Attachment is obsolete: true
3 years ago
No longer depends on: 1299793
You need to log in before you can comment on or make changes to this bug.