852734 - [layers-refactoring] reviews

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Comment 1

[Nick Cameron [:nrc]]

I'm going to put all the patches for review here, where there is no traffic already. We should probably split out patches which will get a lot of discussion into their own bugs. But lets do that as we go along and track here, because there are a lot of patches.

The initial patches here contain all work up to https://tbpl.mozilla.org/?tree=Graphics&rev=628771e1cc4a. New work should get put into new patches.

Comment 2

[Nick Cameron [:nrc]]

Attachment: 1 The compositor interface

Comment 3

[Nick Cameron [:nrc]]

Attachment: 2 effects

Comment 4

[Nick Cameron [:nrc]]

Attachment: 3 compositable hosts and clients

Comment 5

[Nick Cameron [:nrc]]

Attachment: 4 concreate CompositableHosts

Comment 6

[Nick Cameron [:nrc]]

Attachment: 5 texture hosts and clients

Comment 7

[Nick Cameron [:nrc]]

Attachment: 6 changes to layers

Comment 8

[Nick Cameron [:nrc]]

Attachment: 7 composite layers

Comment 9

[Nick Cameron [:nrc]]

Attachment: 8 image stuff

Comment 10

[Nick Cameron [:nrc]]

Attachment: 9 GL Layers and compositor

Comment 11

[Nick Cameron [:nrc]]

Attachment: 10 thebes layers

Comment 12

[Nick Cameron [:nrc]]

Attachment: 11 content-side image layers

Comment 13

[Nick Cameron [:nrc]]

Attachment: 12 content-side canvas layers

Comment 14

[Nick Cameron [:nrc]]

Attachment: 13 content-side color layers

Comment 15

[Nick Cameron [:nrc]]

Attachment: 14 IPC/IPDL stuff

Comment 16

[Nick Cameron [:nrc]]

Attachment: 15 D3D11 backend

Comment 17

[Nick Cameron [:nrc]]

Attachment: 16 changes to D3D9 and D3D10 backends

Comment 18

[Nick Cameron [:nrc]]

Attachment: 17 compositor-side tiled layers

Comment 19

[Nick Cameron [:nrc]]

Attachment: 18 changes in gfx/gl

Comment 20

[Nick Cameron [:nrc]]

Attachment: 19 gfxPlatform

Comment 21

[Nick Cameron [:nrc]]

Attachment: 20 random bits and pieces

Comment 22

[Nick Cameron [:nrc]]

Attachment: 21 makefile changes

Comment 23

[Nick Cameron [:nrc]]

Attachment: 22 (logically earlier, maybe) Azure changes

Comment 24

[Nick Cameron [:nrc]]

Comment on attachment 726906 [details] [diff] [review]
1 The compositor interface

Asking for review for the general API of the whole thing.

Comment 25

[Nick Cameron [:nrc]]

I plan to land this all at once. Queue prefixes may not compile.

Comment 26

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

Comment on attachment 726906 [details] [diff] [review]
1 The compositor interface

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

Generally the documentation here seems a bit underdone. It's not possible to read this patch and actually understand the interface.

::: gfx/layers/Compositor.h
@@ +69,5 @@
> + * dispatch the message to the right actor on the other side. One nice property
> + * of an IPDL actor is that when an actor, say PXChild is sent in a message, the
> + * PXParent comes out in the other side. we use this property a lot to dispatch
> + * messages to the right layers and compositable, each of which have their own
> + * ipdl actor on both side.

I think this background information about IPDL should be somewhere else and just referenced here.

@@ +75,5 @@
> + * Most of the synchronization logic happens in layer transactions and
> + * compositable transactions.
> + * A transaction is a set of changes to the layers and/or the compositables
> + * that are sent and applied together to the compositor thread to keep the
> + * ShadowLayer in a coherent state.

Here you've started talking about compositables without having explained what they are. Can this be moved after the section on texture transfer?

@@ +149,5 @@
> +  INIT_MODE_COPY
> +};
> +
> +/**
> + * Common interface for compositor backends.

Is there one of these per window, or just a global singleton, or what?

We need a lot more documentation explaining the role of this class.

@@ +168,5 @@
> +  virtual void Destroy() = 0;
> +
> +  /* Request a texture host identifier that may be used for creating textures
> +   * accross process or thread boundaries that are compatible with this
> +   * compositor.

/** comment

"across"

@@ +181,5 @@
> +  virtual int32_t GetMaxTextureSize() const = 0;
> +
> +  /**
> +   * Set the target for rendering, intended to be used for the duration of a transaction
> +   */

This comment could be more informative. Specify when the results are sure to have been written to the context.

@@ +188,5 @@
> +  /**
> +   * Make sure that the underlying rendering API selects the right current
> +   * rendering context.
> +   */
> +  virtual void MakeCurrent(bool aForce = false) = 0;

What is the scope of this?

::: gfx/layers/CompositorTypes.h
@@ +35,5 @@
> +  TEXTURE_CONTENT,            // dynamically drawn content
> +  TEXTURE_SHMEM,              // shared memory
> +  TEXTURE_YCBCR,              // ShmemYCbCrImage
> +  TEXTURE_SHARED_GL,          // GLContext::SharedTextureHandle
> +  TEXTURE_SHARED_GL_EXTERNAL, // GLContext::SharedTextureHandle, owned by the caller

What is "the caller"?

@@ +37,5 @@
> +  TEXTURE_YCBCR,              // ShmemYCbCrImage
> +  TEXTURE_SHARED_GL,          // GLContext::SharedTextureHandle
> +  TEXTURE_SHARED_GL_EXTERNAL, // GLContext::SharedTextureHandle, owned by the caller
> +  TEXTURE_STREAM_GL           // WebGL streaming buffer
> +};

Is this type and various other types in this file actually used by the Compositor interface? Doesn't look like it. So why are they part of the same patch?

@@ +40,5 @@
> +  TEXTURE_STREAM_GL           // WebGL streaming buffer
> +};
> +
> +/**
> + * Compositbales have four kinds of interface - image and canvas layers pass

typo

@@ +48,5 @@
> + * SurfaceDescriptor. The SurfaceDescriptor (and the underlying surface) is
> + * created by the content thread and passed on creation to the compositing
> + * thread. Logical ownership is transferred at this point. Image bridge is the
> + * protocol used for async upload of images (for async video). Tiled layers 
> + * have their own, raw memory system.

These comments are not very intelligible, especially the last one.

Comment 27

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 726930 [details] [diff] [review]
19 gfxPlatform

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

::: gfx/thebes/gfxUtils.cpp
@@ +529,5 @@
> +  const gfxFloat epsilon = 0.000001;
> +#define WITHIN_E(a,b) (fabs((a)-(b)) < epsilon)
> +  bool shouldSnap = aSnap && (!WITHIN_E(mat._11,1.0) || !WITHIN_E(mat._22,1.0) ||
> +                              !WITHIN_E(mat._12,0.0) || !WITHIN_E(mat._21,0.0));
> +#undef WITHIN_E

This snapping critera should have some kind of justification.

@@ +535,5 @@
> +  RefPtr<PathBuilder> pb = aTarget->CreatePathBuilder();
> +  nsIntRegionRectIterator iter(aRegion);
> +
> +  const nsIntRect* r;
> +  if (!shouldSnap) {

reversing these conditions would make it easier to read.

i.e. if (shouldSnap) etc.

@@ +547,5 @@
> +  } else {
> +    while ((r = iter.Next()) != nullptr) {
> +      Rect rect(r->x, r->y, r->width, r->height);
> +
> +      rect.Round();

Please add a comment about why Round() and not RoundIn() or RoundOut()

Comment 28

[Joe Drew (not getting mail)]

Comment on attachment 726936 [details] [diff] [review]
22 (logically earlier, maybe) Azure changes

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

::: gfx/2d/ShadersD2D.fx
@@ +308,5 @@
> +       retval.g = 1 - min(1, (1 - background.g) / output.g);
> +    else
> +      retval.g = background.g ? 1 : 0;
> +    if(output.b > 0)
> +     retval.b = 1 - min(1, (1 - background.b) / output.b);

indentation is wrong by 1 space here

Comment 29

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 726908 [details] [diff] [review]
2 effects

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

::: gfx/layers/Effects.h
@@ +14,5 @@
> +namespace layers {
> +
> +/**
> + * Effects and effect chains are used by the compositor API (see Compositor.h).
> + * An effect chain represents a rendering method, for example some shader and

Isn't this what an individual effect is? And the effect chain is the set of effects to apply when rendering.

@@ +19,5 @@
> + * the data required for that shader to run. An effect is some component of the
> + * chain and its data.
> + *
> + * An effect chain consists of a primary effect - how the 'texture' memory should
> + * be interpreted (RGBA, BGRX, YCBCR, etc.) - and any number of secondary effects

I'm not entirely sure why some of this is actually necessary. Surely the TextureHost already knows the byte order (and *needs* to know in some cases, like where we do an upload with GL). This causes some confusing code in the GL backend, where the texture upload code determines which byte order (by deciding which shader program to use), and we basically ignore the effect type that was passed in.

What actual benefit do we get from having separate byte order types rather than just EFFECT_TEXTURE?

@@ +20,5 @@
> + * chain and its data.
> + *
> + * An effect chain consists of a primary effect - how the 'texture' memory should
> + * be interpreted (RGBA, BGRX, YCBCR, etc.) - and any number of secondary effects
> + * - any way in which rendering can be changed, for e.g., applying a mask layer.

'for' is unnecessary here, e.g. means 'for example' already.

@@ +32,5 @@
> +
> +enum EffectTypes
> +{
> +  EFFECT_MASK,
> +  EFFECT_PRIMARY,

This is confusing, it's not really an effect type at all.

Comment 30

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 726934 [details] [diff] [review]
21 makefile changes

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

::: gfx/layers/Makefile.in
@@ +31,4 @@
>  endif
>  
>  EXPORTS = \
> +		BasicCanvasLayer.h \

Indentation!

@@ +51,3 @@
>          ImageTypes.h \
>          Layers.h \
> +		LayersLogging.h \

Same here

@@ +121,4 @@
>          DeviceManagerD3D9.cpp \
>          Nv3DVUtils.cpp \
>          $(NULL)
> +#        ShadowBufferD3D9.cpp 

Why not just get rid of this entirely?

@@ +233,5 @@
> +EXPORTS_mozilla/layers += \
> +                       ShadowLayerUtilsD3D10.h \
> +                       CompositorD3D11.h \
> +                       TextureD3D11.h \
> +                       $(NULL)

Indentation should match the above lists.

Comment 31

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 726931 [details] [diff] [review]
20 random bits and pieces

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

::: ipc/chromium/src/chrome/common/x11_util.h
@@ +23,4 @@
>  namespace gfx {
>  class Size;
>  }
> +*/

If we don't need this definition, then just get rid of it

::: mfbt/RefPtr.h
@@ +81,4 @@
>      }
>  
>    private:
> +    unsigned int refCnt;

I don't really understand the reasoning for these changes, and I probably can't review mfbt changes either.

::: toolkit/xre/nsAppRunner.cpp
@@ +3400,4 @@
>    // An environment variable is used instead of a pref on X11 platforms because we start having 
>    // access to prefs long after the first call to XOpenDisplay which is hard to change due to 
>    // interdependencies in the initialization.
> +  XInitThreads();

I don't think we actually want to land this, as it will enable multithreaded Xlib for all builds.

Maybe we can add a new env var to check here, and get releng to have that specified for all Ripc/Cipc runs before we land.

::: widget/xpwidgets/nsBaseWidget.cpp
@@ +1187,4 @@
>    if (usedAcceleration == mUseLayersAcceleration) {
>      return NS_OK;
>    }
> +// XXX - Bas - Disabled for now.

Why? This seems... important

@@ +1285,1 @@
>    *aResult = nullptr;

Trailing whitespace

@@ +1636,4 @@
>  _GetPrintCount()
>  {
>    static int32_t sCount = 0;
> +  

Trailing whitespace

::: widget/xpwidgets/nsBaseWidget.h
@@ +111,5 @@
>                                            bool* aAllowRetaining = nullptr);
>  
>    virtual void            CreateCompositor();
> +  virtual void            DrawWindowUnderlay() {}
> +  virtual void            DrawWindowOverlay() {}

widget/cocoa/nsChildView.{h|mm} overrides this, and needs to be updated.

Comment 32

[Nick Cameron [:nrc]]

Things that still need to be addressed from those comments:

(In reply to Matt Woodrow (:mattwoodrow) from comment #31)
> Comment on attachment 726931 [details] [diff] [review]
> 20 random bits and pieces
> 
> Review of attachment 726931 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/xre/nsAppRunner.cpp
> @@ +3400,4 @@
> >    // An environment variable is used instead of a pref on X11 platforms because we start having 
> >    // access to prefs long after the first call to XOpenDisplay which is hard to change due to 
> >    // interdependencies in the initialization.
> > +  XInitThreads();
> 
> I don't think we actually want to land this, as it will enable multithreaded
> Xlib for all builds.
> 
> Maybe we can add a new env var to check here, and get releng to have that
> specified for all Ripc/Cipc runs before we land.
> 
> ::: widget/xpwidgets/nsBaseWidget.cpp
> @@ +1187,4 @@
> >    if (usedAcceleration == mUseLayersAcceleration) {
> >      return NS_OK;
> >    }
> > +// XXX - Bas - Disabled for now.
> 
> Why? This seems... important
> 
> ::: widget/xpwidgets/nsBaseWidget.h
> @@ +111,5 @@
> >                                            bool* aAllowRetaining = nullptr);
> >  
> >    virtual void            CreateCompositor();
> > +  virtual void            DrawWindowUnderlay() {}
> > +  virtual void            DrawWindowOverlay() {}
> 
> widget/cocoa/nsChildView.{h|mm} overrides this, and needs to be updated.

Comment 33

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

Comment on attachment 726930 [details] [diff] [review]
19 gfxPlatform

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

::: gfx/thebes/gfxPlatform.cpp
@@ +280,3 @@
>      useOffMainThreadCompositing = Preferences::GetBool(
> +          "layers.offmainthreadcomposition.enabled", false) ||
> +          Preferences::GetBool("browser.tabs.remote", false);

Instead of the first one, should we be using gfxPlatform::GetPrefLayersOffMainThreadCompositionEnabled() for consistency?

Comment 34

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 726928 [details] [diff] [review]
18 changes in gfx/gl

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

::: gfx/gl/GLContext.h
@@ +171,5 @@
>  #ifdef DEBUG
>          PR_SetThreadPrivate(sCurrentGLContextTLS, this);
>  
> +  // XXX this assertion is disabled because it's triggering on Mac;
> +  // we need to figure out why and reenable it.

The indent here was messed up.

@@ +2190,4 @@
>              height = -1;
>              border = -1;
>          }
> +        if ((GLint)format != internalformat) {

I'd prefer to coerce internalFormat to GLenum instead, since that's what it really should be. The spec says GLint for some reason, but it's really a GLenum.

Also, this often isn't slow, but is instead sometimes faster than a matching internalFormat and format. GL_RGBA/GL_BGRA/UNSIGNED_INT_8_8_8_8_REV is a common case of this, iirc.

I would be fine with warning when the sizes of the internalFormat and format don't match, though, since that should indeed be slow.

::: gfx/gl/GLContextProviderCGL.mm
@@ +100,5 @@
>      ~GLContextCGL()
>      {
> +        if (MakeCurrent()) {
> +            if (mTemporaryIOSurfaceTexture != 0) {
> +                fDeleteTextures(1, &mTemporaryIOSurfaceTexture);

This stuff goes in MarkDestroyed.

@@ +177,5 @@
>      }
>  
> +    virtual SharedTextureHandle CreateSharedHandle(SharedTextureShareType aShareType)
> +    {
> +      RefPtr<MacIOSurface> surf =

Indents need to be all 4-space for this file.

@@ +194,5 @@
> +      GLuint prevRead = GetFB();
> +      BindFB(0);
> +
> +      GLint oldtex = -1;
> +      fGetIntegerv(LOCAL_GL_TEXTURE_BINDING_RECTANGLE_ARB, &oldtex);

I would prefer you modified and used ScopedBindTexture with an optional third arg specifying the target, which defaults to GL_TEXTURE_2D, but can be replaced with GL_TEXTURE_BINDING_RECTANGLE, for example.

We should also lop off the ARB suffix at some point, but we can do that later.

@@ +198,5 @@
> +      fGetIntegerv(LOCAL_GL_TEXTURE_BINDING_RECTANGLE_ARB, &oldtex);
> +      MOZ_ASSERT(oldtex != -1);
> +
> +      if (!mTemporaryIOSurfaceTexture) {
> +        fGenTextures(1, &mTemporaryIOSurfaceTexture);

I don't like the use of a central temporary texture used by everyone. Is there a reason for this? It seems it would be prone to tricky errors.

@@ +205,5 @@
> +                       LOCAL_GL_TEXTURE_MIN_FILTER,
> +                       LOCAL_GL_NEAREST);
> +        fTexParameteri(LOCAL_GL_TEXTURE_RECTANGLE_ARB,
> +                       LOCAL_GL_TEXTURE_MAG_FILTER,
> +                       LOCAL_GL_NEAREST);

We also need to set the CLAMP behaviors, as they default to REPEAT, which isn't supported by NPOT textures.

@@ +211,5 @@
> +        fBindTexture(LOCAL_GL_TEXTURE_RECTANGLE_ARB, mTemporaryIOSurfaceTexture);
> +      }
> +
> +      surf->CGLTexImageIOSurface2D(mContext, LOCAL_GL_RGBA, LOCAL_GL_BGRA,
> +                                   LOCAL_GL_UNSIGNED_INT_8_8_8_8_REV, 0);

Surely this stuff should only happen in CreateSharedHandle, instead of reattaching the IOSurf to a texture all the time?

@@ +215,5 @@
> +                                   LOCAL_GL_UNSIGNED_INT_8_8_8_8_REV, 0);
> +
> +      fCopyTexSubImage2D(LOCAL_GL_TEXTURE_RECTANGLE_ARB, 0, 0, 0,
> +                         0, 0, OffscreenSize().width,
> +                         OffscreenSize().height);

`ScopedBindFramebuffer autoFB(this, 0)` should probably go right before this, to keep it more local to the command which wants FB 0. Also it's better than doing GetFB/BindFB yourself.

I don't love the choice of newline splits of the args here. I try to keep this commands in the form:
CopyTexSubImage(target, level,
                xoffset, yoffset,
                x, y, width, height)
With `width, height` being on a new line if they're too long to go with `x, y`.
Without a formatting like this, there's sorta jut a mess of zeros, most of the time.

@@ +244,5 @@
> +                                    SharedTextureHandle sharedHandle)
> +    {
> +      MacIOSurface* surf = reinterpret_cast<MacIOSurface*>(sharedHandle);
> +      surf->CGLTexImageIOSurface2D(mContext, LOCAL_GL_RGBA, LOCAL_GL_BGRA,
> +                                   LOCAL_GL_UNSIGNED_INT_8_8_8_8_REV, 0);

Ok, this gets tricky. AttachSharedHandle is supposed to attach the SharedTexture to the current GL_TEXTURE_2D binding, but this function attaches to the GL_TEXTURE_RECTANGLE binding instead. If this is communicated somehow, it needs to be explained in the root comments for AttachSharedHandle.

::: gfx/gl/GLContextProviderEGL.cpp
@@ +445,5 @@
> +
> +
> +    bool BindExternalBuffer(GLuint texture, void* buffer) MOZ_OVERRIDE
> +    {
> +#if defined(MOZ_WIDGET_GONK)

Pick only one of `#ifdef FOO` or `#if defined(FOO)`. I think the former is preferred.

@@ +465,5 @@
>  #if defined(MOZ_WIDGET_GONK)
>          fActiveTexture(LOCAL_GL_TEXTURE0);
> +        // FIXME [bjacob] There is a bug here: GL_TEXTURE_2D here is incompatible
> +        // with GL_TEXTURE_EXTERNAL in BindExternalBuffer. Specifically, binding a texture to
> +        // two different texture targets in a GL_INVALID_OPERATION.

Ugh, we didn't manage a fix for this yet? Can we not create an EGLImage from a 1x1/2x2 texture, and then attach that as a TEXTURE_EXTERNAL?

::: gfx/gl/GLContextProviderGLX.cpp
@@ +399,4 @@
>          break;
>      }
>      if (matchIndex == -1) {
> +        NS_WARN_IF_FALSE(format->depth == 8, "[GLX] Couldn't find a FBConfig matching Pixmap format");

What's the reason behind only warning if format->depth != 8?

@@ +832,5 @@
>  
>      ~GLContextGLX()
>      {
> +        MakeCurrent();
> +        fFinish();

Why do we Finish here?

@@ +848,5 @@
>          if (mDeleteDrawable) {
>              mGLX->xDestroyPixmap(mDisplay, mDrawable);
>          }
> +        
> +        mGLX->xDestroyContext(mDisplay, mContext);

Destroying the context's backbuffer before destroying the context itself is *probably* fine, but more obtuse than destroying them in the more usual order.

@@ +1118,4 @@
>  
>      MakeCurrent();
>      GLXPixmap pixmap = mGLX->CreatePixmap(surface);
> +    if (!pixmap && imageFormat == gfxASurface::ImageFormatA8) {

Why do we have this special recovery case for only A8?

@@ +1417,4 @@
>  GLContext*
>  GLContextProviderGLX::GetGlobalContext(const ContextFlags aFlag)
>  {
> +#if 0

Why do we remove this? IIRC, we need this for creating share groups to allow fast compositing for GLContexts.

@@ +1430,5 @@
>      }
>  
>      return gGlobalContext[libType];
> +#endif
> +  return nullptr;

2-space in a 4-space file.

::: gfx/gl/GLDefs.h
@@ +46,4 @@
>  
>  // OES_EGL_image (GLES)
>  typedef void* GLeglImage;
> +typedef void* EGLImage;

This goes with the EGL types below, with EGLSync and friends.

::: gfx/gl/GLTextureImage.cpp
@@ +9,5 @@
>  #include "gfxPlatform.h"
>  #include "gfxUtils.h"
>  
> +namespace mozilla {
> +namespace gl {

I actually prefer `using namespace` in CPP files, but this does work.

@@ +595,5 @@
> +{
> +    bool useNearestFilter = aFlags & TextureImage::UseNearestFilter;
> +    aGL->MakeCurrent();
> +
> +    GLuint texture;

Initialize this to 0.

@@ +598,5 @@
> +
> +    GLuint texture;
> +    aGL->fGenTextures(1, &texture);
> +
> +    aGL->fActiveTexture(LOCAL_GL_TEXTURE0);

Setting ActiveTexture isn't necessary, so should be removed.

@@ +599,5 @@
> +    GLuint texture;
> +    aGL->fGenTextures(1, &texture);
> +
> +    aGL->fActiveTexture(LOCAL_GL_TEXTURE0);
> +    aGL->fBindTexture(LOCAL_GL_TEXTURE_2D, texture);

If we ever call this function on a WebGL GLContext, we need to save and restore the previous texture binding.

@@ +601,5 @@
> +
> +    aGL->fActiveTexture(LOCAL_GL_TEXTURE0);
> +    aGL->fBindTexture(LOCAL_GL_TEXTURE_2D, texture);
> +
> +    GLint texfilter = useNearestFilter ? LOCAL_GL_NEAREST : LOCAL_GL_LINEAR;

wrapMode and texFilter should probably be the same type. (lamest nit ever)
Also 'texfilter' should be 'texFilter'.

@@ +607,5 @@
> +    aGL->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_MAG_FILTER, texfilter);
> +    aGL->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_WRAP_S, aWrapMode);
> +    aGL->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_WRAP_T, aWrapMode);
> +
> +    nsRefPtr<BasicTextureImage> teximage(

Did you lose your camels? 'texImage'

@@ +608,5 @@
> +    aGL->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_WRAP_S, aWrapMode);
> +    aGL->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_WRAP_T, aWrapMode);
> +
> +    nsRefPtr<BasicTextureImage> teximage(
> +        new BasicTextureImage(texture, aSize, aWrapMode, aContentType, aGL, aFlags));

Why do we use the explicit constructor instead of the more usual form:
nsRefPtr<FooT> bar = new Qux();

::: gfx/gl/GLTextureImage.h
@@ +322,4 @@
>  
>      GLuint mTexture;
>      TextureState mTextureState;
> +    nsRefPtr<GLContext> mGLContext;

Why does this need to be a RefPtr?

Comment 35

[Nick Cameron [:nrc]]

(In reply to Jeff Gilbert [:jgilbert] from comment #34)
> Comment on attachment 726928 [details] [diff] [review]
> 18 changes in gfx/gl
> 
> Review of attachment 726928 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
 
> ::: gfx/gl/GLContextProviderGLX.cpp
> @@ +399,4 @@
> >          break;
> >      }
> >      if (matchIndex == -1) {
> > +        NS_WARN_IF_FALSE(format->depth == 8, "[GLX] Couldn't find a FBConfig matching Pixmap format");
> 
> What's the reason behind only warning if format->depth != 8?
> 

Because if it is depth 8, then it is expected behaviour which we handle at this next bit...

> @@ +1118,4 @@
> >  
> >      MakeCurrent();
> >      GLXPixmap pixmap = mGLX->CreatePixmap(surface);
> > +    if (!pixmap && imageFormat == gfxASurface::ImageFormatA8) {
> 
> Why do we have this special recovery case for only A8?
> 

GLX sometimes can't handle alpha images, so CreatePixmap will fail. But it is easier (and not too expensive) to try and then recover than to work out if we will fail in advance.

I'll add some comments.

Comment 36

[Matt Woodrow (:mattwoodrow)]

> 
> @@ +1417,4 @@
> >  GLContext*
> >  GLContextProviderGLX::GetGlobalContext(const ContextFlags aFlag)
> >  {
> > +#if 0
> 
> Why do we remove this? IIRC, we need this for creating share groups to allow
> fast compositing for GLContexts.

Because it appears that we're hittin what appears to be a driver bug on the tinderbox machines (amazon ec2 boxes running swrast).

On one of the testsuites that is actually using GL layers (Cipc), we get a shutdown crash when trying to call glXDestroyContext on the shared context.

Since GL layers isn't enabled on linux by default, and none of the testsuites where we force-enable it require sharing, this seemed like a reasonable short term solution to allow us to continue making progress.

Now that I think about it more, we might be better off adding an env var to disable context sharing, and get rel-eng to set this env var when running the Cipc testsuite.

Comment 37

[Nick Cameron [:nrc]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #29)
> Comment on attachment 726908 [details] [diff] [review]
> 2 effects
> 
> Review of attachment 726908 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/Effects.h
> @@ +14,5 @@
> > +namespace layers {
> > +
> > +/**
> > + * Effects and effect chains are used by the compositor API (see Compositor.h).
> > + * An effect chain represents a rendering method, for example some shader and
> 
> Isn't this what an individual effect is? And the effect chain is the set of
> effects to apply when rendering.

I don't think so, although this is a bit confusing. Maybe this is too implementation-centric, but the shader depends on the primary effect and any secondary effects and the parameters to that shader are pulled from more than just one effect.
> 
> @@ +19,5 @@
> > + * the data required for that shader to run. An effect is some component of the
> > + * chain and its data.
> > + *
> > + * An effect chain consists of a primary effect - how the 'texture' memory should
> > + * be interpreted (RGBA, BGRX, YCBCR, etc.) - and any number of secondary effects
> 
> I'm not entirely sure why some of this is actually necessary. Surely the
> TextureHost already knows the byte order (and *needs* to know in some cases,
> like where we do an upload with GL). This causes some confusing code in the
> GL backend, where the texture upload code determines which byte order (by
> deciding which shader program to use), and we basically ignore the effect
> type that was passed in.
> 
> What actual benefit do we get from having separate byte order types rather
> than just EFFECT_TEXTURE?
> 

Bas - can you comment on this?

Comment 38

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 726913 [details] [diff] [review]
5 texture hosts and clients

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

::: gfx/layers/client/TextureClient.cpp
@@ +143,5 @@
> +  nsRefPtr<gfxASurface> surface = pat->GetSurface();
> +  CompositableType type = TextureClient::TypeForImage(aImage);
> +  if (type != BUFFER_IMAGE_SINGLE) {
> +    return type == BUFFER_UNKNOWN;
> +  }

Why not just replace these 4 lines with 'if (!aImage) return false;'

::: gfx/layers/client/TextureClient.h
@@ +53,5 @@
> +  typedef gl::SharedTextureHandle SharedTextureHandle;
> +  typedef gl::GLContext GLContext;
> +  typedef gl::TextureImage TextureImage;
> +
> +  static CompositableType TypeForImage(Image* aImage);

This seems like the wrong place for this function, and the implementation of this seems less than useful.

@@ +92,5 @@
> +  virtual void Unlock() {}
> +
> +  // ensure that the texture client is suitable for the given size and content type
> +  // and that any initialisation has taken place
> +  virtual void EnsureTextureClient(gfx::IntSize aSize, gfxASurface::gfxContentType aType) = 0;

EnsureAllocated()?

@@ +199,5 @@
> + */
> +class AutoLockYCbCrClient : public AutoLockTextureClient
> +{
> +public:
> +  AutoLockYCbCrClient(TextureClient* aTexture) : AutoLockTextureClient(aTexture) {}

This part of the API worries me a bit.

There's nothing here guaranteeing that aTexture is actually a TextureClientShmemYCbCr. In fact, everything would work fine regardless of what the type of aTexture is, except that we might leak on destruction since each TextureClient subclass specifies different logic for releasing the SurfaceDescriptor. This strikes me as confusing and error prone.

It would make more sense to me to get rid of the TextureClient subclasses, and have AutoLock***Client modify the SurfaceDescriptor as well as setting the appropriate destructor callback.

@@ +282,5 @@
> +  }
> +
> +  virtual void SetDescriptor(const SurfaceDescriptor& aDescriptor) MOZ_OVERRIDE
> +  {
> +    MOZ_ASSERT(false, "Tiled texture clients don't use SurfaceDescriptors.");

Shouldn't we remove mDescriptor from TextureClient then, and have an intermediate class for the other types?

As it stands, someone could use an AutoLockYCbCrClient on a TextureClientTile and store YCbCr data into mDescriptor.

::: gfx/layers/composite/TextureHost.h
@@ +203,5 @@
> +  /**
> +   * Leave the texture host in a sane state if we abandon an update part-way
> +   * through.
> +   */
> +  virtual void Abort() {}

This doesn't appear to have any callers. Plus it's not really clear what 'part-way through' an update means, since updating is a single function call.

Comment 39

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 726916 [details] [diff] [review]
8 image stuff

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

I don't know the async video code very well, not sure if I'm the best person to review this.

::: gfx/layers/ImageLayers.h
@@ +57,4 @@
>    const gfxIntSize& GetScaleToSize() { return mScaleToSize; }
>    ScaleMode GetScaleMode() { return mScaleMode; }
>  
> +  void ScaleMatrix(const gfxIntSize& aSourceSize, gfxMatrix& aMatrix)

This doesn't appear to be used anywhere

::: gfx/layers/ipc/ImageContainerParent.h
@@ -24,5 @@
> - * transaction.
> - * All ImageContainerParent's method must be called from the Compositor thread 
> - * only.  
> - */
> -class ImageContainerParent : public PImageContainerParent

I believe ImageContainerParent.cpp should be removed too.

::: gfx/layers/ipc/SharedPlanarYCbCrImage.cpp
@@ +109,5 @@
> +SharedPlanarYCbCrImage::ToSurfaceDescriptor(SurfaceDescriptor& aDesc) {
> +  if (!mAllocated) {
> +    return false;
> +  }
> +  aDesc = YCbCrImage(mShmem, 0, reinterpret_cast<uint64_t>(this));

This is a bit scary :)

Comment 40

[Nick Cameron [:nrc]]

Attachment: 1 The compositor interface

Comment 41

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

Comment on attachment 728012 [details] [diff] [review]
1 The compositor interface

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

Compositor looks a bit like a grab-bag of methods whose relationships are unclear. Can you provide some guidance for how someone would actually *use* one --- e.g. what methods to call and in what order? And what state and invariants, if any, it maintains?

::: gfx/layers/Compositor.h
@@ +162,5 @@
> +
> +  /**
> +   * Set the target for rendering, intended to be used for the duration of a
> +   * transaction. Results will have been written to aTarget by the time that
> +   * EndFrame returns.

Does Compositor know about transactions?

@@ +177,5 @@
> +   *
> +   * Clients of the compositor should call this at the start of the compositing
> +   * process, it might be required by texture uploads etc.
> +   */
> +  virtual void MakeCurrent(bool aForce = false) = 0;

Document the aForce parameter

@@ +198,5 @@
> +  /**
> +   * Creates a Surface that can be used as a rendering target by this
> +   * compositor, and initializes the surface by copying from aSource.
> +   * If aSource is null, then the screen frame in progress
> +   * is used as source.

What is the "screen frame in progress"?

@@ +262,5 @@
> +
> +  // save the current viewport
> +  virtual void SaveViewport() = 0;
> +  // resotre the previous viewport and return its bounds
> +  virtual gfx::IntRect RestoreViewport() = 0;

Use doc-comment format.

What do these viewport methods actually do?

@@ +291,5 @@
> +    NS_ASSERTION(mCompositorID==0, "The compositor ID must be set only once.");
> +    mCompositorID = aID;
> +  }
> +
> +  virtual void NotifyShadowTreeTransaction() = 0;

This needs a comment.

@@ +301,5 @@
> +  /**
> +   * Notify the compositor that composition is being resumed.
> +   * Returns true if succeeded
> +   */
> +  virtual bool Resume() { return true; }

What does it mean for composition to be paused/resumed?

Comment 42

[Nicolas Silva [:nical]]

Comment on attachment 726924 [details] [diff] [review]
15 D3D11 backend

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

::: gfx/layers/d3d11/CompositorD3D11.cpp
@@ +216,5 @@
> +  mDevice->QueryInterface(dxgiDevice.StartAssignment());
> +  dxgiDevice->GetAdapter(getter_AddRefs(dxgiAdapter));
> +
> +#ifdef MOZ_METRO
> +  if (IsRunningInWindowsMetro()) {

Why do we need both the compile-time and run-time test for metro here ?

@@ +374,5 @@
> +    mContext->PSSetShader(mAttachments->mSolidColorShader[aMaskMode], nullptr, 0);
> +    return;
> +  case EFFECT_BGRA:
> +  case EFFECT_RENDER_TARGET:
> +    mContext->PSSetShader(mAttachments->mRGBAShader[aMaskMode], nullptr, 0);

EFFECT_BGRA leads to RGBAShader, shouldn't it be EFFECT_RGBA instead ? There is no case for EFFECT_RGBA. same below with BGRX

@@ +382,5 @@
> +    return;
> +  case EFFECT_YCBCR:
> +    mContext->PSSetShader(mAttachments->mYCbCrShader[aMaskMode], nullptr, 0);
> +    return;
> +  }

What happens for the effect types that are not handled by the switch statement ?
We should at least assert or have a warning.

::: gfx/layers/d3d11/CompositorD3D11.h
@@ +35,5 @@
> +};
> +
> +struct DeviceAttachmentsD3D11;
> +
> +class CompositorD3D11 : public Compositor

It would be nice if the overriding methods would be marked as MOZ_OVERRIDE to make sure we don't run into surprises when small changes are made to the Compositor interface.

@@ +141,5 @@
> +  nsRefPtr<gfxContext> mTarget;
> +
> +  nsIWidget *mWidget;
> +  // XXX - Bas - wth?
> +  nsIntSize mSize;

Looking at how the GL equivalent work, I wonder if mSize is not there to avoid some sort of race conditions maybe due to the widget's size being changed from another thread. in the OGL case mWidgetSize is set in BeginFrame, and GetWidgetSize() returns mWidgetSize.
In D3DCompositor mSize is apparently not used and we always return mWidget's size in GetWidgetSize().
I don't know for sure that the OpenGL backend is doing the right thing (especially with the awful artifacts we get when resizing the window on Linux) but we definitely should pick one strategy and apply it to both GL and D3D.

::: gfx/layers/d3d11/TextureD3D11.cpp
@@ +321,5 @@
> +    mFormat = FORMAT_A8;
> +    dxgiFormat = DXGI_FORMAT_A8_UNORM;
> +    bpp = 1;
> +    break;
> +  }

some gfxImageFormats are not handled in this switch, we should at least assert or place a warning if we run into them.

Comment 43

[Nicolas Silva [:nical]]

(In reply to Jeff Gilbert [:jgilbert] from comment #34)
> @@ +1417,4 @@
> >  GLContext*
> >  GLContextProviderGLX::GetGlobalContext(const ContextFlags aFlag)
> >  {
> > +#if 0
> 
> Why do we remove this? IIRC, we need this for creating share groups to allow
> fast compositing for GLContexts.
> 

I just reenabled this code. Non-OMTC accelerated layers crash without it and WebGL with OMTC on GLX is broken both with and without it anyway :(

Comment 44

[Nicolas Silva [:nical]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #39)
> Comment on attachment 726916 [details] [diff] [review]
> 8 image stuff
> 
> Review of attachment 726916 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/ImageContainerParent.h
> @@ -24,5 @@
> > - * transaction.
> > - * All ImageContainerParent's method must be called from the Compositor thread 
> > - * only.  
> > - */
> > -class ImageContainerParent : public PImageContainerParent
> 
> I believe ImageContainerParent.cpp should be removed too.

I keep removing it and it comes back every time we rebase.

> 
> ::: gfx/layers/ipc/SharedPlanarYCbCrImage.cpp
> @@ +109,5 @@
> > +SharedPlanarYCbCrImage::ToSurfaceDescriptor(SurfaceDescriptor& aDesc) {
> > +  if (!mAllocated) {
> > +    return false;
> > +  }
> > +  aDesc = YCbCrImage(mShmem, 0, reinterpret_cast<uint64_t>(this));
> 
> This is a bit scary :)

Agreed, but most likely less scarry than what we do in m-c at the moment to ensure that the SharedPlanarYCbCrImage is not deleted while its shmem is used on the compositor (cf mediaQueue in ImageContainerChild).

I have plans to fix all this ugliness around the sharing model, but we agreed to wait until the refactoring lands before doing it.

Comment 45

[Nicolas Silva [:nical]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #38)
> Comment on attachment 726913 [details] [diff] [review]
> 5 texture hosts and clients
> 
> Review of attachment 726913 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +199,5 @@
> > + */
> > +class AutoLockYCbCrClient : public AutoLockTextureClient
> > +{
> > +public:
> > +  AutoLockYCbCrClient(TextureClient* aTexture) : AutoLockTextureClient(aTexture) {}
> 
> This part of the API worries me a bit.
> 
> There's nothing here guaranteeing that aTexture is actually a
> TextureClientShmemYCbCr. In fact, everything would work fine regardless of
> what the type of aTexture is, except that we might leak on destruction since
> each TextureClient subclass specifies different logic for releasing the
> SurfaceDescriptor. This strikes me as confusing and error prone.
> 
> It would make more sense to me to get rid of the TextureClient subclasses,
> and have AutoLock***Client modify the SurfaceDescriptor as well as setting
> the appropriate destructor callback.
> 
> @@ +282,5 @@
> > +  }
> > +
> > +  virtual void SetDescriptor(const SurfaceDescriptor& aDescriptor) MOZ_OVERRIDE
> > +  {
> > +    MOZ_ASSERT(false, "Tiled texture clients don't use SurfaceDescriptors.");
> 
> Shouldn't we remove mDescriptor from TextureClient then, and have an
> intermediate class for the other types?
> 
> As it stands, someone could use an AutoLockYCbCrClient on a
> TextureClientTile and store YCbCr data into mDescriptor.

There's some major rework to be done after landing on TextureClient/TextureHost about the sharing model and weird API of TecctureClient. It, among other things, addresses what you mention here. At the moment texture client is the worst part of the refactoring, and it needs to be made better, but fixing it is a significant enough amount of work that we would like to do it after landing.

Comment 46

[Bas Schouten (:bas.schouten)]

Comment on attachment 726910 [details] [diff] [review]
3 compositable hosts and clients

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

As agreed I didn't review this very thoroughly. Let's hope it all goes well.

::: gfx/layers/client/CompositableClient.h
@@ +31,5 @@
> + * Things like opacity or transformation belong to layer and not compositable.
> + *
> + * Since Compositables are independent of layers it is possible to create one,
> + * connect it to the compositor side, and start sending images to it. This alone
> + * is arguably not very useful, but it means that as long as a shdow layer can

nit: shadow

@@ +111,5 @@
> +};
> +
> +/**
> + * IPDL actor used by CompositableClient to match with its corresponding
> + * CompositableHost on the compositor side. 

nit: whitespace

::: gfx/layers/composite/CompositableHost.cpp
@@ +32,5 @@
> +{
> +  TextureSource* source = GetTextureHost();
> +  EffectMask* effect = new EffectMask(source,
> +                                      source->GetSize(),
> +                                      aTransform);

Let's make this a RefPtr for safety if this code is modified.

@@ +102,5 @@
> +}
> +
> +namespace CompositableMap {
> +  typedef std::map<uint64_t, CompositableParent*> Map_t;
> +  static Map_t* sCompositableMap = nullptr;

nit: A more verbose typename would be nice :)

::: gfx/layers/ipc/CompositableTransactionParent.cpp
@@ +104,5 @@
> +      } else {
> +        // if we reach this branch, it most likely means that async textures
> +        // are coming in before we had time to attach the conmpositable to a
> +        // layer. Don't panic, it is okay in this case. it should not be
> +        // happenning continuously, though.

I'm not sure if I like the else with no code in it, I don't see any harm in it either though :-).

@@ +110,5 @@
> +
> +      if (layer) {
> +        RenderTraceInvalidateStart(layer, "FF00FF", layer->GetVisibleRegion().GetBounds());
> +      }
> +      

nit: whitespace

Comment 47

[Bas Schouten (:bas.schouten)]

Comment on attachment 726912 [details] [diff] [review]
4 concreate CompositableHosts

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

Some small whitespace issues in here. I still need to figure out a solution for Accelerated canvas but we can do that later.

BenWa should review the Tiled content host changes to make sure they still work correctly with his traditional assumptions when writing the tiling code.

Comment 48

[Bas Schouten (:bas.schouten)]

Comment on attachment 726913 [details] [diff] [review]
5 texture hosts and clients

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

::: gfx/layers/client/TextureClient.cpp
@@ +207,5 @@
> +  }
> +  return true;
> +}
> +
> +bool AutoLockYCbCrClient::EnsureTextureClient(PlanarYCbCrImage* aImage) {

It's a little weird for this to be in the middle of TextureClientShmem.

::: gfx/layers/client/TextureClient.h
@@ +282,5 @@
> +  }
> +
> +  virtual void SetDescriptor(const SurfaceDescriptor& aDescriptor) MOZ_OVERRIDE
> +  {
> +    MOZ_ASSERT(false, "Tiled texture clients don't use SurfaceDescriptors.");

We have discussed this and I believe nical is going to do this in a follow-up in a more involved way. Where we have several TextureClient subclasses for different access methods.

::: gfx/layers/composite/TextureHost.cpp
@@ +53,5 @@
> +                                                    aTextureFlags);
> +#ifdef MOZ_ENABLE_D3D10_LAYER
> +    case LAYERS_D3D10 : return CreateTextureHostD3D10(aDescriptorType,
> +                                                      aTextureHostFlags,
> +                                                      aTextureFlags);

This doesn't exist, and it won't exist, let's kill it.

::: gfx/layers/ipc/TextureParent.cpp
@@ +25,5 @@
> +  mTextureHost = nullptr;
> +}
> +
> +bool
> +TextureParent::EnsureTextureHost(SurfaceDescriptor::Type aSurfaceType) {

nit: newline

Comment 49

[Bas Schouten (:bas.schouten)]

Comment on attachment 726919 [details] [diff] [review]
10 thebes layers

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

This will take me a little while, the Tiled portion of this should at least get review from BenWa.

Comment 50

[Bas Schouten (:bas.schouten)]

Comment on attachment 726920 [details] [diff] [review]
11 content-side image layers

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

::: gfx/layers/client/ImageClient.cpp
@@ +100,5 @@
> +
> +  if (mLastPaintedImageSerial == image->GetSerial()) {
> +    return true;
> +  }
> +

This function could use a couple of newlines here and there.

Comment 51

[Bas Schouten (:bas.schouten)]

Comment on attachment 726925 [details] [diff] [review]
16 changes to D3D9 and D3D10 backends

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

Please just delete things instead of commenting them out :-)

Comment 52

[Benoit Girard (:BenWa)]

Comment on attachment 726927 [details] [diff] [review]
17 compositor-side tiled layers

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

As long as all this code has been migrated that's fine.

Comment 53

[Benoit Girard (:BenWa)]

Comment on attachment 726912 [details] [diff] [review]
4 concreate CompositableHosts

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

The rest looks fine. Is there a good reason to not migrate the memory pressure bits?

::: gfx/layers/composite/ContentHost.cpp
@@ +396,5 @@
> +
> +void
> +TiledContentHost::MemoryPressure()
> +{
> +}

We shouldn't be regressing the memory pressure functionality that tiled layers had.

Comment 54

[Benoit Girard (:BenWa)]

Comment on attachment 726923 [details] [diff] [review]
14 IPC/IPDL stuff

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +534,5 @@
>              ref->DetachReferentLayer(referent);
>              referent->RemoveUserData(&sPanZoomUserDataKey);
>            }
> +        } else {
> +          ref->DetachReferentLayer(referent);

I have no idea what's going on here. Maybe we need a comment to explain this?

::: gfx/layers/ipc/CompositorUtils.ipdlh
@@ +5,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +
> +struct _TextureFactoryIdentifier_todo

Remove

::: gfx/layers/ipc/ShadowLayerChild.cpp
@@ +8,4 @@
>  #include "ShadowLayerChild.h"
>  #include "Layers.h"
>  #include "ShadowLayers.h"
> +#include "mozilla/layers/TextureChild.h"

Is this really needed? I see no other changes.

::: gfx/layers/ipc/ShadowLayerParent.cpp
@@ +11,5 @@
>  
>  #include "BasicLayers.h"
>  
> +#include "mozilla/layers/TextureParent.h"
> +#include "LayerManagerComposite.h"

Is this really needed? I see no other changes.

::: gfx/layers/ipc/ShadowLayers.cpp
@@ +299,3 @@
>  {
> +  MOZ_ASSERT(aImage.type() != SurfaceDescriptor::T__None, "[debug] STOP");
> +  MOZ_ASSERT(aImage.type() != SurfaceDescriptor::Tnull_t, "[debug] STOP");

These messages don't add value.

::: gfx/layers/ipc/ShadowLayers.h
@@ +421,4 @@
>  
>    virtual void GetBackendName(nsAString& name) { name.AssignLiteral("Shadow"); }
>  
> +/*

Remove

::: gfx/layers/ipc/ShadowLayersParent.cpp
@@ -248,4 @@
>        layer->SetPostScale(common.postXScale(), common.postYScale());
>        layer->SetIsFixedPosition(common.isFixedPosition());
>        layer->SetFixedPositionAnchor(common.fixedPositionAnchor());
> -      layer->SetFixedPositionMargins(common.fixedPositionMargin());

Why is this no longer required?

@@ -313,4 @@
>          ImageLayer* imageLayer = static_cast<ImageLayer*>(layer);
>          const ImageLayerAttributes& attrs = specific.get_ImageLayerAttributes();
>          imageLayer->SetFilter(attrs.filter());
> -        imageLayer->SetForceSingleTile(attrs.forceSingleTile());

Why is this no longer required?

::: gfx/layers/ipc/ShadowLayersParent.h
@@ +67,5 @@
>  
>  protected:
>    virtual bool RecvUpdate(const EditArray& cset,
>                            const TargetConfig& targetConfig,
> +                        const bool& isFirstPaint,

nit: indent

::: ipc/glue/IPCMessageUtils.h
@@ +133,4 @@
>    typedef E paramType;
>  
>    static bool IsLegalValue(const paramType &aValue) {
> +    return smallestLegal <= aValue && aValue <= highBound;

A lot of code is using highBound as len/count. This change will break code such as:
http://mxr.mozilla.org/mozilla-central/source/hal/HalTypes.h#199

::: ipc/ipdl/ipdl/type.py
@@ +306,4 @@
>              if mgr is not self:
>                  return mgr.toplevel()
>  
> +    def toplevels(self):

The changes to ipc/ipdl/**.py need their own bug. I don't understand the motivation and this is outside the gfx component.

Comment 55

[Benoit Girard (:BenWa)]

Comment on attachment 726919 [details] [diff] [review]
10 thebes layers

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

Tiled code looks good to me

::: gfx/layers/client/ContentClient.cpp
@@ +501,5 @@
> +  ClearPaintedRegion();
> +}
> +
> +void
> +BasicTiledLayerBuffer::PaintThebes(const nsIntRegion& aNewValidRegion,

The tiling code should be in it's own file.

Comment 56

[Bas Schouten (:bas.schouten)]

Comment on attachment 726915 [details] [diff] [review]
7 composite layers

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

There's a bunch of whitespace issues in here, I won't point them all out but we should fix them :).

I was once again quite lenient in the review process. We all understand we need to get this in, there's probably things in here we should improve over time though.

::: gfx/layers/composite/ColorLayerComposite.cpp
@@ +24,5 @@
> +
> +  LayerManagerComposite::AddMaskEffect(aLayer->GetMaskLayer(), effects);
> +
> +  gfx::Rect rect(visibleRect.x, visibleRect.y, visibleRect.width, visibleRect.height);
> +  float opacity = aLayer->GetEffectiveOpacity();

A newline here would be nice

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +43,5 @@
> +  nsIntRect cachedScissor = aClipRect;
> +  aContainer->mSupportsComponentAlphaChildren = false;
> +
> +  float opacity = aContainer->GetEffectiveOpacity();
> +  bool needsSurface = aContainer->UseIntermediateSurface();

This piece could again do with some newlines.

@@ +115,5 @@
> +      continue;
> +    }
> +
> +    nsIntRect scissorRect = layerToRender->GetLayer()->
> +        CalculateScissorRect(cachedScissor, &aManager->GetWorldTransform());

We might want to call this clipRect instead of scissorRect and scissorRect is the name of the common implementation, not what it is here.

::: gfx/layers/composite/ContainerLayerComposite.h
@@ +22,5 @@
> +class ContainerLayerComposite : public ShadowContainerLayer,
> +                                public LayerComposite
> +{
> +template<class ContainerT>
> +friend void ContainerRender(ContainerT* aContainer,

indent

@@ +69,5 @@
> +class RefLayerComposite : public ShadowRefLayer,
> +                          public LayerComposite
> +{
> +template<class ContainerT>
> +friend void ContainerRender(ContainerT* aContainer,

indent

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +168,5 @@
> +
> +already_AddRefed<gfxASurface>
> +LayerManagerComposite::CreateOptimalMaskSurface(const gfxIntSize &aSize)
> +{
> +  NS_ERROR("Should only be called on the drawing side");

We should probably make these NS_RUNTIMEABORT

@@ +416,5 @@
> +                       rootMetrics.mCompositionBounds.height);
> +
> +  float lowPrecisionMultiplier = 1.0f;
> +  float highPrecisionMultiplier = 1.0f;
> +#ifdef MOZ_ANDROID_OMTC

Will we be wanting this code on other OMTC platforms as well?

@@ +583,5 @@
> +                         aSize.width > 64 && aSize.height > 64 &&
> +                         gfxPlatformMac::GetPlatform()->UseAcceleratedCanvas();
> +  if (useAcceleration) {
> +    return Factory::CreateDrawTarget(BACKEND_COREGRAPHICS_ACCELERATED,
> +                                     aSize, aFormat);

This will most likely need some work as well, just like D2D accelerated canvas, with the move we've made to TextureClient/Host

::: gfx/layers/composite/LayerManagerComposite.h
@@ +310,5 @@
> +  }
> +
> +protected:
> +  LayerManagerComposite* mCompositeManager;
> +  Compositor* mCompositor;

This could be a strong reference probably? As Compositors don't hold on the Layers. I don't feel very strongly about this though. But it might be a little cleaner if we allow situations where layers outlive their layermanagers.

Comment 57

[Bas Schouten (:bas.schouten)]

Comment on attachment 726919 [details] [diff] [review]
10 thebes layers

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

This will take me a little while, the Tiled portion of this should at least get review from BenWa.

Also I'm reviewing a lot of code here I wrote myself, nical, can you have a quick look as well?

I guess it'll have to do, but this code in particular does not make me superhappy :-).

::: gfx/layers/ThebesLayerBuffer.cpp
@@ +291,4 @@
>  gfxASurface::gfxContentType
>  ThebesLayerBuffer::BufferContentType()
>  {
> +  if (mBuffer)

curly braces!

::: gfx/layers/client/ContentClient.cpp
@@ +107,5 @@
> +ContentClientBasic::CreateDTBuffer(ContentType aType,
> +                                 const nsIntRect& aRect,
> +                                 uint32_t aFlags)
> +{
> +#if 0

Let's replace this by an NS_RUNTIMEABORT("ContentClientBasic does not support Moz2D drawing!");

@@ +147,5 @@
> +void
> +ContentClientRemote::BeginPaint()
> +{
> +  // WOAH! Crazy! So we might not have a TextureClient yet.. because it will
> +  // only be created by CreateBuffer.. which will deliver a locked surface!.

I think this is pretty crazy! We should make this into a TODO for after landing to make this situation of surface allocation and such a whole lot more intuitive and better documented.

Comment 58

[Nick Cameron [:nrc]]

(In reply to Bas Schouten (:bas.schouten) from comment #57)
> I guess it'll have to do, but this code in particular does not make me
> superhappy :-).

What precisely doesn't make you super happy? (Other than the bits with 'Woah!' comments). Not trying to disagree, just because it is a big patch :-)

Comment 59

[Nick Cameron [:nrc]]

(In reply to Benoit Girard (:BenWa) from comment #53)
> Comment on attachment 726912 [details] [diff] [review]
> 4 concreate CompositableHosts
> 
> Review of attachment 726912 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The rest looks fine. Is there a good reason to not migrate the memory
> pressure bits?
> 
> ::: gfx/layers/composite/ContentHost.cpp
> @@ +396,5 @@
> > +
> > +void
> > +TiledContentHost::MemoryPressure()
> > +{
> > +}
> 
> We shouldn't be regressing the memory pressure functionality that tiled
> layers had.

Jeff removed the reusable tile store (I assume he ran that by you), and MemoryPressure only deleted that, so now it does nothing.

Comment 60

[Nick Cameron [:nrc]]

(In reply to Nicolas Silva [:nical] from comment #42)
> Comment on attachment 726924 [details] [diff] [review]
> 15 D3D11 backend
> 
> Review of attachment 726924 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +141,5 @@
> > +  nsRefPtr<gfxContext> mTarget;
> > +
> > +  nsIWidget *mWidget;
> > +  // XXX - Bas - wth?
> > +  nsIntSize mSize;
> 
> Looking at how the GL equivalent work, I wonder if mSize is not there to
> avoid some sort of race conditions maybe due to the widget's size being
> changed from another thread. in the OGL case mWidgetSize is set in
> BeginFrame, and GetWidgetSize() returns mWidgetSize.
> In D3DCompositor mSize is apparently not used and we always return mWidget's
> size in GetWidgetSize().
> I don't know for sure that the OpenGL backend is doing the right thing
> (especially with the awful artifacts we get when resizing the window on
> Linux) but we definitely should pick one strategy and apply it to both GL
> and D3D.

As far as I can tell, this is here only because GetWidgetSize returns a pointer to an IntSize and we need to avoid a dangling pointer.

I think OGL needs to be a bit different to support the case on Android where we rotate the screen (although that uses SetDestinationSurfaceSize, not the widget size, so maybe not).

Comment 61

[Nick Cameron [:nrc]]

(In reply to Bas Schouten (:bas.schouten) from comment #56)
> Comment on attachment 726915 [details] [diff] [review]
> 7 composite layers
> 
> Review of attachment 726915 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There's a bunch of whitespace issues in here, I won't point them all out but
> we should fix them :).
> 
> I was once again quite lenient in the review process. We all understand we
> need to get this in, there's probably things in here we should improve over
> time though.
> 

> @@ +416,5 @@
> > +                       rootMetrics.mCompositionBounds.height);
> > +
> > +  float lowPrecisionMultiplier = 1.0f;
> > +  float highPrecisionMultiplier = 1.0f;
> > +#ifdef MOZ_ANDROID_OMTC
> 
> Will we be wanting this code on other OMTC platforms as well?
> 

Eventually, yes, but for the moment it relies on stuff implemented in Java specifically for Android, so I think it should be ifdef'ed like this. It is taken as is from the pre-refactoring code and is beyond the scope of the refactoring.

Comment 62

[Nick Cameron [:nrc]]

(In reply to Benoit Girard (:BenWa) from comment #54)
> Comment on attachment 726923 [details] [diff] [review]
> 14 IPC/IPDL stuff
> 
> Review of attachment 726923 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ -313,4 @@
> >          ImageLayer* imageLayer = static_cast<ImageLayer*>(layer);
> >          const ImageLayerAttributes& attrs = specific.get_ImageLayerAttributes();
> >          imageLayer->SetFilter(attrs.filter());
> > -        imageLayer->SetForceSingleTile(attrs.forceSingleTile());
> 
> Why is this no longer required?
> 

It has been changed to a flag on the texture client/host.

Comment 63

[Nick Cameron [:nrc]]

Attachment: 1 The compositor interface

Comment 64

[Nick Cameron [:nrc]]

Attachment: 4 concrete CompositableHosts

Carrying r=Bas, re-requesting r=BenWa

Comment 65

[Nick Cameron [:nrc]]

Attachment: 5 texture hosts and clients

Comment 66

[Nick Cameron [:nrc]]

Attachment: 8 image stuff

requesting re-review from Matt.

Nical - can you nominate someone who knows about image stuff for a review here too please?

Comment 67

[Nick Cameron [:nrc]]

Attachment: 9 GL Layers and compositor

Comment 68

[Nick Cameron [:nrc]]

Attachment: 14 IPC stuff

Comment 69

[Nick Cameron [:nrc]]

Attachment: 23 changes to IPDL implementation

Comment 70

[Nick Cameron [:nrc]]

Attachment: 16 changes to D3D9 and D3D10 backends

Comment 71

[Bas Schouten (:bas.schouten)]

Comment on attachment 731446 [details] [diff] [review]
16 changes to D3D9 and D3D10 backends

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

::: gfx/layers/d3d10/LayerManagerD3D10.cpp
@@ +743,5 @@
>  
>    if (mTarget) {
>      PaintToTarget();
>    } else if (mBackBuffer) {
> +    // XXX need to tear all this out really

Maybe we should? :)

Comment 72

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 731441 [details] [diff] [review]
8 image stuff

I'll let someone else review the rest of this, I don't know it well enough.

Comment 73

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

Comment on attachment 731434 [details] [diff] [review]
1 The compositor interface

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

::: gfx/layers/Compositor.h
@@ +132,5 @@
> + * compositor. In practice, we use one compositor per window.
> + *
> + * # Usage
> + *
> + * For an example of a user of Compositor, see LayerManagerComposite

Final full stop.

@@ +212,5 @@
> +   *
> +   * If aForce is true, then we will (re-)set our context on the underlying API
> +   * even if it is already the current context.
> +   */
> +  virtual void MakeCurrent(bool aForce = false) = 0;

Use a flags word with a single named flag instead of this boolean parameter.

@@ +254,5 @@
> +  /**
> +   * Mostly the compositor will pull the size from a widget and this method will
> +   * be ignored, but compositor implementations are free to use it if they like.
> +   */
> +  virtual void SetDestinationSurfaceSize(int aWidth, int aHeight) = 0;

pass a gfx::IntSize.

@@ +275,5 @@
> +   */
> +  virtual void BeginFrame(const gfx::Rect *aClipRectIn,
> +                          const gfxMatrix& aTransform,
> +                          const gfx::Rect& aRenderBounds,
> +                          gfx::Rect *aClipRectOut = nullptr) = 0;

What do these parameters mean?

@@ +280,5 @@
> +
> +  /**
> +   * Flush the current frame to the screen and tidy up.
> +   */
> +  virtual void EndFrame(const gfxMatrix& aTransform) = 0;

What does the parameter mean?

@@ +286,5 @@
> +  /**
> +   * Post-rendering stuff if the rendering is done outside of this Compositor
> +   * e.g., by Composer2D
> +   */
> +  virtual void EndFrameForExternalComposition(const gfxMatrix& aTransform) = 0;

What does the parameter mean?

@@ +298,5 @@
> +   * Setup the viewport and projection matrix for rendering
> +   * to a window of the given dimensions.
> +   */
> +  virtual void PrepareViewport(int aWidth, int aHeight,
> +                               const gfxMatrix& aWorldTransform) = 0;

Pass a gfx::IntSize. And explain how the aWorldTransform is used.

::: gfx/layers/CompositorTypes.h
@@ +22,5 @@
> +const TextureFlags AllowRepeat        = 0x10;
> +// the texture represents a tile which is newly created
> +const TextureFlags NewTile            = 0x20;
> +// The host is responsible for tidying up any shared resources
> +const TextureFlags HostRelease        = 0x40;

These all need to be documented properly.

Comment 74

[Nick Cameron [:nrc]]

Attachment: 1 The compositor interface

Comment 75

[Nick Cameron [:nrc]]

Attachment: 16 changes to D3D9 and D3D10 backends

Comment 76

[Nick Cameron [:nrc]]

Comment on attachment 726914 [details] [diff] [review]
6 changes to layers

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

r=me with the nits addressed

::: gfx/layers/RenderTrace.h
@@ +25,5 @@
>  
> +/**
> + * TODO: document these funtions!
> + */
> +

Please do this.

::: gfx/layers/basic/BasicLayerManager.cpp
@@ -876,5 @@
> -  // Don't attempt to paint layers with a singular transform, cairo will
> -  // just throw an error.
> -  if (aLayer->GetEffectiveTransform().IsSingular()) {
> -    return;
> -  }

Why has this gone?

@@ +1182,4 @@
>          nsRefPtr<gfxASurface> target = mShadowTarget->OriginalSurface();
>          SurfaceDescriptor inSnapshot, snapshot;
> +        if (AllocSurfaceDescriptor(size, contentType,
> +                                   &inSnapshot) &&

nit: this will all fit on one line

Comment 77

[Nick Cameron [:nrc]]

Comment on attachment 726924 [details] [diff] [review]
15 D3D11 backend

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

::: gfx/layers/d3d11/CompositorD3D11.cpp
@@ +30,5 @@
> +    float position[2];
> +};
> +
> +// {1E4D7BEB-D8EC-4A0B-BF0A-63E6DE129425}
> +static const GUID sDeviceAttachmentsD3D11 = 

whitespace

@@ +33,5 @@
> +// {1E4D7BEB-D8EC-4A0B-BF0A-63E6DE129425}
> +static const GUID sDeviceAttachmentsD3D11 = 
> +{ 0x1e4d7beb, 0xd8ec, 0x4a0b, { 0xbf, 0xa, 0x63, 0xe6, 0xde, 0x12, 0x94, 0x25 } };
> +// {88041664-C835-4AA8-ACB8-7EC832357ED8}
> +static const GUID sLayerManagerCount = 

whitespace

rename to sCompositorCount

@@ +77,5 @@
> +    if (!referenceCount) {
> +      DeviceAttachmentsD3D11 *attachments;
> +      size = sizeof(attachments);
> +      mDevice->GetPrivateData(sDeviceAttachmentsD3D11, &size, &attachments);
> +      // No LayerManagers left for this device. Clear out interfaces stored which

s/LayerManagers/Compositors

@@ +127,5 @@
> +                                    sizeof(layout) / sizeof(D3D11_INPUT_ELEMENT_DESC),
> +                                    LayerQuadVS,
> +                                    sizeof(LayerQuadVS),
> +                                    byRef(mAttachments->mInputLayout));
> +    

whitespace

@@ +229,5 @@
> +    swapDesc.Width = rect.width;
> +    swapDesc.Height = rect.height;
> +    // This is the most common swapchain format
> +    swapDesc.Format = DXGI_FORMAT_B8G8R8A8_UNORM;
> +    swapDesc.Stereo = false; 

whitespace

@@ +320,5 @@
> +  if (aSize.width > maxTextureSize || aSize.height > maxTextureSize) {
> +    return false;
> +  }
> +
> +  return true;

return aSize.width <= maxTextureSize && aSize.height <= maxTextureSize;

@@ +385,5 @@
> +    return;
> +  }
> +}
> +
> +// TODO[Bas] this method doesn't actually use aSource

Please address this

@@ +417,5 @@
> +
> +  bool isPremultiplied = true;
> +
> +  MaskMode maskMode = UNMASKED;
> +  

whitespace

@@ +524,5 @@
> +  }
> +}
> +
> +void
> +CompositorD3D11::BeginFrame(const gfx::Rect *aClipRectIn, const gfxMatrix& aTransform,

Is it right to ignore aClipRectIn and aRenderBounds here? And aTransform actually?

@@ +648,5 @@
> +    return;
> +  }
> +
> +  mDefaultRT = nullptr;
> +  if (gfxWindowsPlatform::IsOptimus()) { 

whitespace

@@ +673,5 @@
> +
> +  HRESULT hr;
> +
> +  nsRefPtr<ID3D11Texture2D> backBuf;
> +  

whitespace

@@ +768,5 @@
> +void
> +CompositorD3D11::PaintToTarget()
> +{
> +  nsRefPtr<ID3D11Texture2D> backBuf;
> +  

whitespace

::: gfx/layers/d3d11/CompositorD3D11.fx
@@ +98,5 @@
> +float4 TransformedPostion(float2 aInPosition)
> +{
> +  // the current vertex's position on the quad
> +  float4 position = float4(0, 0, 0, 1);
> +  

whitespace

@@ +121,5 @@
> +  result.w = aTransformedPosition.w;
> +  result.xyz = aTransformedPosition.xyz / aTransformedPosition.w;
> +  result -= vRenderTargetOffset;
> +  result.xyz *= result.w;
> +  

whitespace

::: gfx/layers/d3d11/CompositorD3D11.h
@@ +72,5 @@
> +                        gfx::Float aOpacity, const gfx::Matrix4x4 &aTransform,
> +                        const gfx::Point &aOffset);
> +
> +  /**
> +   * Start a new frame. If aClipRectIn is null, sets *aClipRectOut to the screen dimensions. 

whitespace

@@ +75,5 @@
> +  /**
> +   * Start a new frame. If aClipRectIn is null, sets *aClipRectOut to the screen dimensions. 
> +   */
> +  virtual void BeginFrame(const gfx::Rect *aClipRectIn, const gfxMatrix& aTransform,
> +    const gfx::Rect& aRenderBounds, gfx::Rect *aClipRectOut = nullptr);

indent

@@ +116,5 @@
> +
> +  virtual ID3D11Device *GetDevice() { return mDevice; }
> +
> +private:
> +  enum MaskMode {

Use MaskType from LayerTypes.h, move it to CompositorTypes.h if you don't want to include layers stuff here.

@@ +128,5 @@
> +  bool CreateShaders();
> +  void UpdateConstantBuffers();
> +  void SetSamplerForFilter(gfx::Filter aFilter);
> +  void SetPSForEffect(Effect *aEffect, MaskMode aMaskMode);
> +  void PaintToTarget();

Please document these methods

@@ +134,5 @@
> +  RefPtr<ID3D11DeviceContext> mContext;
> +  RefPtr<ID3D11Device> mDevice;
> +  RefPtr<IDXGISwapChain> mSwapChain;
> +  RefPtr<ID3D11RenderTargetView> mDefaultRT;
> +  

Whitespace

::: gfx/layers/d3d11/TextureD3D11.cpp
@@ +145,5 @@
> +void
> +TextureClientD3D11::Unlock()
> +{
> +  // TODO - Things seem to believe they can hold on to our surface... well...
> +  // They shouldn't!!

Please address

@@ +227,5 @@
> +void
> +TextureClientD3D11::ReleaseTexture()
> +{
> +  // TODO - Bas - We seem to have places that unlock without ever having locked,
> +  // that's kind of bad.

Please address

@@ +229,5 @@
> +{
> +  // TODO - Bas - We seem to have places that unlock without ever having locked,
> +  // that's kind of bad.
> +  if (!mIsLocked) {
> +    return;

should we warn here?

@@ +275,5 @@
> +  IntRect rect = GetTileRect(mCurrentTile);
> +  return nsIntRect(rect.x, rect.y, rect.width, rect.height);
> +}
> +
> +uint32_t GetRequiredTiles(uint32_t aSize, uint32_t aMaxSize)

Calling this Get* seems misleading. Please make static.

@@ +297,5 @@
> +{
> +  MOZ_ASSERT(aImage.type() == SurfaceDescriptor::TShmem);
> +
> +  AutoOpenSurface openSurf(OPEN_READ_ONLY, aImage);
> +  

whitespace

@@ +324,5 @@
> +    break;
> +  }
> +
> +  mSize = IntSize(size.width, size.height);
> +      

whitespace

@@ +339,5 @@
> +    mIsTiled = false;
> +  } else {
> +    mIsTiled = true;
> +    uint32_t tileCount = GetRequiredTiles(size.width, maxSize) *
> +                          GetRequiredTiles(size.height, maxSize);

indent

@@ +404,5 @@
> +void
> +TextureHostDXGID3D11::UpdateImpl(const SurfaceDescriptor& aImage, nsIntRegion *aRegion)
> +{
> +  MOZ_ASSERT(aImage.type() == SurfaceDescriptor::TSurfaceDescriptorD3D10);
> +  

whitespace

@@ +457,5 @@
> +
> +  gfxIntSize gfxCbCrSize = shmemImage.GetCbCrSize();
> +
> +  gfxIntSize size = shmemImage.GetYSize();
> +  

whitespace

@@ +463,5 @@
> +  initData.pSysMem = shmemImage.GetYData();
> +  initData.SysMemPitch = shmemImage.GetYStride();
> +
> +  CD3D11_TEXTURE2D_DESC desc(DXGI_FORMAT_R8_UNORM, size.width, size.height,
> +                              1, 1, D3D11_BIND_SHADER_RESOURCE, D3D11_USAGE_IMMUTABLE);

indent

::: gfx/layers/d3d11/TextureD3D11.h
@@ +107,5 @@
> +  virtual void SetCompositor(Compositor* aCompositor) MOZ_OVERRIDE;
> +
> +  virtual TextureSourceD3D11* AsSourceD3D11() MOZ_OVERRIDE { return this; }
> +
> +  virtual ID3D11Texture2D *GetD3D11Texture() MOZ_OVERRIDE {

opening brace on new line in lots of places

@@ +151,5 @@
> +  uint32_t mCurrentTile;
> +  bool mIterating;
> +};
> +
> +class TextureHostDXGID3D11 : public TextureHost

Please document when this can/should be used (as opposed to the shmem version)

@@ +184,5 @@
> +private:
> +  void LockTexture();
> +  void ReleaseTexture();
> +
> +  gfx::IntRect GetTileRect(uint32_t aID) const; // TODO[Bas] not defined anywhere?

Has this been addressed?

Comment 78

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

Comment on attachment 732061 [details] [diff] [review]
1 The compositor interface

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

::: gfx/layers/Compositor.h
@@ +216,5 @@
> +   *
> +   * If aFlags == CURRENT_FORCE then we will (re-)set our context on the
> +   * underlying API even if it is already the current context.
> +   */
> +  virtual void MakeCurrent(MakeCurrentFlags aFlags = CURRENT_NOFLAGS) = 0;

Just use 0. No need for a NOFLAGS.

@@ +307,5 @@
> +  /**
> +   * Setup the viewport and projection matrix for rendering
> +   * to a window of the given dimensions.
> +   * aWorldTransform is the transform from user space to the new viewport's
> +   * coordinate space.

This is still unclear. Why would anyone call this? What does it do that BeginFrame doesn't do? We have at least three coordinate systems in play here and they should all be documented somewhere.

::: gfx/layers/CompositorTypes.h
@@ +11,5 @@
> +
> +typedef int32_t SurfaceDescriptorType;
> +static const int32_t SURFACEDESCRIPTOR_UNKNOWN = 0;
> +
> +// flags used by texture clients and texture hosts

Used how? Are these flags passed from client to host, or from host to client or set by the Compositable, or what?

Comment 79

[Nick Cameron [:nrc]]

Attachment: 1 The compositor interface

Comment 80

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

Comment on attachment 732152 [details] [diff] [review]
1 The compositor interface

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

::: gfx/layers/CompositorTypes.h
@@ +18,5 @@
> + * by the compositableCient, they may be modified by either the compositable or
> + * texture clients.
> + */
> +typedef uint32_t TextureFlags;
> +const TextureFlags NoFlags            = 0x0;

We shouldn't need this. NoFlags is always 0 in any sane design.

Comment 81

[Bas Schouten (:bas.schouten)]

Comment on attachment 732112 [details] [diff] [review]
16 changes to D3D9 and D3D10 backends

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

::: gfx/layers/d3d10/LayerManagerD3D10.cpp
@@ +848,5 @@
>      effect()->GetVariableByName("tMask")->AsShaderResource()->SetResource(maskSRV);
>      return SHADER_MASK;
>    }
>  
>    return SHADER_NO_MASK; 

whitespace

Comment 82

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 731445 [details] [diff] [review]
23 changes to IPDL implementation

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

Looks good to me!

Comment 83

[Nicolas Silva [:nical]]

Comment on attachment 726919 [details] [diff] [review]
10 thebes layers

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

We still have TODO comments in there to fix.

::: gfx/layers/ThebesLayerBuffer.cpp
@@ +194,5 @@
> +{
> +  SAMPLE_LABEL("RotatedBuffer", "DrawBufferWithRotation");
> +  // See above, in Azure Repeat should always be a safe, even faster choice
> +  // though! Particularly on D2D Repeat should be a lot faster, need to look
> +  // into that. TODO[Bas]

TODO

@@ +556,5 @@
>  
>    gfxUtils::ClipToRegionSnapped(result.mContext, result.mRegionToDraw);
>    if (contentType == gfxASurface::CONTENT_COLOR_ALPHA && !isClear) {
> +    // TODO[Bas] - this is potentially slow for D2D! In reality we can do this with
> +    // a series of Azure ClearRects, I should make this so!

TODO

::: gfx/layers/client/ContentClient.cpp
@@ +123,5 @@
> +  }
> +  return referenceSurface->CreateSimilarSurface(
> +    aType, gfxIntSize(aSize.width, aSize.height));
> +#endif
> +  // TODO[Bas] - Implement me!?

There are still TODO's in various places. We should either fix them now or file bugs for them and place links to the bugs in the TODO comment.

::: gfx/layers/client/ContentClient.h
@@ +187,5 @@
> +  }
> +  virtual const nsIntPoint& BufferRotation()
> +  {
> +    return ThebesLayerBuffer::BufferRotation();
> +  }

Why do we overload BufferRect and BufferRotation (with a slightly different signature, the originals are const) and just call ThebesLayerBuffer's implementation here?
It seems a bit dangerous to me because if we override them in a base class (I suppose it's the intention behind making them virtual), and we call, say, BufferRect on a const instance, it's ThebesLayerBuffer::BufferRect that is going to be called when we'd expect polymorphism to operate. Anyway, I don't see any place where we override them so this seems useless to me.

Comment 84

[Nicolas Silva [:nical]]

Comment on attachment 726915 [details] [diff] [review]
7 composite layers

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

I'd like that we use MOZ_OVERRIDE as much as possible. It makes it less likely to make mistakes when changing interfaces, and provides some useful information about the interface which is nice when reading/reviewing the code.

::: gfx/layers/composite/ImageLayerComposite.h
@@ +39,5 @@
> +  virtual void Disconnect();
> +
> +  virtual void SetCompositableHost(CompositableHost* aHost) MOZ_OVERRIDE;
> +
> +  // LayerOGL impl

OGL ?

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +162,5 @@
> +
> +#ifdef MOZ_LAYERS_HAVE_LOG
> +  Log();
> +  MOZ_LAYERS_LOG(("]----- EndTransaction"));
> +#endif

Is it worth keeping all the logging that we added in m-c or was it mostly useful to fix specific bugs and we're not going to use them anymore (open question)?

Comment 85

[Nicolas Silva [:nical]]

Comment on attachment 726924 [details] [diff] [review]
15 D3D11 backend

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

There are still some comments in here that have not been answered/addressed. Some of them should be fixed before we land and they are easy to fix.

Comment 86

[Nick Cameron [:nrc]]

In reply to my own comments:

(In reply to Nick Cameron [:nrc] from comment #76)
> Comment on attachment 726914 [details] [diff] [review]
> 6 changes to layers
> 
> Review of attachment 726914 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the nits addressed
> 
> ::: gfx/layers/RenderTrace.h
> @@ +25,5 @@
> >  
> > +/**
> > + * TODO: document these funtions!
> > + */
> > +
> 
> Please do this.
> 

Or just remove the comment since the functions are not part of the refactoring.

> ::: gfx/layers/basic/BasicLayerManager.cpp
> @@ -876,5 @@
> > -  // Don't attempt to paint layers with a singular transform, cairo will
> > -  // just throw an error.
> > -  if (aLayer->GetEffectiveTransform().IsSingular()) {
> > -    return;
> > -  }
> 
> Why has this gone?
> 

It seems to have come back.

> @@ +1182,4 @@
> >          nsRefPtr<gfxASurface> target = mShadowTarget->OriginalSurface();
> >          SurfaceDescriptor inSnapshot, snapshot;
> > +        if (AllocSurfaceDescriptor(size, contentType,
> > +                                   &inSnapshot) &&
> 
> nit: this will all fit on one line

Not any more, it has changed.

Comment 87

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Jeff Gilbert [:jgilbert] from comment #34)

> @@ +465,5 @@
> >  #if defined(MOZ_WIDGET_GONK)
> >          fActiveTexture(LOCAL_GL_TEXTURE0);
> > +        // FIXME [bjacob] There is a bug here: GL_TEXTURE_2D here is incompatible
> > +        // with GL_TEXTURE_EXTERNAL in BindExternalBuffer. Specifically, binding a texture to
> > +        // two different texture targets in a GL_INVALID_OPERATION.
> 
> Ugh, we didn't manage a fix for this yet? Can we not create an EGLImage from
> a 1x1/2x2 texture, and then attach that as a TEXTURE_EXTERNAL?

We do have a fix for that: we're not calling that function anymore in gfx/layers code.

I didn't want to touch that function to avoid regressing any other code that is using it and may be relying on bug-for-bug compatibility.


> @@ +832,5 @@
> >  
> >      ~GLContextGLX()
> >      {
> > +        MakeCurrent();
> > +        fFinish();
> 
> Why do we Finish here?

I don't see this Finish() in either the current graphics branch or mozilla-central. If it's in a specific revision I would hg blame it to figure out. This looks unnecessary.

> 
> @@ +848,5 @@
> >          if (mDeleteDrawable) {
> >              mGLX->xDestroyPixmap(mDisplay, mDrawable);
> >          }
> > +        
> > +        mGLX->xDestroyContext(mDisplay, mContext);
> 
> Destroying the context's backbuffer before destroying the context itself is
> *probably* fine, but more obtuse than destroying them in the more usual
> order.

I agree we should destroy the context first, as the context references the surface and not the other way around; that is actually how it's done in the current graphics branch so I assume that this is an earlier revision; more generally a good reference for how these things should be done is toolkit/xre/glxtest.cpp.

Comment 88

[Nick Cameron [:nrc]]

(In reply to Nicolas Silva [:nical] from comment #83)
> Comment on attachment 726919 [details] [diff] [review]
> 10 thebes layers
> 
> Review of attachment 726919 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: gfx/layers/client/ContentClient.h
> @@ +187,5 @@
> > +  }
> > +  virtual const nsIntPoint& BufferRotation()
> > +  {
> > +    return ThebesLayerBuffer::BufferRotation();
> > +  }
> 
> Why do we overload BufferRect and BufferRotation (with a slightly different
> signature, the originals are const) and just call ThebesLayerBuffer's
> implementation here?
> It seems a bit dangerous to me because if we override them in a base class
> (I suppose it's the intention behind making them virtual), and we call, say,
> BufferRect on a const instance, it's ThebesLayerBuffer::BufferRect that is
> going to be called when we'd expect polymorphism to operate. Anyway, I don't
> see any place where we override them so this seems useless to me.

They are there because in the super class they are protected and here they are public. Good catch on the const-ness though, that was a mistake.

Comment 89

[Matt Woodrow (:mattwoodrow)]

(In reply to Jeff Gilbert [:jgilbert] from comment #34)
> Comment on attachment 726928 [details] [diff] [review]
> 
> ::: gfx/gl/GLTextureImage.h
> @@ +322,4 @@
> >  
> >      GLuint mTexture;
> >      TextureState mTextureState;
> > +    nsRefPtr<GLContext> mGLContext;
> 
> Why does this need to be a RefPtr?

Because we need the GLContext to stay alive, since the destructor uses it.

The GLContext in this case comes from CompositorOGL, and is stored on both the TextureImageTextureHostOGL and the BasicTextureImage.

Sometimes the compositor is destroyed before all the textures (this is expected behaviour), and we crash on shutdown.

We could hold a ref to the GLContext in the TextureImageTextureHostOGL (maybe we should do this as well), but I think it make sense for the BasicTextureImage to keep the GLContext alive.

Comment 90

[Nick Cameron [:nrc]]

(In reply to Nicolas Silva [:nical] from comment #84)
> Comment on attachment 726915 [details] [diff] [review]
> 7 composite layers
> 
> Review of attachment 726915 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 
> ::: gfx/layers/composite/LayerManagerComposite.cpp
> @@ +162,5 @@
> > +
> > +#ifdef MOZ_LAYERS_HAVE_LOG
> > +  Log();
> > +  MOZ_LAYERS_LOG(("]----- EndTransaction"));
> > +#endif
> 
> Is it worth keeping all the logging that we added in m-c or was it mostly
> useful to fix specific bugs and we're not going to use them anymore (open
> question)?

This is pre-refactoring. I think a lot of the logging is useful and should be left in. Any that is for a specific bug - just random printf kind of things should go. I hope most of it already has, but if any is left we should kill it.

Comment 91

[Nick Cameron [:nrc]]

Comment on attachment 726924 [details] [diff] [review]
15 D3D11 backend

We're not going to land d3d11 for now.

Comment 92

[Nick Cameron [:nrc]]

Attachment: 9 GL Layers and compositor

Comment 93

[Nick Cameron [:nrc]]

Attachment: 5 texture hosts and clients

also carrying r=mattwoodrow

Comment 94

[Nick Cameron [:nrc]]

Attachment: 18 changes in gfx/gl

Comment 95

[Nick Cameron [:nrc]]

Attachment: 9 GL Layers and compositor

sorry, previous patch was missing some files

Comment 96

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 733138 [details] [diff] [review]
18 changes in gfx/gl

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

Nits, mostly.

::: gfx/gl/GLContext.h
@@ +171,5 @@
>  #ifdef DEBUG
>          PR_SetThreadPrivate(sCurrentGLContextTLS, this);
>  
> +  // XXX this assertion is disabled because it's triggering on Mac;
> +  // we need to figure out why and reenable it.

Bad indenting here.

::: gfx/gl/GLContextProviderEGL.cpp
@@ +465,5 @@
>  #if defined(MOZ_WIDGET_GONK)
>          fActiveTexture(LOCAL_GL_TEXTURE0);
> +        // FIXME [bjacob] There is a bug here: GL_TEXTURE_2D here is incompatible
> +        // with GL_TEXTURE_EXTERNAL in BindExternalBuffer. Specifically, binding a texture to
> +        // two different texture targets in a GL_INVALID_OPERATION.

Does this have a fix yet, somewhere?

::: gfx/gl/GLContextProviderGLX.cpp
@@ +1096,5 @@
>  
>      Display *display = DefaultXDisplay();
>      int xscreen = DefaultScreen(display);
> +    gfxASurface::gfxImageFormat imageFormat =
> +      gfxPlatform::GetPlatform()->OptimalFormatForContent(aContentType);

2-space -> 4-space.

::: gfx/gl/GLTextureImage.cpp
@@ +598,5 @@
> +
> +    GLuint texture = 0;
> +    aGL->fGenTextures(1, &texture);
> +
> +    aGL->fBindTexture(LOCAL_GL_TEXTURE_2D, texture);

Please push/pop the texture binding unless there's a good (documented!) reason not to.

::: gfx/gl/GLTextureImage.h
@@ +321,5 @@
>  protected:
>  
>      GLuint mTexture;
>      TextureState mTextureState;
> +    nsRefPtr<GLContext> mGLContext;

Please detail the lifetime of this class, if it's going to hold on to the GLContext.

::: gfx/gl/SharedSurfaceGL.cpp
@@ +374,1 @@
>      mConsGL->fDeleteSync(mSync);

I prefer to keep this separate, since one is the 'wait for it' func, and the other block is the 'ok, clear us' block.

Comment 97

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 733156 [details] [diff] [review]
9 GL Layers and compositor

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

You want another reviewer for TextureHostOGL.* because I wrote the Gralloc parts, so I shouldn't be its only reviewer. I did my best on the other parts, but I didn't understand all of the SurfaceStreams stuff.

I reviewed all the code, because I didn't have a good way of checking what was already identical in the existing code, especially as files moved around.

Beware: grumpy bjacob had to read a big patch. grumpy grumpy

The DrawFPS code should never have gotten r+ in the first place.

There is a lot of new code that doesn't bother const-qualifying methods that could be. I think that's sad. Example: TextureImageTextureHostOGL::GetTileCount, and many others around there. I dont mention that below.

A general comment that applies throughout: NS_ASSERTION is being overused here, including in new code. Almost everytime a NS_ASSERTION can only fail as a result of a mistake in our own C++ code, it's better replaced by a MOZ_ASSERT. We do want to catch programming mistakes in debug builds.

::: gfx/layers/opengl/ThebesLayerOGL.cpp
@@ +774,5 @@
>  
>    // If we do partial updates, we have to clip drawing to the regionToDraw.
>    // If we don't clip, background images will be fillrect'd to the region correctly,
>    // while text or lines will paint outside of the regionToDraw. This becomes apparent
> +  // with concave regions. Right now the scrollbars invalidate a narrow strip of the TextureImageAsbar

search & replace gone wild

::: gfx/layers/opengl/CompositingRenderTargetOGL.h
@@ +183,5 @@
> +      msg.AppendInt(mInitParams.mFBOTextureTarget, 16);
> +      msg.Append(", aRect.width ");
> +      msg.AppendInt(mInitParams.mSize.width);
> +      msg.Append(", aRect.height ");
> +      msg.AppendInt(mInitParams.mSize.height);

use AppendPrintf please.

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +73,5 @@
> +    };
> +
> +    // convert from 8 bit to 32 bit so that don't have to write the text above out in 32 bit format
> +    // we rely on int being 32 bits
> +    unsigned int* buf = (unsigned int*)malloc(64 * 8 * 4);

all these unsigned int should be uint32_t as you really care. On a platform where ints are 64bit the current code would not render correctly.

@@ +89,5 @@
> +  struct Vertex2D {
> +    float x,y;
> +  };
> +  const Vertex2D vertices[] = {
> +    { -1.0f, 1.0f - 42.f / viewport[3] },

There's no reason for these viewpoint transformations here. That's a job for a vertex shader. That's a lot of divisions, could have a significant performance impact.

Or at least replace x/y by precomputing 1/y and multiplying x by that. The C++ compiler *can't* take the liberty to do that for you unless you compile with -ffast-math.

@@ +132,5 @@
> +  int txn100 = (txnFps % 1000) / 100;
> +
> +  // Feel free to comment these texture coordinates out and use one
> +  // of the ones below instead, or play around with your own values.
> +  const GLfloat texCoords[] = {

Same. All these should go into the vertex shader.

@@ +176,5 @@
> +
> +  copyprog->Activate();
> +  copyprog->SetTextureUnit(0);
> +
> +  // we're going to use client-side vertex arrays for this.

That's bad... performance implications are unclear and driver-dependant.

@@ +181,5 @@
> +  context->fBindBuffer(LOCAL_GL_ARRAY_BUFFER, 0);
> +
> +  // "COPY"
> +  context->fBlendFuncSeparate(LOCAL_GL_ONE, LOCAL_GL_ZERO,
> +                              LOCAL_GL_ONE, LOCAL_GL_ZERO);

This is bad on multiple levels.

This code is turning blend on, and not turning it back off. That's bad as it means that it can severely degrade performance of subsequent draw calls... bad for a perf tool.

This code is also fiddling with the blendfunc without restoring it, which could cause hard to diagnose issues.

This is also using glBlendFuncSeparate where glBlendFunc would be enough, as the separate alpha factors passed here are equal to the RGB ones.

Finally, this "COPY" blendfunc, with sfactor=ONE and dfactor=ZERO, amounts to saying "no blending!" so why bother at all with blending? You might as well glDisable(GL_BLEND) and get the exact same result.

@@ +265,5 @@
> +void
> +CompositorOGL::AddPrograms(ShaderProgramType aType)
> +{
> +  for (PRUint32 maskType = MaskNone; maskType < NumMaskTypes; ++maskType) {
> +    if (ProgramProfileOGL::ProgramExists(aType, static_cast<MaskType>(maskType))) {

If you're only going to use maskType by casting it to MaskType then you should declare it as a MaskType.

@@ +294,5 @@
> +  if (!ctx) {
> +    ctx = mGLContext;
> +  }
> +
> +  ctx->MakeCurrent();

Move this MakeCurrent closer to where you actually make GL calls.

@@ +298,5 @@
> +  ctx->MakeCurrent();
> +
> +  for (PRUint32 i = 0; i < mPrograms.Length(); ++i) {
> +    for (PRUint32 type = MaskNone; type < NumMaskTypes; ++type) {
> +      delete mPrograms[i].mVariations[type];

That's the loop i'm talking about in my comment on the .h file about mPrograms and mVariations. It's very scary that you have to iterate over arrays manually to delete elements. Using nsAutoTArray instead of plain arrays, and nsAutoPtr instead of raw pointers, would avoid this badness.

@@ +301,5 @@
> +    for (PRUint32 type = MaskNone; type < NumMaskTypes; ++type) {
> +      delete mPrograms[i].mVariations[type];
> +    }
> +  }
> +  mPrograms.Clear();

See, with my suggestion, this Clear would be all you need to do.

@@ +345,5 @@
> +  mGLContext->fEnable(LOCAL_GL_BLEND);
> +
> +  mPrograms.AppendElements(NumProgramTypes);
> +  for (int type = 0; type < NumProgramTypes; ++type) {
> +    AddPrograms(static_cast<ShaderProgramType>(type));

type should be a ShaderProgramType in the first place

@@ +723,5 @@
> +FPSState::DrawFrameCounter(GLContext* context)
> +{
> +  profiler_set_frame_number(sFrameCount);
> +
> +  context->fEnable(LOCAL_GL_SCISSOR_TEST);

Enable(SCISSOR_TEST) without matching Disable is scary.

@@ +730,5 @@
> +  for (size_t i = 0; i < 16; i++) {
> +    context->fScissor(3*i, 0, 3, 3);
> +
> +    // We should do this using a single draw call
> +    // instead of 16 glClear()

Rather, since there is already number-drawing code in DrawFPS, should use that.

@@ +1348,5 @@
> +
> +    // XXX[bjacob] copied this from existing code (landed in Bug 804852)
> +    // having a printf_stderr in a performance counter that will trigger a android log call
> +    // looks fishy.
> +    printf_stderr("HWComposer: FPS is %g\n", fps);

Let's actually remove this code now.

::: gfx/layers/opengl/CompositorOGL.h
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 2 -*-

There are some trailing whitespace errors in this file.

@@ +38,5 @@
> +  {
> +    TextureFactoryIdentifier result;
> +    result.mParentBackend = LAYERS_OPENGL;
> +    result.mMaxTextureSize = GetMaxTextureSize();
> +    return result;

return TextureFactoryIdentifier(LAYERS_OPENGL, GetMaxTextureSize());

I think the ctor should generate that ctor for you. If not, it's a worthy addition.

@@ +77,5 @@
> +  }
> +
> +  virtual int32_t GetMaxTextureSize() const MOZ_OVERRIDE
> +  {
> +    GLint texSize = 0;

I don't understand why here we dont check for null mGLContext, while above in CanUseCanvasLayerForSize we did.

@@ +95,5 @@
> +    if (mDestroyed) {
> +      NS_WARNING("Call on destroyed layer manager");
> +      return;
> +    }
> +    mGLContext->MakeCurrent(aFlags == ForceMakeCurrent);

If that aFlags is a bitfield, you probably want to check for (aFlags & ForceMakeCurrent) here.

@@ +118,5 @@
> +  virtual bool Resume() MOZ_OVERRIDE;
> +
> +  virtual nsIWidget* GetWidget() const MOZ_OVERRIDE { return mWidget; }
> +  virtual nsIntSize* GetWidgetSize() MOZ_OVERRIDE {
> +    return &mWidgetSize;

Really? Why return a pointer to our member here?

If the goal is to allow users to mutate it, then 1) that's in contrast with our usual practice of not doing that and instead offering Set...() mutators, and 2) that would be better done by returning a reference rather than a pointer.

@@ +125,5 @@
> +  GLContext* gl() const { return mGLContext; }
> +  gl::ShaderProgramType GetFBOLayerProgramType() const {
> +    if (mFBOTextureTarget == LOCAL_GL_TEXTURE_RECTANGLE_ARB)
> +      return gl::RGBARectLayerProgramType;
> +    return gl::RGBALayerProgramType;

I'd use a ternary here; also, for all I know, {} is mandatory in all if() statements in our coding style.

@@ +146,5 @@
> +  already_AddRefed<mozilla::gl::GLContext> CreateContext();
> +
> +  /** Shader Programs */
> +  struct ShaderProgramVariations {
> +    ShaderProgramOGL* mVariations[NumMaskTypes];

Not a fan of plain arrays, especially not of raw pointers. Use a nsAutoTArray and make use something like nsAutoPtr so that the destructor does destroy the mVariations[i].

In fact this is a Big Deal because having this plain array there forces you to manually iterate over it in CleanupResources in the .cpp, whereas if this was a nsAutoTArray<nsAutoPtr> you could simply truncate your array and trust that destructors be called for you.

@@ +203,5 @@
> +
> +  ShaderProgramOGL* GetProgram(gl::ShaderProgramType aType,
> +                               MaskType aMask = MaskNone) {
> +    NS_ASSERTION(ProgramProfileOGL::ProgramExists(aType, aMask),
> +                 "Invalid program type.");

That would apparently be an error of the part of the C++ code calling us here, so I think that there is no reason to allow debug builds to continue past here. Use MOZ_ASSERT.

@@ +272,5 @@
> +
> +  void BindAndDrawQuad(ShaderProgramOGL *aProg,
> +                       bool aFlipped = false)
> +  {
> +    NS_ASSERTION(aProg->HasInitialized(), "Shader program not correctly initialized");

Same, this should likely be a MOZ_ASSERT, as that can apparently only be caused by an error in our own c++ code. I'm not going to report other occurences.

::: gfx/layers/opengl/ContainerLayerOGL.cpp
@@ +5,5 @@
>  
>  #include "ContainerLayerOGL.h"
>  #include "gfxUtils.h"
>  #include "gfxPlatform.h"
> +#include "GLContext.h"

Why this new include?

::: gfx/layers/opengl/FPSCounter.h
@@ +12,5 @@
> +}
> +namespace layers {
> +
> +static const double kFpsWindowMs = 250.0;
> +static const size_t kNumFrameTimeStamps = 16;

Remove 'static' here. The original code was in a cpp file, where it made sense. Now we're in a header. const allows multiple definitions and the linker will merge them.

@@ +20,5 @@
> +  // We keep a circular buffer of the time points at which the last K
> +  // frames were drawn.  To estimate FPS, we count the number of
> +  // frames we've drawn within the last kFPSWindowMs milliseconds and
> +  // divide by the amount time since the first of those frames.
> +  TimeStamp mFrames[kNumFrameTimeStamps];

I hate how this is a plain array without bounds checks. I suggest making this a nsAutoTArray<TimeStamp, kNumFrameTimeStamps>. I understand it's a preexisting problem.

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +256,5 @@
>    NS_ABORT_IF_FALSE(mGLContext == nullptr, "Don't reinitialize layer managers");
>  
> +  if (!aContext) {
> +    aContext = CreateContext();
> +  }

I don't like how this gives LayerManagerOGL::Initialize dual semantics: now you can either give it a context, or not give it a context and have it create it for you. A quick grep suggests that the ability to give it a context isn't used anymore, so I'd suggest removing the aContext parameter.

::: gfx/layers/opengl/TextureClientOGL.cpp
@@ +21,5 @@
> +void
> +TextureClientSharedOGL::ReleaseResources()
> +{
> +  if (!IsSurfaceDescriptorValid(mDescriptor) ||
> +      mDescriptor.type() != SurfaceDescriptor::TSharedTextureDescriptor) {

Why would we possibly have mDescriptor.type() != SurfaceDescriptor::TSharedTextureDescriptor here, if the descriptor is valid? I.e. what other type of descriptor could we get?

This looks like it should rather be a MOZ_ASSERT. Right now if we somehow get a bad descriptor, we leak it here.

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +196,5 @@
> +    return false;
> +  }
> +
> +  NS_ASSERTION(mTexture->GetContentType() != gfxASurface::CONTENT_ALPHA,
> +                "Image layer has alpha image");

Can this happen without us having made a specific mistake in our own C++ code? If no, then MOZ_ASSERT.

@@ +310,5 @@
> +SurfaceStreamHostOGL::SwapTexturesImpl(const SurfaceDescriptor& aImage,
> +                                       nsIntRegion* aRegion)
> +{
> +  NS_ASSERTION(aImage.type() == SurfaceDescriptor::TSurfaceStreamDescriptor,
> +              "Invalid descriptor");

Likewise this looks like a decent candidate for MOZ_ASSERT. Especially when below in Lock() the switch statement will hit MOZ_NOT_REACHED in this case.

@@ +376,5 @@
> +      return false;
> +  }
> +
> +  if (toUpload) {
> +    // mBounds seems to end up as (0,0,0,0) a lot, so don't use it?

This comment sounds like this code is guesswork. I don't understand it enough to tell. Need another reviewer for this part?

@@ +571,5 @@
> +
> +  return true;
> +}
> +
> +#ifdef MOZ_WIDGET_GONK

I wrote this ifdef MOZ_WIDGET_GONK section with GrallocTextureHostOGL, so someone else should review this.

::: gfx/layers/opengl/TextureHostOGL.h
@@ +48,5 @@
> +  virtual gl::ShaderProgramType GetShaderProgram() const {
> +    MOZ_NOT_REACHED("unhandled shader type");
> +  }
> +  virtual GLenum GetTextureTarget() const { return LOCAL_GL_TEXTURE_2D; }
> +  virtual GLenum GetWrapMode() const { return LOCAL_GL_REPEAT; }

Why is this defaulting to REPEAT?

CLAMP_TO_EDGE makes much more sense here, no? REPEAT is just asking for trouble if/when some TextureHost forgets to override this with CLAMP_TO_EDGE, and we run on ES2 hardware. Most textures around here are NPOT. ES2 only likes NPOT textures with CLAMP_TO_EDGE, otherwise it silently renders as rgba(0,0,0,0).

TextureHost implementations below do override this with CLAMP_TO_EDGE, so this could all be simplified.

Comment 98

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

(In reply to Benoit Jacob [:bjacob] from comment #97)
> A general comment that applies throughout: NS_ASSERTION is being overused
> here, including in new code. Almost everytime a NS_ASSERTION can only fail
> as a result of a mistake in our own C++ code, it's better replaced by a
> MOZ_ASSERT. We do want to catch programming mistakes in debug builds.

New NS_ASSERTION failures are caught in reftests and mochitests, and (as opposed to MOZ_ASSERTs) they let the test suite continue to run instead of killing off the rest of the tests.

Comment 99

[Benoit Jacob [:bjacob] (mostly away)]

Recording NS_ASSERTION failures as test failures (if I understand correctly comment 98) is a nice step in the right direction, that I didn't know about.

But that's not enough. For example, working on the B2G back-end recently, the hard part was to get B2G to boot at all. There, MOZ_ASSERT failures were very helpful; but I didn't get to run any mochitest or reftest, and there were so many NS_ASSERTION failures from various parts of Gecko that I wouldn't have noticed an extra one.

And while allowing tests to continue and record more failures before stopping is neat, it's hardly a must-have feature. Systematically catching serious errors that can only result from programming mistakes in our own code, is a much higher priority IMHO.

Comment 100

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

(In reply to Benoit Jacob [:bjacob] from comment #99)
> But that's not enough. For example, working on the B2G back-end recently,
> the hard part was to get B2G to boot at all. There, MOZ_ASSERT failures were
> very helpful; but I didn't get to run any mochitest or reftest, and there
> were so many NS_ASSERTION failures from various parts of Gecko that I
> wouldn't have noticed an extra one.

Look at it from the point of view of someone who's not working on graphics and isn't interested in your assertions. They're debugging something and instead of some NS_ASSERTION spew they can ignore, their browser crashes and they can't make any progress.

"Ah", you say, "unlike those bad people who create NS_ASSERTION spew, I'll actually fix all my assertion failures!" Not for every possible testcase, you won't. Even if you actually did, that would be a bad thing; elevating every possible assertion failure, no matter how obscure the testcase, to top priority isn't a good use of resources IMHO.

For your B2G situation I would suggest grepping for assertion failures in gfx/layers or wherever the code you were modifying lives.

> And while allowing tests to continue and record more failures before
> stopping is neat, it's hardly a must-have feature.

It's actually pretty important for reducing the number of tryserver iterations when debugging test failures, i.e. reducing turnaround time and tryserver load.

> Systematically catching
> serious errors that can only result from programming mistakes in our own
> code, is a much higher priority IMHO.

Our reftests and mochitests achieve that.

Comment 101

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #100)
> Look at it from the point of view of someone who's not working on graphics
> and isn't interested in your assertions. They're debugging something and
> instead of some NS_ASSERTION spew they can ignore, their browser crashes and
> they can't make any progress.
> 
> "Ah", you say, "unlike those bad people who create NS_ASSERTION spew, I'll
> actually fix all my assertion failures!" Not for every possible testcase,
> you won't. Even if you actually did, that would be a bad thing; elevating
> every possible assertion failure, no matter how obscure the testcase, to top
> priority isn't a good use of resources IMHO.

I didn't mean to say that all NS_ASSERTIONS should unconditionally be replaced by MOZ_ASSERT. In fact I absolutely agree that that would be a bad idea for the reasons you point out. I said that NS_ASSERTIONS _that can only fail as a result of a clear C++ programming mistake on our part_ should be. Even then, I didn't say "everytime" but "almost everytime", thinking about the fact that there will be cases where an NS_ASSERTION is too capricious to be a good idea to make fatal, or guards something that's not severe enough to warrant being fatal, but I don't have a hard rule to give to decide that and the ones that I saw there didn't seem like that.

So when we have a code path that takes a SurfaceDescriptor and absolutely expects it to be of a particular type or else will leak it --- then I absolutely think that it should be a MOZ_ASSERT, because I can't think of a valid reason why this would happen other than a clear programming mistake on our part, and I'd rather have an abort() in debug builds than a memory leak. Even without the memory leak, if an assert can only fail as a result of us being confused about what our own code should be doing, then I'd rather have us be forced to fix it, that being about to continue with code that we don't actually understand.

Again, not all NS_ASSERTIONS are like that, just the ones I saw in this patch. A good example of where to use non-fatal NS_ASSERTIONS would be where floating-point numbers are involved and it would be too capricious to require a strict inequality that could fail due to floating point imprecision in some cases.


> 
> For your B2G situation I would suggest grepping for assertion failures in
> gfx/layers or wherever the code you were modifying lives.

I can't necessarily guess the particular file that the assertion will live in, as the files relevant to me can be further up the stack. To understand my assertion failures I need to break in GDB, print a backtrace, and manually inspect some variables etc. I guess I could come close by setting a breakpoint with GDB commands to automatically bt and continue. In this way I'd get backtraces for all NS_ASSERTION failures. Few people think quickly of doing that (I've only started doing that recently) and even so you still get a lot more work to do than if you could run directly to the particular assertion you're interested in, which you get if it's a MOZ_ASSERT.


> > And while allowing tests to continue and record more failures before
> > stopping is neat, it's hardly a must-have feature.
> 
> It's actually pretty important for reducing the number of tryserver
> iterations when debugging test failures, i.e. reducing turnaround time and
> tryserver load.

It's nice, but I still can't see that as as important as offering a guarantee that in debug builds, past the point where the MOZ_ASSERT is put, the condition is true.

> 
> > Systematically catching
> > serious errors that can only result from programming mistakes in our own
> > code, is a much higher priority IMHO.
> 
> Our reftests and mochitests achieve that.

Again, our reftest and mochitests only help insofar as
 1. we run tests at all, which is not the case of B2G + mozilla-central
 2. we have tests for what's at hand there. Reftests cover much of the compositor, but there are always arcane conditions deep inside our code that are fairly hard to map to something that a reftest would do, and therefore are far easier to protect by an assertion there than by a reftest.
 3. we don't crash during startup before we'd start running reftests. which was my case here in this b2g work.

Comment 102

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

All NS_ASSERTIONS should only fail as a result of a clear C++ programming mistake on our part. That's exactly what an NS_ASSERTION is supposed to mean. And in practice, that's how they're used.

> A good example of where to use non-fatal NS_ASSERTIONS would be where floating-point
> numbers are involved and it would be too capricious to require a strict inequality that
> could fail due to floating point imprecision in some cases.

NS_ASSERTION should not be used in cases like this where it's not clear there is a code bug.

A leak is exactly the sort of programming error that should not be a fatal assertion because it's relatively harmless while I'm debugging something else. If someone adds a fatal leak assertion that makes it impossible for me to debug a completely different bug, they have created more work for me unnecessarily. This is not a theoretical issue; I've run into this with fatal assertions in the JS engine several times.

> if an assert can only fail as a result of us being confused
> about what our own code should be doing, then I'd rather have us be forced to fix it,

I hear this reasoning all the time and I don't understand it at all. Often the person who hits the assertion is not easily able to fix it; do you intend to force them to fix it? Given an NS_ASSERTION indicates a code bug, all reports of them should be filed as bugs (and usually are, by our fuzz testers at least); beyond that I don't think it makes sense to "be forced to fix it" if that means ignoring our bug prioritization policies.

> I guess I could come close by setting a breakpoint with GDB commands to automatically bt and
> continue.

XPCOM_DEBUG_BREAK=stack

Alternatively feel free to include the token "SHOULD_BE_FATAL_BUT_ISNT___SHAKES_FIST_AT_ROC" in your assertion text and grep for that.

> even so you still get a lot more work to do than if you could run directly to the
> particular assertion you're interested in, which you get if it's a MOZ_ASSERT.

You can do that as long as MOZ_ASSERT is only used for assertions you're interested in. When other people start using it and their assertions you don't care about are firing, then you can't do that anymore, and in fact you're much worse off than before.

> I still can't see that as as important as offering a guarantee that in debug builds, past
> the point where the MOZ_ASSERT is put, the condition is true.

It's easy enough to grep the log to see if any given assertion has fired.

> we have tests for what's at hand there. Reftests cover much of the compositor, but there
> are always arcane conditions deep inside our code that are fairly hard to map to something
> that a reftest would do, and therefore are far easier to protect by an assertion there than
> by a reftest.

Someone has to actually run a debug build on input that triggers the assertion. Who's going to do that? I stopped dogfooding debug builds years ago due to hitting too many fatal JS assertions.

Not running enough B2G tests is definitely a problem, and fatal assertions are definitely the wrong solution.

Sorry for spamming this bug :-(. But I've blogged about it (http://robert.ocallahan.org/2011/12/case-for-non-fatal-assertions.html) and we've had multiple discussions on dev.platform and I don't know where else to go.

Comment 103

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 731441 [details] [diff] [review]
8 image stuff

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

nit: remove all the trailing white spaces introduced in this patch.

::: gfx/layers/ImageContainer.cpp
@@ +188,2 @@
>      } else {
> +      // here we used to have a SetIdle90 call on the image bridge to tell

SetIdle()

@@ +190,5 @@
> +      // the compositor that the video element is not going to be seen for
> +      // moment and that it can release its shared memory. It was causing
> +      // crashes so it has been removed.
> +      // Could be reimplemented if need be (but beware, the memory model is 
> +      // tricky).

Does this mean that we could probably stall the producer because the shared memory is not released?

::: gfx/layers/ImageContainer.h
@@ +301,5 @@
>     *
>     * Implementations must call CurrentImageChanged() while holding
>     * mReentrantMonitor.
>     *
> +   * If this ImageContainer has uses async video: 

If this ImageContainer has an ImageClient for async video?

::: gfx/layers/ipc/ImageBridgeChild.cpp
@@ +100,5 @@
>  
>  // dispatched function
>  static void StopImageBridgeSync(ReentrantMonitor *aBarrier, bool *aDone)
>  {
> +ReentrantMonitorAutoEnter autoMon(*aBarrier);

indent

::: gfx/layers/ipc/ImageBridgeChild.h
@@ +48,5 @@
> + *
> + * ImageBridgeChild is also a ISurfaceAllocator. It can be used to allocate or
> + * deallocate data that is shared with the compositor. The main differerence
> + * with other ISurfaceAllocators is that some of its overriden methods can be
> + * invoked from ant thread.

any

::: gfx/layers/ipc/ImageBridgeParent.cpp
@@ +109,5 @@
>      static_cast<ImageContainerParent*>(
>        ManagedPImageContainerParent()[i]
>      )->DoStop();
>    }
> +*/

To be removed?

@@ +158,5 @@
>  {
>    delete toDealloc;
>    return true;
>  }
> +*/

To be removed?

@@ +161,5 @@
>  }
> +*/
> +PCompositableParent*
> +ImageBridgeParent::AllocPCompositable(const CompositableType& aType,
> +                                                         uint64_t* aID)

indent

::: gfx/layers/ipc/ImageBridgeParent.h
@@ +64,5 @@
> +  }
> +
> +  bool AllocUnsafeShmem(size_t aSize,
> +                           ipc::SharedMemory::SharedMemoryType aType,
> +                           ipc::Shmem* aShmem) MOZ_OVERRIDE

indent

Comment 104

[Nick Cameron [:nrc]]

Comment on attachment 733156 [details] [diff] [review]
9 GL Layers and compositor

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

Just reviewing bjacob's bits, most comments are nits and I've corrected them in the source. Bjacob, could you add the comment please?

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +603,5 @@
> +void
> +GrallocTextureHostOGL::DeleteTextures()
> +{
> +  if (mGLTexture || mEGLImage) {
> +  mGL->MakeCurrent();

indent

@@ +641,5 @@
> +
> +  if (mGLTexture) {
> +    mGL->fDeleteTextures(1, &mGLTexture);
> +    mGLTexture = 0;
> +  }

Use DeleteTextures()

@@ +666,5 @@
> +  MOZ_ASSERT(mGraphicBuffer.get());
> +
> +  mGL->MakeCurrent();
> +  if (!mGLTexture) {
> +    mGL->MakeCurrent();

remove

@@ +688,5 @@
> +  mGL->fTexImage2D(LOCAL_GL_TEXTURE_2D, 0,
> +                   LOCAL_GL_RGBA,
> +                   1, 1, 0,
> +                   LOCAL_GL_RGBA, LOCAL_GL_UNSIGNED_BYTE,
> +                   nullptr);

Please add comments explaining what is going on in Lock and Unlock()

Comment 105

[Nick Cameron [:nrc]]

(In reply to Benoit Jacob [:bjacob] from comment #97)
> Comment on attachment 733156 [details] [diff] [review]
> 9 GL Layers and compositor
> 
> Review of attachment 733156 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You want another reviewer for TextureHostOGL.* because I wrote the Gralloc
> parts, so I shouldn't be its only reviewer.

done.

> ::: gfx/layers/opengl/TextureHostOGL.cpp
> @@ +196,5 @@
> +    return false;
> +  }
> +
> +  NS_ASSERTION(mTexture->GetContentType() != gfxASurface::CONTENT_ALPHA,
> +                "Image layer has alpha image");
> Can this happen without us having made a specific mistake in our own C++ code?
> If no, then MOZ_ASSERT.

I think this should be non-fatal - we should fail tests (so shouldn't be a warning), but it is probably useful to be able to continue debugging if we hit it, it is fine to ignore.

> @@ +376,5 @@
> > +      return false;
> > +  }
> > +
> > +  if (toUpload) {
> > +    // mBounds seems to end up as (0,0,0,0) a lot, so don't use it?
> 
> This comment sounds like this code is guesswork. I don't understand it
> enough to tell. Need another reviewer for this part?
> 

This code is moved, not new, so I think it is OK to leave it (for now) and not get re-review.

Comment 106

[Bas Schouten (:bas.schouten)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #27)
> Comment on attachment 726930 [details] [diff] [review]
> 19 gfxPlatform
> 
> Review of attachment 726930 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxUtils.cpp
> @@ +529,5 @@
> > +  const gfxFloat epsilon = 0.000001;
> > +#define WITHIN_E(a,b) (fabs((a)-(b)) < epsilon)
> > +  bool shouldSnap = aSnap && (!WITHIN_E(mat._11,1.0) || !WITHIN_E(mat._22,1.0) ||
> > +                              !WITHIN_E(mat._12,0.0) || !WITHIN_E(mat._21,0.0));
> > +#undef WITHIN_E
> 
> This snapping critera should have some kind of justification.
 
> @@ +547,5 @@
> > +  } else {
> > +    while ((r = iter.Next()) != nullptr) {
> > +      Rect rect(r->x, r->y, r->width, r->height);
> > +
> > +      rect.Round();
> 
> Please add a comment about why Round() and not RoundIn() or RoundOut()

I'm not sure what the initial justifications were, it does the exact same as UserToDevicePixelSnapped, being consistent with what we do in the Thebes case seemed important.

Comment 107

[Bas Schouten (:bas.schouten)]

(In reply to Nick Cameron [:nrc] from comment #37)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #29)
> > Comment on attachment 726908 [details] [diff] [review]
> > 2 effects
> > 
> > Review of attachment 726908 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/layers/Effects.h
> > @@ +14,5 @@
> > > +namespace layers {
> > > +
> > > +/**
> > > + * Effects and effect chains are used by the compositor API (see Compositor.h).
> > > + * An effect chain represents a rendering method, for example some shader and
> > 
> > Isn't this what an individual effect is? And the effect chain is the set of
> > effects to apply when rendering.
> 
> I don't think so, although this is a bit confusing. Maybe this is too
> implementation-centric, but the shader depends on the primary effect and any
> secondary effects and the parameters to that shader are pulled from more
> than just one effect.
> > 
> > @@ +19,5 @@
> > > + * the data required for that shader to run. An effect is some component of the
> > > + * chain and its data.
> > > + *
> > > + * An effect chain consists of a primary effect - how the 'texture' memory should
> > > + * be interpreted (RGBA, BGRX, YCBCR, etc.) - and any number of secondary effects
> > 
> > I'm not entirely sure why some of this is actually necessary. Surely the
> > TextureHost already knows the byte order (and *needs* to know in some cases,
> > like where we do an upload with GL). This causes some confusing code in the
> > GL backend, where the texture upload code determines which byte order (by
> > deciding which shader program to use), and we basically ignore the effect
> > type that was passed in.
> > 
> > What actual benefit do we get from having separate byte order types rather
> > than just EFFECT_TEXTURE?
> > 
> 
> Bas - can you comment on this?

Yes, the assumption that on a fundamental level TextureHost has to know about its texture format is kind of false. The shader program that's used -should- be decided based on the effect. This also provides the cleaner abstraction where the effect maps to a shader and the texture maps to a texture. For example textures in D3D will always be RGBA, but we'll ignore the alpha value in the shader, the texture is theoretically ignorant of this.

Comment 108

[Nick Cameron [:nrc]]

Matt, based on comments https://bugzilla.mozilla.org/show_bug.cgi?id=852734#c37 and https://bugzilla.mozilla.org/show_bug.cgi?id=852734#c107, is that enough for you to r+ part 2?

Comment 109

[Nick Cameron [:nrc]]

Attachment: 20 random bits and pieces

Comment 110

[Jeff Muizelaar [:jrmuizel]]

Attachment: Disable Skia on OS X to get the build working

I convinced gw280 to agree to this

Comment 111

[Nick Cameron [:nrc]]

Attachment: 8 image stuff

Comment 112

[Matt Woodrow (:mattwoodrow)]

(In reply to Nick Cameron [:nrc] from comment #108)
> Matt, based on comments
> https://bugzilla.mozilla.org/show_bug.cgi?id=852734#c37 and
> https://bugzilla.mozilla.org/show_bug.cgi?id=852734#c107, is that enough for
> you to r+ part 2?

Alright, so Bas' explanation seems reasomable. However, all our GL code still expects the approach that I suggested.

We're currently in the weird situation where the Effect should be defining the shader type, but our GL texture host implementation entirely ignore what the effect is and use their internal information.

We should file a follow-up to refactor that code to fit in with the new architecture. I believe this would entail removing the shader type information from TextureImage.

My original comment about EFFECT_PRIMARY being entirely out of place inside that enum stands.

r=me with those.

Comment 113

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 734954 [details] [diff] [review]
8 image stuff

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

::: gfx/layers/ImageContainer.h
@@ +301,5 @@
>     *
>     * Implementations must call CurrentImageChanged() while holding
>     * mReentrantMonitor.
>     *
> +   * If this ImageContainer has an ImageClient fo async video:

nit: for

::: gfx/layers/ipc/ImageBridgeChild.cpp
@@ +477,5 @@
> +  mCompositorBackend = LAYERS_D3D11;
> +#else
> +  mCompositorBackend = LAYERS_OPENGL;
> +#endif
> +  RefPtr<ImageClient> client 

nit: whitespace

::: gfx/layers/ipc/ImageBridgeParent.h
@@ +24,5 @@
>  
>  public:
> +  typedef InfallibleTArray<CompositableOperation> EditArray;
> +  typedef InfallibleTArray<EditReply> EditReplyArray;
> +  

nit: whitespace

Comment 114

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 732152 [details] [diff] [review]
1 The compositor interface

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

Much impressed by the documentation.

::: gfx/layers/Compositor.h
@@ +65,5 @@
> + * and TextureHost are to synchronize, serialize and deserialize texture data.
> + * TextureHosts provide access to TextureSources that are views on the
> + * Texture data providing the necessary api for Compositor backend to composite
> + * them.
> + * 

Trailing whitespace

@@ +134,5 @@
> + * # Usage
> + *
> + * For an example of a user of Compositor, see LayerManagerComposite.
> + *
> + * Initialisation: create a Compositor object, call Initialize().

Nit: inconsistent use of s/z in initializ*.

@@ +161,5 @@
> + * call SetTargetContext or SetRenderTarget, the latter with a target created
> + * by CreateRenderTarget or CreateRenderTargetFromSource.
> + *
> + * The target and viewport methods can be called before any DrawQuad call and
> + * affect any subsequent DrawQuad calls. 

Trailing whitespace

@@ +163,5 @@
> + *
> + * The target and viewport methods can be called before any DrawQuad call and
> + * affect any subsequent DrawQuad calls. 
> + */
> +class Compositor : public RefCounted<Compositor>

Include mozilla/RefPtr.h directly in this file.

@@ +171,5 @@
> +    : mCompositorID(0)
> +  {
> +    MOZ_COUNT_CTOR(Compositor);
> +  }
> +  virtual ~Compositor() {

Gecko style puts the { on the next line.

@@ +184,5 @@
> +   * across process or thread boundaries that are compatible with this
> +   * compositor.
> +   */
> +  virtual TextureFactoryIdentifier
> +    GetTextureFactoryIdentifier() = 0;

Why not on one line?

@@ +189,5 @@
> +
> +  /**
> +   * Properties of the compositor.
> +   */
> +  virtual bool CanUseCanvasLayerForSize(const gfxIntSize &aSize) = 0;

This file is inconsistent about the location of & and *. Gecko style puts them to the left, so please follow that throughout.

@@ +225,5 @@
> +   */
> +  virtual void FallbackTextureInfo(TextureInfo& aInfo) {}
> +
> +  /**
> +   * This creates a Surface that can be used as a rendering target by this

Nit: "Creates...", dropping the "This"

@@ +242,5 @@
> +                               const CompositingRenderTarget* aSource) = 0;
> +
> +  /**
> +   * Sets the given surface as the target for subsequent calls to DrawQuad.
> +   * Passing nullptr as aSurface sets the screen as the target.

s/nullptr/null/ for ease of reading and consistency with the comments above and below.

@@ +254,5 @@
> +  virtual CompositingRenderTarget* GetCurrentRenderTarget() = 0;
> +
> +  /**
> +   * Mostly the compositor will pull the size from a widget and this method will
> +   * be ignored, but compositor implementations are free to use it if they like.

I can't say this helps me understand what the contract is here. Are consumers required to call it?

@@ +274,5 @@
> +  /**
> +   * Start a new frame.
> +   * aClipRectIn is the clip rect for the window in window space (optional).
> +   * aTransform is the transform from user space to window space.
> +   * aRenderBounds bounding rect for rendering, in user space.  

Trailing whitespace

@@ +337,5 @@
> +    return mCompositorID;
> +  }
> +  void SetCompositorID(uint32_t aID)
> +  {
> +    NS_ASSERTION(mCompositorID==0, "The compositor ID must be set only once.");

MOZ_ASSERT, and spaces around '=='.

@@ +344,5 @@
> +
> +  /**
> +   * Notify the compositor that a layers transaction has occured. This is only
> +   * used for FPS information at the moment.
> +   * XXX: surely there is a better way to do this?

File a bug if this isn't fixed here.

@@ +362,5 @@
> +   */
> +  virtual bool Resume() { return true; }
> +
> +  // XXX I expect we will want to move mWidget into this class and implement
> +  // these methods properly.

Same.

@@ +373,5 @@
> +   * static method. We need this for creating texture clients/hosts etc. when we
> +   * don't have a reference to a Compositor.
> +   */
> +  static LayersBackend GetBackend();
> +protected:

Nit: empty line above 'protected:'

@@ +380,5 @@
> +};
> +
> +
> +}
> +}

But one fewer newline between the end of the class and the end of the namespaces, and add "// namespace {layers,mozilla}" at the end of those lines.

::: gfx/layers/CompositorTypes.h
@@ +9,5 @@
> +namespace mozilla {
> +namespace layers {
> +
> +typedef int32_t SurfaceDescriptorType;
> +static const int32_t SURFACEDESCRIPTOR_UNKNOWN = 0;

static const SurfaceDescriptorType...?

@@ +20,5 @@
> + */
> +typedef uint32_t TextureFlags;
> +const TextureFlags NoFlags            = 0x0;
> +// Use nearest-neighbour texture filtering (as opposed to linear filtering).
> +const TextureFlags UseNearestFilter   = 0x1;

Not static?

@@ +105,5 @@
> +  {}
> +};
> +
> +
> +} // namespace

"namespace layers"

@@ +106,5 @@
> +};
> +
> +
> +} // namespace
> +} // namespace

"namespace mozilla"

Comment 115

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 726908 [details] [diff] [review]
2 effects

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

::: gfx/layers/Effects.cpp
@@ +1,4 @@
> +/* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> +* This Source Code Form is subject to the terms of the Mozilla Public
> +* License, v. 2.0. If a copy of the MPL was not distributed with this
> +* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Also in the previous patch: modeline and license header should be in separate comments, like so:
/* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

@@ +39,5 @@
> +  }
> +
> +  if (mMaskTexture) {
> +    nsAutoCString pfx(aPrefix);
> +    pfx += "  ";

Anything wrong with "prefix"?

::: gfx/layers/Effects.h
@@ +45,5 @@
> +  EFFECT_RENDER_TARGET,
> +  EFFECT_MAX
> +};
> +
> +struct Effect : public RefCounted<Effect>

Include RefPtr

@@ +53,5 @@
> +  EffectTypes mType;
> +
> +#ifdef MOZ_LAYERS_HAVE_LOG
> +  virtual void PrintInfo(nsACString& aTo, const char* aPrefix) =0;
> +  virtual ~Effect() {}

Why is this inside the ifdef?

@@ +61,5 @@
> +// Render from a texture
> +struct TexturedEffect : public Effect
> +{
> +  TexturedEffect(EffectTypes aType,
> +                 TextureSource *aTexture,

Left

@@ +63,5 @@
> +{
> +  TexturedEffect(EffectTypes aType,
> +                 TextureSource *aTexture,
> +                 bool aPremultiplied,
> +                 mozilla::gfx::Filter aFilter)

Drop the "mozilla::"

@@ +65,5 @@
> +                 TextureSource *aTexture,
> +                 bool aPremultiplied,
> +                 mozilla::gfx::Filter aFilter)
> +     : Effect(aType)
> +     ,  mTextureCoords(0, 0, 1.0f, 1.0f)

Drop one space

@@ +67,5 @@
> +                 mozilla::gfx::Filter aFilter)
> +     : Effect(aType)
> +     ,  mTextureCoords(0, 0, 1.0f, 1.0f)
> +     , mTexture(aTexture)
> +     , mPremultiplied(aPremultiplied), mFilter(aFilter)

Move mFilter to the next line.

@@ +78,5 @@
> +
> +  gfx::Rect mTextureCoords;
> +  TextureSource* mTexture;
> +  bool mPremultiplied;
> +  mozilla::gfx::Filter mFilter;;

Drop "mozilla::" and the extra semicolon.

@@ +124,5 @@
> +struct EffectBGRX : public TexturedEffect
> +{
> +  EffectBGRX(TextureSource *aBGRXTexture,
> +             bool aPremultiplied,
> +             mozilla::gfx::Filter aFilter,

mozilla::

@@ +138,5 @@
> +struct EffectRGBX : public TexturedEffect
> +{
> +  EffectRGBX(TextureSource *aRGBXTexture,
> +             bool aPremultiplied,
> +             mozilla::gfx::Filter aFilter)

Same

@@ +151,5 @@
> +struct EffectBGRA : public TexturedEffect
> +{
> +  EffectBGRA(TextureSource *aBGRATexture,
> +             bool aPremultiplied,
> +             mozilla::gfx::Filter aFilter)

Same

@@ +164,5 @@
> +struct EffectRGBA : public TexturedEffect
> +{
> +  EffectRGBA(TextureSource *aRGBATexture,
> +             bool aPremultiplied,
> +             mozilla::gfx::Filter aFilter)

Same

@@ +178,5 @@
> +{
> +  EffectRGBAExternal(TextureSource *aRGBATexture,
> +                     const gfx::Matrix4x4 &aTextureTransform,
> +                     bool aPremultiplied,
> +                     mozilla::gfx::Filter aFilter)

Same

@@ +192,5 @@
> +};
> +
> +struct EffectYCbCr : public TexturedEffect
> +{
> +  // todo - mFlipped should be removed and texture coords should be used

File a bug.

@@ +195,5 @@
> +{
> +  // todo - mFlipped should be removed and texture coords should be used
> +  // that way we'd prevent TexturedEffect pushing this bogus bool onto this
> +  // effect.
> +  EffectYCbCr(TextureSource *aSource, mozilla::gfx::Filter aFilter)

mozilla::

@@ +208,5 @@
> +struct EffectComponentAlpha : public TexturedEffect
> +{
> +  EffectComponentAlpha(TextureSource *aOnWhite, TextureSource *aOnBlack)
> +    : TexturedEffect(EFFECT_COMPONENT_ALPHA, nullptr, false, gfx::FILTER_LINEAR)
> +    , mOnWhite(aOnWhite), mOnBlack(aOnBlack)

Move mOnBlack to the next line

@@ +222,5 @@
> +
> +struct EffectSolidColor : public Effect
> +{
> +  EffectSolidColor(const mozilla::gfx::Color &aColor)
> +    : Effect(EFFECT_SOLID_COLOR), mColor(aColor)

mozilla::, and mColor to the next line

@@ +229,5 @@
> +#ifdef MOZ_LAYERS_HAVE_LOG
> +  virtual void PrintInfo(nsACString& aTo, const char* aPrefix);
> +#endif
> +
> +  mozilla::gfx::Color mColor;

mozilla::

@@ +270,5 @@
> +  return result;
> +}
> +
> +} // namespace
> +} // namespace

As in the previous patch.

Comment 116

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 726910 [details] [diff] [review]
3 compositable hosts and clients

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

::: gfx/layers/client/CompositableClient.cpp
@@ +1,4 @@
> +/* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> +* This Source Code Form is subject to the terms of the Mozilla Public
> +* License, v. 2.0. If a copy of the MPL was not distributed with this
> +* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

As before.

@@ +7,5 @@
> +#include "mozilla/layers/TextureClient.h"
> +#include "mozilla/layers/TextureClientOGL.h"
> +#include "mozilla/layers/TextureChild.h"
> +#include "mozilla/layers/ShadowLayersChild.h"
> +#include "mozilla/layers/CompositableForwarder.h"

Sort these alphabetically, while keeping CompositableClient.h first.

@@ +15,5 @@
> +
> +namespace mozilla {
> +namespace layers {
> +
> +CompositableClient::~CompositableClient() {

{ on the next line

@@ +21,5 @@
> +  Destroy();
> +}
> +
> +LayersBackend
> +CompositableClient::GetCompositorBackendType() const{

Same

@@ +32,5 @@
> +  mCompositableChild = aChild;
> +}
> +
> +CompositableChild*
> +CompositableClient::GetIPDLActor() const {

Same

@@ +62,5 @@
> +{
> +  if (mCompositableChild) {
> +    return mCompositableChild->GetAsyncID();
> +  }
> +  return 0; // zero is always an invalid async ID

I would swap the condition around, as:

if (!mCompositableChild) {
  // zero is always an invalid async ID
  return 0;
}
return mCompositableChild->GetAsyncID();

@@ +89,5 @@
> +bool
> +CompositableChild::DeallocPTexture(PTextureChild* aActor)
> +{
> +    delete aActor;
> +    return true;

Indentation is off.

@@ +98,5 @@
> +                                        TextureFlags aFlags)
> +{
> +  MOZ_ASSERT(GetForwarder(), "Can't create a texture client if the compositable is not connected to the compositor.");
> +  LayersBackend parentBackend = GetForwarder()->GetCompositorBackendType();
> +  RefPtr<TextureClient> result = nullptr;

Doesn't need initialization.

@@ +102,5 @@
> +  RefPtr<TextureClient> result = nullptr;
> +
> +  switch (aTextureClientType) {
> +  case TEXTURE_SHARED_GL:
> +     if (parentBackend == LAYERS_OPENGL) {

Three-space indentation is quite uncommon, so let's keep it that way.

@@ +120,5 @@
> +  case TEXTURE_YCBCR:
> +     result = new TextureClientShmemYCbCr(GetForwarder(), GetType());
> +     break;
> +  case TEXTURE_CONTENT:
> + #ifdef XP_WIN

# should not be indented. (Same for endif.)

@@ +121,5 @@
> +     result = new TextureClientShmemYCbCr(GetForwarder(), GetType());
> +     break;
> +  case TEXTURE_CONTENT:
> + #ifdef XP_WIN
> +     if (GetForwarder()->GetCompositorBackendType() == LAYERS_D3D11) {

parentBackend

@@ +137,5 @@
> +    MOZ_ASSERT(false, "Unhandled texture client type");
> +   }
> +
> +  MOZ_ASSERT(result, "Failed to create TextureClient");
> +  MOZ_ASSERT(result->SupportsType(aTextureClientType), "Created the wrong texture client?");

Overlong

@@ +140,5 @@
> +  MOZ_ASSERT(result, "Failed to create TextureClient");
> +  MOZ_ASSERT(result->SupportsType(aTextureClientType), "Created the wrong texture client?");
> +  result->SetFlags(aFlags);
> +  TextureChild* textureChild
> +    = static_cast<TextureChild*>(GetIPDLActor()->SendPTextureConstructor(result->GetTextureInfo()));

Overlong

::: gfx/layers/client/CompositableClient.h
@@ +18,5 @@
> +class ShadowLayersChild;
> +class ImageBridgeChild;
> +class ShadowableLayer;
> +class CompositableForwarder;
> +class CompositableChild;

Sort these

@@ +27,5 @@
> + * CompositableHost pair.
> + *
> + * CompositableClient's purpose is to send texture data to the compositor side
> + * along with any extra information about how the texture is to be composited.
> + * Things like opacity or transformation belong to layer and not compositable.

Is this sentence missing a few words?

@@ +58,5 @@
> + * Canvas layers use CanvasClients (but ImageHosts). We have a different subclass
> + * where we have a different way of interfacing with the textures - in terms of
> + * drawing into the compositable and/or passing its contents to the compostior.
> + */
> +class CompositableClient : public RefCounted<CompositableClient>

Include refptr

@@ +62,5 @@
> +class CompositableClient : public RefCounted<CompositableClient>
> +{
> +public:
> +  CompositableClient(CompositableForwarder* aForwarder)
> +  : mCompositableChild(nullptr), mForwarder(aForwarder)

Initializers should be indented four space, and have a line each, like so:

  CompositableClient(CompositableForwarder* aForwarder)
    : mCompositableChild(nullptr)
    , mForwarder(aForwarder)

@@ +77,5 @@
> +  }
> +
> +  LayersBackend GetCompositorBackendType() const;
> +
> +  TemporaryRef<TextureClient> CreateTextureClient(TextureClientType aTextureClientType,

Overlong line

@@ +119,5 @@
> +class CompositableChild : public PCompositableChild
> +{
> +public:
> +  CompositableChild()
> +  : mCompositableClient(nullptr), mID(0)

Same as before

@@ +143,5 @@
> +  {
> +    return mCompositableClient;
> +  }
> +
> +  void SetAsyncID(uint64_t aID) { mID = aID; }

Might as well put this on four lines.

::: gfx/layers/composite/CompositableHost.cpp
@@ +12,5 @@
> +
> +namespace mozilla {
> +namespace layers {
> +
> +bool CompositableHost::Update(const SurfaceDescriptor& aImage,

Gecko style puts the 'bool' on its own line.

@@ +13,5 @@
> +namespace mozilla {
> +namespace layers {
> +
> +bool CompositableHost::Update(const SurfaceDescriptor& aImage,
> +                              SurfaceDescriptor* aResult) {

{ on its own line

@@ +27,5 @@
> +}
> +
> +bool CompositableHost::AddMaskEffect(EffectChain& aEffects,
> +                               const gfx::Matrix4x4& aTransform,
> +                               bool aIs3D)

Indentation

@@ +60,5 @@
> +  case BUFFER_CONTENT_DIRECT:
> +    result = new ContentHostDoubleBuffered(aCompositor);
> +    return result;
> +  default:
> +    NS_ERROR("Unknown CompositableType");

MOZ_NOT_REACHED or MOZ_ASSERT

@@ +82,5 @@
> +
> +CompositableParent::CompositableParent(CompositableParentManager* aMgr,
> +                                       CompositableType aType,
> +                                       uint64_t aID)
> +: mManager(aMgr)

Indent two spaces

@@ +101,5 @@
> +  CompositableMap::Erase(mID);
> +}
> +
> +namespace CompositableMap {
> +  typedef std::map<uint64_t, CompositableParent*> Map_t;

We don't indent code inside a namespace, even if it's a pseudo-type.

@@ +104,5 @@
> +namespace CompositableMap {
> +  typedef std::map<uint64_t, CompositableParent*> Map_t;
> +  static Map_t* sCompositableMap = nullptr;
> +  bool IsCreated() {
> +    return sCompositableMap != nullptr;

'return sCompositableMap;' or 'return !!sCompositableMap;'

@@ +108,5 @@
> +    return sCompositableMap != nullptr;
> +  }
> +  CompositableParent* Get(uint64_t aID)
> +  {
> +    if (!IsCreated() || (aID == 0)) {

Too many parens

@@ +119,5 @@
> +    return it->second;
> +  }
> +  void Set(uint64_t aID, CompositableParent* aParent)
> +  {
> +    if (!IsCreated() || (aID == 0)) {

Same

@@ +126,5 @@
> +    (*sCompositableMap)[aID] = aParent;
> +  }
> +  void Erase(uint64_t aID)
> +  {
> +    if (!IsCreated() || (aID == 0)) {

Same

@@ +143,5 @@
> +    sCompositableMap->clear();
> +  }
> +  void Create()
> +  {
> +    if (sCompositableMap == nullptr) {

if (!sCompositableMap) {

@@ +157,5 @@
> +} // CompositableMap
> +
> +
> +
> +} // namespace

Drop some newlines above

::: gfx/layers/composite/CompositableHost.h
@@ +43,5 @@
> + * Composite is called by the owning layer when it is composited. CompositableHost
> + * will use its TextureHost(s) and call Compositor::DrawQuad to do the actual
> + * rendering.
> + */
> +class CompositableHost : public RefCounted<CompositableHost>

Include refptr

@@ +59,5 @@
> +    MOZ_COUNT_DTOR(CompositableHost);
> +  }
> +
> +  static TemporaryRef<CompositableHost> Create(CompositableType aType,
> +                                               Compositor* aCompositor = nullptr);

Ever so slightly overlong.

@@ +63,5 @@
> +                                               Compositor* aCompositor = nullptr);
> +
> +  virtual CompositableType GetType() = 0;
> +
> +  virtual void CleanupResources() {

{ on the next line

@@ +92,5 @@
> +   * @return true if we should schedule a composition.
> +   */
> +  virtual bool Update(const SurfaceDescriptor& aImage,
> +                      SurfaceDescriptor* aResult = nullptr);
> +  

Trailing whitespace

@@ +107,5 @@
> +                            const nsIntRegion& aUpdated,
> +                            const nsIntRegion& aOldValidRegionBack,
> +                            nsIntRegion* aUpdatedRegionBack)
> +  {
> +    NS_RUNTIMEABORT("should be implemented or not used");

Crash even in opt builds?

@@ +116,5 @@
> +  virtual TextureHost* GetTextureHost() { return nullptr; }
> +
> +  virtual LayerRenderState GetRenderState() = 0;
> +
> +  virtual void SetPictureRect(const nsIntRect& aPictureRect) {

{ on the next line

@@ +122,5 @@
> +  }
> +
> +  /**
> +   * Adds a mask effect using this texture as the mask, if possible.
> +   * \return true if the effect was added, false otherwise.

@return, if you need syntax.

@@ +139,5 @@
> +
> +#ifdef MOZ_LAYERS_HAVE_LOG
> +  virtual void PrintInfo(nsACString& aTo, const char* aPrefix) { }
> +#endif
> +  

Trailing whitespace

@@ +150,5 @@
> +
> +class CompositableParent : public PCompositableParent
> +{
> +public:
> +  CompositableParent(CompositableParentManager* aMgr, CompositableType aType, uint64_t aID = 0);

Overlong

@@ +155,5 @@
> +  ~CompositableParent();
> +  PTextureParent* AllocPTexture(const TextureInfo& aInfo) MOZ_OVERRIDE;
> +  bool DeallocPTexture(PTextureParent* aActor) MOZ_OVERRIDE;
> +
> +  CompositableHost* GetCompositableHost() const {

{ on the next line

@@ +158,5 @@
> +
> +  CompositableHost* GetCompositableHost() const {
> +    return mHost;
> +  }
> +  void SetCompositableHost(CompositableHost* aHost) {

Same

@@ +181,5 @@
> +  uint64_t GetCompositorID() const
> +  {
> +    return mCompositorID;
> +  }
> +private:

Empty line before 'private:'

@@ +208,5 @@
> + *
> + * CompositableMap must be global because the image bridge doesn't have any
> + * reference to whatever we have created with PLayers. So, the only way to
> + * actually connect these two worlds is to have something global that they can
> + * both query (in the same  thread). I didn't allocate the map on the stack to

Two spaces between same and thread. Who is "I"?

@@ +219,5 @@
> + * thread itself.
> + */
> +namespace CompositableMap {
> +  void Create();
> +  void Destroy();

No indentation.

::: gfx/layers/ipc/CompositableForwarder.h
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> + * vim: sw=2 ts=8 et :

Suddenly, a vim user? Coding style expects:

/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
/* vim: set ts=8 sts=2 et sw=2 tw=80: */

@@ +7,5 @@
> +
> +#ifndef MOZILLA_LAYERS_COMPOSITABLEFORWARDER
> +#define MOZILLA_LAYERS_COMPOSITABLEFORWARDER
> +
> +#include <stdint.h>

Never include stdint.h directly, we still support compilers that don't have it. Include mozilla/StandardInteger.h instead.

@@ +37,5 @@
> +  friend class AutoOpenSurface;
> +  friend class TextureClientShmem;
> +public:
> +  CompositableForwarder()
> +  : mMaxTextureSize(0), mCompositorBackend(mozilla::layers::LAYERS_NONE)

CompositableForwarder()
    : mMaxTextureSize(0)
    , mCompositorBackend(LAYERS_NONE)

(Indentation, and drop "mozilla::layers::").

@@ +65,5 @@
> +  /**
> +   * Tell the compositor that a Compositable is killing its buffer(s),
> +   * that is TextureClient/Hosts.
> +   */
> +  virtual void DestroyThebesBuffer(CompositableClient* aCompositable) = 0;  

Trailing whitespace

@@ +96,5 @@
> +   * shadow is told to destroy its front buffer.  This can happen when
> +   * a new front/back buffer pair have been created because of a layer
> +   * resize, e.g.
> +   */
> +  virtual void DestroyedThebesBuffer(const SurfaceDescriptor& aBackBufferToDestroy) = 0;

Overlong

@@ +110,5 @@
> +   * Returns the type of backend that is used off the main thread.
> +   * We only don't allow changing the backend type at runtime so this value can
> +   * be queried once and will not change until Gecko is restarted.
> +   */
> +  LayersBackend GetCompositorBackendType() {

{ on the next line. Make it const?

::: gfx/layers/ipc/CompositableTransactionParent.cpp
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> + * vim: sw=2 ts=8 et :

Same

@@ +23,5 @@
> +//--------------------------------------------------
> +// Convenience accessors
> +template<class OpPaintT>
> +static TextureHost*
> +AsTextureHost(const OpPaintT& op)

I don't see any users?

@@ +33,5 @@
> +Layer* GetLayerFromOpPaint(const OpPaintT& op)
> +{
> +  PTextureParent* textureParent = op.textureParent();
> +  CompositableHost* compoHost
> +    = static_cast<CompositableParent*>(textureParent->Manager())->GetCompositableHost();

Overlong

@@ +38,5 @@
> +  return compoHost ? compoHost->GetLayer() : nullptr;
> +}
> +
> +bool
> +CompositableParentManager::ReceiveCompositableUpdate(const CompositableOperation& aEdit,

Overlong

@@ +39,5 @@
> +}
> +
> +bool
> +CompositableParentManager::ReceiveCompositableUpdate(const CompositableOperation& aEdit,
> +                                                     EditReplyVector& replyv)

aArgument

@@ +42,5 @@
> +CompositableParentManager::ReceiveCompositableUpdate(const CompositableOperation& aEdit,
> +                                                     EditReplyVector& replyv)
> +{
> +  switch (aEdit.type()) {
> +    case CompositableOperation::TOpCreatedSingleBuffer: {

Elsewhere in this bug, 'case' was aligned with 'switch'. Please follow that.

@@ +49,5 @@
> +      CompositableParent* compositableParent = static_cast<CompositableParent*>(op.compositableParent());
> +      TextureParent* textureParent = static_cast<TextureParent*>(op.bufferParent());
> +
> +
> +      textureParent->EnsureTextureHost(op.descriptor().type());

No need for two empty lines

@@ +104,5 @@
> +      } else {
> +        // if we reach this branch, it most likely means that async textures
> +        // are coming in before we had time to attach the conmpositable to a
> +        // layer. Don't panic, it is okay in this case. it should not be
> +        // happenning continuously, though.

"happening"

@@ +130,5 @@
> +        }
> +      }
> +
> +      if (layer) {
> +        RenderTraceInvalidateEnd(layer, "FF00FF");

There's a lot of "FF00FF" in this function, with no explanation.

@@ +173,5 @@
> +      MOZ_ASSERT(false, "bad type");
> +    }
> +  }
> +
> +  return true;

There's a lot of static_casting in this function... I hope they're all safe.

Comment 117

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 731436 [details] [diff] [review]
4 concrete CompositableHosts

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

::: gfx/layers/composite/ContentHost.cpp
@@ +29,5 @@
> +void
> +ContentHostBase::DestroyFrontHost()
> +{
> +  if (mTextureHost) {
> +    MOZ_ASSERT(mTextureHost->GetDeAllocator(), "We won't be able to destroy our SurfaceDescriptor");

Overlong

@@ +67,5 @@
> +  } else {
> +    renderRegion = aVisibleRegion;
> +  }
> +
> +  nsIntRegion region(*renderRegion);

I don't see the point of the tmpRegion and renderRegion variables above. How about:

nsIntRegion region;
if (PaintWillResample()) {
  region = aVisibleRegion->GetBounds();
} else {
  region = *aVisibleRegion;
}

@@ +107,5 @@
> +  }
> +
> +  if (mTextureHostOnWhite) {
> +    iterOnWhite = mTextureHostOnWhite->AsTileIterator();
> +    NS_ASSERTION((!tileIter) || tileIter->GetTileCount() == iterOnWhite->GetTileCount(),

Too many parens, and MOZ_ASSERT

@@ +114,5 @@
> +      iterOnWhite->BeginTileIteration();
> +    }
> +  }
> +
> +  bool usingTiles = (tileIter && tileIter->GetTileCount() > 1);

Too many parens

@@ +117,5 @@
> +
> +  bool usingTiles = (tileIter && tileIter->GetTileCount() > 1);
> +  do {
> +    if (iterOnWhite) {
> +      NS_ASSERTION(iterOnWhite->GetTileRect() == tileIter->GetTileRect(),

MOZ_ASSERT

@@ +261,5 @@
> +  // There's code to make sure that updated regions don't cross rotation
> +  // boundaries, so assert here that this is the case
> +  NS_ASSERTION(((destBounds.x % size.width) + destBounds.width <= size.width) &&
> +               ((destBounds.y % size.height) + destBounds.height <= size.height),
> +               "updated region lies across rotation boundaries!");

Split into two MOZ_ASSERTs, for a clearer failure mode.

@@ +386,5 @@
> +    aTo += " [paint-will-resample]";
> +  }
> +
> +  nsAutoCString pfx(aPrefix);
> +  pfx += "  ";

"prefix"?

::: gfx/layers/composite/ContentHost.h
@@ +39,5 @@
> +#endif
> +
> +protected:
> +  ContentHost(Compositor* aCompositor)
> +  : CompositableHost(aCompositor) {}

Make it

  ContentHost(Compositor* aCompositor)
    : CompositableHost(aCompositor)
  {}

@@ +112,5 @@
> +  // destroy our front buffer so that we can continue to composite.
> +  virtual void DestroyTextures() = 0;
> +
> +protected:
> +  virtual nsIntPoint GetOriginOffset() {

{ on the next line

::: gfx/layers/composite/ImageHost.cpp
@@ +16,5 @@
> +
> +namespace layers {
> +
> +void
> +ImageHostSingle::SetCompositor(Compositor* aCompositor) {

{ on the next line

@@ +55,5 @@
> +    return;
> +  }
> +
> +  if (!mTextureHost->Lock()) {
> +    MOZ_ASSERT(false, "failed to lock texture host");

If this can happen, it shouldn't be a MOZ_ASSERT; if it can't happen, why doesn't Lock() return void?

@@ +61,5 @@
> +  }
> +  
> +  RefPtr<TexturedEffect> effect =
> +    CreateTexturedEffect(mTextureHost, aFilter);
> +  

Trailing whitespace *2

@@ +121,5 @@
> +#endif
> +
> +bool
> +ImageHostBuffered::Update(const SurfaceDescriptor& aImage,
> +                          SurfaceDescriptor* aResult) {

{ on the next line

::: gfx/layers/composite/ImageHost.h
@@ +24,5 @@
> +  TextureHost* GetTextureHost() MOZ_OVERRIDE { return nullptr; }
> +
> +protected:
> +  ImageHost(Compositor* aCompositor)
> +  : CompositableHost(aCompositor)

More spaces

@@ +95,5 @@
> +
> +// Double buffered ImageHost. We have a single TextureHost and double buffering
> +// is done at the TextureHost/Client level. This is in contrast with buffered
> +// ContentHosts which do their own double buffering (XXX that is better, 
> +// ImageHosts should be changed to follow that model).

Trailing whitespace; file a bug for the XXX comment.

::: gfx/layers/composite/TiledContentHost.cpp
@@ +17,5 @@
> +                                  const nsIntRegion& aInvalidateRegion,
> +                                  const gfxSize& aResolution)
> +{
> +#ifdef GFX_TILEDLAYER_PREF_WARNINGS
> +  printf_stderr("Upload %i, %i, %i, %i\n", aInvalidateRegion.GetBounds().x, aInvalidateRegion.GetBounds().y, aInvalidateRegion.GetBounds().width, aInvalidateRegion.GetBounds().height);

Rather overlong

@@ +84,5 @@
> +void
> +TiledContentHost::ProcessLowPrecisionUploadQueue()
> +{
> +  if (!mPendingLowPrecisionUpload)
> +    return;

{}

@@ +90,5 @@
> +  mLowPrecisionRegionToUpload.And(mLowPrecisionRegionToUpload,
> +                                  mLowPrecisionMainMemoryTiledBuffer.GetValidRegion());
> +  mLowPrecisionVideoMemoryTiledBuffer.SetResolution(
> +    mLowPrecisionMainMemoryTiledBuffer.GetResolution());
> +  // XXX It's assumed that the video memory tiled buffer has an up-to-date

Bug

@@ +111,5 @@
> +TiledContentHost::ProcessUploadQueue(nsIntRegion* aNewValidRegion,
> +                                     TiledLayerProperties* aLayerProperties)
> +{
> +  if (!mPendingUpload)
> +    return;

{}

@@ +127,5 @@
> +  mMainMemoryTiledBuffer.ReadUnlock();
> +  // Release all the tiles by replacing the tile buffer with an empty
> +  // tiled buffer. This will prevent us from doing a double unlock when
> +  // calling  ~TiledThebesLayerComposite.
> +  // XXX: This wont be needed when we do progressive upload and lock

Bug

@@ +233,5 @@
> +    rowCount++;
> +    int32_t tileStartX = aLayerBuffer.GetTileStart(x);
> +    int32_t w = aLayerBuffer.GetScaledTileLength() - tileStartX;
> +    if (x + w > aVisibleRect.x + aVisibleRect.width)
> +      w = aVisibleRect.x + aVisibleRect.width - x;

{}

@@ +239,5 @@
> +    for (int32_t y = aVisibleRect.y; y < aVisibleRect.y + aVisibleRect.height;) {
> +      int32_t tileStartY = aLayerBuffer.GetTileStart(y);
> +      int32_t h = aLayerBuffer.GetScaledTileLength() - tileStartY;
> +      if (y + h > aVisibleRect.y + aVisibleRect.height)
> +        h = aVisibleRect.y + aVisibleRect.height - y;

{}

@@ +273,5 @@
> +  }
> +}
> +
> +void
> +TiledTexture::Validate(gfxReusableSurfaceWrapper* aReusableSurface, Compositor* aCompositor, uint16_t aSize) {

{ on the next line, and arguments too, to get under 80ch

::: gfx/layers/composite/TiledContentHost.h
@@ +35,5 @@
> +    mTextureHost = o.mTextureHost;
> +  }
> +  TiledTexture& operator=(const TiledTexture& o) {
> +    if (this == &o)
> +      return *this;

{}

@@ +87,5 @@
> +
> +  // do nothing, the desctructor in the texture host takes care of releasing resources
> +  void ReleaseTile(TiledTexture aTile) {}
> +
> +  void SwapTiles(TiledTexture& aTileA, TiledTexture& aTileB) {

{ on the next line

@@ +101,5 @@
> +class TiledThebesLayerComposite;
> +
> +/**
> + * ContentHost for tiled Thebes layers. Since tiled layers are special snow
> + * flakes, we don't call UpdateThebes or AddTextureHost, etc. We do call Composite

"special snow flakes", eh? That doesn't say much for outsiders.

Comment 118

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 733135 [details] [diff] [review]
5 texture hosts and clients

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

::: gfx/layers/client/TextureClient.cpp
@@ +62,5 @@
> +  }
> +}
> +
> +void
> +TextureClient::SetIPDLActor(PTextureChild* aChild) {

{ on the next line

@@ +107,5 @@
> +    mSize = aSize;
> +
> +    if (!mForwarder->AllocSurfaceDescriptor(gfxIntSize(mSize.width, mSize.height),
> +                                            mContentType, &mDescriptor)) {
> +      NS_ERROR("creating SurfaceDescriptor failed!");

If this can happen, NS_WARNING

@@ +214,5 @@
> +}
> +
> +
> +TextureClientTile::TextureClientTile(const TextureClientTile& aOther)
> +: TextureClient(mForwarder

Indent two spaces, and, uh, something's not quite right here. Sure you don't want aOther's mForwarder and mTextureInfo?

@@ +263,5 @@
> +    return false;
> +  }
> +
> +  nsRefPtr<gfxPattern> pattern;
> +  pattern =  pat ? pat : new gfxPattern(surface);

How about

nsRefPtr<gfxPattern> pattern = pat ? pat : new gfxPattern(surface);

? At least drop the double space after the =.

@@ +325,5 @@
> +  }
> +  return true;
> +}
> +
> +bool AutoLockYCbCrClient::EnsureTextureClient(PlanarYCbCrImage* aImage) {

{ on the next line

::: gfx/layers/client/TextureClient.h
@@ +14,5 @@
> +
> +namespace mozilla {
> +
> +namespace gl {
> +  class GLContext;

No indentation

@@ +48,5 @@
> + *
> + * Ownership of the surface descriptor depends on how the TextureClient/Host is
> + * used by the CompositableClient/Host.
> + */
> +class TextureClient : public RefCounted<TextureClient>

Include refptr

@@ +119,5 @@
> +  virtual void Updated();
> +  virtual void Destroyed();
> +
> +  void SetIPDLActor(PTextureChild* aTextureChild);
> +  PTextureChild* GetIPDLActor() const {

{ on the next line

@@ +123,5 @@
> +  PTextureChild* GetIPDLActor() const {
> +    return mTextureChild;
> +  }
> +
> +  CompositableForwarder* GetForwarder() const {

Same

@@ +182,5 @@
> +  virtual void EnsureAllocated(gfx::IntSize aSize, gfxASurface::gfxContentType aType);
> +
> +  virtual void ReleaseResources();
> +  virtual void SetDescriptor(const SurfaceDescriptor& aDescriptor) MOZ_OVERRIDE;
> +  virtual gfxASurface::gfxContentType GetContentType() MOZ_OVERRIDE { return mContentType; }

The others aren't MOZ_OVERRIDE?

@@ +260,5 @@
> + */
> +class AutoLockTextureClient
> +{
> +public:
> +  AutoLockTextureClient(TextureClient* aTexture) {

{ on the next line *3

::: gfx/layers/composite/TextureHost.cpp
@@ +19,5 @@
> +TemporaryRef<TextureHost> CreateTextureHostD3D9(SurfaceDescriptorType aDescriptorType,
> +                                                uint32_t aTextureHostFlags,
> +                                                uint32_t aTextureFlags)
> +{
> +  NS_RUNTIMEABORT("not implemented");

Fun.

@@ +36,5 @@
> +                               uint32_t aTextureHostFlags,
> +                               uint32_t aTextureFlags)
> +{
> +  switch (Compositor::GetBackend()) {
> +    case LAYERS_OPENGL : return CreateTextureHostOGL(aDescriptorType,

Make this:

  switch (Compositor::GetBackend()) {
  case LAYERS_OPENGL:
    return CreateTextureHostOGL(aDescriptorType, ...

::: gfx/layers/composite/TextureHost.h
@@ +54,5 @@
> + * access to these interfaces.
> + *
> + * This class is used on the compositor side.
> + */
> +class TextureSource : public RefCounted<TextureSource>

As usual

@@ +60,5 @@
> +public:
> +  TextureSource()
> +  {
> +    MOZ_COUNT_CTOR(TextureSource);
> +  };

Drop the extra semicolon, it can cause build failures.

@@ +64,5 @@
> +  };
> +  virtual ~TextureSource()
> +  {
> +    MOZ_COUNT_DTOR(TextureSource);
> +  };

Same.

@@ +157,5 @@
> +   * @param aTextureFlags Flags to pass to the new TextureHost
> +   */
> +  static TemporaryRef<TextureHost> CreateTextureHost(SurfaceDescriptorType aDescriptorType,
> +    		                                             uint32_t aTextureHostFlags,
> +    		                                             uint32_t aTextureFlags);

Indentation is off due to tabs

@@ +250,5 @@
> +    return mTextureParent;
> +  }
> +
> +#ifdef MOZ_LAYERS_HAVE_LOG
> +  virtual const char *Name() =0;

Space after the =

@@ +300,5 @@
> +
> +  // An internal identifier for this texture host. Two texture hosts
> +  // should be considered equal iff their identifiers match. Should
> +  // not be exposed publicly.
> +  virtual uint64_t GetIdentifier() const {

{ to the next line

@@ +324,5 @@
> +public:
> +  virtual ~CompositingRenderTarget() {}
> +
> +#ifdef MOZ_DUMP_PAINTING
> +  virtual already_AddRefed<gfxImageSurface> Dump(Compositor* aCompositor) { return nullptr; }

Overlong

::: gfx/layers/ipc/TextureParent.cpp
@@ +15,5 @@
> +namespace layers {
> +
> +TextureParent::TextureParent(const TextureInfo& aInfo,
> +                             CompositableParent* aCompositable)
> +: mTextureInfo(aInfo), mLastSurfaceType(SurfaceDescriptor::Tnull_t)

Indentation; one member per line

@@ +23,5 @@
> +
> +TextureParent::~TextureParent()
> +{
> +  MOZ_COUNT_DTOR(TextureParent);
> +  mTextureHost = nullptr;

This shouldn't be necessary

@@ +62,5 @@
> +
> +CompositableHost* TextureParent::GetCompositableHost() const
> +{
> +  CompositableParent* actor
> +    = static_cast<CompositableParent*>(Manager());

One line

::: gfx/layers/ipc/TextureParent.h
@@ +27,5 @@
> +
> +  TextureHost* GetTextureHost() const;
> +  CompositableHost* GetCompositableHost() const;
> +
> +  const TextureInfo& GetTextureInfo() const {

{ on the next line * 2

Comment 119

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 733156 [details] [diff] [review]
9 GL Layers and compositor

Several of my comments had't been addressed, so I've tried to address them in 

  http://hg.mozilla.org/projects/graphics/rev/7deb5003e4ad

for most of them, and in

  http://hg.mozilla.org/projects/graphics/rev/0882a5ca6a92

for the GetWidgetSize comment.

I dropped the comments on MaskType enums as it turns out that they use mfbt/TypedEnum.h which disables all arithmetic operators.  Filed bug 859874 about that.

With that, r+.

Comment 120

[Nick Cameron [:nrc]]

Recipe for making a patch:

* apply the 'remove d3d11' patch, rebasing as necessary (https://dl.dropbox.com/u/74741329/d3d11.patch).

* do a rebase (hg pull https://hg.mozilla.org/mozilla-central, hg merge, hg commit)

* hg diff -U 8 -r XXXXXX >all.patch

where XXXXXX is the revision number for the rebase.

Comment 121

[Jeff Muizelaar [:jrmuizel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee5ca214e87c