1238337 - don't do a predictive image decode if the predicted size is likely to be wrong

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Timothy Nikkel (:tnikkel)]

Observed this while debugging something else.

nsImageFrame::ComputeSize calls EnsureIntrinsicSizeAndRatio, there's no mImage yet, so it gives the intrinsic size the size of the icon it would draw. And then if we don't have a css specified size we reflow at that size and request a decode at that size.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 2

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #0)
> nsImageFrame::ComputeSize calls EnsureIntrinsicSizeAndRatio, there's no
> mImage yet, so it gives the intrinsic size the size of the icon it would
> draw. And then if we don't have a css specified size we reflow at that size
> and request a decode at that size.

Actually, that's not the problem. The useless decode request was coming via OnLoadComplete->NotifyNewCurrentRequest. We would first get OnSizeAvailable called, this would request a reflow because we got a new intrinsic ration. Then OnLoadComplete would be called, and it would (wrongly) do a predictive decode because the intrinsic size hadn't changed. This is a bug in bug 1151359.

The first patch fixed the problem because both things in the previous paragraph happened between the reflow at the initial ("wrong") size, and the reflow at the final size.

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 4

[Timothy Nikkel (:tnikkel)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc96e1114d36

Comment 5

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8706075 [details] [diff] [review]
patch

Mats, do you think you could review? Feel free to decline.

Comment 6

[Mats Palmgren (inactive)]

Comment on attachment 8706075 [details] [diff] [review]
patch

Looks reasonable to me, although I'm only vaguely familiar with this code.

One observation though, most of the code in this method is looks identical
to the code in OnSizeAvailable.
Did you intentionally not update that method?

Comment 7

[Timothy Nikkel (:tnikkel)]

Yeah, the code in OnSizeAvailable is very similar, but slightly different. OnSizeAvailable doesn't have the InvalidateFrame() call so the structure would be slightly different. Since there are a lot of subtleties in this code I wanted to keep this fix focused.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6ae2095c564

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b6ae2095c564

Comment 10

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8706075 [details] [diff] [review]
patch

Review of attachment 8706075 [details] [diff] [review]:
-----------------------------------------------------------------

r+ FWIW.