After bug 1247152 was fixed, downscaled gif files in http://www.dmm.com/netgame/ aren't rendered normally with Win64 build. (I'll test with win32 build after my job...) STR 1. set image.downscale-during-decode.enabled true (though it is default) 2. Open http://www.dmm.com/netgame/ and see 人気ランキング Expected result Downscaled gif files are rendered correctly. Actual result Downscaled gif files aren't rendered correctly. By reverting the patches for bug 1247152 or set image.downscale-during-decode.enabled false, images are rendered as expected. My Graphic section in about:support Graphics Adapter Description AMD Radeon HD 7700 Series Adapter Drivers aticfx64 aticfx64 aticfx64 aticfx32 aticfx32 aticfx32 atiumd64 atidxx64 atidxx64 atiumdag atidxx32 atidxx32 atiumdva atiumd6a atitmm64 Adapter RAM 1024 Asynchronous Pan/Zoom wheel input enabled; touch input enabled ClearType Parameters Gamma: 2200 Pixel Structure: R ClearType Level: 100 Enhanced Contrast: 200 Device ID 0x683d Direct2D Enabled true DirectWrite Enabled true (6.2.9200.17568) Driver Date 2-26-2016 Driver Version 15.301.1901.0 GPU #2 Active false GPU Accelerated Windows 1/1 Direct3D 11 (OMTC) Subsys ID 00000000 Supports Hardware H264 Decoding Yes Vendor ID 0x1002 WebGL Renderer Google Inc. -- ANGLE (AMD Radeon HD 7700 Series Direct3D11 vs_5_0 ps_5_0) windowLayerManagerRemote true AzureCanvasAccelerated 0 AzureCanvasBackend direct2d 1.1 AzureContentBackend direct2d 1.1 AzureFallbackCanvasBackend cairo
Can you upload a screenshot?
Same problem here. mozregression says >INFO: Looks like the following bug has the changes which introduced the regression: >https://bugzilla.mozilla.org/show_bug.cgi?id=1247152 See http://www.123gifs.eu/spring/ for example. Do right-click the broken image and select 'View Image', and click(enlarge) the image, you can see normal image. Click(shrink) the image, you can see broken image again.
It's a screenshot with my PC. Broken graphics are all gif and a normal one is jpeg. And all gifs are rendered normally when their size becomes same as original by zooming the page. As a note, same problem has appeared once by enabling downscale during decode about half a year ago (It's Bug 1208696 fixed in Bug 1207378)
[Tracking Requested - why for this release]: Regression
The framerect is getting passed wrongly for the SurfacePipe. Questions: why didn't any of our tests catch this? I suspect it's because the test pages that test downscaling gifs only have a single image on the page, and it gets turned into an image layer, thus bypassing the downscaling code. SurfaceSink::Configure says that "Non-paletted surfaces should not have frame rects" which is true now because we only use SurfacePipe for gif and icon. But once we use it for PNGs I think that will be false.
Assignee: nobody → tnikkel
Attachment #8730040 - Flags: review?(seth)
Is Bug 1255958 a duplicate of this bug?
(In reply to Eiichi from comment #10) > Is Bug 1255958 a duplicate of this bug? Probably, Yes. The patch above fixes the problem, too.
Duplicate of this bug: 1255958
I'm taking a look at this now.
Oh wait, I see Tim has a patch already. That's what I get for immediately scrolling to the bottom of the bug. =)
(In reply to Timothy Nikkel (:tnikkel) from comment #6) > SurfaceSink::Configure says that "Non-paletted surfaces should not have > frame rects" which is true now because we only use SurfacePipe for gif and > icon. But once we use it for PNGs I think that will be false. Nah, it'll always be true, because in the case of non-paletted surfaces, we remove the frame rect via RemoveFrameRectFilter, earlier in the pipeline. If we didn't have that, it'd already be an issue for GIF. We'll be killing frame rect support totally before long, but we can't as long as we're using the old animation code.
Comment on attachment 8730040 [details] [diff] [review] patch Review of attachment 8730040 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. This is definitely what we should've been doing.
Attachment #8730040 - Flags: review?(seth) → review+
I'm going to go ahead and push this patch so we can get this fixed ASAP.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a067f06038551a22b29b470aa365f7c5b8bd3a3 Bug 1255675. SurfacePipe handles removing the frame rect, so pass the frame rect directly.
Thanks for the fix, Tim!
(In reply to Timothy Nikkel (:tnikkel) from comment #6) > Questions: why didn't any of our tests catch this? I suspect it's because > the test pages that test downscaling gifs only have a single image on the > page, and it gets turned into an image layer, thus bypassing the downscaling > code. Turns out there of the handful of tests that run with the downscale during decode pref enabled. They live in image/test/reftest/downscaling and none of them use gifs. So our testing coverage of our downscale during decode code is very low :(.
I wrote some basic reftests to test the downscale code for every image format. I used these try pushes to calibrate how much fuzz to add (and verify that they do indeed catch this bug). https://treeherder.mozilla.org/#/jobs?repo=try&revision=d480a6cc7f2f https://treeherder.mozilla.org/#/jobs?repo=try&revision=1fc6285407eb https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ca6adf11b41
(In reply to Timothy Nikkel (:tnikkel) from comment #23) > Turns out there of the handful of tests that run with the downscale during > decode pref enabled. They live in image/test/reftest/downscaling and none of > them use gifs. So our testing coverage of our downscale during decode code > is very low :(. That's actually not exactly true; for the new SurfacePipe-based code, we have a *titantic* number of tests in image/test/gtest/TestDownscalingFilter.cpp. We should have total coverage of that code. All decoders will move over to using that code path soon. However, what we lack is integration tests. The bug here obviously was not in the downscaling code at all, but in the small amount of glue that integrates it with the GIF decoder. For that we do need reftests, such as the ones you've added above. Thanks for tackling that. I'd eventually like to experiment with turning DDD on globally for reftests, which would obviously increase our coverage *a lot*. However, as I'm sure you know, there are some challenges there.
You need to log in before you can comment on or make changes to this bug.