865104 - Create a software compositor

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: WIP

We want this to at least work, since the e10s team are working on machines that don't support hardware acceleration.

Attaching what I've got so far. It kind of draws some things.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This is really fantastic. Thanks Matt. Would it be worth it for me to try this out, or is it not ready yet?

Comment 2

[Matt Woodrow (:mattwoodrow)]

Unless you're interested in spending your time debugging it, then I wouldn't bother just yet.

The platform integration component is only done for mac (The nsIWidget functions), and even then it doesn't render about:home correctly.

Comment 3

[Matt Woodrow (:mattwoodrow)]

The main issue with this currently appears to be with the Mac widget integration.

It appears that the CGContext/DrawTarget that we're getting inside StartRemoteDrawing is clipped to only part of the window area.

If I direct all drawing to a temporary surface, then it draws the whole window correctly, but copying that to the window context only updates part of the screen.

I can sometimes change the areas that get updates by moving the window, resizing it, moving other windows around above it etc. My guess is that the setNeedsDisplay call isn't doing enough, but I'm not sure what else would be required.

CC'ing some cocoa experts in case they have any ideas.

I'll also have a go at doing the linux part tomorrow. Karl, does this link sound similar to what we are going to want? http://cairographics.org/threaded_animation_with_cairo/

Comment 4

[Markus Stange [:mstange]]

You might be missing a [window flushWindow]. Other than that I don't have any ideas, I'll try your patch tomorrow.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Thanks Markus, that indeed fixes it.

Unfortunately we now can get into a deadlock situation when resizing, but that's probably not too hard to fix.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: WIP v2

Rather than waste too much more time on platform integration, I decided to synchronously wait on the compositor whenever we get a paint event on the main thread. This ruins a lot of the point of having OMTC, but should be sufficient for what we need right now.

You'll need to set layers.offmainthreadcomposition.enabled = true, and layers.offmainthreadcomposition.prefer-basic = true.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: WIP v3

Rewrote texture coords handling code to be correct, which fixed a bunch of drawing/scrolling bugs.

Also fixed plugins and video on mac. I think this should be fairly feature complete now, will start running reftests to find more bugs.

Comment 8

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

Comment on attachment 742912 [details] [diff] [review]
WIP v3

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

::: gfx/layers/Makefile.in
@@ +32,5 @@
>          BasicImages.cpp \
>          BasicLayerManager.cpp \
>          BasicCanvasLayer.cpp \
>          BasicColorLayer.cpp \
> +		BasicCompositor.cpp \

Fix indent

::: gfx/layers/composite/TextureHost.h
@@ +77,5 @@
>     * Cast to an TextureSource for the OpenGL backend.
>     */
>    virtual TextureSourceOGL* AsSourceOGL() { return nullptr; }
> +
> +  virtual BasicTextureSource* AsBasicSource() { return nullptr; }

AsSourceBasic

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This works great as far as I can tell. I started with MOZ_USE_OMTC=1 and set the prefs from comment 6, and it worked! Are there any big issues involved with landing this patch?

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Implement BasicCompositor

This should be landable, and usable for testing e10s.

Comment 11

[Nick Cameron [:nrc]]

Comment on attachment 743372 [details] [diff] [review]
Implement BasicCompositor

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

Reviewed everything except BasicCompositor.* Looks good so far.

::: dom/ipc/TabChild.cpp
@@ +2171,5 @@
>  void
>  TabChild::NotifyPainted()
>  {
> +    if (UseDirectCompositor() &&
> +        (!mNotified || mTextureFactoryIdentifier.mParentBackend == LAYERS_BASIC)) {

Why do we care about the backend type here? This at least needs a comment, but it would be nice if this could be abstracted away some how.

::: gfx/layers/basic/BasicLayerManager.cpp
@@ +1266,5 @@
> +      }
> +    }
> +    mShadowTarget = nullptr;
> +    mDummyTarget = nullptr;
> +  }

Can we factor this lot out into a method rather than copy/pasting it

::: gfx/layers/client/CompositableClient.cpp
@@ +112,5 @@
>      MOZ_ASSERT(false, "Unhandled texture client type");
>    }
>  
> +  if (!result) {
> +    return nullptr;

This worries me a bit because all callers assume this method will return a texture client. Could we return some kind of dummy texture client which just renders a black box or something?

::: gfx/layers/client/ImageClient.cpp
@@ +28,5 @@
>    RefPtr<ImageClient> result = nullptr;
>    switch (aCompositableHostType) {
>    case BUFFER_IMAGE_SINGLE:
> +    if (aForwarder->GetCompositorBackendType() == LAYERS_OPENGL ||
> +        aForwarder->GetCompositorBackendType() == LAYERS_BASIC) {

we should factor this out into a method, or multiple methods, it will get ridiculous once we have a few different compositors

::: gfx/layers/client/ImageClient.h
@@ +81,5 @@
>                       CompositableType aType);
>  
>    virtual bool UpdateImage(ImageContainer* aContainer, uint32_t aContentFlags);
>  
> +  bool EnsureTextureClient(TextureClientType aType);

Please comment on what the return value means

::: gfx/layers/ipc/CompositorParent.cpp
@@ +1130,5 @@
> +  mLayerManager->SetCompositorID(mCompositorID);
> +
> +  if (!mLayerManager->Initialize()) {
> +    NS_ERROR("Failed to init Compositor");
> +    return NULL;

nullptr

::: gfx/layers/opengl/GLManager.cpp
@@ +73,5 @@
> +    if (Compositor::GetBackend() == LAYERS_OPENGL) {
> +      return new GLManagerCompositor(static_cast<CompositorOGL*>(
> +        static_cast<LayerManagerComposite*>(aManager)->GetCompositor()));
> +    } else {
> +      return nullptr;

drop the else branch and let it fall through to the error

::: layout/ipc/RenderFrameParent.cpp
@@ +714,2 @@
>      nsIntPoint offset = GetContentRectLayerOffset(aFrame, aBuilder);
> +    layer->SetVisibleRegion(aVisibleRect - offset);

I have no idea if this is right, please ask someone else if you are not 100% sure

@@ +911,5 @@
>      // to /dev/null.
>      return;
>    }
>  
> +  docFrame->InvalidateLayer(nsDisplayItem::TYPE_REMOTE);

ditto

::: widget/xpwidgets/nsBaseWidget.cpp
@@ +897,5 @@
>  
>    TextureFactoryIdentifier textureFactoryIdentifier;
>    PLayerTransactionChild* shadowManager;
> +  mozilla::layers::LayersBackend backendHint;
> +  if (Preferences::GetBool("layers.offmainthreadcomposition.prefer-basic", false)) {

Can we just use the existing HWA disabled pref, rather than adding a new one?

Comment 12

[Nick Cameron [:nrc]]

Comment on attachment 743372 [details] [diff] [review]
Implement BasicCompositor

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

LGTM, only thing I worry about is the mCopyTarget business, which doesn't really look right to me, but maybe I missed something. Might be worth getting Bas or roc to glance at DrawSurfaceWithTextureCoords, it looks good to me, but I don't think I would notice if you missed something out.

::: gfx/layers/basic/BasicCompositor.cpp
@@ +15,5 @@
> +using namespace mozilla::gfx;
> +
> +namespace layers {
> +
> +class TextureSourceBasic : public TextureHost

TextureHostBasic?

@@ +60,5 @@
> +
> +  BasicCompositor *mCompositor;
> +  RefPtr<SourceSurface> mSurface;
> +  nsRefPtr<gfxImageSurface> mThebesImage;
> +  nsRefPtr<gfxASurface> mThebes;

mThebesSurface?

@@ +142,5 @@
> +BasicCompositor::GetCurrentRenderTarget()
> +{
> +  return mRenderTarget;
> +}
> +static void

nit newline

@@ +143,5 @@
> +{
> +  return mRenderTarget;
> +}
> +static void
> +DrawSurfaceWithTextureCoords(DrawTarget *aDest,

a few comments in this method would be nice

@@ +244,5 @@
> +                                   aOpacity, sourceMask, maskTransform);
> +      break;
> +    }
> +    case EFFECT_COMPONENT_ALPHA: {
> +      NS_RUNTIMEABORT("Can't (easily) support component alpha with BasicCompositor!");

if YCbCr only gives a warning, so should this

@@ +270,5 @@
> +  Rect rect = Rect(0, 0, intRect.width, intRect.height);
> +  mWidgetSize = intRect.Size();
> +
> +  if (mCopyTarget) {
> +    mDrawTarget = gfxPlatform::GetPlatform()->CreateOffscreenDrawTarget(IntSize(1,1), FORMAT_B8G8R8A8);

I'm not sure that all these targets are doing the right thing. We set mDrawTarget here, but later in the method use mRenderTarget->mDrawTarget unconditionally. We never seem to use mCopyTarget except to set it up and tear it down. And at the very least we need a comment here saying why we create a 1x1 draw target if there is a copy target.

@@ +273,5 @@
> +  if (mCopyTarget) {
> +    mDrawTarget = gfxPlatform::GetPlatform()->CreateOffscreenDrawTarget(IntSize(1,1), FORMAT_B8G8R8A8);
> +  }
> +  if (!mDrawTarget) {
> +    *aRenderBoundsOut = Rect();

check for null aRenderBoundsOut

@@ +284,5 @@
> +  if (aRenderBoundsOut) {
> +    *aRenderBoundsOut = rect;
> +  }
> +
> +  if (!aClipRectIn) {

switch branches to avoid testing for !

@@ +342,5 @@
> +void
> +BasicCompositor::PrepareViewport(const gfx::IntSize& aSize,
> +                                 const gfxMatrix& aWorldTransform)
> +{
> +}

can you stick these trivial methods in the header rather than in the cpp? And the earlier trivial methods too.

::: gfx/layers/basic/BasicCompositor.h
@@ +80,5 @@
> +
> +  virtual const char* Name() const { return "Basic"; }
> +
> +  virtual nsIWidget* GetWidget() const MOZ_OVERRIDE { return mWidget; }
> +  virtual const nsIntSize& GetWidgetSize() MOZ_OVERRIDE {

nit: newline for opening brace

@@ +93,5 @@
> +  nsIntSize mWidgetSize;
> +
> +  RefPtr<gfx::DrawTarget> mDrawTarget;
> +  RefPtr<BasicCompositingRenderTarget> mRenderTarget;
> +  nsRefPtr<gfxContext> mCopyTarget;

please add a comment to state what this is for

Comment 13

[Matt Woodrow (:mattwoodrow)]

Making a list of things that I know need to be finished before we could use this by default:

* Add widget integration so we can actually paint off the main thread
* Only composite the changed region of the widget, rather than compositing the entire window every time
* Fix bugs with Mask layers
* Implement 3d transforms rendering
* Copy BasicLayers double buffering optimization code, currently we always double-buffer.
* Convert videos to RGBA / readback plugins (this may be a mac only issue) on the compositor thread rather than the main thread.

Comment 14

[Matt Woodrow (:mattwoodrow)]

> ::: gfx/layers/client/CompositableClient.cpp
> @@ +112,5 @@
> >      MOZ_ASSERT(false, "Unhandled texture client type");
> >    }
> >  
> > +  if (!result) {
> > +    return nullptr;
> 
> This worries me a bit because all callers assume this method will return a
> texture client. Could we return some kind of dummy texture client which just
> renders a black box or something?

The intent here is to return nothing if the current compositor can't support that TextureClient type. That way the caller can chose a different type and try that instead.

> 
> ::: gfx/layers/client/ImageClient.cpp
> @@ +28,5 @@
> >    RefPtr<ImageClient> result = nullptr;
> >    switch (aCompositableHostType) {
> >    case BUFFER_IMAGE_SINGLE:
> > +    if (aForwarder->GetCompositorBackendType() == LAYERS_OPENGL ||
> > +        aForwarder->GetCompositorBackendType() == LAYERS_BASIC) {
> 
> we should factor this out into a method, or multiple methods, it will get
> ridiculous once we have a few different compositors

What did you have in mind exactly?

> 
> ::: gfx/layers/opengl/GLManager.cpp
> @@ +73,5 @@
> > +    if (Compositor::GetBackend() == LAYERS_OPENGL) {
> > +      return new GLManagerCompositor(static_cast<CompositorOGL*>(
> > +        static_cast<LayerManagerComposite*>(aManager)->GetCompositor()));
> > +    } else {
> > +      return nullptr;
> 
> drop the else branch and let it fall through to the error

I explicitly want to avoid the error condition here (since it signifies a BasicCompositor). We can then just skip trying to draw the overlay and not explode.

Comment 15

[Nick Cameron [:nrc]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #14)
> > ::: gfx/layers/client/CompositableClient.cpp
> > @@ +112,5 @@
> > >      MOZ_ASSERT(false, "Unhandled texture client type");
> > >    }
> > >  
> > > +  if (!result) {
> > > +    return nullptr;
> > 
> > This worries me a bit because all callers assume this method will return a
> > texture client. Could we return some kind of dummy texture client which just
> > renders a black box or something?
> 
> The intent here is to return nothing if the current compositor can't support
> that TextureClient type. That way the caller can chose a different type and
> try that instead.
> 

The problem is that none of the compositables expect this. If you want to do this, then I think you need to add documentation here, and null checks to all the callers.

> > 
> > ::: gfx/layers/client/ImageClient.cpp
> > @@ +28,5 @@
> > >    RefPtr<ImageClient> result = nullptr;
> > >    switch (aCompositableHostType) {
> > >    case BUFFER_IMAGE_SINGLE:
> > > +    if (aForwarder->GetCompositorBackendType() == LAYERS_OPENGL ||
> > > +        aForwarder->GetCompositorBackendType() == LAYERS_BASIC) {
> > 
> > we should factor this out into a method, or multiple methods, it will get
> > ridiculous once we have a few different compositors
> 
> What did you have in mind exactly?
> 

Possibly a static method on the compositable clients, e.g., ImageClientSingle::SupportsBackend(LayersBackend aBackend). I'm open to other ideas.

> > 
> > ::: gfx/layers/opengl/GLManager.cpp
> > @@ +73,5 @@
> > > +    if (Compositor::GetBackend() == LAYERS_OPENGL) {
> > > +      return new GLManagerCompositor(static_cast<CompositorOGL*>(
> > > +        static_cast<LayerManagerComposite*>(aManager)->GetCompositor()));
> > > +    } else {
> > > +      return nullptr;
> > 
> > drop the else branch and let it fall through to the error
> 
> I explicitly want to avoid the error condition here (since it signifies a
> BasicCompositor). We can then just skip trying to draw the overlay and not
> explode.

ok

Comment 16

[Karl Tomlinson (:karlt)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> I'll also have a go at doing the linux part tomorrow. Karl, does this link
> sound similar to what we are going to want?
> http://cairographics.org/threaded_animation_with_cairo/

Although "It is considered good practice to draw on a widget during its
expose_event only", doing the final copy to the window on the main thread
loses some of the advantage of OMTC.  (I haven't looked at what Gecko does on
other platforms.)

  There are times when drawing during the GTK expose signal is going to make
  some things easier such as synchronization with the window manager during
  resize events [1] because GTK handles that for us.

  For most animations though, it should be possible to update the window on
  another thread.  If Gecko keeps track of the parts of the window that it
  wants to update (or just that it wants to update the whole window) without
  calling gdk_window_invalidate_rect, then it can distinguish between the
  drawing that is best done synchronously (GTK expose signal) and that which
  can be done off another thread (Gecko's timing).

  However, if/when we eventually support the new extended window compositor
  synchronization [2], which we and GTK 2 don't yet use, for gating drawing on
  compositor updates, it may be best to handle that ourselves so we can do
  that on the compositing thread.

We don't want to use a GdkPixmap for the cross-thread communication.  GTK3 no
longer has GdkPixmaps and using a cairo or Xlib surface instead will hopefully
remove the need for dealing with GDK's global lock.

[1] See _NET_WM_SYNC_REQUEST in
http://standards.freedesktop.org/wm-spec/wm-spec-1.5.html#idp6363872

[2] http://fishsoup.net/misc/wm-spec-synchronization.html

Comment 17

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0cdabaff0636

Let's leave this open for now to track the remaining work for this to be used by default.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0cdabaff0636

Comment 19

[Bas Schouten (:bas.schouten)]

Comment on attachment 743372 [details] [diff] [review]
Implement BasicCompositor

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

Some drive by comments.

::: gfx/layers/basic/BasicCompositor.cpp
@@ +102,5 @@
> +TemporaryRef<CompositingRenderTarget>
> +BasicCompositor::CreateRenderTarget(const IntRect& aRect, SurfaceInitMode aInit)
> +{
> +  MOZ_ASSERT(aInit != INIT_MODE_COPY);
> +  RefPtr<DrawTarget> target = mDrawTarget->CreateSimilarDrawTarget(aRect.Size(), FORMAT_B8G8R8A8);

We probably want to check the result here.

@@ +106,5 @@
> +  RefPtr<DrawTarget> target = mDrawTarget->CreateSimilarDrawTarget(aRect.Size(), FORMAT_B8G8R8A8);
> +
> +  RefPtr<BasicCompositingRenderTarget> rt = new BasicCompositingRenderTarget(target, aRect.Size());
> +
> +  return rt.forget();

nit: You shouldn't need to do .forget() here, with mfbt refptrs.

@@ +113,5 @@
> +TemporaryRef<CompositingRenderTarget>
> +BasicCompositor::CreateRenderTargetFromSource(const IntRect &aRect,
> +                                              const CompositingRenderTarget *aSource)
> +{
> +  RefPtr<DrawTarget> target = mDrawTarget->CreateSimilarDrawTarget(aRect.Size(), FORMAT_B8G8R8A8);

Same here.

@@ +128,5 @@
> +
> +  RefPtr<SourceSurface> snapshot = source->Snapshot();
> +
> +  rt->mDrawTarget->CopySurface(snapshot, aRect, IntPoint(0, 0));
> +  return rt.forget();

Same here.

::: gfx/layers/client/ImageClient.cpp
@@ +28,5 @@
>    RefPtr<ImageClient> result = nullptr;
>    switch (aCompositableHostType) {
>    case BUFFER_IMAGE_SINGLE:
> +    if (aForwarder->GetCompositorBackendType() == LAYERS_OPENGL ||
> +        aForwarder->GetCompositorBackendType() == LAYERS_BASIC) {

Nicolas and I already agreed we just want to kill these completely. I'm doing this in my landing of D3D11 Compositor.

Comment 20

[Matt Woodrow (:mattwoodrow)]

Attachment: Support YCbCr images with BasicCompositor

Comment 21

[Matt Woodrow (:mattwoodrow)]

Attachment: Use double buffering texture hosts with BasicCompositor

BasicCompositor uses the shmem directly to composite, so we can't have the main thread editing it underneath us.

Comment 22

[Nick Cameron [:nrc]]

Comment on attachment 767108 [details] [diff] [review]
Support YCbCr images with BasicCompositor

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

LGTM, but adding nical to check YCbCr stuff is done in the best way.

::: gfx/layers/basic/BasicCompositor.cpp
@@ +120,5 @@
> +      return;
> +    }
> +
> +    mThebesSurface = mThebesImage =
> +    new gfxImageSurface(size, format);

indent

Comment 23

[Nicolas Silva [:nical]]

Comment on attachment 767108 [details] [diff] [review]
Support YCbCr images with BasicCompositor

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

::: gfx/layers/YCbCrImageDataSerializer.h
@@ +67,5 @@
>     * Return a pointer to the begining of the data buffer.
>     */
>    uint8_t* GetData();
> +
> +  void ToPlanarYCbCrImageData(PlanarYCbCrImage::Data& aData);

I would prefer not to add a dependency to ImageContainer.h just for this. This method seems to only be used in one place. maybe it could be an external function somewhere else. I wouldn't block the patch for that, but in general I'd like that we avoid adding too many dependencies in headers just for helper functions that are not used in many places.

Comment 24

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/70e712e831e6
https://hg.mozilla.org/integration/mozilla-inbound/rev/62c2d5e2f60a

Comment 25

[Matt Woodrow (:mattwoodrow)]

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

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/70e712e831e6
https://hg.mozilla.org/mozilla-central/rev/62c2d5e2f60a
https://hg.mozilla.org/mozilla-central/rev/b71a993bce69