Closed Bug 1234077 Opened 6 years ago Closed 6 years ago

Gif scrambled when displayed

Categories

(Core :: ImageLib, defect)

43 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox43 --- wontfix
firefox44 + wontfix
firefox45 + wontfix
firefox46 + wontfix
firefox47 --- wontfix
firefox48 --- verified

People

(Reporter: pauljt, Assigned: eflores)

References

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

Attached image c5VETDV.gif
The attached GIF appears scrambled when viewed in release, developer and nightly on Mac. Similar to other commenters, I see something like this:
https://imgur.com/wfIsjOZ

Others see slightly different issues. Rendering looks 'normal' in Chrome but I see a blinking black border in Safari & finder.
Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e276d93dfe4d1018c13d1e310166ace5e011f057&tochange=63c676ad8d86a39e62c39761f7c78b77b66cfc4e

Suspect;
a69d71324577	Edwin Flores — Bug 1223465 - Clamp GIF frame rects to their screen rects - r=seth
Flags: needinfo?(seth)
Flags: needinfo?(edwin)
Keywords: regression
Assignee: nobody → edwin
Flags: needinfo?(seth)
Flags: needinfo?(edwin)
Duplicate of this bug: 1234293
The bug here was that we weren't clearing enough of the stack. We have to persist between DoLzw() calls how much data still needs to be thrown away. Patch is pretty fugly, but it works.

Interestingly, when comparing with Chromium, I noticed that they expand the screen size to accommodate the frame rect, rather than cropping the frame rect down. I *think* our behaviour is correct from reading the GIF89a spec.
Status: NEW → ASSIGNED
Blocks: 1223465
Duplicate of this bug: 1234804
Tracking as it is a regression.
Comment on attachment 8701092 [details] [diff] [review]
1234077-scramble.patch

Removing review request. After making some tests, I found interlaced GIFs are still broken.
Attachment #8701092 - Flags: review?(seth)
Tracked for 44, displaying gifs incorrectly is bad and I hope we can get a fix soon into Beta44.
Duplicate of this bug: 1236138
Attached patch 1234077.patchSplinter Review
Not super happy with this patch -- it adds another buffer copy for animated GIFs -- but it works and Seth reportedly has a much deeper refactor on the way.
Attachment #8701092 - Attachment is obsolete: true
Attachment #8705285 - Flags: review?(tnikkel)
We are in RC mode, it's too late and this is now a wontfix for Fx44.
Comment on attachment 8705285 [details] [diff] [review]
1234077.patch

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

Patch comments inline, on request.

::: image/decoders/nsGIFDecoder2.cpp
@@ +283,5 @@
>      rv = AllocateFrame(mGIFStruct.images_decoded, targetSize,
>                         targetFrameRect, format);
>    }
>  
> +  mLineBuffer = MakeUnique<uint8_t[]>(mGIFStruct.width);

This patch introduces an intermediate buffer for storing the current row of BMP data before passing it on to the output buffer. This makes it much easier to throw out the data that falls outside of the frame rect.

@@ +386,5 @@
> +    // If the image is not interlaced, we're done.
> +    if (!mGIFStruct.interlaced) {
> +      NS_WARNING("GIF2.cpp::OutputRow - too much image data");
> +      return 0;
> +    }

If the image isn't interlaced, and the current row is past the image height, then we are finished decoding.

@@ +391,5 @@
> +
> +    // If the image is interlaced, there is likely more data to come later.
> +    // Quietly skip this row.
> +    IncrementInterlacedRow();
> +    return mGIFStruct.rows_remaining;

On the other hand, if the image *is* interlaced, we probably still have more image data to decode. Ignore this row and return early.

@@ +426,5 @@
>      // Row to process
>      uint8_t* rowp = GetCurrentRowBuffer();
>  
>      // Convert color indices to Cairo pixels
> +    uint8_t* from = mLineBuffer.get() + mGIFStruct.clamped_width;

Previously we used the output buffer to store the GIF palette values, and then mapped those to RGBX in place. Now we map from the temp buffer to the output buffer.

This only happens for the first frame. For animated GIFs we have compositor magic (I think) that does the palette colour mapping automagically.

Since we're no longer using the output buffer to store the palette values...

@@ +466,5 @@
>          }
>        }
>      }
> +  } else {
> +    memcpy(GetCurrentRowBuffer(), mLineBuffer.get(), mGIFStruct.clamped_width);

...we have to copy from the temp buffer here. Unfortunately this will introduce a bit of perf suckage for animated GIFs, but hopefully not too much.

@@ +479,5 @@
>    if (!mGIFStruct.interlaced) {
>      MOZ_ASSERT(!mDeinterlacer);
>      mGIFStruct.irow++;
>    } else {
> +    IncrementInterlacedRow();

Set mGIFStruct.irow to the next row, taking interlacing into account.

@@ +490,5 @@
> +void
> +nsGIFDecoder2::IncrementInterlacedRow()
> +{
> +  static const uint8_t kjump[5] = { 1, 8, 8, 4, 2 };
> +  int currentPass = mGIFStruct.ipass;

This function finds the next row to be decoded in an interlaced image, and sets mGIFStruct.irow. Hoisted from OutputRow() since this is done in multiple places now.

@@ +542,5 @@
>    uint8_t* suffix   = mGIFStruct.suffix;
>    uint8_t* stack    = mGIFStruct.stack;
>    uint8_t* rowp     = mGIFStruct.rowp;
>  
> +  uint8_t* rowend = mLineBuffer.get() + mGIFStruct.width;

Two important changes here -- use the new intermediate buffer for storing decoded data, and decode the entire width of the row before calling OutputRow().

@@ +548,5 @@
>  #define OUTPUT_ROW()                                        \
>    PR_BEGIN_MACRO                                            \
>      if (!OutputRow())                                       \
>        goto END;                                             \
> +    rowp = mLineBuffer.get();                         \

Again, use the temp buffer.

@@ -637,5 @@
>            OUTPUT_ROW();
> -
> -          // Consume decoded data that falls past the end of the clamped width.
> -          stackp -= mGIFStruct.width - mGIFStruct.clamped_width;
> -          stackp = std::max(stackp, stack);

This was the source of the bug. |stack| holds the data we have from the LZW decode above. I originally assumed that |stack| held the rest of the row of image data, but that isn't the case -- there can be a lot of excess image data to throw out between runs of DoLzw().

@@ +1212,5 @@
>  
>        // Clear state from last image
>        mGIFStruct.irow = 0;
>        mGIFStruct.rows_remaining = mGIFStruct.clamped_height;
> +      mGIFStruct.rowp = mLineBuffer.get();

And again, use the temp buffer instead of writing directly to the output buffer.
Attachment #8705276 - Flags: review?(seth)
Attachment #8705285 - Flags: review?(seth)
As it is not ready yet and we already did two releases with this bug, I am going to wontfix it for 45. Please submit an uplift to aurora (46) when it is ready.
Duplicate of this bug: 1250232
Comment on attachment 8705285 [details] [diff] [review]
1234077.patch

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

Let's *try* to avoid using this approach. Bug 1247152 should solve this much more cleanly. (If for some reason it doesn't, we'll revisit the issue.) I don't want to keep hammering away on this old and frankly terrible code.
Attachment #8705285 - Flags: review?(tnikkel)
Attachment #8705285 - Flags: review?(seth)
Depends on: 1247152
Comment on attachment 8705276 [details] [diff] [review]
1234077-tests.patch

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

The comments below apply to both files. r+ once those are fixed. Feel free to check this test in now (marked as failing) and we can enable it once the SurfacePipe stuff lands.

::: image/test/reftest/gif/truncated-framerect-ref.html
@@ +1,1 @@
> +<html>

You're missing the public domain copyright header.

@@ +1,5 @@
> +<html>
> +  <head>
> +    <title>Bug 1234077 - Make sure GIFs still render correctly with a truncated frameRect</title>
> +    <style type="text/css">
> +div, img {

The indentation is all kinds of messed up here. Please fix it so things are properly indented.
Attachment #8705276 - Flags: review?(tnikkel)
Attachment #8705276 - Flags: review?(seth)
Attachment #8705276 - Flags: review+
(In reply to Seth Fowler [:seth] [:s2h] from comment #16)
> Comment on attachment 8705285 [details] [diff] [review]
> 1234077.patch
> 
> Review of attachment 8705285 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Let's *try* to avoid using this approach. Bug 1247152 should solve this much
> more cleanly. (If for some reason it doesn't, we'll revisit the issue.) I
> don't want to keep hammering away on this old and frankly terrible code.

Would be nice if we had a fix to uplift for this bug though.
Backed out for unexpected reftest passes.

Push with issue: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e7b960c2c8e9
If you want to see it on tier 1, see https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d31bea45f96b
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=23694545&repo=mozilla-inbound
19:33:00     INFO -  REFTEST TEST-START | file:///home/worker/workspace/build/tests/reftest/tests/image/test/reftest/gif/truncated-framerect.html
19:33:00     INFO -  REFTEST TEST-LOAD | file:///home/worker/workspace/build/tests/reftest/tests/image/test/reftest/gif/truncated-framerect.html | 580 / 1664 (34%)
19:33:00     INFO -  ++DOMWINDOW == 55 (0x7fa507818800) [pid = 1123] [serial = 1517] [outer = 0x7fa50d8c6400]
19:33:00     INFO -  [1123] WARNING: 'mGIFStruct.pixels_remaining > 0', file /home/worker/workspace/build/src/image/decoders/nsGIFDecoder2.cpp, line 432
19:33:00     INFO -  REFTEST TEST-LOAD | file:///home/worker/workspace/build/tests/reftest/tests/image/test/reftest/gif/truncated-framerect-ref.html | 580 / 1664 (34%)
19:33:00     INFO -  ++DOMWINDOW == 56 (0x7fa508960400) [pid = 1123] [serial = 1518] [outer = 0x7fa50d8c6400]
19:33:00     INFO -  REFTEST TEST-UNEXPECTED-PASS | file:///home/worker/workspace/build/tests/reftest/tests/image/test/reftest/gif/truncated-framerect.html | image comparison (==)
Flags: needinfo?(edwin)
Flags: needinfo?(edwin)
Seeing this issue the past couple of days too.  Here is example of scrambled logo on the realclearpolitics.com website   http://ibin.co/2aL82WG79WiW

ver 48.0a1 20160314030215
(In reply to larrybird from comment #23)
> ver 48.0a1 20160314030215

That is a newer regression, fixed in bug 1255675. It should be in Nightly today or tomorrow.
(In reply to Timothy Nikkel (:tnikkel) from comment #18)
> Would be nice if we had a fix to uplift for this bug though.

It would, but I have zero confidence in the ability of *anyone* to review changes in the old code and be sure they are correct.

I'll let release management make the call here. I'm willing to review Edwin's patch as an Aurora-only patch. Liz, do you think it's worth it?

I absolutely will not request uplift for it to beta.
Flags: needinfo?(lhenry)
Edwin, can we mark this fixed now?
Flags: needinfo?(edwin)
I appreciate the frankness! 
For aurora 47 this is ritu's call. I feel like I can predict the future, but let's ask.
Since this landed in m-c we can mark the bug fixed.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: qe-verify+
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Resolution: --- → FIXED
Summary: Gif scrambled when dispayed → Gif scrambled when displayed
Paul, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks.
Flags: needinfo?(ptheriault)
Hi Seth, Timothy, could you please help me understand the pros and cons of taking this fix in Aurora 47? On the one hand the gif looks terrible in 46 (Beta), it seems we have had this bug for a few releases now so there is no urgency to uplift but we could let it ride the trains. The flip side is if there are many websites which will show messed up gifs, it not an ideal situation to live with. From the commit message in comment 25, I could only make out test changes and not the actual fix.
Flags: needinfo?(tnikkel)
Flags: needinfo?(seth)
Flags: needinfo?(rkothari)
Seth is a better person to comment on that.
Flags: needinfo?(tnikkel)
(In reply to Ritu Kothari (:ritu) from comment #30)
> Hi Seth, Timothy, could you please help me understand the pros and cons of
> taking this fix in Aurora 47? On the one hand the gif looks terrible in 46
> (Beta), it seems we have had this bug for a few releases now so there is no
> urgency to uplift but we could let it ride the trains. The flip side is if
> there are many websites which will show messed up gifs, it not an ideal
> situation to live with. From the commit message in comment 25, I could only
> make out test changes and not the actual fix.

It's honestly hard to guess at the scope of the issue, in terms of how many sites are affected. Affected GIFs are essentially corrupt, but we (and other browsers) try to be permissive in what we accept, so generally we work around this kind of corruption and the GIFs display anyway. "Good" encoders should not produce a GIF like this, so I'd hope that the issue is not common (and I'd expect more uproar on Bugzilla if it was).

I've just looked over the dupes and I don't see any evidence that major sites are affected or anything like that.

As you say, I believe we've had this issue for several releases, as well, so it's not clear to me how much risk we should accept to get it fixed one release sooner.

Note that on Nightly, this was not fixed by the patch in this bug, but by bug 1247152, which in turn depends on bug 1246851. The patches in those bugs make major changes to how GIF decoding works, and while there is a new, massive test suite and we've only seen one minor regression from them (which is already fixed), I'd be wary about uplifting them either. However, if we want to uplift *something* to Aurora, I'd recommend at least considering the alternative of uploading those patches, which at least have had some time on Nightly for problems to shake out and are drastically better tested. (Though they are many, many more lines of code.)
Flags: needinfo?(seth)
(In reply to Ritu Kothari (:ritu) from comment #29)
> Paul, could you please verify this issue is fixed as expected on a latest
> Nightly build? Thanks.

Looks fixed to me on nightly.
Flags: needinfo?(ptheriault)
Flags: needinfo?(edwin)
Thanks Seth. Based on your detailed assessment of the code risks, severity of regression, likelihood of this happening in the wild, I am leaning towards leaving it in Nightly without uplifting to Aurora47.
(In reply to Paul Theriault [:pauljt] from comment #33)
> (In reply to Ritu Kothari (:ritu) from comment #29)
> > Paul, could you please verify this issue is fixed as expected on a latest
> > Nightly build? Thanks.
> 
> Looks fixed to me on nightly.

Thanks Paul. I've updated Fx48 status to fixed.
Comment on attachment 8705285 [details] [diff] [review]
1234077.patch

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

Edwin, I'm just r-'ing this to document the fact that we don't plan on landing it. No offense meant. =)
Attachment #8705285 - Flags: review-
First of all, I verified what platforms were affected by this issue. I used 46.0a1 (2016-01-01)version. As result, 
- Windows 10 x64 
- Ubuntu 14.04 x86 
- Mac OS X 10.11 
are all affected.
 
I also verified the same OSs for the fix, using:
- 48.0a2 (2016-05-05)
- 49.0a1 (2016-05-05)   
and both Fx versions are verified fixed (attachment 8700428 [details] is properly rendered).

Note: The same border as in https://imgur.com/wfIsjOZ (User Story) still appears in both affected and fixed versions, only it is white.
Status: RESOLVED → VERIFIED
OS: Unspecified → All
Hardware: Unspecified → All
Keywords: leave-open
Target Milestone: --- → mozilla48
Version: unspecified → 43 Branch
You need to log in before you can comment on or make changes to this bug.