646987 - Improved test for bug 641198

Status: NEW

(Core :: Graphics: ImageLib, defect)

Description

[Alon Zakai (:azakai)]

The test in bug 641198 works, but is at risk of causing oranges, and takes 1 second to run. We should write a better test, especially if the test does start to orange (but even if not), see bz's direction in bug 641198 comment 24 for details.

Comment 1

[Alon Zakai (:azakai)]

Looks like we did actually get an orange for that test. However it isn't the orange I was expecting, the image shown is blank (was expecting one of the previous frames to be shown). So my guess is a problem happened and no rendering was done (before we did the comparison). That might be easier to prevent than a more general fix.

Comment 2

[Daniel Holbert [:dholbert]]

For reference -- see the test patch in bug 649440 for an implementation of bz's direction in bug 641198 comment 24, in a different test.

Comment 3

[Alon Zakai (:azakai)]

I'm trying to adapt the methods in bug 649440 to this bug, but I don't see how I can get the image element here. The relevant gif is shown as part of the css style of an <a> element. Am I missing something?

Comment 4

[Daniel Holbert [:dholbert]]

All references to the same image URI should end up resolving to the same RasterImage internally -- so perhaps you could add a <display:none> block with <img> element(s) for the image URIs involved, and hook your listener onto that?

(unless having an additional image consumer like that would keep the bug from reproducing in old builds)

Comment 5

[Alon Zakai (:azakai)]

Hmm, having an additional img element with the same href would change the behavior, sadly (the animation would not reset, since there is no reason for it to reset on the additional img element).

Comment 6

[Boris Zbarsky [:bzbarsky]]

Hmm.  So bug 641198 only shows up with images loaded via nsImageLoader, not with an <img> that has its src set?

Seems like you should still be able to test this via a manual loadImage call on imgILoader, right?

Comment 7

[Joe Drew (not getting mail)]

Are you sure about that, Alon? All the <img> elements should have the same RasterImage backing them, right?

Comment 8

[Alon Zakai (:azakai)]

Sorry for not getting around to this earlier.

It looks like my previous concerns were misplaced - I converted the test to use a normal img element (instead of the image being in the css style of an a element), and the same bug exists without the patch, and is fixed by it. So the test still tests the same thing, when using an img.

However, even with that, I can't find how to make this work. The OnStopFrame and so forth are fired when frames are decoded. But the bug we are concerned here is with frames that are shown *after* the initial decode. In other words, we show the animation once, show another, then show the original again. The original was already decoded, so the frames are just rendered. So OnStartFrame and OnStopFrame are not fired.

I don't think we have a way to listen for frames being drawn, as opposed to decoded - or is there?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Hmm.  MozAfterPaint events might work....

Comment 10

[Joe Drew (not getting mail)]

onStartFrame/onStopFrame/frameChanged doesn't work?

Comment 11

[Alon Zakai (:azakai)]

(In reply to comment #9)
> Hmm.  MozAfterPaint events might work....

I don't quite understand how MozAfterPaint events work. I can't get the test to work without a timeout, and depending on the timeout size, I get different results, which seems to imply that MozAfterPaint happens before the paint was actually shown on the screen.

(In reply to comment #10)
> onStartFrame/onStopFrame/frameChanged doesn't work?

onStartFrame/onStopFrame are for decoding, and in this test, decoding was already done in the past. frameChanged might have worked, but it is [noScript] and gets a C++ parameter (so can't be made scriptable trivially).

Comment 12

[Traci Alvarez]

Attachment: same problem

Thanks from tersmontheoclub1976.