839808 - (basic-metro-omtc) Get basic omtc support running in metro

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Jim Mathies [:jimm]]

Using the current graphics branch.

Comment 1

[Jim Mathies [:jimm]]

Attachment: basics

Here's a basic patch that gets omtc running under metrofx. 

Unfortunately the initial perf results are a bit of a downer. Using peacekeeper, I get the following numbers:

1427 - 6/7 - rel, non-pgo, omtc
1650 - 6/7 - rel, non-pgo, non-omtc
1772 - 6/7 - nightly

Comment 2

[Jim Mathies [:jimm]]

Attachment: patch v.1

Lets get this landed so we can switch this on by simply changing a pref. Note this only works on the gfx branch at this point.

Comment 3

[Jim Mathies [:jimm]]

Comment on attachment 720618 [details] [diff] [review]
patch v.1

I need to get this over on mc first and test. The layers changes aren't going to work there.

Comment 4

[Jim Mathies [:jimm]]

Attachment: patch v.2

Comment 5

[Jim Mathies [:jimm]]

Attachment: patch v.3

Sorry for the bugspam. I've changed the layer selection helpers around a bit. D3D should only be selected for a top level window.

Comment 6

[Brian R. Bondy [:bbondy]]

Comment on attachment 720632 [details] [diff] [review]
patch v.3

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

::: widget/windows/winrt/MetroWidget.cpp
@@ +828,5 @@
> +      if (layerManager->Initialize(true)) {
> +        mLayerManager = layerManager;
> +      }
> +    } else if (UseBasicManager()) {
> +      mLayerManager = CreateBasicLayerManager();

do we need to call Initialize here?

@@ +835,5 @@
> +    // Either we're not ready to initialize yet due to a missing view pointer,
> +    // or something has gone wrong.
> +    if (!mLayerManager) {
> +      if (!mView) {
> +        mLayerManager = new BasicShadowLayerManager(this);

nit: Maybe NS_WARNING here.
Do we need to call Initialize here

::: widget/windows/winrt/MetroWidget.h
@@ +115,5 @@
>    virtual bool  IsEnabled() const;
> +  // UseOffMainThreadCompositing is defined in base widget
> +  virtual bool  UseOffMainThreadCompositing();
> +  bool          UseMainThreadD3D10Manager();
> +  bool          UseBasicManager();

nit: I would prefer these to be prefixed with "ShouldUse" or just "Is", they just sound like setters the way it is.

Comment 7

[Jim Mathies [:jimm]]

> ::: widget/windows/winrt/MetroWidget.cpp
> @@ +828,5 @@
> > +      if (layerManager->Initialize(true)) {
> > +        mLayerManager = layerManager;
> > +      }
> > +    } else if (UseBasicManager()) {
> > +      mLayerManager = CreateBasicLayerManager();
> 
> do we need to call Initialize here?

apparently not. basic manager can't fail where as the d3d one can.

> @@ +835,5 @@
> > +    // Either we're not ready to initialize yet due to a missing view pointer,
> > +    // or something has gone wrong.
> > +    if (!mLayerManager) {
> > +      if (!mView) {
> > +        mLayerManager = new BasicShadowLayerManager(this);
> 
> nit: Maybe NS_WARNING here.

will do.

> @@ +115,5 @@
> >    virtual bool  IsEnabled() const;
> > +  // UseOffMainThreadCompositing is defined in base widget
> > +  virtual bool  UseOffMainThreadCompositing();
> > +  bool          UseMainThreadD3D10Manager();
> > +  bool          UseBasicManager();
> 
> nit: I would prefer these to be prefixed with "ShouldUse" or just "Is", they
> just sound like setters the way it is.

ok, sounds good.

Comment 8

[Jim Mathies [:jimm]]

Attachment: patch v.4

Added changes applied to all platforms so reseeking r?.

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

Comment 9

[Jim Mathies [:jimm]]

Comment on attachment 720688 [details] [diff] [review]
patch v.4

Bas, would it be alright for me to land this on graphics?

Comment 10

[Jim Mathies [:jimm]]

Comment on attachment 720688 [details] [diff] [review]
patch v.4

actually, the two repos are out of sync. I can't. I guess I'll just wait for the next merge.

Comment 11

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1473f6edfcad

Comment 12

[Jim Mathies [:jimm]]

One additional note, to get this building on gfx you need to add:

#ifdef MOZ_METRO
#include "DXGI1_2.h"
#endif

to 

gfx/layers/d3d11/CompositorD3D11.cpp

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1473f6edfcad