Black or Brown lines or garbage or glitch in GIF playback

RESOLVED FIXED in Firefox 48

Status

()

Core
ImageLib
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: BenWa, Assigned: seth)

Tracking

({correctness, regression})

48 Branch
mozilla49
correctness, regression
Points:
---

Firefox Tracking Flags

(firefox47 unaffected, firefox48+ fixed, firefox49+ fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8754081 [details]
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.
(Reporter)

Comment 1

2 years ago
Created attachment 8754082 [details]
bug screenshot
[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
Blocks: 1247152
status-firefox48: --- → affected
status-firefox49: --- → affected
tracking-firefox48: --- → ?
tracking-firefox49: --- → ?
Flags: needinfo?(seth)
Keywords: correctness, regression
Whiteboard: [gfx-noted]
OS: Unspecified → All
Hardware: Unspecified → All
Tracking this regression for 49/49.
tracking-firefox48: ? → +
tracking-firefox49: ? → +
Duplicate of this bug: 1274402
Summary: Brown lines in GIF playback → Black or Brown lines in GIF playback
status-firefox47: --- → unaffected
More examples from the duped bug

http://forum.shrimprefuge.be/customprofilepics/profilepic155_2.gif
http://forum.shrimprefuge.be/customavatars/avatar427_4.gif
Summary: Black or Brown lines in GIF playback → Black or Brown lines or garbage or glitch in GIF playback
(Assignee)

Comment 6

2 years ago
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.
Flags: needinfo?(seth)
(Assignee)

Comment 7

2 years ago
Created attachment 8756686 [details] [diff] [review]
(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!
Attachment #8756686 - Flags: review?(n.nethercote)
(Assignee)

Comment 8

2 years ago
Created attachment 8756689 [details] [diff] [review]
(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.
Attachment #8756689 - Flags: review?(n.nethercote)
(Assignee)

Comment 9

2 years ago
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.
Attachment #8756686 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

2 years ago
Attachment #8756689 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

2 years ago
Assignee: nobody → seth
Attachment #8756686 - Flags: review?(n.nethercote) → review+
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.
Attachment #8756689 - Flags: review?(n.nethercote) → review+
(Assignee)

Comment 11

2 years ago
Created attachment 8756735 [details] [diff] [review]
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.
Attachment #8756735 - Flags: review?(n.nethercote)
(Assignee)

Updated

2 years ago
Attachment #8756735 - Attachment is obsolete: true
Attachment #8756735 - Flags: review?(n.nethercote)
(Assignee)

Comment 12

2 years ago
Created attachment 8756736 [details] [diff] [review]
(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.
(Assignee)

Updated

2 years ago
Attachment #8756689 - Attachment is obsolete: true
Attachment #8756689 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

2 years ago
Attachment #8756736 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 13

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/095bfc5ca5e7
https://hg.mozilla.org/mozilla-central/rev/6725f1c8401b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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.
Attachment #8756686 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8756736 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 16

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/a58be6883471
https://hg.mozilla.org/releases/mozilla-aurora/rev/18b896da95a7
status-firefox48: affected → fixed
Version: unspecified → 48 Branch
You need to log in before you can comment on or make changes to this bug.