Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2870333003:
gpu: Remove gpu channel filter and queue from header. (Closed)
Descriptiongpu: Remove gpu channel filter and queue from header.
Remove GpuChannelMessageQueue and GpuChannelMessageFilter from header.
Also remove the dependency between MediaGpuChannel and the filter.
R=piman
BUG=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2870333003
Cr-Commit-Position: refs/heads/master@{#472212}
Committed: https://chromium.googlesource.com/chromium/src/+/7fdbae2153717b20e3fa27e03927f9dde9cde730
Patch Set 1 #
Total comments: 6
Patch Set 2 : review #Patch Set 3 : rebase #
Messages
Total messages: 26 (13 generated)
Description was changed from ========== gpu: Remove gpu channel filter and queue from header. Remove GpuChannelMessageQueue and GpuChannelMessageFilter from header. Also remove the dependency between MediaGpuChannel and the filter. R=piman BUG=none ========== to ========== gpu: Remove gpu channel filter and queue from header. Remove GpuChannelMessageQueue and GpuChannelMessageFilter from header. Also remove the dependency between MediaGpuChannel and the filter. R=piman BUG=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
sunnyps@chromium.org changed reviewers: + sandersd@chromium.org
I based this on top of GPU scheduler although it's not really dependent on that PTAL
https://codereview.chromium.org/2870333003/diff/1/media/gpu/ipc/service/media... File media/gpu/ipc/service/media_gpu_channel.cc (right): https://codereview.chromium.org/2870333003/diff/1/media/gpu/ipc/service/media... media/gpu/ipc/service/media_gpu_channel.cc:76: : channel_(channel), filter_(new MediaGpuChannelFilter(channel_token)) { I'm not very comfortable keeping a reference to the filter, because it makes it very easy to get the threading wrong (and in particular, the filter lifetime is no longer tied to the channel lifetime). The new code doesn't fundamentally change anything, so I suppose for now I'm just asking you to add comments explaining the threading and lifetime considerations that future maintainers should be aware of. https://codereview.chromium.org/2870333003/diff/1/media/gpu/ipc/service/media... File media/gpu/ipc/service/media_gpu_channel.h (right): https://codereview.chromium.org/2870333003/diff/1/media/gpu/ipc/service/media... media/gpu/ipc/service/media_gpu_channel.h:10: #include "base/memory/weak_ptr.h" Nit: Only needed by the impl.
https://codereview.chromium.org/2870333003/diff/1/media/gpu/ipc/service/media... File media/gpu/ipc/service/media_gpu_channel.cc (right): https://codereview.chromium.org/2870333003/diff/1/media/gpu/ipc/service/media... media/gpu/ipc/service/media_gpu_channel.cc:76: : channel_(channel), filter_(new MediaGpuChannelFilter(channel_token)) { On 2017/05/11 21:09:18, sandersd wrote: > I'm not very comfortable keeping a reference to the filter, because it makes it > very easy to get the threading wrong (and in particular, the filter lifetime is > no longer tied to the channel lifetime). > > The new code doesn't fundamentally change anything, so I suppose for now I'm > just asking you to add comments explaining the threading and lifetime > considerations that future maintainers should be aware of. Filter lifetime has never been tied to the (gpu or media) channel lifetime. Can you clarify what you mean by that? I can't remove GpuChannelMessageFilter from the header if MediaGpuChannel depends on it. It seems fundamentally better to rely on the lifetime and behavior of MediaGpuChannelFilter than GpuChannelMessageFilter here. I was also hoping to remove GpuChannel::AsWeakPtr and have MediaGpuChannel have a WeakPtrFactory of its own. WDYT? I'll add a comment about this nonetheless. Side note: MediaGpuChannelFilter should implement OnFilterRemoved and set channel_ to nullptr (and if (channel_) in Send). GpuChannelMessageFilter also doesn't check ipc_channel_ in Send but that's ok because it's not used in a PostTask (maybe needs a DCHECK in Send?).
https://codereview.chromium.org/2870333003/diff/1/media/gpu/ipc/service/media... File media/gpu/ipc/service/media_gpu_channel.cc (right): https://codereview.chromium.org/2870333003/diff/1/media/gpu/ipc/service/media... media/gpu/ipc/service/media_gpu_channel.cc:76: : channel_(channel), filter_(new MediaGpuChannelFilter(channel_token)) { > Filter lifetime has never been tied to the (gpu or media) channel lifetime. Can > you clarify what you mean by that? The specific implementation of this filter guarantees that it won't access the channel unless the channel exists. That's no longer true if its methods can be called by something other than the channel. > I can't remove GpuChannelMessageFilter from the header if MediaGpuChannel > depends on it. It seems fundamentally better to rely on the lifetime and > behavior of MediaGpuChannelFilter than GpuChannelMessageFilter here. I was also > hoping to remove GpuChannel::AsWeakPtr and have MediaGpuChannel have a > WeakPtrFactory of its own. WDYT? This makes sense, but the current implementation takes no special steps to manage that lifetime. We shouldn't rely on it until it is proven safe. > Side note: MediaGpuChannelFilter should implement OnFilterRemoved and set > channel_ to nullptr (and if (channel_) in Send). GpuChannelMessageFilter also > doesn't check ipc_channel_ in Send but that's ok because it's not used in a > PostTask (maybe needs a DCHECK in Send?). The current implementation is safe (because the methods are all sync and are only called by the channel), but these are standard checks that are reasonable to add, especially if the implementation is going to become accessible to code other than the channel.
https://codereview.chromium.org/2870333003/diff/1/media/gpu/ipc/service/media... File media/gpu/ipc/service/media_gpu_channel.cc (right): https://codereview.chromium.org/2870333003/diff/1/media/gpu/ipc/service/media... media/gpu/ipc/service/media_gpu_channel.cc:76: : channel_(channel), filter_(new MediaGpuChannelFilter(channel_token)) { On 2017/05/11 22:45:03, sandersd wrote: > > Filter lifetime has never been tied to the (gpu or media) channel lifetime. > Can > > you clarify what you mean by that? > > The specific implementation of this filter guarantees that it won't access the > channel unless the channel exists. That's no longer true if its methods can be > called by something other than the channel. Oh you mean accessing |channel_| right? That should be solved by implementing OnFilterRemoved and not accessing the channel after that. > > > I can't remove GpuChannelMessageFilter from the header if MediaGpuChannel > > depends on it. It seems fundamentally better to rely on the lifetime and > > behavior of MediaGpuChannelFilter than GpuChannelMessageFilter here. I was > also > > hoping to remove GpuChannel::AsWeakPtr and have MediaGpuChannel have a > > WeakPtrFactory of its own. WDYT? > > This makes sense, but the current implementation takes no special steps to > manage that lifetime. We shouldn't rely on it until it is proven safe. IPC::MessageFilter (the base class) is RefCountedThreadSafe so I don't understand your concern here about lifetime modulo implementing OnFilterRemoved as mentioned above. GpuChannelMessageFilter also doesn't take any special steps here. > > > Side note: MediaGpuChannelFilter should implement OnFilterRemoved and set > > channel_ to nullptr (and if (channel_) in Send). GpuChannelMessageFilter also > > doesn't check ipc_channel_ in Send but that's ok because it's not used in a > > PostTask (maybe needs a DCHECK in Send?). > > The current implementation is safe (because the methods are all sync and are > only called by the channel), but these are standard checks that are reasonable > to add, especially if the implementation is going to become accessible to code > other than the channel. Agreed.
https://codereview.chromium.org/2870333003/diff/1/media/gpu/ipc/service/media... File media/gpu/ipc/service/media_gpu_channel.cc (right): https://codereview.chromium.org/2870333003/diff/1/media/gpu/ipc/service/media... media/gpu/ipc/service/media_gpu_channel.cc:76: : channel_(channel), filter_(new MediaGpuChannelFilter(channel_token)) { > I don't understand your concern here about lifetime modulo implementing > OnFilterRemoved as mentioned above. GpuChannelMessageFilter also doesn't take > any special steps here. I believe that OnFilterRemoved is a sufficient solution to the concerns I have.
lgtm
sandersd@ ptal thanks!
lgtm
The CQ bit was checked by sunnyps@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org Link to the patchset: https://codereview.chromium.org/2870333003/#ps20001 (title: "review")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
The CQ bit was checked by sunnyps@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sunnyps@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org, sandersd@chromium.org Link to the patchset: https://codereview.chromium.org/2870333003/#ps40001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494966794920710,
"parent_rev": "2d5a6e2316c416cd9e012430dd473fe1fadc953d", "commit_rev":
"7fdbae2153717b20e3fa27e03927f9dde9cde730"}
Message was sent while issue was closed.
Description was changed from ========== gpu: Remove gpu channel filter and queue from header. Remove GpuChannelMessageQueue and GpuChannelMessageFilter from header. Also remove the dependency between MediaGpuChannel and the filter. R=piman BUG=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== gpu: Remove gpu channel filter and queue from header. Remove GpuChannelMessageQueue and GpuChannelMessageFilter from header. Also remove the dependency between MediaGpuChannel and the filter. R=piman BUG=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2870333003 Cr-Commit-Position: refs/heads/master@{#472212} Committed: https://chromium.googlesource.com/chromium/src/+/7fdbae2153717b20e3fa27e03927... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/7fdbae2153717b20e3fa27e03927... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
