5.95 KB, patch
Joe Drew (not getting mail): review+
|Details | Diff | Splinter Review|
1.46 KB, patch
Joe Drew (not getting mail): review+
|Details | Diff | Splinter Review|
User Agent: Mozilla/5.0 (Windows NT 5.1; rv:15.0) Gecko/20120623 Firefox/15.0a2 Build ID: 20120623042007 Steps to reproduce: To reset: clear cache for the site (Ctrl+h, search for "dmi.dk", right-click a page from the "DMI" site and choose "Forget About This Site") Goto http://www.dmi.dk/dmi/index/danmark/radar.htm or directly http://www.dmi.dk/dmi/radar-animation640.gif Actual results: The animation might play correctly at first and perhaps even after a reload, but after a shift-reload (also try shift-reload followed by reload) the image will either, - not loop (stop at last frame) - loop once and then be an white image - just show a white image (no animation) Expected results: The animation should loop repeatedly (with a small pause after the last frame).
WFM in current nightly. Might have been a single fluke as well. There is no white frame according to http://gif-explode.com/
Thanks for checking it out. I did not mean to imply that the animation had a white frame in it. Checking the nightly (I had only observed it in Aurora with my normal profile) I do however also see the problem there, so I take the liberty of re-unconfirming for reconsideration. A bisect using mozregression gives (bug 733553 in the range could sound related): Last good nightly: 2012-05-19 First bad nightly: 2012-05-20 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=642d1a36702f&tochange=0e2cc686b07b
Something specific to XP? 2012-06-23 is good on 7
(In reply to andershol from comment #0) > The animation might play correctly at first and perhaps even after a reload, > but after a shift-reload (also try shift-reload followed by reload) the > image will either, > - loop once and then be an white image I'm observing this situation on Win 7 and directly after the 1st load. It works fine in IE9, the animation plays.
Confirmed against Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0 ID:20120624030537 i.e. on Win 7 x64 too. I used CTRL + F5 for hard Reload. The following F5 produces a White Picture.
ah, I have layers.acceleration.disabled set to true If I set it to false, then I am able to reproduce this So if someone can reproduce my observation then I'd say we're pretty close to saying "grrrr, hardware acceleration again"
Mozregression for m-i: last good=2012-05-19 fisrt bad=2012-05-20 http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=175bcd2c7912&tochange=443c0c79e54f So same observation than andershol, suspected bug is: Adam Dane [:hobophobe] — Bug 733553 - Allow multipart image streams to cope with stream changes. r=joe
Another observation: when the animated GIF is stuck at the 1st frame, changing the focus (like switching to another tab or another application) forces the blank white frame to appear.
(In reply to Jesper Hansen from comment #6) > ah, I have layers.acceleration.disabled set to true > If I set it to false, then I am able to reproduce this > > So if someone can reproduce my observation then I'd say we're pretty close > to saying "grrrr, hardware acceleration again" I can reproduce with and without layers.acceleration.disabled, but my laptop is so old, I believe it don't have acceleration in any case ("GPU Accelerated Windows" is blocked under Troubleshooting Information). I don't have a Win7, but I tried a Win8 which worked with a old Aurora (14.0a2 2012-05-11) and showed the bug on a current Aurora (15.0a2 2012-06-24) (again layers.acceleration.disabled made no difference, but this machine is also old and have "GPU Accelerated Windows", as well as "Direct2D Enabled", blocked). But if the bug is timing related, it could be indirectly caused by acceleration. Since it happens in relation to reload, disk speed could be a factor (I have old machines with non-solid-state-disks on a slow internet connection), otherwise I don't know why you don't see the problem. I tried a mozregression on Win8 which gave the same interval, i.e.: Last good nightly: 2012-05-19 First bad nightly: 2012-05-20 (Loic's infomation is more precise, but I had already started the bisect when I saw those results)
5 years ago
Created attachment 636542 [details] [diff] [review] Explicitly check for multipart before cleanup. This was my bad assumption about when |mBytesDecoded == 0| in |AddSourceData|. When we're storing the source data, |mBytesDecoded| alone doesn't reliably signal that we're starting a new part/can clean up from the previous animation. Fix is to check for |mMultipart| there.
Comment on attachment 636542 [details] [diff] [review] Explicitly check for multipart before cleanup. Review of attachment 636542 [details] [diff] [review]: ----------------------------------------------------------------- Can we add a test for this?
Created attachment 642835 [details] [diff] [review] Fix the problem for multipart too. I looked at writing a test for this, and found the previous patch isn't comprehensive enough. I'll add a test soon. The GIFs in question contain trailing garbage. I'm not familiar enough with GIFs to know why they have the garbage, but using |gifsicle --info| says: > gifsicle: While reading 'radar-animation640.gif' frame #7: > gifsicle: warning: trailing garbage after GIF ignored That's only a problem over the network: my initial attempts to recreate locally failed because the data came too fast. So I used |iprelay| to simulate a slow network with a local webserver to recreate this. After the valid data is loaded, the decoder is finished. But Necko doesn't know the extra data is garbage, so |AddSourceData| gets called anyway. The fix is to politely return |NS_OK| when the decoder isn't open for business, as the data is useless to us. Also keeping the |mMultipart| check for the cleanup, because that should be true whenever the cleanup is needed.
(In reply to Adam [:hobophobe] from comment #12) > The GIFs in question contain trailing garbage. Note that the gif is replaced about every 10 minutes, so the garbage might not always be present. If it is a persistent problem, we could try reaching out to the site. The image does however load correctly in e.g. IE. The site is the 3th largest in the country (according to http://fdim.dk/ ), so I assume most browsers handle the images.
Created attachment 643588 [details] [diff] [review] Test slow-loading GIF with trailing garbage Separate patch because I wanted to test without the fix to make sure this failed properly on the bad code. Test adds animated GIF with trailing garbage, then sends the good data from it at once, followed by chunks of the garbage. The timing of the data should mean that this should always properly fail on the bad code.
Created attachment 643590 [details] [diff] [review] Forgot to include the GIF in the patch
Comment on attachment 643590 [details] [diff] [review] Forgot to include the GIF in the patch Review of attachment 643590 [details] [diff] [review]: ----------------------------------------------------------------- Excellent!
https://hg.mozilla.org/mozilla-central/rev/446b788ab99d Backed out because of major image painting problems (or perhaps images aren't loaded fully or something).
For testing, the first build from inbound where the problem was observed was http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-win32/1342830043/ (should be able to substitute macosx64, linux, linux64 for win32 to get other platforms) The full set of changes for that build includes this bug - http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=139a8f2a8538&tochange=045c11dd41a6 The behaviour we were seeing was that only part of an image was being displayed, typically the top was present and the bottom issing. http://www.neowin.net was one site this was observed on.
Should we be at all disturbed that none of our other reftests caught this?
Created attachment 644704 [details] [diff] [review] Use mDecoded to properly detect decoded state Sorry about that. We can lack a decoder (|!mDecoder|) when storing data, as it looks like a worker handles decodes in that case. Normally it's not busy enough for that to be a problem, which is why only image-heavy/generally heavy pages like neowin and yahoo! had the problem. This changes the condition to |mDecoded| which will work for both normal and multipart images (the latter will work because |NewSourceData| will reset it before calling |AddSourceData|). I've tried to create a test for this, using the same sort of slow-loading as the test I added, but haven't successfully created a failing test. I'll try to poke around with that a bit more, maybe add some more weight and see if that bogs it down enough to reproduce in a test scenario?
Try results look good, thanks Ms2ger!
Take two! https://hg.mozilla.org/integration/mozilla-inbound/rev/ebf03b8f1fda https://hg.mozilla.org/integration/mozilla-inbound/rev/c4183e6918aa
Please nominate for uplift if deemed low risk, given this is a new regression in FF15.
Comment on attachment 644704 [details] [diff] [review] Use mDecoded to properly detect decoded state [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 733553 User impact if declined: Certain animated GIFs will stop animating instead of looping Testing completed (on m-c, etc.): On m-c for 10 days or so Risk to taking this patch (and alternatives if risky): Could cause different brokenness with regard to multipart/x-mixed-replace webcams. String or UUID changes made by this patch: none
Comment on attachment 644704 [details] [diff] [review] Use mDecoded to properly detect decoded state Let's get this landed today so it's in the beta this week for more user coverage to shake out any issues. Since this is a regression in 15 it's worth trying to get it in there.
There was an Android Ts regression on mozilla-beta blamed on this check-in: see bug 781917.
(In reply to andershol from comment #0) > User Agent: Mozilla/5.0 (Windows NT 5.1; rv:15.0) Gecko/20120623 > Firefox/15.0a2 > Build ID: 20120623042007 > > Steps to reproduce: > > To reset: clear cache for the site (Ctrl+h, search for "dmi.dk", right-click > a page from the "DMI" site and choose "Forget About This Site") > > Goto http://www.dmi.dk/dmi/index/danmark/radar.htm > or directly http://www.dmi.dk/dmi/radar-animation640.gif > > > > Actual results: > > The animation might play correctly at first and perhaps even after a reload, > but after a shift-reload (also try shift-reload followed by reload) the > image will either, > - not loop (stop at last frame) > - loop once and then be an white image > - just show a white image (no animation) > > > Expected results: > > The animation should loop repeatedly (with a small pause after the last > frame). Able to see the issue on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101 Firefox/15.0b3. Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101 Firefox/15.0b4