Closed Bug 1255675 Opened 4 years ago Closed 4 years ago

Downscaled gif files aren't rendered correctly in http://www.dmm.com/netgame/ after bug 1247152 fixed

Categories

(Core :: ImageLib, defect)

48 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- unaffected
firefox48 + fixed

People

(Reporter: yamadat501, Assigned: tnikkel)

References

Details

(Keywords: regression, Whiteboard: gfx-noted)

Attachments

(2 files)

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
Blocks: 1247152
Can you upload a screenshot?
Flags: needinfo?(yamadat501)
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.
Attached image Bug1255675.png
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)
Flags: needinfo?(yamadat501)
[Tracking Requested - why for this release]: Regression
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Version: Trunk → 48 Branch
Duplicate of this bug: 1256123
Attached patch patchSplinter Review
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)
Duplicate of this bug: 1256185
Duplicate of this bug: 1255774
Duplicate of this bug: 1255930
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.
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!
https://hg.mozilla.org/mozilla-central/rev/6a067f060385
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Duplicate of this bug: 1256406
(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.