Closed
Bug 1274057
Opened 8 years ago
Closed 8 years ago
Black or Brown lines or garbage or glitch in GIF playback
Categories
(Core :: Graphics: ImageLib, defect)
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox47 | --- | unaffected |
firefox48 | + | fixed |
firefox49 | + | fixed |
People
(Reporter: BenWa, Assigned: seth)
References
Details
(Keywords: correctness, regression, Whiteboard: [gfx-noted])
Attachments
(4 files, 2 obsolete files)
3.53 MB,
image/gif
|
Details | |
17.57 KB,
image/png
|
Details | |
1.16 KB,
patch
|
n.nethercote
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
6.52 KB,
patch
|
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
I get brown lines during the playback of this GIF. It seems to be unique to Firefox.
Reporter | ||
Comment 1•8 years ago
|
||
Comment 2•8 years ago
|
||
[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
Updated•8 years ago
|
Summary: Brown lines in GIF playback → Black or Brown lines in GIF playback
Updated•8 years ago
|
status-firefox47:
--- → unaffected
Comment 5•8 years ago
|
||
More examples from the duped bug http://forum.shrimprefuge.be/customprofilepics/profilepic155_2.gif http://forum.shrimprefuge.be/customavatars/avatar427_4.gif
Updated•8 years ago
|
Summary: Black or Brown lines in GIF playback → Black or Brown lines or garbage or glitch in GIF playback
Assignee | ||
Comment 6•8 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•8 years ago
|
||
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•8 years ago
|
||
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•8 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•8 years ago
|
Attachment #8756689 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → seth
Updated•8 years ago
|
Attachment #8756686 -
Flags: review?(n.nethercote) → review+
Comment 10•8 years ago
|
||
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•8 years ago
|
||
(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•8 years ago
|
Attachment #8756735 -
Attachment is obsolete: true
Attachment #8756735 -
Flags: review?(n.nethercote)
Assignee | ||
Comment 12•8 years ago
|
||
Whoops. I hit the wrong key and 'git bz attach' messed up the attachment. Let's try that again.
Assignee | ||
Updated•8 years ago
|
Attachment #8756689 -
Attachment is obsolete: true
Attachment #8756689 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•8 years ago
|
Attachment #8756736 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 13•8 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/095bfc5ca5e7 https://hg.mozilla.org/mozilla-central/rev/6725f1c8401b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 15•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8756736 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 16•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/a58be6883471 https://hg.mozilla.org/releases/mozilla-aurora/rev/18b896da95a7
Updated•8 years ago
|
Version: unspecified → 48 Branch
You need to log in
before you can comment on or make changes to this bug.
Description
•