Closed Bug 1006198 Opened 10 years ago Closed 10 years ago

[e10s] Google Maps is upside down on OS X

Categories

(Core :: Graphics: CanvasWebGL, defect)

All
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla32
Tracking Status
e10s + ---
firefox31 --- unaffected
firefox32 + verified

People

(Reporter: cpeterson, Assigned: mattwoodrow)

References

Details

(Keywords: regression, reproducible)

Attachments

(7 files)

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.
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.
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?
Component: Graphics: Layers → Canvas: WebGL
Flags: needinfo?(roc)
Keywords: reproducible
I suspect the latter.
Flags: needinfo?(roc)
(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
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.
Flags: needinfo?(paul.silaghi)
(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
Flags: needinfo?(paul.silaghi)
Also not repro on Ubuntu, but repro on OS X 10.9.2
OS: All → Mac OS X
Summary: [e10s] Google Maps tiles are upside down → [e10s] Google Maps is upside down on OS X
Assignee: nobody → matt.woodrow
Attachment #8419957 - Flags: review?(nical.bugzilla)
Attachment #8419959 - Flags: review?(nical.bugzilla)
Attachment #8419962 - Flags: review?(jgilbert)
Basically the same as the last time we did this, but for the moz2d version.
Attachment #8419963 - Flags: review?(jgilbert)
Attachment #8419964 - Flags: review?(jgilbert)
Attachment #8419957 - Flags: review?(nical.bugzilla) → review+
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.
(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.
Attachment #8419962 - Flags: review?(jgilbert) → review+
Attachment #8419966 - Flags: review?(nical.bugzilla) → review+
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.
Attachment #8419959 - Flags: review?(nical.bugzilla)
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.
Attachment #8419959 - Flags: review?(nical.bugzilla)
Attachment #8419959 - Flags: review?(nical.bugzilla) → review+
Attachment #8419963 - Flags: review?(jgilbert) → review+
Attachment #8419964 - Flags: review?(jgilbert) → review+
Looks ok in 32.0a1 (2014-05-15), OS X 10.8.5
Status: RESOLVED → VERIFIED
Meh. Some of the patches here duplicated the patch in bug 1001687.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: