882523 - Get BasicCompositor to composite outside of a paint event

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Currently we have to invalidate with the OS, and then wait for a paint event before we get a native drawing context.

Then we synchronously composite, and copy the contents back into the native context.

The gtk2 paint event code is here: http://mxr.mozilla.org/mozilla-central/source/widget/gtk2/nsWindow.cpp#1998

What we really want is BasicCompositor::BeginFrame to be able to grab a native drawing context from the widget at any time, and composite immediately.

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/basic/BasicCompositor.cpp#268

We want an else branch after the if (mCopyTarget) that calls into a threadsafe mWidget function and returns a useful context.

Karl had some suggestions for doing this here: https://bugzilla.mozilla.org/show_bug.cgi?id=865104#c16

Comment 1

[Matt Woodrow (:mattwoodrow)]

This patch had my WIP approach for doing this: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=865104&attachment=742912

See the BasicCompositor::BeginFrame, nsIWidget, nsChildView.h/mm changes.

Comment 2

[Matt Woodrow (:mattwoodrow)]

The main unresolved problem I had with that patch was the potential for a deadlock.

Drawing on cocoa requires taking a lock on the view.

If we get a paint event on the main thread (which takes the view lock), that can sometimes attempt to send a transaction to the compositor.

The transaction is synchronous, and will wait for the compositor thread to be ready to receive it.

The compositor thread however will often attempt to composite first, enter nsChildView::StartRemoteDrawing and block waiting for the view lock.

Not sure if the linux version will have that problem.

Comment 3

[Matt Woodrow (:mattwoodrow)]

When we get the cocoa drawRect call, the only actual drawing will happen from the compositor, and within a lock/unlockFocus call (from the Start/EndRemoteDrawing calls).

So it might be possible to adds calls to unlockFocus and lockFocus at the start/end of drawRect respectively. This should remove the implicit lock that is causing our deadlock, while still ensuring that all drawing is done while the view is locked.

Markus, does this sound remotely possible?

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Let BasicCompositor draw outside the paint event

Attempt to let the BasicCompositor work correctly. Requires the patches from bug 873944.

Unfortunately this still doesn't work on mac, it deadlocks during resizing sometimes. Seems to work fine at other times though.

http://www.pastebin.mozilla.org/2518321

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Gtk2 implementation

This is a huge guess, I have no idea if it compiles or works.

Should hopefully be a start at least.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Let BasicCompositor draw outside the paint event v2

Comment 7

[Markus Stange [:mstange]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> When we get the cocoa drawRect call, the only actual drawing will happen
> from the compositor, and within a lock/unlockFocus call (from the
> Start/EndRemoteDrawing calls).
> 
> So it might be possible to adds calls to unlockFocus and lockFocus at the
> start/end of drawRect respectively. This should remove the implicit lock
> that is causing our deadlock, while still ensuring that all drawing is done
> while the view is locked.
> 
> Markus, does this sound remotely possible?

Possible yes, but rather unclean, and not guaranteed to work.

Can we instead set a flag inside drawRect, something like "hasLockedFocusOnMainThreadAndIsWaitingForOMTC", which, when set, will prevent Start/EndRemoteDrawing from calling (un)lockFocus? I don't know whether that works, I'll try it later today if I get to it.

Comment 8

[Markus Stange [:mstange]]

I've played around with this a little and still haven't got anything that works.

Is CoreAnimation / CALayer stuff supported on the system which we use BasicLayers on? Maybe that has better omt-drawing properties.

Comment 9

[Markus Stange [:mstange]]

Attachment: demo app for drawing on background thread using CALayer

Looks like it's possible. Matt, do you think this approach is worth investigating more?

Comment 10

[Markus Stange [:mstange]]

Attachment: demo app for drawing on background thread using CALayer

Now with no-deadlock demo. Unfortunately, during window resizing the layer doesn't get its contents to the right point synchronously, so you can see transparent spots during resizing. But at least no black flashing.

Comment 11

[Matt Woodrow (:mattwoodrow)]

I don't see how this would help the deadlock problem, but I might be missing something. Unless CALayer drawing just doesn't require locking.

Is there any way to just get a drawing context rather than going through the callback?

It's going to be a bit ugly to get the compositor side code working with that, but we can probably make it work.

Comment 12

[Markus Stange [:mstange]]

(In reply to Markus Stange from comment #8)
> I've played around with this a little and still haven't got anything that
> works.

The last problem I had was a deadlock during viewWillDraw. The main thread was blocked in mozilla::layers::PLayerTransactionChild::SendUpdate (not in SendFlushRendering!) and the compositor thread in -[NSView lockFocusIfCanDraw]. Until we can guarantee that the compositor thread will never block on the main thread, I don't think we can get the lockFocus approach deadlock-free.

Comment 13

[Markus Stange [:mstange]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> I don't see how this would help the deadlock problem, but I might be missing
> something. Unless CALayer drawing just doesn't require locking.

In the demo it looks like it doesn't require any locking.

> Is there any way to just get a drawing context rather than going through the
> callback?

I don't think so, at least not in a well-documented way. Yeah, that's a little problem, I agree.

Comment 14

[Matt Woodrow (:mattwoodrow)]

I tried setting a 'mLocked' bool inside of our lockFocus override (and clearing it in an unlockFocus) override. And then have StartRemoteDrawing check that, and not try to grab focus if it was set.

That doesn't work either. :(

Comment 15

[Markus Stange [:mstange]]

I had the same experience. That's why I was ready to resort to such extreme measures...
[window flushWindow] blocks, too, as long as the main thread is anywhere in viewWillDraw or drawRect.
I'll look a little more into the CoreAnimation approach.

Comment 16

[Markus Stange [:mstange]]

Taking the Mac part of this bug for now, if you don't mind.

Comment 17

[Markus Stange [:mstange]]

After playing a little with the CoreAnimation approach, I've come to the conclusion that while that approach would be feasible, we wouldn't gain much from it. And we'd have to reimplement the whole DrawWindowOverlay stuff we're doing for OpenGL again for CoreAnimation, because that has all the same drawbacks: no rounded window corners, hides window buttons, and probably hides the window resizer on 10.6 (unconfirmed). The reason for this is simple: CoreAnimation uses an OpenGL context behind the scenes. That's also the reason why it can update the window from a secondary thread without locking - it just uses normal OpenGL drawing calls, just like our CompositorOGL would do in accelerated mode.

But if CoreAnimation is supported on all the systems that Firefox runs on (which it is, right?), and that uses OpenGL compositing, why are we turning off hardware acceleration on some of these systems? I don't have a complete picture of the failure scenarios which that avoids. Are we working around conditions like "Firefox crashes as soon as we attempt to use OpenGL" or only around "the driver doesn't support large enough textures" and "the driver crashes or gives incorrect results in a certain complicated testcase"? If we could assume that simple layer trees always work, then I have a new proposal for this bug: We could use BasicCompositor to composite the basic layers into a GL texture buffer and draw this one texture to the screen using CompositorOGL. Would that work on acceleration blacklisted devices?

Comment 18

[Markus Stange [:mstange]]

Attachment: draw BasicCompositor result to screen using CompositorOGL

It would look something like this. (rough prototype)

Comment 19

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

Benoits, see comment #17.

Comment 20

[Matt Woodrow (:mattwoodrow)]

Having multiple compositors of different types will break some assumptions.

It might be easier to just do manual gl calls within nsChildView.

We also might want to consider people who are building for OSX 10.5.

Comment 21

[Benoit Girard (:BenWa)]

We've dropped support for 10.5 officially. Configurations that get software compositing on mac is (1) 10.6.(old) where we see a slanted/bad stride compositing and (2) GPUs that requires ARB_texture_rectangle like my old macbookpro which has since died. The second problem could be fixed but it's for such a small market share.

That being said according to our estimate our mac accelerated layers usage sits at 99%:
http://people.mozilla.org/~bjacob/gfx_features_stats/#mac

If we make a decision on this data we should confirm it is well collected. Note that we regressed blacklist 10.6.(old) and we got bug report leading to restoring that blacklist so we still have some users with it.

Comment 22

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

Sounds like it might be time to just drop support for non-accelerated layers on Mac.

Comment 23

[Matt Woodrow (:mattwoodrow)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> Sounds like it might be time to just drop support for non-accelerated layers
> on Mac.

I'd quite like to be able to develop and test BasicCompositor without having to leave my comfort zone :)

Comment 24

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

OK then, "manual gl calls within nsChildView" sounds good.

Comment 25

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

(In reply to Benoit Girard (:BenWa) from comment #21)
> That being said according to our estimate our mac accelerated layers usage
> sits at 99%:
> http://people.mozilla.org/~bjacob/gfx_features_stats/#mac

Wait, as discussed on IRC, there is uncertainty over the reliability of this figure: it at least doesn't account for old (< 10.6 ?) versions of OSX where we do not even attempt to get GL layers. Before making a decision based on this data, you definitely want to double check how it's measured. Look for reporter.SetSuccessful(true); in the code for GL layers initialization, and check under what conditions it's attempted on mac. The figure on this graph is the ratio (successful / attempts).

Comment 26

[Markus Stange [:mstange]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #20)
> Having multiple compositors of different types will break some assumptions.
> 
> It might be easier to just do manual gl calls within nsChildView.

I'm ready to fix the static compositor::GetBackend assumption. What other assumptions do you have in mind?

Flipping layers.offmainthreadcomposition.prefer-basic and opening a new window without restarting first already trips over one of these assumptions, so we may want to fix them anyway.

I'm not too keen on the idea of inserting manual raw gl calls. Especially since we'd also have to convert DrawWindowOverlay, along with the texture region uploading we do in UpdateTitlebarImage, and which we also may want to do in Start/EndRemoteDrawing so that we only reupload what we need to (bug 882447).

I'll now take a look at the way we do blocklisting + telemetry and do some bugzilla archeology.

Comment 27

[Nicolas Silva [:nical]]

(In reply to Markus Stange from comment #26)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #20)
> > Having multiple compositors of different types will break some assumptions.
> > 
> > It might be easier to just do manual gl calls within nsChildView.
> 
> I'm ready to fix the static compositor::GetBackend assumption. What other
> assumptions do you have in mind?

It's not simple, partly because of the way we do async texture transfer (the PImageBridge protocol). in short the Compositable client/host pair that is used for texture transfer in, say, async-video, can outlive it's layer tree and be attached to other layer trees. It is created by the ImageBridge protocol rather than the layers and it's lifetime is bound to the video element, rather than to a given layer. If you send video frames to a GL compositor and switch to a software compositor, the second compositor will receive GL textures and it's not going to like it. Then on the compositor side you have received textures that you can't deallocate because you don't have the gl context to do so, etc.
supporting multiple backends at the same time would require for us rethink a lot of the ImageBridge protocol (which is on of the tricky parts of layers) among other things.
Fixing this is not trivial and I think we also used the one backend assumption to simplify a few other things in layers.

Comment 28

[Markus Stange [:mstange]]

Thank you, that's very helpful. However, I think most of what you said applies rather to CompositorParent and maybe LayerManagerComposite than to CompositorOGL, doesn't it? I'm not instantiating any of the former classes, only their actual renderer which doesn't seem to take notice of most of the threading + IPC stuff. The only place I've found where the fact that there exists a separate CompositorOGL at all is leaked to the outside world is the sBackend static variable. But that seems to fall under the "simplify things in layers" part, as I've seen now - Compositor::GetBackend is used in some places where it's hard to get to the actual compositor, for example when the information is needed on the main thread.

In any case, I've looked at the rendering code again and saw that my worries about manual GL calls were mostly unfounded. I can still use TextureImage in there, just not Compositor::BindAndDrawQuad, but that's not that much to reimplement. I'll give it a try.

Comment 29

[Matt Woodrow (:mattwoodrow)]

It might be worthwhile to try use ARB_texture_rectangle (the GL_TEXTURE_RECTANGLE_ARB texture target instead of GL_TEXTURE_2D), since some of the older machines only support this.

Being able to do that would be an advantage over using CompositorOGL (we could make that support texture_rectangle, but with significantly more work).

The main difficulty is that texture_rectangle requires texture sizes that are powers of 2.

Comment 30

[Matt Woodrow (:mattwoodrow)]

Attachment: Let BasicCompositor draw outside the paint event v3

Updated patch. Seems to work really well on both linux and osx (apart from the ocassional deadlock).

Over to Marcus to do the texture upload part for OSX

Comment 31

[Markus Stange [:mstange]]

Attachment: Let BasicCompositor draw outside the paint event v4

Comment 32

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 767110 [details] [diff] [review]
Let BasicCompositor draw outside the paint event v4

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

Looks awesome!

I'll go through it in detail tomorrow, just a few quick comments for (on parts that I wrote, sorry).

Adding nrc as a second reviewer since I wrote parts of the initial patch and probably shouldn't be reviewing them.

The use of objective-c blocks is pretty cool (I didn't know they existed). If you're not confident that they're ok to use etc, then you might want to ask someone familiar with them to review that part. Or not, your call.

::: widget/gtk2/nsWindow.cpp
@@ +5997,5 @@
> +void
> +nsWindow::EndRemoteDrawing()
> +{
> +  //TODO: Do we need to do some sort of flush here to notify
> +  // the OS that we drew things?

Lets just drop this function entirely, since it's not doing anything.

::: widget/nsIWidget.h
@@ +1196,5 @@
>      virtual void DrawWindowOverlay(LayerManager* aManager, nsIntRect aRect) = 0;
>  
> +    virtual mozilla::TemporaryRef<mozilla::gfx::DrawTarget> StartRemoteDrawing() = 0;
> +    virtual void EndRemoteDrawing() = 0;
> +    virtual void CleanupRemoteDrawing() = 0;

Comments on these (including what thread they are called from) would be good.

Comment 33

[Markus Stange [:mstange]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #32)
> The use of objective-c blocks is pretty cool (I didn't know they existed).
> If you're not confident that they're ok to use etc, then you might want to
> ask someone familiar with them to review that part. Or not, your call.

I just got the green light from smichaud on IRC.

Comment 34

[Steven Michaud [:smichaud] (Retired)]

> I just got the green light from smichaud on IRC.

I'm fine with using blocks, in this bug and others.

But there've been bugs with them in the past (when using gcc, see particularly bug 862556 comment #8 and bug 862556 comment #9), and I think we should proceed with caution, at least for a few months.

Comment 35

[Nick Cameron [:nrc]]

Comment on attachment 767110 [details] [diff] [review]
Let BasicCompositor draw outside the paint event v4

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

It makes me pretty sad that we are eroding the modularity of gfx even more by adding graphics code outside of gfx and exposing pretty internal stuff like shader types etc. I feel that this is a bad thing and we should be trying to do the reverse by making gfx more modular and making more of an effort at hiding these kind of APIs. I would be much happier to see some kind of gfx API exposed that meant we didn't have to do this kind of thing. Or, at least, moving the GLPresenter and RectTextureImage into gfx somewhere (if that is practical). But (given that gfx modularity is a distant and probably unacheivable dream) I don't think we should block this bug on that, so I am happy(-ish) for you to take r=me without doing either of those things if you and Matt prefer things this way.

I don't have much experience with nsChildView.mm, so my review of that stuff was pretty cursory. I hope Matt can be detailed there.

::: gfx/gl/GLContext.cpp
@@ +2070,5 @@
>      GLenum format;
>      GLenum type;
>      int32_t pixelSize = gfxASurface::BytePerPixelFromFormat(imageSurface->Format());
>      ShaderProgramType shader;
> +    bool targetIsRect = (aTextureTarget == LOCAL_GL_TEXTURE_RECTANGLE_ARB);

May as well inline this line. If you strongly object to doing that, then please move it inside the case clause so it is nearer to its use and has a smaller scope.

::: gfx/gl/GLContext.h
@@ +876,5 @@
>       * \param aTextureUnit, the texture unit used temporarily to upload the
>       *  surface. This testure may be overridden, clients should not rely on
>       *  the contents of this texture after this call or even on this
>       *  texture unit being active.
> +     * \param aTextureTarget The texture target.

This is not a helpful comment - it just spells out the parameter name. Either give a proper explanation or remove it, I am happy either way.

::: gfx/layers/basic/BasicCompositor.cpp
@@ +313,5 @@
> +                             Rect(0, 0, mWidgetSize.width, mWidgetSize.height),
> +                             DrawSurfaceOptions(),
> +                             DrawOptions());
> +    mDrawTarget = nullptr;
> +    mWidget->EndRemoteDrawing();

Why do we need to snapshot every frame on the common path here? Should this be |else if...|? If this is correct please add a comment to explain why we need to do this.

::: gfx/layers/opengl/LayerManagerOGLProgram.cpp
@@ +126,5 @@
>      AddCommonTextureArgs(result);
>      result.mTextureCount = 1;
>      break;
> +  case gl::BGRARectLayerProgramType:
> +    if (aMask == Mask3d) {

remove the mask cases, but add an assertion for them

::: gfx/layers/opengl/LayerManagerOGLShaders.h
@@ +592,5 @@
> +gl_FragColor = vec4(0.0, 0.0, 1.0, 1.0);\n\
> +}\n\
> +#endif\n\
> +";
> +

I'm assuming this is generated code from genshaders.sh so I don't need to review it. Please let me know if I should check it.

::: gfx/layers/opengl/LayerManagerOGLShaders.txt
@@ +258,5 @@
>  @end
>  
> +// Single texture in BGRA format, but with a Rect texture.
> +// nsChildView needs this for old Mac hardware.
> +@shader sBGRARectTextureLayer<mask:,Mask,Mask3D>FS

it looks like you never use masking, so may as well leave them out - save the shader compile time etc.

@@ +263,5 @@
> +#extension GL_ARB_texture_rectangle : enable
> +
> +$LAYER_FRAGMENT<mask>$
> +
> +/* This should not be used on GL ES */

Either explain why or just remove this comment. Also, prefer // for single line comments

::: widget/cocoa/nsChildView.mm
@@ +251,5 @@
> +// LOCAL_GL_TEXTURE_RECTANGLE_ARB texture target and is automatically backed
> +// by a power-of-two size GL texture. The latter two features are used for
> +// compatibility with older Mac hardware which we block GL layers on.
> +// RectTextureImages are used both for accelerated GL layers drawing and for
> +// OMTC BasicLayers drawing.

Will there be any hardware where even this minimal GL stuff will not work? If this is all we need to support GL on older hardware we should really just do this in Layers.

@@ +252,5 @@
> +// by a power-of-two size GL texture. The latter two features are used for
> +// compatibility with older Mac hardware which we block GL layers on.
> +// RectTextureImages are used both for accelerated GL layers drawing and for
> +// OMTC BasicLayers drawing.
> +class RectTextureImage {

I was a little confused that this class has nothing to do with our TextureImage classes. Is there a better name?

@@ +299,5 @@
> +  GLuint mTexture;
> +  bool mInUpdate;
> +};
> +
> +// GLPresenter

remove this line

@@ +300,5 @@
> +  bool mInUpdate;
> +};
> +
> +// GLPresenter
> +// Used for OpenGL drawing from the compositor thread for OMTC BasicLayers.

Could you explain here why we need to do OpenGL drawing if we are using basic layers.

@@ +312,5 @@
> +  }
> +
> +  GLPresenter(GLContext* aContext);
> +  virtual ~GLPresenter();
> + 

whitespace

@@ +326,5 @@
> +  void EndFrame();
> +
> +  NSOpenGLContext* GetNSOpenGLContext()
> +  {
> +    return (NSOpenGLContext*)mGLContext->GetNativeData(GLContext::NativeGLContext);

use static_cast

@@ +2331,5 @@
> +{
> +  if (!mGLPresenter) {
> +    mGLPresenter = GLPresenter::CreateForWindow(this);
> +
> +    if (!mGLPresenter)

{}

@@ +2409,5 @@
> +  program->SetTextureUnit(0);
> +
> +  aManager->BindAndDrawQuad(program);
> +
> +  aManager->gl()->fBindTexture(LOCAL_GL_TEXTURE_RECTANGLE_ARB, 0);

Could we encapsulate all of this inside RectTextureImage?

@@ +2548,5 @@
> +}
> +
> +void
> +GLPresenter::BindAndDrawQuad(ShaderProgramOGL* aProgram)
> +{ 

whitespace

::: widget/gtk2/nsWindow.cpp
@@ +5984,5 @@
> +
> +  gint width, height;
> +  gdk_drawable_get_size(GDK_DRAWABLE(mGdkWindow), &width, &height);
> +
> +  // Owen Taylor says this is the right thing to do!

I have no idea who Owen Taylor is. Could you explain why this is the right thing to do please? Also use a const rather than the magic numbers please.

@@ +5989,5 @@
> +  width = std::min(32767, width);
> +  height = std::min(32767, height);
> +  gfx::IntSize size(width, height);
> +
> +  return gfx::Factory::CreateDrawTargetForCairoSurface(surf->CairoSurface(),

prefer gfxPlatform::CreateDrawTargetForSurface

::: widget/nsIWidget.h
@@ +23,5 @@
>  #include "nsXULAppAPI.h"
>  #include "mozilla/layers/LayersTypes.h"
>  
> +#include "mozilla/RefPtr.h"
> +#include "mozilla/gfx/2D.h"

we don't need this include, just forward declare DrawTarget

::: widget/xpwidgets/nsBaseWidget.h
@@ +136,5 @@
>    virtual void            CleanupWindowEffects() {}
>    virtual void            PreRender(LayerManager* aManager) {}
>    virtual void            DrawWindowUnderlay(LayerManager* aManager, nsIntRect aRect) {}
>    virtual void            DrawWindowOverlay(LayerManager* aManager, nsIntRect aRect) {}
> +  virtual mozilla::TemporaryRef<mozilla::gfx::DrawTarget> StartRemoteDrawing() { return nullptr; }

Please add a comment explaining the returned DrawTarget. On second thoughts, that comment should be in nsIWidget

Comment 36

[Matt Woodrow (:mattwoodrow)]

(In reply to Nick Cameron [:nrc] from comment #35)
> 
> Why do we need to snapshot every frame on the common path here? Should this
> be |else if...|? If this is correct please add a comment to explain why we
> need to do this.

Currently we do, yes. The majority of platforms require us to buffer drawing to the widget surface, and this is doing that.

OSX happens to be one that doesn't, I have a patch that fixes this.

> 
> Will there be any hardware where even this minimal GL stuff will not work?
> If this is all we need to support GL on older hardware we should really just
> do this in Layers.

We're hoping that all macs (10.6 and up) will be fine with this.

Layers does a lot more than just blitting a single texture to the screen, so we're more likely to hit other bugs.

Supporting TEXTURE_REXTANGLE through all of the compositor would probably let us un-blacklist some drivers (but not all), not sure if it's worth the effort though.

> 
> I have no idea who Owen Taylor is. Could you explain why this is the right
> thing to do please? Also use a const rather than the magic numbers please.

This is my fault, it's copy-pasted from the function below.

We should probably just move this into a shared helper.

Comment 37

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 767110 [details] [diff] [review]
Let BasicCompositor draw outside the paint event v4

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

Looks good, with both of our sets of comments fixed.

::: gfx/gl/GLContext.cpp
@@ +2076,5 @@
>      switch (imageSurface->Format()) {
>          case gfxASurface::ImageFormatARGB32:
>              format = LOCAL_GL_RGBA;
>              type = LOCAL_GL_UNSIGNED_BYTE;
> +            shader = targetIsRect ? BGRARectLayerProgramType : BGRALayerProgramType;

We also probably want to assert if targetIsRect and the format *isn't* ImageFormatARGB32.

We'd use the wrong shaders in that case and silently just draw nothing.

::: widget/cocoa/nsChildView.mm
@@ +2251,5 @@
>    aManager->gl()->fBlendFuncSeparate(LOCAL_GL_ZERO, LOCAL_GL_SRC_ALPHA,
>                                       LOCAL_GL_ZERO, LOCAL_GL_SRC_ALPHA);
> +
> +  gfx3DMatrix flipH = gfx3DMatrix::ScalingMatrix(-1, 1, 1);
> +  gfx3DMatrix flipV = gfx3DMatrix::ScalingMatrix(1, -1, 1);

H and V? X and Y would be clearer to me.

@@ +2352,5 @@
> +    return nullptr;
> +  }
> +
> +  return gfxPlatform::GetPlatform()->
> +    CreateDrawTargetForSurface(surface, gfx::IntSize(renderSize.width, renderSize.height));

This will create a DrawTargetCairo about the gfxQuartzSurface.

I think we'd prefer to be using azure directly (so a DrawTargetCG).

My patch had CreateDrawTargetFromNativeSurface which did this. This could probably be called from within a gfxPlatformMac override of CreateDrawTargetForSurface though.

@@ +2382,5 @@
> +
> +  DrawTextureImage(mGLPresenter, mBasicCompositorImage, nsIntPoint(0, 0));
> +  MaybeDrawTitlebar(mGLPresenter, aRenderRect);
> +  MaybeDrawResizeIndicator(mGLPresenter, aRenderRect);
> +  MaybeDrawRoundedCorners(mGLPresenter, aRenderRect);

DrawWindowOverlay is going to do these too right?

I don't think we want to do them twice.

@@ +2409,5 @@
> +  program->SetTextureUnit(0);
> +
> +  aManager->BindAndDrawQuad(program);
> +
> +  aManager->gl()->fBindTexture(LOCAL_GL_TEXTURE_RECTANGLE_ARB, 0);

Please :)

@@ +2504,5 @@
> +                                     overwriteTexture,
> +                                     updateRegion.GetBounds().TopLeft(),
> +                                     false,
> +                                     LOCAL_GL_TEXTURE0,
> +                                     LOCAL_GL_TEXTURE_RECTANGLE_ARB);

This param seems weird to be last. Might make more sense to have directly after mTexture. Not a big deal though.

Comment 38

[Markus Stange [:mstange]]

Attachment: wip

addressed some of the comments, converted RectTextureImage to use DrawTarget, not finished yet

Comment 39

[Markus Stange [:mstange]]

Attachment: Let BasicCompositor draw outside the paint event v5

(In reply to Matt Woodrow (:mattwoodrow) from comment #32)
> ::: widget/gtk2/nsWindow.cpp
> @@ +5997,5 @@
> > +void
> > +nsWindow::EndRemoteDrawing()
> > +{
> > +  //TODO: Do we need to do some sort of flush here to notify
> > +  // the OS that we drew things?
> 
> Lets just drop this function entirely, since it's not doing anything.

Done.

> ::: widget/nsIWidget.h
> @@ +1196,5 @@
> >      virtual void DrawWindowOverlay(LayerManager* aManager, nsIntRect aRect) = 0;
> >  
> > +    virtual mozilla::TemporaryRef<mozilla::gfx::DrawTarget> StartRemoteDrawing() = 0;
> > +    virtual void EndRemoteDrawing() = 0;
> > +    virtual void CleanupRemoteDrawing() = 0;
> 
> Comments on these (including what thread they are called from) would be good.

Added.

(In reply to Nick Cameron [:nrc] from comment #35)
> ::: gfx/gl/GLContext.cpp
> @@ +2070,5 @@
> >      GLenum format;
> >      GLenum type;
> >      int32_t pixelSize = gfxASurface::BytePerPixelFromFormat(imageSurface->Format());
> >      ShaderProgramType shader;
> > +    bool targetIsRect = (aTextureTarget == LOCAL_GL_TEXTURE_RECTANGLE_ARB);
> 
> May as well inline this line. If you strongly object to doing that, then
> please move it inside the case clause so it is nearer to its use and has a
> smaller scope.

Moved down into an extra if-block which overwrites shader.

> ::: gfx/gl/GLContext.h
> @@ +876,5 @@
> >       * \param aTextureUnit, the texture unit used temporarily to upload the
> >       *  surface. This testure may be overridden, clients should not rely on
> >       *  the contents of this texture after this call or even on this
> >       *  texture unit being active.
> > +     * \param aTextureTarget The texture target.
> 
> This is not a helpful comment - it just spells out the parameter name.
> Either give a proper explanation or remove it, I am happy either way.

Removed.

> ::: gfx/layers/basic/BasicCompositor.cpp
> @@ +313,5 @@
> > +                             Rect(0, 0, mWidgetSize.width, mWidgetSize.height),
> > +                             DrawSurfaceOptions(),
> > +                             DrawOptions());
> > +    mDrawTarget = nullptr;
> > +    mWidget->EndRemoteDrawing();
> 
> Why do we need to snapshot every frame on the common path here? Should this
> be |else if...|? If this is correct please add a comment to explain why we
> need to do this.

Added comment.

> ::: gfx/layers/opengl/LayerManagerOGLProgram.cpp
> @@ +126,5 @@
> >      AddCommonTextureArgs(result);
> >      result.mTextureCount = 1;
> >      break;
> > +  case gl::BGRARectLayerProgramType:
> > +    if (aMask == Mask3d) {
> 
> remove the mask cases, but add an assertion for them

Done.

> 
> ::: gfx/layers/opengl/LayerManagerOGLShaders.h
> @@ +592,5 @@
> > +gl_FragColor = vec4(0.0, 0.0, 1.0, 1.0);\n\
> > +}\n\
> > +#endif\n\
> > +";
> > +
> 
> I'm assuming this is generated code from genshaders.sh so I don't need to
> review it.

Correct.

> ::: gfx/layers/opengl/LayerManagerOGLShaders.txt
> @@ +258,5 @@
> >  @end
> >  
> > +// Single texture in BGRA format, but with a Rect texture.
> > +// nsChildView needs this for old Mac hardware.
> > +@shader sBGRARectTextureLayer<mask:,Mask,Mask3D>FS
> 
> it looks like you never use masking, so may as well leave them out - save
> the shader compile time etc.

Done.

> @@ +263,5 @@
> > +#extension GL_ARB_texture_rectangle : enable
> > +
> > +$LAYER_FRAGMENT<mask>$
> > +
> > +/* This should not be used on GL ES */
> 
> Either explain why or just remove this comment. Also, prefer // for single
> line comments

Removed the comment. This was copied from the other rect shader.

> ::: widget/cocoa/nsChildView.mm
> @@ +251,5 @@
> > +// LOCAL_GL_TEXTURE_RECTANGLE_ARB texture target and is automatically backed
> > +// by a power-of-two size GL texture. The latter two features are used for
> > +// compatibility with older Mac hardware which we block GL layers on.
> > +// RectTextureImages are used both for accelerated GL layers drawing and for
> > +// OMTC BasicLayers drawing.
> 
> Will there be any hardware where even this minimal GL stuff will not work?
> If this is all we need to support GL on older hardware we should really just
> do this in Layers.

I'd be inclined to agree but I don't actually have any old hardware to test with.

> @@ +252,5 @@
> > +// by a power-of-two size GL texture. The latter two features are used for
> > +// compatibility with older Mac hardware which we block GL layers on.
> > +// RectTextureImages are used both for accelerated GL layers drawing and for
> > +// OMTC BasicLayers drawing.
> > +class RectTextureImage {
> 
> I was a little confused that this class has nothing to do with our
> TextureImage classes. Is there a better name?

Don't know - it is pretty similar to TextureImage, just not directly related.

> @@ +299,5 @@
> > +  GLuint mTexture;
> > +  bool mInUpdate;
> > +};
> > +
> > +// GLPresenter
> 
> remove this line

Done.

> @@ +300,5 @@
> > +  bool mInUpdate;
> > +};
> > +
> > +// GLPresenter
> > +// Used for OpenGL drawing from the compositor thread for OMTC BasicLayers.
> 
> Could you explain here why we need to do OpenGL drawing if we are using
> basic layers.

Added a comment.
// We need to use OpenGL for this because there seems to be no other robust
// way of drawing from a secondary thread without locking, which would cause
// deadlocks in our setup. See bug 882523.

> @@ +312,5 @@
> > +  }
> > +
> > +  GLPresenter(GLContext* aContext);
> > +  virtual ~GLPresenter();
> > + 
> 
> whitespace

fixed

> @@ +326,5 @@
> > +  void EndFrame();
> > +
> > +  NSOpenGLContext* GetNSOpenGLContext()
> > +  {
> > +    return (NSOpenGLContext*)mGLContext->GetNativeData(GLContext::NativeGLContext);
> 
> use static_cast

fixed

> @@ +2331,5 @@
> > +{
> > +  if (!mGLPresenter) {
> > +    mGLPresenter = GLPresenter::CreateForWindow(this);
> > +
> > +    if (!mGLPresenter)
> 
> {}

fixed

> @@ +2409,5 @@
> > +  program->SetTextureUnit(0);
> > +
> > +  aManager->BindAndDrawQuad(program);
> > +
> > +  aManager->gl()->fBindTexture(LOCAL_GL_TEXTURE_RECTANGLE_ARB, 0);
> 
> Could we encapsulate all of this inside RectTextureImage?

Moved to RectTextureImage::Draw.

> @@ +2548,5 @@
> > +}
> > +
> > +void
> > +GLPresenter::BindAndDrawQuad(ShaderProgramOGL* aProgram)
> > +{ 
> 
> whitespace

fixed

> ::: widget/gtk2/nsWindow.cpp
> @@ +5984,5 @@
> > +
> > +  gint width, height;
> > +  gdk_drawable_get_size(GDK_DRAWABLE(mGdkWindow), &width, &height);
> > +
> > +  // Owen Taylor says this is the right thing to do!
> 
> I have no idea who Owen Taylor is. Could you explain why this is the right
> thing to do please? Also use a const rather than the magic numbers please.

Replaced this by surf->GetSize().

> @@ +5989,5 @@
> > +  width = std::min(32767, width);
> > +  height = std::min(32767, height);
> > +  gfx::IntSize size(width, height);
> > +
> > +  return gfx::Factory::CreateDrawTargetForCairoSurface(surf->CairoSurface(),
> 
> prefer gfxPlatform::CreateDrawTargetForSurface

Done, but not tested.

> ::: widget/nsIWidget.h
> @@ +23,5 @@
> >  #include "nsXULAppAPI.h"
> >  #include "mozilla/layers/LayersTypes.h"
> >  
> > +#include "mozilla/RefPtr.h"
> > +#include "mozilla/gfx/2D.h"
> 
> we don't need this include, just forward declare DrawTarget

fixed

> ::: widget/xpwidgets/nsBaseWidget.h
> @@ +136,5 @@
> >    virtual void            CleanupWindowEffects() {}
> >    virtual void            PreRender(LayerManager* aManager) {}
> >    virtual void            DrawWindowUnderlay(LayerManager* aManager, nsIntRect aRect) {}
> >    virtual void            DrawWindowOverlay(LayerManager* aManager, nsIntRect aRect) {}
> > +  virtual mozilla::TemporaryRef<mozilla::gfx::DrawTarget> StartRemoteDrawing() { return nullptr; }
> 
> Please add a comment explaining the returned DrawTarget. On second thoughts,
> that comment should be in nsIWidget

Added comments to nsIWidget.

(In reply to Matt Woodrow (:mattwoodrow) from comment #36)
> > Will there be any hardware where even this minimal GL stuff will not work?
> > If this is all we need to support GL on older hardware we should really just
> > do this in Layers.
> 
> We're hoping that all macs (10.6 and up) will be fine with this.

For 10.6.[0-2] I've also disabled subtexture upload now, which seems to have been the cause of bug 629016.

> > I have no idea who Owen Taylor is. Could you explain why this is the right
> > thing to do please? Also use a const rather than the magic numbers please.
> 
> This is my fault, it's copy-pasted from the function below.
> 
> We should probably just move this into a shared helper.

I'm now calling surf->GetSize() instead which should already have the adjusted size.

(In reply to Matt Woodrow (:mattwoodrow) from comment #37)
> ::: gfx/gl/GLContext.cpp
> @@ +2076,5 @@
> >      switch (imageSurface->Format()) {
> >          case gfxASurface::ImageFormatARGB32:
> >              format = LOCAL_GL_RGBA;
> >              type = LOCAL_GL_UNSIGNED_BYTE;
> > +            shader = targetIsRect ? BGRARectLayerProgramType : BGRALayerProgramType;
> 
> We also probably want to assert if targetIsRect and the format *isn't*
> ImageFormatARGB32.

Added assert.

> ::: widget/cocoa/nsChildView.mm
> @@ +2251,5 @@
> >    aManager->gl()->fBlendFuncSeparate(LOCAL_GL_ZERO, LOCAL_GL_SRC_ALPHA,
> >                                       LOCAL_GL_ZERO, LOCAL_GL_SRC_ALPHA);
> > +
> > +  gfx3DMatrix flipH = gfx3DMatrix::ScalingMatrix(-1, 1, 1);
> > +  gfx3DMatrix flipV = gfx3DMatrix::ScalingMatrix(1, -1, 1);
> 
> H and V? X and Y would be clearer to me.

Huh? Sure, as you wish.

> @@ +2352,5 @@
> > +    return nullptr;
> > +  }
> > +
> > +  return gfxPlatform::GetPlatform()->
> > +    CreateDrawTargetForSurface(surface, gfx::IntSize(renderSize.width, renderSize.height));
> 
> This will create a DrawTargetCairo about the gfxQuartzSurface.
> 
> I think we'd prefer to be using azure directly (so a DrawTargetCG).

Converted RectTextureImage to create DrawTargets directly, so we don't need this conversion anymore.

> @@ +2382,5 @@
> > +
> > +  DrawTextureImage(mGLPresenter, mBasicCompositorImage, nsIntPoint(0, 0));
> > +  MaybeDrawTitlebar(mGLPresenter, aRenderRect);
> > +  MaybeDrawResizeIndicator(mGLPresenter, aRenderRect);
> > +  MaybeDrawRoundedCorners(mGLPresenter, aRenderRect);
> 
> DrawWindowOverlay is going to do these too right?

No, because it will be called with a BasicCompositor and refuse to do it on non-GL.

I added a DrawWindowOverlay method overload to reduce code duplication.

> @@ +2409,5 @@
> > +  program->SetTextureUnit(0);
> > +
> > +  aManager->BindAndDrawQuad(program);
> > +
> > +  aManager->gl()->fBindTexture(LOCAL_GL_TEXTURE_RECTANGLE_ARB, 0);
> 
> Please :)

Is this wrong? I don't actually know.

> @@ +2504,5 @@
> > +                                     overwriteTexture,
> > +                                     updateRegion.GetBounds().TopLeft(),
> > +                                     false,
> > +                                     LOCAL_GL_TEXTURE0,
> > +                                     LOCAL_GL_TEXTURE_RECTANGLE_ARB);
> 
> This param seems weird to be last. Might make more sense to have directly
> after mTexture. Not a big deal though.

I added it at the last position because it's a parameter that no other caller needs to change, so it can keep its default value. I could make it switch positions with aTextureUnit, though, if you'd prefer that.

I made two other changes: I turned off OMTC for popups and I moved GLPresenter and RectTextureImage to an unnamed namespace because they don't have an ns prefix and were'n encapsulated in any other namespace. I don't if that's the right thing to do, though.

Comment 40

[Markus Stange [:mstange]]

Attachment: Let BasicCompositor draw outside the paint event v5

Did some small cleanup.

https://tbpl.mozilla.org/?tree=Try&rev=2e46bfdb7250
https://tbpl.mozilla.org/?tree=Try&rev=4e75906c9135

Comment 41

[Markus Stange [:mstange]]

Comment on attachment 768927 [details] [diff] [review]
Let BasicCompositor draw outside the paint event v5

still some bugs left

Comment 42

[Markus Stange [:mstange]]

Attachment: Let BasicCompositor draw outside the paint event v5

Comment 43

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 769343 [details] [diff] [review]
Let BasicCompositor draw outside the paint event v5

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

::: widget/cocoa/nsChildView.mm
@@ +1515,5 @@
>  
>  bool
>  nsChildView::ShouldUseOffMainThreadCompositing()
>  {
> +  // XXX I think we can remove this pref now.

I don't agree.

We have the main OMTC pref enabled by default for mac, but we only want to respect it if we'd get CompositorOGL.

But we still want a way to force-enable OMTC+BasicCompositor for testing purposes.

@@ +2397,5 @@
> +{
> +  [(ChildView*)mView preRender:mGLPresenter->GetNSOpenGLContext()];
> +  mGLPresenter->BeginFrame(aRenderRect.Size());
> +  mBasicCompositorImage->Draw(mGLPresenter, nsIntPoint(0, 0));
> +  DrawWindowOverlay(mGLPresenter, aRenderRect);

This is already being called from DrawWindowOverlay!

::: widget/gtk2/nsWindow.cpp
@@ +5981,5 @@
> +  if (!surf) {
> +    return nullptr;
> +  }
> +
> +  gfx::IntSize size(surf->GetSize().width, surf->GetSize().height);

Return null if this is -1,-1, or maybe assert.

Some surface types don't implement GetSize() and return that as a default (gfxXlibSurface does though).

I think it can also happen if the surface is invalid. Shouldn't be possible to have the window surface invalid though.

Comment 44

[Markus Stange [:mstange]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #43)
> We have the main OMTC pref enabled by default for mac, but we only want to
> respect it if we'd get CompositorOGL.

Why?

> @@ +2397,5 @@
> > +{
> > +  [(ChildView*)mView preRender:mGLPresenter->GetNSOpenGLContext()];
> > +  mGLPresenter->BeginFrame(aRenderRect.Size());
> > +  mBasicCompositorImage->Draw(mGLPresenter, nsIntPoint(0, 0));
> > +  DrawWindowOverlay(mGLPresenter, aRenderRect);
> 
> This is already being called from DrawWindowOverlay!

I added a comment that clarifies that the first DrawWindowOverlay doesn't do anything.

> ::: widget/gtk2/nsWindow.cpp
> @@ +5981,5 @@
> > +  if (!surf) {
> > +    return nullptr;
> > +  }
> > +
> > +  gfx::IntSize size(surf->GetSize().width, surf->GetSize().height);
> 
> Return null if this is -1,-1, or maybe assert.

Returning null now.

I also included the following change:

diff --git a/widget/cocoa/nsChildView.mm b/widget/cocoa/nsChildView.mm
--- a/widget/cocoa/nsChildView.mm
+++ b/widget/cocoa/nsChildView.mm
@@ -2505,21 +2505,16 @@ CanUploadSubtextures()
 void
 RectTextureImage::EndUpdate(bool aKeepSurface)
 {
   MOZ_ASSERT(mInUpdate, "Ending update while not in update");
 
   bool overwriteTexture = false;
   nsIntRegion updateRegion = mUpdateRegion;
   if (!mTexture || (mTextureSize != mBufferSize)) {
-    if (mTexture) {
-      mGLContext->MakeCurrent();
-      mGLContext->fDeleteTextures(1, &mTexture);
-      mTexture = 0;
-    }
     overwriteTexture = true;
     mTextureSize = mBufferSize;
   }
 
   if (overwriteTexture || !CanUploadSubtextures()) {
     updateRegion = nsIntRect(nsIntPoint(0, 0), mTextureSize);
   }
 
Textures apparently don't need to be deleted when being resized. That's great because the fDeleteTextures call is actually really slow.

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

Comment 45

[Matt Woodrow (:mattwoodrow)]

(In reply to Markus Stange [:mstange] from comment #44)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #43)
> > We have the main OMTC pref enabled by default for mac, but we only want to
> > respect it if we'd get CompositorOGL.
> 
> Why?

Even with this awesome patch, BasicCompositor is still nowhere near production quality, but CompositorOGL is.

We want most people to get OMTC+OGL, but for people that can't have nice things (those with blacklisted drivers), we need to stick to main thread BasicLayers.

That might change in the future, but we need to invest a lot more time into BasicCompositor before that can happen.

Comment 46

[Markus Stange [:mstange]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #45)
> BasicCompositor is still nowhere near production quality,

This I hadn't realized. All right :)

Comment 47

[Ryan VanderMeulen [:RyanVM]]

Backed out for Linux reftest/crashtest-ipc crashes.
http://hg.mozilla.org/integration/mozilla-inbound/rev/a659fed0d743

https://tbpl.mozilla.org/php/getParsedLog.php?id=24883803&tree=Mozilla-Inbound

Comment 48

[Markus Stange [:mstange]]

Matt, do I just need to null check mWidget in ~BasicCompositor? Or do I need more checks, like (!mCopyTarget)? Or is mCopyTarget nulled out by then anyway?

Comment 49

[Matt Woodrow (:mattwoodrow)]

Not sure entirely, but we should make sure that both EndFrame and AbortFrame clear mDrawTarget, mRenderTarget and mCopyTarget.

Comment 50

[Matt Woodrow (:mattwoodrow)]

Looks like we also don't hold a reference to mWidget, so that could have been destroyed already.

Comment 51

[Matt Woodrow (:mattwoodrow)]

We should get BasicCompositor::Destroy() called before the widget is torn down.

nsBaseWidget::DestroyCompositor -> CompositorChild::SendWillStop -> CompositorParent::RecvWillStop -> LayerManagerComposite::Destroy -> BasicCompositor::Destroy

We should do any cleanup on mWidget at that point, and then clear the pointer.

Comment 52

[Markus Stange [:mstange]]

Attachment: fix destruction

Is the PopClip change in AbortFrame correct?

Comment 53

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 771809 [details] [diff] [review]
fix destruction

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

Yes, but it's unnecessary. mRenderTarget should be destroyed here anyway.

Comment 54

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f542b4bfbfa

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9f542b4bfbfa

Comment 56

[Olli Pettay [:smaug][[e-mail]]]

Did this cause 
Regression: Fx-Team - Tp5 Optimized MozAfterPaint - MacOSX 10.6 (rev4) - 8.65% increase
---------------------------------------------------------------------------------------
    Previous: avg 427.833 stddev 16.943 of 12 runs up to revision b3ff36cb6a1a
    New     : avg 464.833 stddev 5.952 of 12 runs since revision dde4dcd6fa46
    Change  : +37.000 (8.65% / z=2.184)
    Graph   : http://mzl.la/134Rbac

http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=b3ff36cb6a1a&tochange=dde4dcd6fa46