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

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Jeremy Brack]

Attachment: WATR_PRR_EXTRAORDINARY_300x600__BANNER_200K.html

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.

Comment 1

[Loic]

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.

Comment 2

[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.

Comment 3

[Jeremy Brack]

Attachment: firefox_45_osx_bug.jpg

Comment 4

[Loic]

Sorry, wrong link about HWA:
https://support.mozilla.org/en-US/kb/forum-response-disable-hardware-acceleration

Comment 5

[Jeremy Brack]

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.

Comment 6

[Loic]

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.

Comment 7

[Jeremy Brack]

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.

Comment 8

[Loic]

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.

Comment 9

[ovidiu boca[:Ovidiu]]

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

Comment 10

[Jonathan Kew [:jfkthame]]

A b2g-inbound pushlog doesn't look very helpful here; can you get a mozilla-inbound regression window?

Comment 11

[ovidiu boca[:Ovidiu]]

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

Comment 12

[Jonathan Kew [:jfkthame]]

From a brief look at that pushlog, I wonder if SkiaGL (enabled in bug 1150944) is the most likely culprit?

Comment 13

[Mason Chang [Inactive] [:mchang]]

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!

Comment 14

[ovidiu boca[:Ovidiu]]

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.

Comment 15

[Matt Woodrow (:mattwoodrow)]

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.

Comment 16

[Lee Salzman [:lsalzman]]

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.

Comment 17

[u279076]

[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.

Comment 18

[Ethan Lin[:ethlin]]

I will work on it.

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 20

[Ethan Lin[:ethlin]]

The test case attachment 8728486 [details] is too complicate. I will try to narrow down the test to find the possible canvas api problem.

Comment 21

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 22

[Ethan Lin[:ethlin]]

Attachment: 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)".

Comment 23

[Milan Sreckovic [:milan] (needinfo for best results)]

Once we have a fix, let's turn the example from comment 22 into an automated test?

Comment 24

[Ethan Lin[:ethlin]]

Attachment: 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

Comment 25

[Ethan Lin[:ethlin]]

(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.

Comment 26

[Ethan Lin[:ethlin]]

Attachment: Part2. Add reftest for clip.

Add a reftest. For this test, Chrome also has the same problem.

Comment 27

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8756164 [details] [diff] [review]
Remove redundant move point in skia

From comment 27, I'll let Lee decide on this then.

Comment 28

[Milan Sreckovic [:milan] (needinfo for best results)]

(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.

Comment 29

[Lee Salzman [:lsalzman]]

(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.

Comment 30

[Ethan Lin[:ethlin]]

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

Comment 31

[Peter Chang[:pchang]]

Clear the needinfo since Ethan is working on this with a simple test case.

Comment 32

[Lee Salzman [:lsalzman]]

Attachment: allow Skia GrConvexPolyEffect to handle degenerate contours

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.

Comment 33

[Mason Chang [Inactive] [: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.

Comment 34

[Lee Salzman [:lsalzman]]

Upstream Skia bug report and patch: https://codereview.chromium.org/2018613003/

Comment 35

[Peter Chang[:pchang]]

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.

Comment 36

[Lee Salzman [:lsalzman]]

(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.

Comment 37

[Peter Chang[:pchang]]

Attachment: 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.

Comment 38

[Peter Chang[:pchang]]

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.

Comment 39

[Ethan Lin[:ethlin]]

Attachment: 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].

Comment 40

[Ethan Lin[:ethlin]]

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?

Comment 41

[Ethan Lin[:ethlin]]

Attachment: test case for GrConvexPolyEffect

The test case for GrConvexPolyEffect problem. With multiple moveTo and transform can hit this problem.

Comment 42

[Mason Chang [Inactive] [: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?

Comment 43

[Ethan Lin[:ethlin]]

(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.

Comment 44

[Ethan Lin[:ethlin]]

Attachment: Add reftest for multiple moveTo

Per discussion with :lsalzman, he will fix both problems in this bug. So I combine both test cases together.

Comment 45

[Lee Salzman [:lsalzman]]

Attachment: disallow multiple moveTos in SkPath::isRectContour

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.

Comment 46

[Lee Salzman [:lsalzman]]

Upstream bug report for the isRectContour bug: https://bugs.chromium.org/p/skia/issues/detail?id=5355

Comment 47

[Ethan Lin[:ethlin]]

Try result looks good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=850f05fb3820

Comment 48

[Mason Chang [Inactive] [:mchang]]

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!

Comment 49

[Ethan Lin[:ethlin]]

Attachment: Add reftest for multiple moveTo (carry r+: mchang)

Add comment in the test.

Comment 50

[Pulsebot]

Pushed by [e-mail]:
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

Comment 51

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4c907bc97030
https://hg.mozilla.org/mozilla-central/rev/b3b8576fae4d
https://hg.mozilla.org/mozilla-central/rev/351a2e0209b7

Comment 52

[Ethan Lin[:ethlin]]

Lee, should we uplift patches to firefox 47 and 48?

Comment 53

[Ethan Lin[:ethlin]]

(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.

Comment 54

[Ethan Lin[:ethlin]]

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

Comment 55

[Ethan Lin[:ethlin]]

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

Comment 56

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 57

[Sylvestre Ledru [:Sylvestre]]

[Risks and why]: low risk. 
Could you detail why? Thanks

Comment 58

[Lee Salzman [:lsalzman]]

(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.

Comment 59

[Sylvestre Ledru [:Sylvestre]]

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

Comment 60

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/e41ee1c8c1ec
https://hg.mozilla.org/releases/mozilla-beta/rev/60480e46f246

Comment 61

[Alexandru Simonca [:asimonca], Release Desktop QA]

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.