Closed
Bug 1391430
Opened 7 years ago
Closed 7 years ago
Animated GIFs render incorrectly on Android
Categories
(Core :: Graphics: ImageLib, defect)
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
blocking-fx | --- | ? |
fennec | + | --- |
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | fixed |
firefox57 | --- | fixed |
People
(Reporter: connect, Assigned: aosmond)
References
(Blocks 1 open bug, )
Details
(Keywords: nightly-community, regression, Whiteboard: [webcompat])
Attachments
(2 files, 2 obsolete files)
232.06 KB,
image/png
|
Details | |
3.09 KB,
patch
|
tnikkel
:
review+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Step to reproduce: visit http://forums.mozillazine.org/viewtopic.php?p=14761664#p14761664 Actual results: distorted animated GIFs Expected results: undistorted animated GIFs Reproducible on my BlackBerry Priv and Nexus 7 (2013). I'll try to find a regression range as soon as possible.
Reporter | ||
Comment 1•7 years ago
|
||
(In reply to Arjen de Jager from comment #0) > I'll try to find a regression range as soon as possible. Unfortunately I'm unable to test fennec with mozregression-gui yet, see bug 1231950 comment 1. I would appreciate any help to get testing fennec with mozregression-gui working.
Updated•7 years ago
|
Component: ImageLib → General
Product: Core → Firefox for Android
Reporter | ||
Comment 2•7 years ago
|
||
Comment 3•7 years ago
|
||
Caused by the fix for bug 1383499.
Blocks: 1383499
tracking-fennec: --- → ?
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox56:
--- → unaffected
Component: General → ImageLib
Flags: needinfo?(aosmond)
Keywords: regressionwindow-wanted
Product: Firefox for Android → Core
Hardware: ARM → Unspecified
Summary: Animated GIFs render incorrectly → Animated GIFs render incorrectly on Android
Version: Trunk → 57 Branch
Comment 4•7 years ago
|
||
I opened Bug 1392150 which might be a duplicate of this one. Spin off of https://webcompat.com/issues/9126
Updated•7 years ago
|
Comment 7•7 years ago
|
||
I can reproduce with the gif in the webcompat.com issue just linked here. I will try to get a regression range today/tonight.
Comment 8•7 years ago
|
||
(In reply to Thomas Wisniewski from comment #7) > I can reproduce with the gif in the webcompat.com issue just linked here. I > will try to get a regression range today/tonight. No need to - see comment 3.
Comment 9•7 years ago
|
||
Ah, yes, that would have been prudent for my eyes to notice earlier. Thanks!
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/9136
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/9137
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/9144
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/9160
Updated•7 years ago
|
blocking-fx: --- → ?
Flags: webcompat?
See Also: → https://webcompat.com/issues/9114
Whiteboard: [webcompat]
Comment 10•7 years ago
|
||
Hi Jan May I know is there anything Fennect team can do here? I saw you checked fennec?
Flags: needinfo?(jh+bugzilla)
Comment 11•7 years ago
|
||
Probably not directly. My intention was just that this shouldn't drop off from our radar, since in practice this issue is affecting only Fennec.
Flags: needinfo?(jh+bugzilla)
Milan, can you folks take a look?
tracking-fennec: ? → +
Flags: needinfo?(milan)
Comment 13•7 years ago
|
||
Andrew should have time to look at this when he gets back next week.
Flags: needinfo?(milan)
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/9255
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/9252
Updated•7 years ago
|
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/9275
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/9280
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/9301
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/9317
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/9315
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/9346
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/9386
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/9375
Assignee | ||
Comment 14•7 years ago
|
||
I was away while this blew up. Investigating now, reproduced on Linux by simply taking away with #ifdef ANDROID in the patch. Very bizarre.
Assignee | ||
Comment 15•7 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e918b2ea29b5552b9b790d57d0d5895e84d36c07 Turns out we should probably be using the stride everywhere in imagelib, but we aren't and just assume it is width * 4...
Flags: needinfo?(aosmond)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Assignee | ||
Comment 16•7 years ago
|
||
(In reply to Andrew Osmond [:aosmond] from comment #15) > Created attachment 8902410 [details] [diff] [review] > 0001-Bug-1391430-Image-decoding-should-Use-the-surface-s-.patch > > try: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=e918b2ea29b5552b9b790d57d0d5895e84d36c07 > > Turns out we should probably be using the stride everywhere in imagelib, but > we aren't and just assume it is width * 4... Try failed, because the Android version was updated and trychooser was not :). try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ec529a881adb71b307ec6ce6353dbe9ef466d47
Assignee | ||
Comment 17•7 years ago
|
||
Fix a signed/unsigned int build failure. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6be5c122330ee3d752b598b237167e30120f585
Attachment #8902410 -
Attachment is obsolete: true
Assignee | ||
Comment 18•7 years ago
|
||
Due to the try failures, the patch to fix this just keeps growing and growing. For now, I'm just going to fix the allocation. Turns out I missed the existence of Factory::CreateDataSourceSurfaceWithStride which makes life easy: try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=91cce8dc464ec411cdb45629dd7bb65f6a592a9f
Attachment #8902421 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8902653 -
Flags: review?(tnikkel)
Updated•7 years ago
|
Attachment #8902653 -
Flags: review?(tnikkel) → review+
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/9432
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/9476
Comment 20•7 years ago
|
||
Pushed by aosmond@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/cb089f5f0cf7 Force heap allocated surfaces for image decoding to use an unaligned stride. r=tnikkel
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/9498
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/9502
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/9510
Comment 21•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cb089f5f0cf7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/9514
Comment 23•7 years ago
|
||
Looks like there weren't any tests in the patch -- is it possible to land a regression test for this? (maybe a specially-crafted animated GIF with a small number of frames that is sufficient to trigger this, which should compare identically to a static image, which we validate via a reftest)
Flags: needinfo?(aosmond)
Updated•7 years ago
|
status-firefox55:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Assignee | ||
Comment 24•7 years ago
|
||
Comment on attachment 8902653 [details] [diff] [review] Force heap allocated surfaces for image decoding to use an unaligned stride., v1 Approval Request Comment [Feature/Bug causing the regression]: Bug 1383499 [User impact if declined]: None if bug 1383499 is not uplifted as well. Otherwise they will see some/many broken APNGs (although not all). [Is this code covered by automated tests?]: No, we test APNGs which exercise the affected code but none of them have the right dimensions to trigger the bug. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: Yes. Visit http://forums.mozillazine.org/viewtopic.php?p=14761664#p14761664 and ensure the animated similies render correctly. They look distorted on all frames but the first if still broken (see attachment 8898833 [details]). [List of other uplifts needed for the feature/fix]: Bug 1383499 needs to land first. [Is the change risky?]: No. [Why is the change risky/not risky?]: Since we hit the regression, the change is much better understand. I expected to swap the original surface with a similar surface without the file handle issue in the first bug, but I forgot to account for differences in buffer layout. The new surface that this patch uses has the exact same property as the original. It is otherwise a very small and well contained change. [String changes made/needed]: None.
Attachment #8902653 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 25•7 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #23) > Looks like there weren't any tests in the patch -- is it possible to land a > regression test for this? > > (maybe a specially-crafted animated GIF with a small number of frames that > is sufficient to trigger this, which should compare identically to a static > image, which we validate via a reftest) A reftest won't do it, it will need to be a mochitest, but yes. I'll file a new bug about this.
Flags: needinfo?(aosmond)
Comment 26•7 years ago
|
||
Comment on attachment 8902653 [details] [diff] [review] Force heap allocated surfaces for image decoding to use an unaligned stride., v1 Fix a regression. Beta56+
Attachment #8902653 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 27•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/e8a9bb8e42a2
Updated•7 years ago
|
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/9398
You need to log in
before you can comment on or make changes to this bug.
Description
•