TextureForwarder and CompositableForwarder need a cleaner separation

VERIFIED FIXED in Firefox 50

Status

()

Core
Graphics: Layers
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: nical, Assigned: nical)

Tracking

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 verified)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 1

a year ago
Currently there's a lot of confusion around TextureForwarder. It is intended to be implemented by the top-level ipdl protocols that own the channel in which a texture may be used (PCompositorBridge and PImageBridge), but it's also implemented by ShadowLayerForwarder for legacy reasons. We end up in some cases where textures are using ShadowLayerForwarder as their TextureForwarder when they should be using the CompositorBridgeChild.
Also TextureChild::mTextureForwarder is never set.
Summary: TextureForwarder isn → TextureForwarder and CompositableForwarder need a cleaner separation
Whiteboard: [gfx-noted]
(Assignee)

Comment 2

a year ago
Created attachment 8764581 [details] [diff] [review]
Forward all TextureForwarder calls on ShadowLayerForwarder to the CompositorBridgeChild

Fixing this the ideal way got a fair bit messy so I fell back to an intermediate solution where ShadowLayerForwarder lies about itself being a TextureForwarder and actually forwards all of the TF methods to the CompositorBridgeChild.

At least this way we make sure that all of the texture stuff is managed by the top-level ipdl protocol even though some of our current code is talking to the ShadowLayerForwarder.

The patch also moves the TextureFactoryIdentifier stuff back to CompositableForwarder, because different ShadowLayerFOrwarders on the same PCOmpositorBridge channel may be using different backends, so that can't be at the TF level (So I also had to do dome plumbing with the TexturePool).

It's not ideal. We need to further separate the two abstractions, and more importantly we have to get to a point where TextureClient can work without a CompositableForwarder (we should not be too far from that).
Assignee: nobody → nical.bugzilla
Attachment #8764581 - Flags: review?(gwright)
(Assignee)

Updated

a year ago
Blocks: 1281169
Blocks: 1281775
(Assignee)

Updated

a year ago
No longer blocks: 1281775
Comment on attachment 8764581 [details] [diff] [review]
Forward all TextureForwarder calls on ShadowLayerForwarder to the CompositorBridgeChild

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

Looks fine to me. Some minor comment nits.

However, I'm curious as to where we're using the ShadowLayerForwarder as our allocator?

::: gfx/layers/client/TextureClient.cpp
@@ +867,5 @@
> +    TextureForwarder* currentTexFwd = mActor->mTextureForwarder;
> +    if (currentFwd != aForwarder) {
> +      // It's a bit iffy but right now ShadowLayerForwarder inherits TextureForwarder
> +      // even though it should not. ShadowLayerForwarder::AsTextureForwarder actually
> +      // returns a pointer to the CompositorBridgeChild.

can you add a comment here saying that CompositorBridgeChild is a singleton in the content process, so this should, in theory, always be the same?

@@ +957,5 @@
> +                                TextureFlags aTextureFlags,
> +                                TextureAllocationFlags aAllocFlags)
> +{
> +  // What we want here is the "real" TextureForwarder. ShadowLayerForwarder,
> +  // while inheriting TextureForwarder, actually forwards all of its TF methids

typo: methods.
Attachment #8764581 - Flags: review?(gwright) → review+
See Also: → bug 1281456
(Assignee)

Comment 4

a year ago
(In reply to George Wright (:gw280) (:gwright) from comment #3)
> Comment on attachment 8764581 [details] [diff] [review]
> Forward all TextureForwarder calls on ShadowLayerForwarder to the
> CompositorBridgeChild
> 
> Review of attachment 8764581 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine to me. Some minor comment nits.
> 
> However, I'm curious as to where we're using the ShadowLayerForwarder as our
> allocator?

More places than I thought when after reviewing the TextureForwarder stuff. Here is a try push that has an assertion that the allocator passed to TextureClient::CreateForDrawing is not using a ShadowLayerForwarder https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bc036e879aada77c036a3fc3c320e6a82431304
The try run isn't done yet but the assert has already blown up in a few tests.


(In reply to George Wright (:gw280) (:gwright) from comment #3)
> can you add a comment here saying that CompositorBridgeChild is a singleton
> in the content process, so this should, in theory, always be the same?

It is a singleton in child processes but this code also runs in the parent process where we may have several CompositorBridgeChilds, so i'd rather not make it sound like we can count on it being a singleton.

Comment 5

a year ago
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a901563fd181
forward ShadowLayerForwarder texture-related methods to CompositorBridgeChild. r=gw280
Backed bug 1281780, bug 1281775 and bug 1167235 out for for OS X 10.10 debug for assertion in TextureClient.cpp during R(C) 1246775-1.html:

Bug 1281780:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9db95c763b66b79f5f46b497f7f769fc7692e6a3

Bug 1281775:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f5b0befdac9c69b63ea60183be171a193012f4f

Bug 1167235:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb7effd4b6fccb7a6ba6314968de49c218a41a77
https://hg.mozilla.org/integration/mozilla-inbound/rev/73deeeaaeb8644a8e1031e599aa2bcca4cdc047a
https://hg.mozilla.org/integration/mozilla-inbound/rev/b773c8510ff9e06f646e9797fd4265d2b0d13cd1
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dcc4d0ee3d713b40bd3347430b78542a15ee1c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/3267fb29a5e14feff10840dabbbcbeefe5ce1f58
https://hg.mozilla.org/integration/mozilla-inbound/rev/d66eb486e0f1d1a297e0aafc7c9c00d962416aff

First push which run the tests: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8ab05556d3264ee4799e25ec32baa21ae145fc95
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=30781702&repo=mozilla-inbound

07:18:27     INFO -  REFTEST TEST-START | file:///builds/slave/test/build/tests/reftest/tests/dom/canvas/crashtests/1246775-1.html
07:18:27     INFO -  REFTEST TEST-LOAD | file:///builds/slave/test/build/tests/reftest/tests/dom/canvas/crashtests/1246775-1.html | 279 / 3044 (9%)
07:18:27     INFO -  ++DOMWINDOW == 88 (0x11bc83000) [pid = 2001] [serial = 880] [outer = 0x12bd5e000]
07:18:27     INFO -  Assertion failure: mBorrowedDrawTarget->refCount() <= mExpectedDtRefs, at /builds/slave/m-in-m64-d-0000000000000000000/build/src/gfx/layers/client/TextureClient.cpp:513
07:18:27     INFO -  #01: XRE_main (in XUL) + 225
07:18:27     INFO -  #02: 0x0204612c (in XUL) + 252
07:18:27     INFO -  #03: 0x00f75a65 (in XUL) + 85
07:18:27     INFO -  #04: 0x00f83b28 (in XUL) + 472
07:18:27     INFO -  #05: 0x00f82e4b (in XUL) + 1195
07:18:27     INFO -  #06: 0x00f83a76 (in XUL) + 294
07:18:27     INFO -  #07: 0x00f82e4b (in XUL) + 1195
07:18:27     INFO -  #08: 0x00f81af9 (in XUL) + 1577
07:18:27     INFO -  #09: 0x030955c7 (in XUL) + 3767
07:18:27     INFO -  #10: 0x030d77e1 (in XUL) + 6577
07:18:27     INFO -  #11: 0x03105ad3 (in XUL) + 1267
07:18:27     INFO -  #12: 0x02058661 (in XUL) + 1681
07:18:27     INFO -  #13: 0x01a07faf (in XUL) + 1151
07:18:27     INFO -  #14: 0x01fe033a (in XUL) + 282
07:18:27     INFO -  #15: 0x04a225a0 (in XUL) + 96
07:18:27     INFO -  #16: 0x04a22282 (in XUL) + 690
07:18:27     INFO -  #17: 0x04bfe772 (in XUL) + 242
07:18:27     INFO -  #18: 0x04bfc681 (in XUL) + 897
07:18:27     INFO -  #19: 0x04a225a0 (in XUL) + 96
07:18:27     INFO -  #20: 0x04a22282 (in XUL) + 690
07:18:27     INFO -  #21: 0x04a1a287 (in XUL) + 36391
07:18:27     INFO -  #22: 0x04a11245 (in XUL) + 517
07:18:27     INFO -  #23: 0x04a22221 (in XUL) + 593
07:18:27     INFO -  #24: 0x04a229de (in XUL) + 46
07:18:27     INFO -  #25: 0x049358de (in XUL) + 990
07:18:27     INFO -  #26: 0x0492c7e1 (in XUL) + 241
07:18:27     INFO -  #27: 0x0492e0d6 (in XUL) + 166
07:18:27     INFO -  #28: 0x04a225a0 (in XUL) + 96
07:18:27     INFO -  #29: 0x04a22282 (in XUL) + 690
07:18:27     INFO -  #30: 0x04a229de (in XUL) + 46
07:18:27     INFO -  #31: 0x04938f62 (in XUL) + 194
07:18:27     INFO -  #32: 0x0491ffcf (in XUL) + 431
07:18:27     INFO -  #33: 0x0492c7e1 (in XUL) + 241
07:18:27     INFO -  #34: 0x0492e0d6 (in XUL) + 166
07:18:27     INFO -  #35: 0x04a225a0 (in XUL) + 96
07:18:27     INFO -  #36: 0x04a22282 (in XUL) + 690
07:18:27     INFO -  #37: 0x04a1a287 (in XUL) + 36391
07:18:27     INFO -  #38: 0x04a11245 (in XUL) + 517
07:18:27     INFO -  #39: 0x04a22221 (in XUL) + 593
07:18:27     INFO -  #40: 0x04a229de (in XUL) + 46
07:18:27     INFO -  #41: 0x047ab3d6 (in XUL) + 198
07:18:27     INFO -  #42: 0x01e08719 (in XUL) + 425
07:18:27     INFO -  #43: 0x021b4ac7 (in XUL) + 359
07:18:27     INFO -  #44: 0x021b3a37 (in XUL) + 1015
07:18:27     INFO -  #45: 0x0219d0df (in XUL) + 223
07:18:27     INFO -  #46: 0x0219dd89 (in XUL) + 1993
07:18:27     INFO -  #47: 0x0218e485 (in XUL) + 421
07:18:27     INFO -  #48: 0x0218df16 (in XUL) + 566
07:18:27     INFO -  #49: 0x0218facc (in XUL) + 4492
07:18:27     INFO -  #50: 0x030bb5c1 (in XUL) + 1617
07:18:27     INFO -  #51: 0x035b8acf (in XUL) + 927
07:18:27     INFO -  #52: 0x035b6d9b (in XUL) + 2955
07:18:27     INFO -  #53: 0x035ba010 (in XUL) + 16
07:18:27     INFO -  #54: 0x00d2e80d (in XUL) + 717
07:18:27     INFO -  #55: 0x00d2e0cb (in XUL) + 459
07:18:27     INFO -  #56: 0x00d2cab2 (in XUL) + 1010
07:18:27     INFO -  #57: 0x00d2d84d (in XUL) + 1149
07:18:27     INFO -  #58: 0x00d2de7d (in XUL) + 13
07:18:27     INFO -  #59: 0x001d87da (in XUL) + 1434
07:18:27     INFO -  #60: 0x01377e8c (in XUL) + 236
07:18:27     INFO -  #61: 0x013668f1 (in XUL) + 1665
07:18:27     INFO -  #62: 0x013e93f7 (in XUL) + 39
07:18:27     INFO -  #63: 0x000f7d90 (in XUL) + 896
07:18:27     INFO -  #64: 0x0013816f (in XUL) + 79
07:18:27     INFO -  #65: 0x02c9d411 (in XUL) + 113
07:18:27     INFO -  #66: CFRunLoopRunSpecific (in CoreFoundation) + 296
07:18:27     INFO -  #67: __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ (in CoreFoundation) + 17
07:18:27     INFO -  #68: __CFRunLoopDoSources0 (in CoreFoundation) + 269
07:18:27     INFO -  #69: __CFRunLoopRun (in CoreFoundation) + 927
07:18:27     INFO -  #70: _BlockUntilNextEventMatchingListInModeWithFilter (in HIToolbox) + 71
07:18:27     INFO -  #71: RunCurrentEventLoopInMode (in HIToolbox) + 235
07:18:27     INFO -  #72: ReceiveNextEventCommon (in HIToolbox) + 431
07:18:27     INFO -  #73: -[NSApplication run] (in AppKit) + 594
07:18:27     INFO -  #74: _DPSNextEvent (in AppKit) + 978
07:18:27     INFO -  #75: 0x02d07d45 (in XUL) + 261
07:18:27     INFO -  #76: -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (in AppKit) + 346
07:18:27     INFO -  #77: 0x02d07036 (in XUL) + 86
07:18:28     INFO -  #78: 0x02d08495 (in XUL) + 421
07:18:28     INFO -  #79: 0x0393fd21 (in XUL) + 97
07:18:28     INFO -  #80: 0x039cc0cd (in XUL) + 7469
07:18:28     INFO -  #81: 0x039cc683 (in XUL) + 675
07:18:28     INFO -  #82: 0x0000000100002003 (in firefox) + 2227
07:18:28  WARNING -  TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/dom/canvas/crashtests/1246775-1.html | application terminated with exit code 1
Flags: needinfo?(nical.bugzilla)

Comment 7

a year ago
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4675dc158077
Forward ShadowLayerForwarder texture-related methods to CompositorBridgeChild. r=gw280
(Assignee)

Comment 8

a year ago
I landed a bunch of independent bugs in one go, but the failure appears to be caused by the canvas patches in bug 1167235.
Flags: needinfo?(nical.bugzilla)

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4675dc158077
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1283269
Depends on: 1283439
No longer depends on: 1283439
I believe we can safely mark this verified fixed on Fx50, based on the crash
stats available for the last 3 months.

  SIGNATURE   | mozilla::layers::ShmemDIBTextureData::Create
  ----------------------------------------------------------
  CRASH STATS | http://tinyurl.com/h2tttmb
  ----------------------------------------------------------
  OVERVIEW    | 0 crashes on nightly 52
	      | 57 crashes on nightly 51
	      | 0 crashes on aurora 51
	      | 0 crashes on beta 50
  ----------------------------------------------------------
  LAST CRASH  | 2016-07-10
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified
You need to log in before you can comment on or make changes to this bug.