Closed Bug 1391430 Opened 3 years ago Closed 3 years ago

Animated GIFs render incorrectly on Android

Categories

(Core :: ImageLib, defect, major)

57 Branch
Unspecified
Android
defect
Not set
major

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)

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.
(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.
Component: ImageLib → General
Product: Core → Firefox for Android
Caused by the fix for bug 1383499.
Blocks: 1383499
tracking-fennec: --- → ?
Has Regression Range: --- → yes
Has STR: --- → yes
Component: General → ImageLib
Flags: needinfo?(aosmond)
Product: Firefox for Android → Core
Hardware: ARM → Unspecified
Summary: Animated GIFs render incorrectly → Animated GIFs render incorrectly on Android
Version: Trunk → 57 Branch
I opened Bug 1392150 which might be a duplicate of this one.
Spin off of https://webcompat.com/issues/9126
Duplicate of this bug: 1392150
Duplicate of this bug: 1392364
I can reproduce with the gif in the webcompat.com issue just linked here. I will try to get a regression range today/tonight.
(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.
Ah, yes, that would have been prudent for my eyes to notice earlier. Thanks!
blocking-fx: --- → ?
Flags: webcompat?
Whiteboard: [webcompat]
Hi Jan
May I know is there anything Fennect team can do here?
I saw you checked fennec?
Flags: needinfo?(jh+bugzilla)
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)
Andrew should have time to look at this when he gets back next week.
Flags: needinfo?(milan)
I was away while this blew up. Investigating now, reproduced on Linux by simply taking away with #ifdef ANDROID in the patch. Very bizarre.
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: nobody → aosmond
Status: NEW → ASSIGNED
(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
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
Attachment #8902653 - Flags: review?(tnikkel)
Duplicate of this bug: 1395288
Attachment #8902653 - Flags: review?(tnikkel) → review+
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
https://hg.mozilla.org/mozilla-central/rev/cb089f5f0cf7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Duplicate of this bug: 1396033
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)
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?
(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)
Blocks: 1398222
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+
You need to log in before you can comment on or make changes to this bug.