1006198 - [e10s] Google Maps is upside down on OS X

Status: VERIFIED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: upside-down-screenshot.jpg

STR:
1. Load the New Google Maps: https://www.google.com/maps/preview
2. The map loads as expected.
3. Now open a new e10s window and load Google Maps there.

RESULT:
The map image is upside down. See the attached screenshot.

This is a recent regression in the past couple days.

Comment 1

[Chris Peterson [:cpeterson]]

I bisected this regression to the following pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b227a707080f&tochange=e2e1b19fcffc

This bug is probably a regression from WebGL bug 995745.

Comment 2

[Chris Peterson [:cpeterson]]

roc: does this upside down WebGL bug look like a direct regression from your fix for bug 995745 or did the bug 995745 just reveal an earlier regression that happened during period Google Maps was black on OMTC?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I suspect the latter.

Comment 4

[Paul Silaghi, QA [:pauly]]

(In reply to Chris Peterson (:cpeterson) from comment #0)
> RESULT:
> The map image is upside down. See the attached screenshot.
I can't reproduce, 32.0a1 (2014-05-06), Win 7 x64

Comment 5

[Chris Peterson [:cpeterson]]

Paul: Did you test Windows in an e10s window ("File > Open e10s Window") and/or with OMTC enabled (set "layers.offmainthreadcomposition.enabled" pref to true)? I can still reproduce with Nightly 32 (2014-05-06) on OS X.

Comment 6

[Paul Silaghi, QA [:pauly]]

(In reply to Chris Peterson (:cpeterson) from comment #5)
> Paul: Did you test Windows in an e10s window ("File > Open e10s Window")
> and with OMTC enabled (set "layers.offmainthreadcomposition.enabled" pref
> to true)?
Exactly, but on Win 7 x64

Comment 7

[Paul Silaghi, QA [:pauly]]

Also not repro on Ubuntu, but repro on OS X 10.9.2

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: Flip Image even if they are tiled

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: Add PremultiplyDataSurface

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Use more Moz2D in WebGL

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Use a temporary surface if alignment is wrong

Basically the same as the last time we did this, but for the moz2d version.

Comment 12

[Matt Woodrow (:mattwoodrow)]

Attachment: Remove all the dead code

Comment 13

[Matt Woodrow (:mattwoodrow)]

Attachment: Be smarter with our surface allocation for CopyableCanvasLayer

Comment 14

[Matt Woodrow (:mattwoodrow)]

https://tbpl.mozilla.org/?tree=Try&rev=cbfb0e30d65f

Comment 15

[Nicolas Silva [:nical]]

Comment on attachment 8419959 [details] [diff] [review]
Add PremultiplyDataSurface

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

::: gfx/thebes/gfxUtils.cpp
@@ +89,5 @@
>      }
>  }
>  
>  void
> +gfxUtils::PremultiplyDataSurface(DataSourceSurface *aSurface)

What's the difference between this PremultiplyDataSurface, and the original PremultiplySurface with srcSurface == destSurface ?

Ah, on my laptop, PremultiplySurface takes DataSourceSurfaces so it must have been Moz2dified while you were writing this. So if the motivation was to have a moz2d version then I think you can remove this function now.

@@ +92,5 @@
>  void
> +gfxUtils::PremultiplyDataSurface(DataSourceSurface *aSurface)
> +{
> +    // Only premultiply ARGB32
> +    if (aSurface->GetFormat() != SurfaceFormat::B8G8R8A8) {

nit: We should assert here or print a warning if the format is not ARGB32.

Comment 16

[Matt Woodrow (:mattwoodrow)]

(In reply to Nicolas Silva [:nical] from comment #15)
> Comment on attachment 8419959 [details] [diff] [review]
> Add PremultiplyDataSurface
> 
> Review of attachment 8419959 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxUtils.cpp
> @@ +89,5 @@
> >      }
> >  }
> >  
> >  void
> > +gfxUtils::PremultiplyDataSurface(DataSourceSurface *aSurface)
> 
> What's the difference between this PremultiplyDataSurface, and the original
> PremultiplySurface with srcSurface == destSurface ?
> 
> Ah, on my laptop, PremultiplySurface takes DataSourceSurfaces so it must
> have been Moz2dified while you were writing this. So if the motivation was
> to have a moz2d version then I think you can remove this function now.

Nothing really, this is basically just moving it to gfxUtils instead of LayerUtils.

A later patch in the series gets rid of the duplication.

Comment 17

[Nicolas Silva [:nical]]

Comment on attachment 8419959 [details] [diff] [review]
Add PremultiplyDataSurface

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

Canceling the review because if I understand correctly you don't need this anymore, don't hesitate to put the "r?" back if I was wrong.

Comment 18

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8419959 [details] [diff] [review]
Add PremultiplyDataSurface

Please look at the 'remove all the dead code' patch, since it removes all the duplicate code you're worried about.

Comment 19

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

These all landed in https://tbpl.mozilla.org/?tree=Mozilla-Inbound&onlyunstarred=1&rev=5be3c8c44eed , and then now I backed them all out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e228877ffe64 for causing WebGL assertions like this in multiple test suites: https://tbpl.mozilla.org/php/getParsedLog.php?id=39536317&tree=Mozilla-Inbound

Comment 20

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/98a354e678c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/d60356c63c4f
https://hg.mozilla.org/integration/mozilla-inbound/rev/0675a2349cfc
https://hg.mozilla.org/integration/mozilla-inbound/rev/66efc0d84a37
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbbe64df866e
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ff7d00bb9fb

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/98a354e678c2
https://hg.mozilla.org/mozilla-central/rev/d60356c63c4f
https://hg.mozilla.org/mozilla-central/rev/0675a2349cfc
https://hg.mozilla.org/mozilla-central/rev/66efc0d84a37
https://hg.mozilla.org/mozilla-central/rev/bbbe64df866e
https://hg.mozilla.org/mozilla-central/rev/0ff7d00bb9fb

Comment 22

[Paul Silaghi, QA [:pauly]]

Looks ok in 32.0a1 (2014-05-15), OS X 10.8.5

Comment 23

[Jonathan Watt [:jwatt]]

Meh. Some of the patches here duplicated the patch in bug 1001687.