Closed
Bug 1281780
Opened 8 years ago
Closed 8 years ago
TextureForwarder and CompositableForwarder need a cleaner separation
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
VERIFIED
FIXED
mozilla50
| Tracking | Status | |
|---|---|---|
| firefox50 | --- | verified |
People
(Reporter: nical, Assigned: nical)
References
Details
(Whiteboard: [gfx-noted])
Attachments
(1 file)
|
29.90 KB,
patch
|
gw280
:
review+
|
Details | Diff | Splinter Review |
No description provided.
|
Assignee | |
Comment 1•8 years 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•8 years ago
|
||
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)
|
||
Comment 3•8 years ago
|
||
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+
|
|
Assignee | |
Comment 4•8 years 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.
Pushed by nsilva@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a901563fd181 forward ShadowLayerForwarder texture-related methods to CompositorBridgeChild. r=gw280
|
||
Comment 6•8 years ago
|
||
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)
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•8 years 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•8 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/4675dc158077
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
|
||
Comment 10•8 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•