Closed Bug 1255062 Opened 8 years ago Closed 8 years ago

Swiffy-based banner ads display graphical glitches on Mac OS X

Categories

(Core :: Graphics, defect)

44 Branch
Unspecified
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla49
Tracking Status
firefox45 --- wontfix
firefox46 + wontfix
firefox47 + wontfix
firefox48 + verified
firefox49 + verified
firefox-esr38 --- unaffected
firefox-esr45 --- affected

People

(Reporter: jeremy.brack, Assigned: ethlin)

Details

(Keywords: regression, Whiteboard: gfx-noted)

Attachments

(8 files, 4 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2486.0 Safari/537.36 Edge/13.10586

Steps to reproduce:

Opened a Swiffy banner ad on Firefox 44 and 45 on Mac 10.10.5 and El Capitan.


Actual results:

During the play back of the banner ad, there are glaring graphical glitches.
For example, in this banner there is a large bitmap sky which scrolls down. In Firefox, the sky gets cut off at the top. In the original Flash file, there is a blue gradient layer above the bitmap which smoothly blends from a solid blue into the top of the cloud and sky photo. Instead of the gradient blend, we see a harsh solid rectangle cutting off the sky photo.


Expected results:

In all PC versions of Firefox on all versions of Windows, the banner plays perfectly and all of the layers and gradients work perfectly. Also, this banner looks perfect on all versions of Chrome, Edge, IE, and Safari.
Did you test with HWA disabled in Firefox?
https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles


In addition, could you attach a screenshot of the graphics glitches.
Flags: needinfo?(jeremy.brack)
Thank you for your prompt reply!

I couldn't find anywhere in the options to disable HWA. What is HWA?

But, we tested it on 4 different mac computers here (desktops and laptops), and then we tested it on Browserstack with 2 versions of OSX. None of our mac versions of Firefox had any extensions or extra plugins installed. Just the stock versions of Firefox 44 and 45.

I am attaching a screenshot showing the Firefox 44/45 OSX version with the bug glitches vs the PC and all other browser version which shows no bugs or glitches.

Thanks,
Jeremy

(In reply to Loic from comment #1)
> Did you test with HWA disabled in Firefox?
> https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-
> firefox-profiles
> 
> 
> In addition, could you attach a screenshot of the graphics glitches.
Flags: needinfo?(jeremy.brack)
Attached image firefox_45_osx_bug.jpg
We disabled the HWA in 44 and 45 for Firefox in OSX and restarted Firefox for both versions after disabling HWA. The bug glitch is still showing after disabling HWA.
Ok, so HWA doesn't seem to be the culprit. Do you remember if the rendering of this banner was fine with some old versions of Firefox? (before FF44 like 40 or 41)

Maybe there is a regression since FF44.
Yes, we just tested it again now in FF43 on OSX and the banner works perfectly with no glitches. 
So this bug started in OSX FF44 and is still present in OSX FF45.
Ok, so there is probably a regression since FF44. I will have some testing for you to narrow down the issue.

1) Test the Nightly build, maybe the issue is already fixed in the next versions: https://nightly.mozilla.org/

2) To find a regression range, could you install the tool Mozregression.
See http://mozilla.github.io/mozregression/ for the FAQ (you need to install python 2.7 because mozreg is a python 2.7 package).
After that, run the command "mozregression --good=43" then paste here the final pushlog provided in the console output.
Flags: needinfo?(jeremy.brack)
OS: Unspecified → Mac OS X
Status: UNCONFIRMED → NEW
Component: Untriaged → Graphics
Ever confirmed: true
Product: Firefox → Core
Reproduce it on Mac OS X 10.10 with FF Nighty 48.0a1

This are the results from running mozregression:

Last good revision: 001942e4617b2324bfa6cdfb1155581cbc3f0cc4
First bad revision: a515a700781a024c05c04763a19995bb284807d8
Pushlog:
https://hg.mozilla.org/integration/b2g-inbound/pushloghtml?fromchange=001942e4617b2324bfa6cdfb1155581cbc3f0cc4&tochange=a515a700781a024c05c04763a19995bb284807d8
A b2g-inbound pushlog doesn't look very helpful here; can you get a mozilla-inbound regression window?
Flags: needinfo?(ovidiu.boca)
Hi, this is everything that I can discover after this mozregression doesn't want to work. Please tell me if this is enough?


Last good revision: bf2bc1aa78c0b72d9b6b13f7a8c6ae61c60a51dc (2015-09-24)
First bad revision: e5effeb8e57ceddf679f7784faab0c2cebb1e1e6 (2015-09-25)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bf2bc1aa78c0b72d9b6b13f7a8c6ae61c60a51dc&tochange=e5effeb8e57ceddf679f7784faab0c2cebb1e1e6



Last good revision: bf2bc1aa78c0b72d9b6b13f7a8c6ae61c60a51dc
First bad revision: 001942e4617b2324bfa6cdfb1155581cbc3f0cc4
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bf2bc1aa78c0b72d9b6b13f7a8c6ae61c60a51dc&tochange=001942e4617b2324bfa6cdfb1155581cbc3f0cc4
Flags: needinfo?(ovidiu.boca)
From a brief look at that pushlog, I wonder if SkiaGL (enabled in bug 1150944) is the most likely culprit?
Flags: needinfo?(matt.woodrow)
Can you try going to about:Config, set the preference "gfx.canvas.azure.accelerated" to false, restart firefox and see if you still see the same problem? Thanks!
Flags: needinfo?(ovidiu.boca)
Hi Manson, 

I retested again on Mac 10.10 Nightly 48.0a1 having the preference "gfx.canvas.azure.accelerated" to false and I can't reproduce it. I compared with Chrome and on Nightly the graphic is not smoothly.
Flags: needinfo?(ovidiu.boca)
Sounds like it is SkiaGL then.

Are you going to take a look at this Mason?

cc'ing Lee as well, who has been looking at a lot of Skia issues recently.
Flags: needinfo?(matt.woodrow)
This is definitely a SkiaGL issue. The problem is the testcase provided here is only really data for the Swiffy runtime. The Swiffy runtime is huge, minified, and not open source, so unfortunately there's no friendly way to debug it. I tried pumping the Swiffy runtime through a code beautifier, but the results weren't very usable in terms of seeing where the canvas issue is happening.

I don't have any good guesses offhand of where the problem might be occurring. Possibly in copying to the canvas from either images or itself, depending on how its implementing the animation.

Ideally, it would be nice to have a non-Swiffy canvas test-case that exemplifies the same issue so we could do a reasonable investigation of it.
Keywords: testcase-wanted
[Tracking Requested - why for this release]:
This is a regression however it's been around for a couple of releases now and is specific to a certain category of ads. I don't know if this has real world implications on other web content.

Lee or Mason, while this isn't a regression from bug 1207332 I wonder if it should be tracked against it?

Jeremy, any chance you can provide a minimized testcase? We're not likely to fix this issue without one.
Summary: Firefox 44 and 45 on Mac OSX Yosemite 10.10.5 and on the latest El Capitan are both showing huge graphical glitches in banner advertisements built using Swiffy. These glitches do not show up on any Firefox version on PC of any version of Windows. → Swiffy-based banner ads display graphical glitches on Mac OS X
Version: 45 Branch → 44 Branch
Flags: needinfo?(howareyou322)
I will work on it.
Flags: needinfo?(howareyou322)
Assignee: nobody → ethlin
Status: NEW → ASSIGNED
Too late to fix on 46, but I'll track it for 49 and we can think  about possibly uplifting a fix once we have a verified fix.
Flags: qe-verify+
The test case attachment 8728486 [details] is too complicate. I will try to narrow down the test to find the possible canvas api problem.
Lee, can you also make sure Skia people are pointed to this bug?

In the meantime, we're probably only a few days away from wontfix-ing this for 47.
Flags: needinfo?(lsalzman)
Attached file simple test
This is a simple test to reproduce the problem. The problem occurs after doing transform and clip. In this test, the result will be correct if I mark "ctx.moveTo(0,0)" or mark "ctx.moveTo(0,-1)" or change "ctx.transform(5,0,0,5,0,0)" to "ctx.transform(3,0,0,3,0,0)".
Once we have a fix, let's turn the example from comment 22 into an automated test?
Keywords: testcase-wanted
In the test case there are 2 move points after beginPath. The first move point should be meaningless. From skia code [1], when checking 'isRectContour', it will use the first point of the path as the close point, but that should be wrong when there are 2 more move points. So my patch makes sure there is only one move point at the beginning of the path for skia.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/skia/skia/src/core/SkPath.cpp#447
Attachment #8756164 - Flags: review?(mchang)
(In reply to Milan Sreckovic [:milan] from comment #23)
> Once we have a fix, let's turn the example from comment 22 into an automated
> test?

Okay, I will write a test case for it in the bug.
Attached patch Part2. Add reftest for clip. (obsolete) — Splinter Review
Add a reftest. For this test, Chrome also has the same problem.
Attachment #8756191 - Flags: review?(mchang)
Attachment #8756191 - Flags: review?(mchang) → review+
Comment on attachment 8756164 [details] [diff] [review]
Remove redundant move point in skia

From comment 27, I'll let Lee decide on this then.
Attachment #8756164 - Flags: review?(mchang) → review?(lsalzman)
(In reply to Lee Salzman [:lsalzman] from comment #27)
> ...a minimized test case for this...it might be
> better if rather than just checking for an initial point, we try to see if
> we can compact any sequence of move-tos...

If we can cook up a test case for a more generic scenario of duplicated move-tos and have things fail in all those situations, it'll be much easier to come up with a general solution.

I do like the idea of landing the solution for this simple case, assuming it passes a review, to deal with this tracked bug.  Then sort things out before we ask for the Skia uplift.
(In reply to Ethan Lin[:ethlin] from comment #24)
> Created attachment 8756164 [details] [diff] [review]
> Remove redundant move point in skia
> 
> In the test case there are 2 move points after beginPath. The first move
> point should be meaningless. From skia code [1], when checking
> 'isRectContour', it will use the first point of the path as the close point,
> but that should be wrong when there are 2 more move points. So my patch
> makes sure there is only one move point at the beginning of the path for
> skia.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/gfx/skia/skia/src/core/SkPath.
> cpp#447

I am rather excited you managed to hunt down a minimized test case for this, but at the same time I am not sure this is the right way to fix this. This fix is mostly only useful to solve this one case. For instance, it might be better if rather than just checking for an initial point, we try to see if we can compact any sequence of move-tos, which at least mirrors exactly how Cairo handles move-to.

If this, on the other hand, turns out to be the best fix we can manage without altering Skia's interface, then I guess it would be acceptable.

Another possibility is to check if there is rather a bug in Skia that needs to be resolved, so that we can submit an upstream fix while doing a local spot fix until the next Skia update. Should the problem like in Skia itself, this would be my recommendation.

I am still digging into this a bit to see what would be the best course of action.
Flags: needinfo?(lsalzman)
After more study, I found some more details in SkPath. The rect will be computed by [1] and [2]. For [2], it takes all points to compute without checking the type. Maybe we should just compact any sequence of move-tos as comment 27 said.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/skia/skia/src/core/SkPath.cpp#566
[2] https://dxr.mozilla.org/mozilla-central/source/gfx/skia/skia/include/core/SkPathRef.h#319
Clear the needinfo since Ethan is working on this with a simple test case.
Flags: needinfo?(jeremy.brack)
A GrConvexPolyEffect is created to represent the clipped polygon of the path under the SkiaGL path.

GrConvexPolyEffect::Create checks if the path is convex and only contains LineTo verbs, but the convexity check filters out degenerate contours (as might be created by redundant MoveTos) when it does its magic.

However, GrConvexPolyEffect::Create builds its edge list from the raw points in the path that have not filtered out the degenerate verbs/points yet, which is why we get this bug.

This patch uses the same iterator that the convexity check does to filter out these degenerate contours when building the edge list, thus making the SkiaGL backend happy again.
Attachment #8756164 - Attachment is obsolete: true
Attachment #8756164 - Flags: review?(lsalzman)
Attachment #8756921 - Flags: review?(mchang)
Comment on attachment 8756921 [details] [diff] [review]
allow Skia GrConvexPolyEffect to handle degenerate contours

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

::: gfx/skia/skia/src/gpu/effects/GrConvexPolyEffect.cpp
@@ +274,3 @@
>      int n = 0;
> +    while ((verb = iter.next(pts, true, true)) != SkPath::kDone_Verb) {
> +        switch (verb) {

nit: Add a comment here saying why we filter out everything but line verbs.
Attachment #8756921 - Flags: review?(mchang) → review+
Upstream Skia bug report and patch: https://codereview.chromium.org/2018613003/
After applying attachment 8756921 [details] [diff] [review], there is another minor problem in https://jsfiddle.net/6b6rz3wm/9/.
The result was different when set canvas backend as cairo and skia.

If you uncomment the closePath, then the result was correct.
(In reply to Peter Chang[:pchang] from comment #35)
> After applying attachment 8756921 [details] [diff] [review], there is
> another minor problem in https://jsfiddle.net/6b6rz3wm/9/.
> The result was different when set canvas backend as cairo and skia.
> 
> If you uncomment the closePath, then the result was correct.

Can you describe in more detail what the difference is? They both seem to be drawing similar-ish results, plus-or-minus some wiggle room.
Attached image cairo_vs_skia.png
Attached the result between cairo(same as safari) and skia. You can find the path in the right side(skia) looked like bold.
OK, the test case in comment 35 call into stroke(), but didn't call the FillRect() which invoke GrConvexPolyEffect later. I think this is another issue and we don't have to address this in this bug.
Attached file simple test2
This is the test case in attachment 8756191 [details] [diff] [review]. The result is still wrong after :lsalzman's fix in attachment 8756921 [details] [diff] [review].
I think there are 2 clip problems in skia. One is in GrConvexPolyEffect like :lsalzman's fix. Another one is 'isRect' logic I described in comment 30. Removing redundant 'moveTo' can fix both problems.

The simple test (attachment 8755506 [details]) corresponds to GrConvexPolyEffect problem.
The simple test2 (attachment 8757264 [details]) corresponds to 'isRect' logic problem.

The swiffy html (attachment 8728486 [details]) should be GrConvexPolyEffect problem. Maybe we can land the GrConvexPolyEffect fix and another test case for GrConvexPolyEffect problem in this bug. And open another tracking bug for 'isRect' problem with the test case 'attachment 8756191 [details] [diff] [review]'. :lsalzman, do you think it's good?
Flags: needinfo?(lsalzman)
Attached patch test case for GrConvexPolyEffect (obsolete) — Splinter Review
The test case for GrConvexPolyEffect problem. With multiple moveTo and transform can hit this problem.
Attachment #8757275 - Flags: review?(mchang)
Comment on attachment 8757275 [details] [diff] [review]
test case for GrConvexPolyEffect

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

::: dom/canvas/test/reftest/clip-multiple-move.html
@@ +16,5 @@
> +ctx.lineTo(0,150);
> +ctx.lineTo(150,150);
> +ctx.lineTo(150,-1);
> +ctx.closePath();
> +ctx.transform(5,0,0,5,0,0);

Why do you need the scale transform here? Shouldn't the clip already cover the fillRect here?
(In reply to Mason Chang [:mchang] from comment #42)
> Comment on attachment 8757275 [details] [diff] [review]
> test case for GrConvexPolyEffect
> 
> Review of attachment 8757275 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/test/reftest/clip-multiple-move.html
> @@ +16,5 @@
> > +ctx.lineTo(0,150);
> > +ctx.lineTo(150,150);
> > +ctx.lineTo(150,-1);
> > +ctx.closePath();
> > +ctx.transform(5,0,0,5,0,0);
> 
> Why do you need the scale transform here? Shouldn't the clip already cover
> the fillRect here?

Without the transform, it won't enter GrConvexPolyEffect. After discussing with :lsalzman, I found that's because the transform makes the coordinates into something like 149.99999 and skia will use GrConvexPolyEffect to handle it. I will remove the transform and just use 149.99999 directly as it's coordinate.
Also I will combine both test cases into one patch.
Attached patch Add reftest for multiple moveTo (obsolete) — Splinter Review
Per discussion with :lsalzman, he will fix both problems in this bug. So I combine both test cases together.
Attachment #8756191 - Attachment is obsolete: true
Attachment #8757275 - Attachment is obsolete: true
Attachment #8757275 - Flags: review?(mchang)
Attachment #8757372 - Flags: review?(mchang)
Assuming the only real way a moveTo can end up being NOT the first verb in a path is if some other sequence of verbs starting with a moveTo preceded it, this just checks if the moveTo we run in to is in fact the first verb.

This might be too strong a test long-term, but it should ideally not harm us in the short-term. I will try to file an upstream Skia bug to see if they have a better fix for us, but this gets our reftests passing for now.
Flags: needinfo?(lsalzman)
Attachment #8757559 - Flags: review?(ethlin)
Upstream bug report for the isRectContour bug: https://bugs.chromium.org/p/skia/issues/detail?id=5355
Attachment #8757559 - Flags: review?(ethlin) → review+
Comment on attachment 8757372 [details] [diff] [review]
Add reftest for multiple moveTo

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

::: dom/canvas/test/reftest/clip-multiple-move-2.html
@@ +15,5 @@
> +ctx.moveTo(0,0);
> +ctx.moveTo(0,-1);
> +ctx.lineTo(0,150);
> +ctx.lineTo(150,150);
> +ctx.lineTo(149.99999,-1);

nit: Add a comment as to what should be happening here. Thanks!
Attachment #8757372 - Flags: review?(mchang) → review+
Add comment in the test.
Attachment #8757372 - Attachment is obsolete: true
Attachment #8758513 - Flags: review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c907bc97030
allow Skia GrConvexPolyEffect to handle degenerate contours. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3b8576fae4d
disallow multiple moveTos in SkPath::isRectContour. r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/351a2e0209b7
Add reftest to check clip with multiple moveTo. r=mchang
Keywords: checkin-needed
Lee, should we uplift patches to firefox 47 and 48?
Flags: needinfo?(lsalzman)
(In reply to Ethan Lin[:ethlin] from comment #52)
> Lee, should we uplift patches to firefox 47 and 48?

Per discussion with Lee, I will uplift patches to 47 and 48.
Flags: needinfo?(lsalzman)
Comment on attachment 8756921 [details] [diff] [review]
allow Skia GrConvexPolyEffect to handle degenerate contours

Approval Request Comment
[Feature/regressing bug #]: 1150944
[User impact if declined]: Canvas may display wrong with skiagl.
[Describe test coverage new/current, TreeHerder]: test on locally and tryserver
[Risks and why]: low risk. 
[String/UUID change made/needed]: none
Attachment #8756921 - Flags: approval-mozilla-release?
Attachment #8756921 - Flags: approval-mozilla-beta?
Comment on attachment 8757559 [details] [diff] [review]
disallow multiple moveTos in SkPath::isRectContour

Approval Request Comment
[Feature/regressing bug #]: 1150944
[User impact if declined]: Canvas may display wrong with skiagl.
[Describe test coverage new/current, TreeHerder]: test on locally and tryserver
[Risks and why]: low risk. 
[String/UUID change made/needed]: none
Attachment #8757559 - Flags: approval-mozilla-release?
Attachment #8757559 - Flags: approval-mozilla-beta?
Comment on attachment 8756921 [details] [diff] [review]
allow Skia GrConvexPolyEffect to handle degenerate contours

This is not a new regression and does not need to be included in a dot release.
Attachment #8756921 - Flags: approval-mozilla-release? → approval-mozilla-release-
Attachment #8757559 - Flags: approval-mozilla-release? → approval-mozilla-release-
[Risks and why]: low risk. 
Could you detail why? Thanks
Flags: needinfo?(ethlin)
(In reply to Sylvestre Ledru [:sylvestre] from comment #57)
> [Risks and why]: low risk. 
> Could you detail why? Thanks

We upstreamed the fixes to Skia, so they underwent their review as well. They don't particularly add any new functionality. Due to enhancing correctness in certain places, they actually prevent rendering paths that were erroneously being used from getting triggered when they shouldn't have been.
Flags: needinfo?(ethlin)
Comment on attachment 8756921 [details] [diff] [review]
allow Skia GrConvexPolyEffect to handle degenerate contours

ok, you + upstream, we should be fine, thanks!
should be 48 beta 3
Attachment #8756921 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8757559 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Hello,
I have tested this issue on OSx 10.11, OSx 10.10 and OSx 10.9 with Firefox Beta 48.0b4 and Firefox DevEdition 49.0a2 and it is not reproducible anymore.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: