Closed Bug 1163856 Opened 9 years ago Closed 9 years ago

Considering delaying the image load event until the size is available instead of performing a sync size decode

Categories

(Core :: Graphics: ImageLib, defect)

Product:
Core
Component:
Graphics: ImageLib
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla42
Tracking Flags:
Tracking Status
firefox42 --- fixed

People

(Reporter: seth, Assigned: seth)

References

(Blocks 1 open bug)

Details

(Whiteboard: gfx-noted)

Attachments

(2 files, 3 obsolete files)

(Part 2) - Fix tests that depended on image load event timing
(deleted), patch
tnikkel
: review+
Details | Diff | Splinter Review
(Part 1) - Delay the image load event until the size decoder gets finalized
(deleted), patch
Details | Diff | Splinter Review 
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.
Whiteboard: gfx-noted
Depends on: 1174923
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.
Blocks: 1175371
No longer blocks: 1175371
Blocks: 1177550
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+
And a few random other places, like WinXP m-oth a couple of times and Linux64 m-oth once.
Blocks: 1120511
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
Blocks: 1180931
Depends on: 1299793
Depends on: 1299776
Depends on: 1325080
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: