1274057 - Black or Brown lines or garbage or glitch in GIF playback

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Benoit Girard (:BenWa)]

Attachment: bug.gif (https://googleblog.blogspot.ca/2016/05/allo-duo-apps-messaging-video.html?m=1)

I get brown lines during the playback of this GIF. It seems to be unique to Firefox.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: bug screenshot

Comment 2

[Timothy Nikkel (:tnikkel)]

[Tracking Requested - why for this release]:
regression in playback of animated gifs (is the internet for anything else besides gifs??)

regression range
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5880d1449a79f790e4dbd3fedfe835bd4291d7c6&tochange=cb965dc05ec7d56b64581b6bbdb70357f6435052

bug 1247152

Comment 3

[Marcia Knous [:marcia]]

Tracking this regression for 49/49.

Comment 5

[Timothy Nikkel (:tnikkel)]

More examples from the duped bug

http://forum.shrimprefuge.be/customprofilepics/profilepic155_2.gif
http://forum.shrimprefuge.be/customavatars/avatar427_4.gif

Comment 6

[Seth Fowler [:seth] [:s2h]]

I've been debugging this one for a while and it's apparent that the data output by the GIF decoder itself is fine. I've isolated the bug to DeinterlacingFilter; there appears to be a bug in the handling of the last line of a frame. This is almost certainly an off-by-one error somewhere in the DeinterlacingFilter code. Still tracking it down, but just wanted to give a quick update.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - DeinterlacingFilter shouldn't duplicate rows when advancing from one pass to the next.

This patch fixes the problem, which is a bug in
DeinterlacingFilter::AdvanceRow(). When the next output row would be past the
end of the image, we advance to the next pass. When we do that, we output the
rows we're skipping over at the end of the image to the next filter in the
pipeline. However, the current code also duplicates the first row we're skipping
over all the rows until the end of the image. This is wrong, since it can cause
us to overwrite rows that we've already written (and hence won't be written
again). Because the different write order we use for progressive display avoided
this bug, and we only have reftests for the first frames of images, which always
use progressive display, none of our existing tests caught this.

In this patch, I've simply removed the code that duplicated the rows. This fixes
the bug. Even better, it also improves performance, since we do less work this
way!

Comment 8

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add a regression test.

This patch adds a regression test that would've caught this issue. The
requirements are:

- The number of rows has to be such that we trigger the bug. There are tons of
  possibilities, of course, but in the interest of pragmatism I just used the
  dimensions of one of the testcases in this bug.

- The test is run with progressive display mode *off*, since having it on avoids
  the bug.

Comment 9

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8756686 [details] [diff] [review]
(Part 1) - DeinterlacingFilter shouldn't duplicate rows when advancing from one pass to the next.

Approval Request Comment
[Feature/regressing bug #]: See the blocked bug.
[User impact if declined]: Animated GIFs do not display correctly.
[Describe test coverage new/current, TreeHerder]: A new test is added in part 2.
[Risks and why]: Extremely low risk. Just deletes three lines of code that do unnecessary and harmful stuff.
[String/UUID change made/needed]: None.

Comment 10

[Nicholas Nethercote [inactive]]

Comment on attachment 8756689 [details] [diff] [review]
(Part 2) - Add a regression test.

Review of attachment 8756689 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/test/gtest/TestDeinterlacingFilter.cpp
@@ +214,5 @@
> +        case 2:  // Output row 16.
> +        case 3:  // Output row 24.
> +        case 4:  // Output row 32.
> +        case 5:  // Output row 40.
> +        case 6:  // Output row 48.

I would use |if (0 <= row && row <= 6) {| and likewise for the subsequent conditions, rather than this verbose switch.

Comment 11

[Seth Fowler [:seth] [:s2h]]

Attachment: Thanks for the reviews!

(Part 2) - Add a regression test.

Per our discussion on IRC, I've left in the switch statement to make life easier
when debugging in the future. I've added comments explaining that reasoning.

Comment 12

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add a regression test.

Whoops. I hit the wrong key and 'git bz attach' messed up the attachment. Let's
try that again.

Comment 13

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/095bfc5ca5e7df28e00943a9f1c0cf25998db101
Bug 1274057 (Part 1) - DeinterlacingFilter shouldn't duplicate rows when advancing from one pass to the next. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/6725f1c8401b88e00bdf3a3ba7929bb377dfbe13
Bug 1274057 (Part 2) - Add a regression test. r=njn

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/095bfc5ca5e7
https://hg.mozilla.org/mozilla-central/rev/6725f1c8401b

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8756686 [details] [diff] [review]
(Part 1) - DeinterlacingFilter shouldn't duplicate rows when advancing from one pass to the next.

Visual glitch, taking it.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a58be6883471
https://hg.mozilla.org/releases/mozilla-aurora/rev/18b896da95a7