512435 - document onload should not fire until all visible images have 1 decoded frame

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Bobby Holley (:bholley)]

See https://bugzilla.mozilla.org/show_bug.cgi?id=435296#c35

Comment 1

[Bobby Holley (:bholley)]

Attachment: patch v1

added a patch.

Comment 2

[Bobby Holley (:bholley)]

Attachment: tests v1

added tests. pushed this to try as f00402ced2e7

Comment 3

[Bobby Holley (:bholley)]

Attachment: patch v2

So the Flush_Display strategy doesn't work on mac at least because nsChildView::Update is a no-op:
http://mxr.mozilla.org/mozilla-central/source/widget/src/cocoa/nsChildView.mm#1659

Regardless of that, roc was concerned that it would be a perf hit, because we'd paint once before firing onload and then often afterwards once onload does something to the page.

from #gfx:
roc: on reflection I think bz is right and we shouldn't fire onload until we've decoded the first frame of visible images
vlad: hm, will we know that we have?
roc: firing onload before that is not script-visible but it could result in flicker when scripts try to manipulate these images
roc: so
roc: what I suggest is that we start decoding images that we get data for before paint suppression ends
roc: however
roc: once paint suppression ends, then we don't decode any more of those images until we observe them being drawn
vlad: that sounds fine

I've implemented that strategy here. 2 things to note:

1) I have the onload blocking for both nsImageLoadingContent and nsImageLoader, but I only have the pre-initial-paint handling for nsImageLoadingContent. According to a comment somewhere the initial paint delay doesn't affect backgrounds, but if the background image is loaded before the initial reflow, we would still have a problem.

2) This patch would benefit a lot from the machinery in bug 512260 so that we would only do this for images under the currently active presshell. Nonetheless, we can probably still land the patch as is and fix it later.

Since with this patch we should be "cheating" on Tp, I'm curious of the perf impact. pushed to try as f0a69f165daf.

Comment 4

[Bobby Holley (:bholley)]

Attachment: tests v2

altered the tests a bit in a way that really only makes sense when you see my entire patch stack of 8 patches.

Comment 5

[Bobby Holley (:bholley)]

actually, the changes to the tests do stand on their own. Since we kick off decoding for early loaded images, we can no longer guarantee that we don't decode hidden images. This was really a test for decode-on-draw anyway, so I moved it up the patch stack to its own patch that I'll post over at bug 435296.

Comment 6

[Bobby Holley (:bholley)]

roc says that right now we don't block onload for background images, so we can continue that for now. Updated patch coming shortly.

Comment 7

[Bobby Holley (:bholley)]

Attachment: tests v3

ugh - getting confused with my patch-stack. Small test fix.

Comment 8

[Bobby Holley (:bholley)]

aaaand that makefile bug will break the last try push. repushed as 21ffbf600bb4

Comment 9

[Bobby Holley (:bholley)]

Attachment: patch v3

Updated patch with the handling for nsImageLoader removed.

pushed to try as 08dfd6587e3d

Comment 10

[Bobby Holley (:bholley)]

unittests and talos were hanging due to the fact that we didn't always call OnStop* in erroneous cases.

Fixed this over at bug 435296, and pushed the result to try as 49715af3bdc0.

Comment 11

[Bobby Holley (:bholley)]

Attachment: patch v4

the last patch was hanging on Tp4 (http://www.it168.com/) due to the fact that we didn't stop blocking onload in when we cancel the request before OnStopFrame fires.

Fixed here. Pushed to try as dac31de7ed5f.

maybe now we can finally get try numbers (grumble grumble...)

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 398019 [details] [diff] [review]
patch v4

flagging for review on this since we want to land it at the same time as bug 435296.

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 397292 [details] [diff] [review]
tests v3

flagging joe for the tests as well

Comment 14

[Bobby Holley (:bholley)]

Comment on attachment 398019 [details] [diff] [review]
patch v4

this will change a bit soon because of some changes in bug 435296. Canceling review in the mean time.

Comment 15

[Bobby Holley (:bholley)]

Attachment: patch v5

updated patch. This one no longer does anything per se in imagelib, so I'm flagging bz for review and roc for sr.

pushed to try as 8223b3e547ed. This will land at the same time as bug 435296.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 398681 [details] [diff] [review]
patch v5

>+++ b/content/base/src/nsImageLoadingContent.cpp
> nsImageLoadingContent::OnStopDecode(imgIRequest* aRequest,


So this will decode all images that finish loading before the paint suppression timer fires, right?

>+nsImageLoadingContent::SetBlockingOnload(PRBool aBlocking)
>+  nsCOMPtr<nsIDocument> doc = GetOurDocument();

Do you need the strong ref here?

>+      doc->UnblockOnload(PR_TRUE);

Can you prove the PR_TRUE is safe?  Or needed, for that matter?  It looks unsafe to me.

>+   * Up until bug 435296, the image would always be completely decoded
>+   * as it was loaded, so a loaded image was also a decoded one. With
>+   * decode-on-draw, this isn't necessarily the case. Thus, we don't want
>+   * to fire onload for a document before the images actually show up.

I don't think you need the history.  Just start at "With decode-on-draw".

> Thus,
>+   * we force a paint right before firing onload for the document, and this
>+   * paint kicks off decoding in all visible images.

We do?  Where does that happen?  I don't see it in this patch....

Comment 17

[Bobby Holley (:bholley)]

Attachment: patch v6

Updated patch to address bz's review comments.

(In reply to comment #16)
> So this will decode all images that finish loading before the paint suppression
> timer fires, right?

yes. well, it will kick off their decoding, and block onload until it finishes.


> Do you need the strong ref here?

guess not.
 
> >+      doc->UnblockOnload(PR_TRUE);
> 
> Can you prove the PR_TRUE is safe?  Or needed, for that matter?  It looks
> unsafe to me.

probably isn't. I was just copying how things were done in the docloader. fixed.


> I don't think you need the history.  Just start at "With decode-on-draw".
...
> We do?  Where does that happen?  I don't see it in this patch....

That comment should have been there - it was obsoleted by the big one in .cpp. Removed it.

Comment 18

[Bobby Holley (:bholley)]

/me is in too much of a hurry.

s/should have been there/shouldn't have been there/

Comment 19

[Bobby Holley (:bholley)]

Attachment: tests v4

updated tests on top of some changes in bug 435296. Reflagging joe for review.

Comment 20

[Bobby Holley (:bholley)]

Attachment: tests v5

updated patch to pre-empt some of of the concerns dolske expressed over at bug 435296. Flagging him for review here now instead of joe.

Comment 21

[Bobby Holley (:bholley)]

pushed to mozilla-central:

patch: c4f251a78318
tests: 8f546b09e2e5

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Shouldn't this be marked FIXED?

Comment 23

[Bobby Holley (:bholley)]

yep.