1222711 - Some pictures flicker on Wikipedia when enlarged.

Status: NEW

(Core :: DOM: Core & HTML, defect, P5)

Description

[Gary [:streetwolf52]]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20151107013031

Steps to reproduce:

1. Clear cache and restart
2. Go to https://en.wikipedia.org/wiki/L'Umbracle
3. Click on the picture on the right.



Actual results:

When opening a large image, the image flickers once or twice.


Expected results:

Image should not flicker.

Comment 1

[Gary [:streetwolf52]]

This problem goes back at least to Fx44.

Comment 2

[B.J. Herbison]

Not Windows specific, also observed on Ubuntu.

Comment 3

[Mayank Bansal]

Yes, the image flickers

Comment 4

[u279076]

mozilla-central
===============
Last good revision: cac6192956ab (2015-01-16)
First bad revision: 369a8f14ccf8 (2015-01-17)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cac6192956ab&tochange=369a8f14ccf8

This is a regression going back to Firefox 38. Unfortunately I am unable to bisect this further as mozregression is unable to find builds that far back. That said I suspect either bug 985193 or bug 1079627 to be the cause.

Comment 5

[Gary [:streetwolf52]]

Just wanted to point out that the flashing only occurs when you use Wikipedia's Media Player. Disabling the Media Player reverts things back to the way they were before the Media Player was introduced which I think was only a year or so ago.  Without Media Player there is no flashing of pictures.  I think Media Player is the default.

Comment 6

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

My initial guess is that this is caused by the |srcset| attribute which Wikipedia is using. When you click on the image, we are loading the higher res version - i.e., we are downloading and decoding an entirely different image.

Can you confirm that if you remove the |srcset| attribute in the inspector, so only the |src| attribute remains, the flashing goes away? It seems to work for me.

Obviously this is not the behavior we want, even if |srcset| is the cause. =) Just want to make sure we know where the issue is coming from.

Comment 7

[Gary [:streetwolf52]]

What exactly do I do?  Some steps would be appreciated Seth.

Comment 8

[Gary [:streetwolf52]]

I think I got it right.  I get to the thumbnail picture of the URL I gave and then in Inspector I click on the image and I see the code where the srcset is. I edit the HTML and delete srcset and everything after it.

When I click on the thumbnail again the picture appears with no flashing.  However, if I zoom in on the image and then go back using the back arrow (or my mouse button) in the Navigator bar to the previous size I get the flickering.

Comment 9

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

(In reply to Gary [:streetwolf] from comment #8)
> When I click on the thumbnail again the picture appears with no flashing. 
> However, if I zoom in on the image and then go back using the back arrow (or
> my mouse button) in the Navigator bar to the previous size I get the
> flickering.

So when you do that the |srcset| attribute gets restored, because the changes you make in devtools don't get saved across page loads.

I think the problem is the |srcset| attribute here; what I suspect is happening is that when we redraw the image at a different size, we change which image we're selecting from the |srcset| attribute. This requires a new load, and since <img> elements get drawn progressively, you see flashing because there is a split second where we have only partially loaded/decoded the image, and the rest of the area is drawn black.

John, does this hypothesis sound believable to you?

Comment 10

[John Schoenick [:johns]]

It does sound plausible. AFAIK our image loading code is not quite matching the spec when it comes to new loads -- in some cases, such as changing srcset sources, we're supposed to keep displaying the old image until the new image is ready, but I believe that that doesn't happen in some/many/all cases.

(In that case this would be entirely a DOM bug)

Comment 11

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

Thanks John. I'm going to move this to DOM for now. I'd needinfo someone, but I'm honestly not sure who is the best person to look at this right now. (It may well be that it's me! But unfortunately I don't know the DOM side of things very well, so I'm hoping someone more experienced can pick this up.)

Comment 12

[Boris Zbarsky [:bzbarsky]]

We do have infrastructure to continue showing the old thing until the new one is ready.  That's the whole mCurrentRequest/mPendingRequest stuff in nsImageLoadingContent.  At first glance srcset should play along fine with this.

The one caveat is when, precisely, we call MakePendingRequestCurrent.  Looks like we do it sync under LoadImage if we got an image cache hit and the status is imgIRequest::STATUS_LOAD_COMPLETE; otherwise we wait until we get the OnLoadComplete notification from imagelib.  I suspect the issue may be that nowadays OnLoadComplete fires before decoding completes, so we get the flicker until the decode finishes.  Does this suspicion have any basis in reality?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Ah, per docs in http://mxr.mozilla.org/mozilla-central/source/image/imgINotificationObserver.idl my suspicion totally has a basis in reality.

So right now, we swap the request in OnLoadComplete, then notify the frame, which looks like it will kick off the decode either immediately (if its intrinsic size hasn't changed) or after layout (if it has).

So what behavior do we want here?  We can have the flicker we have now.  We can have rescaling of the old image to the intrinsic size of the new one while we decode, then the decoded image showing up once decoding is done.  What else can we do?

Comment 14

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

It's a good question. I'll think on this.

Comment 15

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

(In reply to Boris Zbarsky [:bz] from comment #13)
> We can have rescaling of the old image to the intrinsic size of the new one
> while we decode, then the decoded image showing up once decoding is done.

Actually, to clarify, this is what I think we should do, but the problem is that it's a bit awkward to implement as things stand. This code is just kinda hairy and things are intertwined in an ugly way.

The most straightforward thing to do is this (some details are ignored here, but this is the general idea) - let's call it option A:

(1) When we change the requests in OnLoadComplete, if the change was due to a |srcset| resolution change, we store the previous request in a new mPreviousRequest member.

(2) When the frame gets painted, we draw the previous request (scaled if need be) and draw the new request on top of it. If drawing the new request results in DrawResult::SUCCESS, we clear mPreviousRequest and from that point on we don't draw it anymore.

The effect of this will be that the new image materializes over top of the old image. If the two images have the same content, just at different resolutions (which should be the case in any sane use of srcset), then this should have the same effect visually as a progressive JPEG decode, where the image is drawn immediately but gets progressively clearer. If the two images have different content, there will be flashing as before, but now the area of the new image that's not decoded will contain the previous image instead of just being blank.

This is the simplest implementation I can think of, and I think the results will be quite nice for all sane uses of srcset, though I will note that I think this technically does not follow the letter of the spec.

The easiest way to implement this while following the letter of the spec - let's call it option B - would be to draw in the opposite order - first the new image, then if we don't get DrawResult::SUCCESS, the old image on top of it. That will work, although it seems a bit of a shame to me as we could display the image at least partially in a higher resolution earlier.

What we unfortunately cannot do (or at least, should not do) is wait for a DECODE_COMPLETE notification; this won't be reliable, because a variety of circumstances may cause the image to not be available by the time we paint, even though we received the notification. I have plans for a mechanism that will replace DECODE_COMPLETE notifications and make this more reliable, but it's not there yet. =\

I'll assign this bug to myself, but I do not have time to work on it right away. If anyone feels like stealing it, feel free. Also, if there are any comments on the options above, please let me know. I feel like option A is a better user experience, and I'm quite tempted to go that route, but if anyone feels that option B is preferable I can be easily swayed. =)

Comment 16

[Gary [:streetwolf52]]

I'm also seeing what I believe is this bug when viewing the photos in the slideshow:

http://www.cnn.com/2015/11/09/arts/listen-to-me-marlon-documentary-film/index.html

Comment 17

[Gary [:streetwolf52]]

AnNy update on this?

Comment 18

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

(In reply to Gary [:streetwolf] from comment #17)
> AnNy update on this?

Yeah, though the action is happening in a different bug. I believe the patch in bug 1253995 will either solve this problem, or can easily be extended to solve this problem.

I'll make bug 1253995 block this bug.

Comment 19

[Gary [:streetwolf52]]

Thanks Seth.

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.