1391430 - Animated GIFs render incorrectly on Android

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Arjen de Jager]

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.

Comment 1

[Arjen de Jager]

(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.

Comment 2

[Arjen de Jager]

Attachment: distorted animated GIFs Nexus 7 (2013).png

Comment 3

[Jan Henning [:JanH]]

Caused by the fix for bug 1383499.

Comment 4

[Karl Dubost💡 :karlcow]

I opened Bug 1392150 which might be a duplicate of this one.
Spin off of https://webcompat.com/issues/9126

Comment 7

[Thomas Wisniewski]

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

[Jan Henning [:JanH]]

(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

[Thomas Wisniewski]

Ah, yes, that would have been prudent for my eyes to notice earlier. Thanks!

Comment 10

[Nevin Chen(Not active on Bugzilla)]

Hi Jan
May I know is there anything Fennect team can do here?
I saw you checked fennec?

Comment 11

[Jan Henning [:JanH]]

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.

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

Milan, can you folks take a look?

Comment 13

[Timothy Nikkel (:tnikkel)]

Andrew should have time to look at this when he gets back next week.

Comment 14

[Andrew Osmond [:aosmond] (he/him)]

I was away while this blew up. Investigating now, reproduced on Linux by simply taking away with #ifdef ANDROID in the patch. Very bizarre.

Comment 15

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 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...

Comment 16

[Andrew Osmond [:aosmond] (he/him)]

(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

Comment 17

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0001-Bug-1391430-Image-decoding-should-Use-the-surface-s-.patch, v2

Fix a signed/unsigned int build failure.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6be5c122330ee3d752b598b237167e30120f585

Comment 18

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Force heap allocated surfaces for image decoding to use an unaligned stride., v1

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb089f5f0cf7
Force heap allocated surfaces for image decoding to use an unaligned stride. r=tnikkel

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cb089f5f0cf7

Comment 23

[Daniel Holbert [:dholbert]]

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)

Comment 24

[Andrew Osmond [:aosmond] (he/him)]

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.

Comment 25

[Andrew Osmond [:aosmond] (he/him)]

(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.

Comment 26

[Gerry Chang [:gchang]]

Comment on attachment 8902653 [details] [diff] [review]
Force heap allocated surfaces for image decoding to use an unaligned stride., v1

Fix a regression. Beta56+

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e8a9bb8e42a2