Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1949753007:
Detach the output surface if bind fails. (Closed)
|
Created:
4 years, 7 months ago by danakj Modified:
4 years, 7 months ago CC:
cc-bugs_chromium.org, chromium-reviews, darin-cc_chromium.org, jam, Ken Russell (switch to Gerrit), mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, piman+watch_chromium.org, piman, no sievers Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDetach the output surface if bind fails.
This destroys the compositor context provider on the correct thread.
R=enne, piman
BUG=608946
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/cbd17b97df7b1d54eae6ceb1a4d5a75d60d61245
Cr-Commit-Position: refs/heads/master@{#392168}
Patch Set 1 #
Total comments: 5
Patch Set 2 : detach: . #Patch Set 3 : detach: . #Patch Set 4 : detach: . #
Messages
Total messages: 26 (8 generated)
Description was changed from ========== Detach the output surface if bind fails. This destroys the compositor context provider on the correct thread. R=enne, piman BUG=608946 ========== to ========== Detach the output surface if bind fails. This destroys the compositor context provider on the correct thread. R=enne, piman BUG=608946 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949753007/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949753007/1
https://codereview.chromium.org/1949753007/diff/1/cc/output/output_surface.cc File cc/output/output_surface.cc (right): https://codereview.chromium.org/1949753007/diff/1/cc/output/output_surface.cc... cc/output/output_surface.cc:217: DetachFromClient(); this will do the MemoryDumpManager::GetInstance()->UnregisterDumpProvider thing - before the Register below. Should we either: 1- do this after the register (i.e. in case of failure we register and unregister immediately) 2- only register & unregister in case of success?
https://codereview.chromium.org/1949753007/diff/1/content/renderer/gpu/compos... File content/renderer/gpu/compositor_output_surface.cc (left): https://codereview.chromium.org/1949753007/diff/1/content/renderer/gpu/compos... content/renderer/gpu/compositor_output_surface.cc:84: if (!HasClient()) I don't understand the changes here in this function, both this and the conditional for remove handler. Does this not get called twice anymore and you're counting on the DCHECK in the base class to catch that?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1949753007/diff/1/content/renderer/gpu/compos... File content/renderer/gpu/compositor_output_surface.cc (left): https://codereview.chromium.org/1949753007/diff/1/content/renderer/gpu/compos... content/renderer/gpu/compositor_output_surface.cc:84: if (!HasClient()) On 2016/05/06 19:43:30, enne wrote: > I don't understand the changes here in this function, both this and the > conditional for remove handler. Does this not get called twice anymore and > you're counting on the DCHECK in the base class to catch that? This calls cc::OutputSurface::DetachFromClient which DCHECKs that client_ is null. There's nothing special about use of this implementation that would call this twice here but not on other output surface impls, and others do not do this early out. I ended up not needing it to make this patch work, cuz I'm doing the detach from inside the base class, so I could undo this change tho. https://codereview.chromium.org/1949753007/diff/1/content/renderer/gpu/compos... content/renderer/gpu/compositor_output_surface.cc:88: output_surface_filter_->RemoveHandlerOnCompositorThread( Moving this inside the if is needed because we can be halfway bound. There's a client and all, but our own BindToClient method early outted so we didn't make our filter_handler_ yet. We did if we made a proxy though.
https://codereview.chromium.org/1949753007/diff/1/cc/output/output_surface.cc File cc/output/output_surface.cc (right): https://codereview.chromium.org/1949753007/diff/1/cc/output/output_surface.cc... cc/output/output_surface.cc:217: DetachFromClient(); On 2016/05/06 19:41:40, piman wrote: > this will do the MemoryDumpManager::GetInstance()->UnregisterDumpProvider thing > - before the Register below. > > Should we either: > 1- do this after the register (i.e. in case of failure we register and > unregister immediately) > 2- only register & unregister in case of success? Argh OutputSurface. There are a lot of bad options here. We can rely on the sub classes calling the base class BindToClient first and early outting if it failed. Then we don't need to call the virtual DetachFromClient and we could just clean up ourselves. Or we can avoid that dependency, but then we need to register just to unregister, as you say. Or store some extra state on this class to know that we didn't fully finish BindToClient.
Oh okay idea from the enne department: OS::BindToClient and DetachFromClient are non-virtual on the base class. They call pure virtual internal methods after doing their own thing that subclasses define. This way we can ensure ordering and clean up more simply...
On 2016/05/06 20:25:08, danakj wrote: > Oh okay idea from the enne department: > > OS::BindToClient and DetachFromClient are non-virtual on the base class. > > They call pure virtual internal methods after doing their own thing that > subclasses define. This way we can ensure ordering and clean up more simply... My tiny patch is suddenly enormous. I'm just going to register always and unregister on failure.
PTAL
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949753007/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949753007/40001
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949753007/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949753007/60001
Seems legit. lgtm
lgtm
The CQ bit was unchecked by danakj@chromium.org
The CQ bit was checked by danakj@chromium.org
Thanks v much both.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949753007/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949753007/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Detach the output surface if bind fails. This destroys the compositor context provider on the correct thread. R=enne, piman BUG=608946 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Detach the output surface if bind fails. This destroys the compositor context provider on the correct thread. R=enne, piman BUG=608946 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/cbd17b97df7b1d54eae6ceb1a4d5a75d60d61245 Cr-Commit-Position: refs/heads/master@{#392168} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/cbd17b97df7b1d54eae6ceb1a4d5a75d60d61245 Cr-Commit-Position: refs/heads/master@{#392168} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
