703484 - Support OMTC Basic layers

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

When we have a compositor thread, then we'll need to draw on the Gecko thread but composite on another.  So we need a gfx lib happy to run on multiple threads (but does *NOT* need to share state).  That's what this bug covers.

The lib might be cairo or azure.

Comment 1

[Joe Drew (not getting mail)]

Don't need this for fennec, as we're using GL layers for this purpose.

Comment 2

[Marco Castelluccio [:marco]]

I'm trying to get this working.

Comment 3

[Marco Castelluccio [:marco]]

Attachment: Enable software omtc

This needs the patches for OMTC on Linux and the patches for building cairo with mutexes enabled.
To enable software OMTC on Linux, set the environment variable MOZ_USE_OMTC and disable layers acceleration.
To enable software OMTC on Windows, set layers.offmainthreadcomposition.enabled to true and disable layers acceleration.

- On Windows, there's a problem with IPC (really similar to bug 717658).
- Failed assertions in SetRoot and EndTransactionInternal (class BasicLayerManager), "Should be in construction phase: 'InConstruction()'".

Comment 4

[Benoit Girard (:BenWa)]

Comment on attachment 623798 [details] [diff] [review]
Enable software omtc

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

Some trivial nits that would be nice to get fixed. There isn't much point in getting unit tests for these since they wont hit any tests.

::: widget/xpwidgets/nsBaseWidget.cpp
@@ +894,5 @@
>      AsyncChannel *parentChannel = mCompositorParent->GetIPCChannel();
>      AsyncChannel::Side childSide = mozilla::ipc::AsyncChannel::Child;
>      mCompositorChild->Open(parentChannel, childMessageLoop, childSide);
> +    PLayersChild* shadowManager;
> +    if (mUseAcceleratedRendering)

Nit please use {}

@@ +908,5 @@
>          return;
>        }
>        lf->SetShadowManager(shadowManager);
> +
> +      if (mUseAcceleratedRendering)

Nit please use {}

Comment 5

[Benoit Girard (:BenWa)]

Attachment: Part 1: Allow OMTC to be used with basic layers

clean up nit and rebased on trunk

Comment 6

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c6b302a313d

Comment 7

[Marco Castelluccio [:marco]]

Attachment: Part 2: Fix some failing assertions in BasicLayerManager and add assertions to the other layer managers

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Backed out for blowing up Android.

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

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Did we enable thread safety in cairo?

Comment 10

[Benoit Girard (:BenWa)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #9)
> Did we enable thread safety in cairo?

No, that's being handled in bug 705691. Marco seems to have the failures from it under control which were really use using Cairo incorrectly.

I wanted to land part 1 right away to be able to turn on OMTC basic with prefs for testing but we need to look into the failure before that happens.

Comment 11

[Marco Castelluccio [:marco]]

Attachment: Part 3: Fix android whitelist and compositor thread creation

Comment 12

[Ali Juma [:ajuma]]

Comment on attachment 626105 [details] [diff] [review]
Part 3: Fix android whitelist and compositor thread creation

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

Great!

::: widget/android/GfxInfo.cpp
@@ +253,5 @@
>  {
>    if (!mDriverInfo->Length()) {
>      /* The following entry, when uncommented, will allow us to whitelist a
>       * specific device. See the long comment in GetFeatureStatusImpl for more
>       * info. */

The comment above should be changed to say that we're whitelisting all devices on Native Fennec and blocklisting all devices on XUL fennec.

Comment 13

[Marco Castelluccio [:marco]]

(In reply to Ali Juma [:ajuma] from comment #12)
> Comment on attachment 626105 [details] [diff] [review]
> Part 3: Fix android whitelist and compositor thread creation
> 
> Review of attachment 626105 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great!
> 
> ::: widget/android/GfxInfo.cpp
> @@ +253,5 @@
> >  {
> >    if (!mDriverInfo->Length()) {
> >      /* The following entry, when uncommented, will allow us to whitelist a
> >       * specific device. See the long comment in GetFeatureStatusImpl for more
> >       * info. */
> 
> The comment above should be changed to say that we're whitelisting all
> devices on Native Fennec and blocklisting all devices on XUL fennec.

Ahah, I forgot to change the comment :D
I'm waiting for the try build results to see if this completely fixes the android talos regressions: https://tbpl.mozilla.org/?tree=Try&rev=f438a176d311

Comment 14

[Marco Castelluccio [:marco]]

It's still failing the robocheck2 test. I wonder if it's this patch that causes the fail or not.

Comment 15

[Marco Castelluccio [:marco]]

I've tried to redo for the nth time the robocheck2 and it's now green!

Comment 16

[Marco Castelluccio [:marco]]

Attachment: Part 1: Allow OMTC to be used with basic layers

Sorry, forgot to upload unbitrotted patches.

Comment 17

[Marco Castelluccio [:marco]]

Attachment: Part 3: Fix android whitelist and compositor thread creation

Comment 18

[Benoit Girard (:BenWa)]

Do I need to land part 2 before part 3?

Bas any chance you could review part 2 today?

Comment 19

[Marco Castelluccio [:marco]]

The part 1 is needed to enable software omtc.
The part 2 is needed to avoid some crashes, but it introduces other crashes on Windows (because before there wasn't transaction tracking with DirectX layers). Maybe I can split this patch and move the Windows bits to another bug.
The part 3 is needed to make Android use OpenGL layers and not basic layers.

However, there's still another crash I've found on some pages with canvases: "ABORT: Swapped-in buffer size doesn't match old buffer's!: 'newSize == prevSize'" (in BasicLayers.cpp). I'm going to investigate in the coming days.

Comment 20

[Benoit Girard (:BenWa)]

Comment on attachment 628375 [details] [diff] [review]
Part 1: Allow OMTC to be used with basic layers

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

Comment 21

[Benoit Girard (:BenWa)]

Comment on attachment 628376 [details] [diff] [review]
Part 3: Fix android whitelist and compositor thread creation

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

Comment 22

[Cameron Kaiser [:spectre]]

Does this work on Mac?

Comment 23

[Marco Castelluccio [:marco]]

(In reply to Cameron Kaiser from comment #22)
> Does this work on Mac?

If you want, you can try. I'd like to know if it works (you need to apply the part 2 patch too).

Comment 24

[Ed Morley [:emorley]]

> Part 1: Allow OMTC to be used with basic layers

https://hg.mozilla.org/mozilla-central/rev/bea5301cf21f

> Part 3: Fix android whitelist and compositor thread creation

https://hg.mozilla.org/mozilla-central/rev/dd81ac5bb79d

Comment 26

[Benoit Girard (:BenWa)]

Backed out for regression Mac OMTC OGL. Normally I wouldn't backout considering Mac OMTC OGL is so far away but it's being used to develop Async CSS animation.

Sorry David for taking so long in the backout. I wanted to debug the issues but I was not able to reproduce it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bb09242fbb8
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c806c9077d2

Marco, sorry for backing out your patch. For now maybe you can maintain a patch queue and once we find the issue here we can work on landing your patches, I'm still very interested in working towards OMTC basic for windows.

Comment 27

[David Zbarsky (:dzbarsky)]

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000000
0x0000000102eb82fe in nsChildView::CreateCompositor (this=0x1151af580) at /Users/dzbarsky/mozilla/inbound/widget/cocoa/nsChildView.mm:1745
1745	      (NSOpenGLContext *) manager->gl()->GetNativeData(GLContext::NativeGLContext);
(gdb) bt
#0  0x0000000102eb82fe in nsChildView::CreateCompositor (this=0x1151af580) at /Users/dzbarsky/mozilla/inbound/widget/cocoa/nsChildView.mm:1745
#1  0x0000000102f0adf1 in nsBaseWidget::GetLayerManager (this=0x1151af580, aShadowManager=0x0, aBackendHint=mozilla::layers::LayerManager::LAYERS_NONE, aPersistence=nsIWidget::LAYER_MANAGER_CURRENT, aAllowRetaining=0x0)
    at /Users/dzbarsky/mozilla/inbound/widget/xpwidgets/nsBaseWidget.cpp:929
#2  0x0000000102ebc483 in -[ChildView drawRect:inContext:] (self=0x117a80040, _cmd=0x10451cf66, aRect={origin = {x = 0, y = 0}, size = {width = 502, height = 215}}, aContext=0x10d6f19c0) at /Users/dzbarsky/mozilla/inbound/widget/cocoa/nsChildView.mm:2536
#3  0x0000000102ebbef5 in -[ChildView drawRect:] (self=0x117a80040, _cmd=0x7fff926c3158, aRect={origin = {x = 0, y = 0}, size = {width = 502, height = 215}}) at /Users/dzbarsky/mozilla/inbound/widget/cocoa/nsChildView.mm:2447
#4  0x00007fff91dfb97a in -[NSView _drawRect:clip:] ()
#5  0x00007fff91e28b4f in -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] ()
#6  0x00007fff91e28f7b in -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] ()
#7  0x00007fff91df8f55 in -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] ()
#8  0x00007fff91f45560 in -[NSNextStepFrame _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] ()
#9  0x00007fff91df386f in -[NSView _displayRectIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:] ()
#10 0x00007fff91dec2ed in -[NSView displayIfNeeded] ()
#11 0x00007fff91f45438 in -[NSNextStepFrame displayIfNeeded] ()
#12 0x00007fff91deba2d in _handleWindowNeedsDisplayOrLayoutOrUpdateConstraints ()
#13 0x00007fff99367f40 in __NSFireTimer ()
#14 0x00007fff93f12934 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ ()
#15 0x00007fff93f12486 in __CFRunLoopDoTimer ()
#16 0x00007fff93ef2e11 in __CFRunLoopRun ()
#17 0x00007fff93ef2486 in CFRunLoopRunSpecific ()
#18 0x00007fff980b94d3 in RunCurrentEventLoopInMode ()
#19 0x00007fff980c06d3 in ReceiveNextEventCommon ()
#20 0x00007fff980c060e in BlockUntilNextEventMatchingListInMode ()
#21 0x00007fff91dafe31 in _DPSNextEvent ()
#22 0x00007fff91daf735 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] ()
#23 0x0000000102e9d5e7 in -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (self=0x1003276b0, _cmd=0x7fff926876b0, mask=18446744073709551615, expiration=0x422d63c37f00000d, mode=0x7fff7dbe1ae0, flag=1 '\001')
    at /Users/dzbarsky/mozilla/inbound/widget/cocoa/nsAppShell.mm:176
#24 0x00007fff91dac071 in -[NSApplication run] ()
#25 0x0000000102e9f88c in nsAppShell::Run (this=0x10b87fca0) at /Users/dzbarsky/mozilla/inbound/widget/cocoa/nsAppShell.mm:764
#26 0x0000000102b43516 in nsAppStartup::Run (this=0x10a4b6ce0) at /Users/dzbarsky/mozilla/inbound/toolkit/components/startup/nsAppStartup.cpp:256
#27 0x000000010125836c in XREMain::XRE_mainRun (this=0x7fff5fbfed78) at /Users/dzbarsky/mozilla/inbound/toolkit/xre/nsAppRunner.cpp:3786
#28 0x0000000101258af0 in XREMain::XRE_main (this=0x7fff5fbfed78, argc=5, argv=0x7fff5fbff968, aAppData=0x1000081c0) at /Users/dzbarsky/mozilla/inbound/toolkit/xre/nsAppRunner.cpp:3863
#29 0x0000000101258ecf in XRE_main (argc=5, argv=0x7fff5fbff968, aAppData=0x1000081c0, aFlags=0) at /Users/dzbarsky/mozilla/inbound/toolkit/xre/nsAppRunner.cpp:3939
#30 0x0000000100001d0e in do_main (argc=5, argv=0x7fff5fbff968) at /Users/dzbarsky/mozilla/inbound/browser/app/nsBrowserApp.cpp:157
#31 0x00000001000015cc in main (argc=5, argv=0x7fff5fbff968) at /Users/dzbarsky/mozilla/inbound/browser/app/nsBrowserApp.cpp:296

Comment 28

[Ed Morley [:emorley]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/4bb09242fbb8
https://hg.mozilla.org/mozilla-central/rev/9c806c9077d2

Comment 29

[Marco Castelluccio [:marco]]

Attachment: Part 2: Fix some failing assertions in BasicLayerManager

I've moved the transaction debugging related bits to bug 762660.

Comment 30

[Ali Juma [:ajuma]]

Attachment: Part 1b: Disable Basic Layers OMTC on OS X.

The crashes for OMTC with OGL on OS X are happening because we're trying to use OMTC for the Basic Layer manager that's created for the awesome bar. This, in turn, is a problem since nsChildView assumes that whenever we're using OMTC, we're using OMTC with a GL layer manager.

This patch disables OMTC with Basic Layers on OS X; that is, whenever accelerated layers are turned off, so is OMTC. To be clear, we should still fix up OMTC with Basic Layers on OS X, but that shouldn't block the other work in this bug.

This will allow the backed out patches to be re-landed now.

Comment 31

[Marco Castelluccio [:marco]]

Comment on attachment 631145 [details] [diff] [review]
Part 2: Fix some failing assertions in BasicLayerManager

We could land also this patch that fixes two crashes.

On Windows (and on Mac) there's the crash in bug 760439 that we need to fix (on Windows it happens right at startup).
On Linux there's another crash (doesn't happen at startup but I think it's however related to bug 760439).

Comment 32

[Marco Castelluccio [:marco]]

Attachment: Stacktrace on Linux

The crash doesn't happen at the creation of the buffer, but just a bit after (on the buffer size check). This is why I think the crashes are related.

Comment 33

[Benoit Girard (:BenWa)]

Comment on attachment 631145 [details] [diff] [review]
Part 2: Fix some failing assertions in BasicLayerManager

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

I'm not sure above moving the EndTransaction. Can you justify it?

::: gfx/layers/ipc/ShadowLayers.cpp
@@ +249,5 @@
>  
>  bool
>  ShadowLayerForwarder::EndTransaction(InfallibleTArray<EditReply>* aReplies)
>  {
> +  SAMPLE_LABEL("ShadowLayerForwarder", "EndTransaction");

Yay, typo fix!

::: gfx/layers/ipc/ShadowLayersParent.cpp
@@ +393,5 @@
>  
>    // Ensure that any pending operations involving back and front
>    // buffers have completed, so that neither process stomps on the
>    // other's buffer contents.
>    ShadowLayerManager::PlatformSyncBeforeReplyUpdate();

I think these operations need to happen after the Transaction. If that's not the case it would be good to mention in this bug and perhaps add a comment why that's not the case because it's not obvious to me.

Comment 34

[Marco Castelluccio [:marco]]

(In reply to Benoit Girard (:BenWa) from comment #33)
> Comment on attachment 631145 [details] [diff] [review]
> Part 2: Fix some failing assertions in BasicLayerManager
> 
> Review of attachment 631145 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure above moving the EndTransaction. Can you justify it?

ShadowLayersUpdated calls SetRoot that asserts if not in construction phase. EndTransaction could set the phase to PHASE_NONE, so it should be called after ShadowLayersUpdated.

Comment 35

[Marco Castelluccio [:marco]]

Attachment: Stacktrace on Windows

This crash happens at startup if layers.offmainthreadcomposition.enabled is set to true and layers.acceleration.disabled to true (that is the situation when basic omtc should be really used).
It happens when you type into the awesomebar if layers.offmainthreadcomposition.enabled is set to true and layers.acceleration.disabled to false.

The stacktrace is exactly the same.

Comment 36

[Benoit Girard (:BenWa)]

(In reply to Marco Castelluccio from comment #35)

This is probably the problem fixed in part 1b for mac but this time on Windows. The awesome bar is trying to use OMTC when it shouldn't.

Comment 37

[Ali Juma [:ajuma]]

Parts 1b, 1, and 3 pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=553184a30e40

Comment 38

[Ali Juma [:ajuma]]

Attachment: Part 1c: Disable OMTC on Windows because it crashes.

This disables OMTC on Windows (by ignoring the OMTC pref), preventing users who pref it on from crashing (see Bug 760439).

Comment 39

[Ali Juma [:ajuma]]

Relanded the backed-out patches (Part 1 and Part 3), along with Parts 1b and 1c:
https://hg.mozilla.org/integration/mozilla-inbound/rev/167f6b583776
https://hg.mozilla.org/integration/mozilla-inbound/rev/a85e6820b017
https://hg.mozilla.org/integration/mozilla-inbound/rev/93ae7e5c471c
https://hg.mozilla.org/integration/mozilla-inbound/rev/ada741a2c92f

Keeping as [leave open] since Part 2 isn't ready to land yet.

Comment 40

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/167f6b583776
https://hg.mozilla.org/mozilla-central/rev/a85e6820b017
https://hg.mozilla.org/mozilla-central/rev/93ae7e5c471c
https://hg.mozilla.org/mozilla-central/rev/ada741a2c92f

Comment 41

[Florian Bender]

(In reply to Ali Juma [:ajuma] from comment #39)
> Keeping as [leave open] since Part 2 isn't ready to land yet.

How's that going along?

Comment 42

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

More conversation in bug 913249.  This is coming up, but I don't know if we'll take the patch here or have to start from scratch.  Enough things have changed in the past year+

Comment 43

[Chris Peterson [:cpeterson]]

Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".

Comment 46

[Florian Bender]

(In reply to Milan Sreckovic [:milan] from comment #42)
> More conversation in bug 913249.  This is coming up, but I don't know if
> we'll take the patch here or have to start from scratch.  Enough things have
> changed in the past year+

That bug is now fixed. What are the next steps here?

Comment 47

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

(In reply to Florian Bender from comment #46)
> (In reply to Milan Sreckovic [:milan] from comment #42)
> > More conversation in bug 913249.  This is coming up, but I don't know if
> > we'll take the patch here or have to start from scratch.  Enough things have
> > changed in the past year+
> 
> That bug is now fixed. What are the next steps here?

Linux - bug 994541. I believe you've been following it, but the dependency wasn't listed explicitly.  I just added it.

Comment 48

[Nicolas Silva [:nical]]

The basic compositor is used on several platforms, now. Closing.