Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2902733003:
RemoteMediaStreamImpl using WebRtcMediaStreamTrackMap. (Closed)
|
Created:
3 years, 7 months ago by hbos_chromium Modified:
3 years, 6 months ago Reviewers:
Guido Urdaneta CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, chfremer+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemoteMediaStreamImpl using WebRtcMediaStreamTrackMap.
Make the RemoteMediaStreamImpl, which represent remote streams, use
the WebRtcMediaStreamTrackMap and AdapterRef classes for the handling
of initializing, getting and uninitializing remote tracks.
This is one step closer to decoupling streams and tracks.
PeerConnectionDependencyFactory's GetWebRtcSignalingThread is only
callable from the main thread, and during initialization of the map
this thread does not exist yet. Since the webrtc signaling thread was
only used for DCHECKs and getting ahold of the webrtc signaling thread
would require significant refactoring, the DCHECKs are updated to
check that we are not on the main thread instead of checking that we
are on the webrtc signaling thread. This is good enough for our
purposes.
BUG=705901, 700916
Review-Url: https://codereview.chromium.org/2902733003
Cr-Commit-Position: refs/heads/master@{#478943}
Committed: https://chromium.googlesource.com/chromium/src/+/7ff812ea2691302e2307ff0e5c645cce989d420b
Patch Set 1 #
Total comments: 20
Patch Set 2 : Addressed comments #Patch Set 3 : "Must be invoked on the main thread." #Depends on Patchset: Messages
Total messages: 39 (32 generated)
Description was changed from ========== RemoteMediaStreamImpl using WebRtcMediaStreamTrackCollection. WIP... BUG= ========== to ========== RemoteMediaStreamImpl using WebRtcMediaStreamTrackCollection. WIP... BUG=705901, 700916 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by hbos@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...
Description was changed from ========== RemoteMediaStreamImpl using WebRtcMediaStreamTrackCollection. WIP... BUG=705901, 700916 ========== to ========== RemoteMediaStreamImpl using WebRtcMediaStreamTrackMap. WIP... BUG=705901, 700916 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hbos@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: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by hbos@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...
Description was changed from ========== RemoteMediaStreamImpl using WebRtcMediaStreamTrackMap. WIP... BUG=705901, 700916 ========== to ========== RemoteMediaStreamImpl using WebRtcMediaStreamTrackMap. Make the RemoteMediaStreamImpl, which represent remote streams, use the WebRtcMediaStreamTrackMap and AdapterRef classes for the handling of initializing, getting and uninitializing remote tracks. This is one step closer to decoupling streams and tracks. BUG=705901, 700916 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #4 (id:100001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
The CQ bit was checked by hbos@chromium.org to run a CQ dry run
Patchset #1 (id:120001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== RemoteMediaStreamImpl using WebRtcMediaStreamTrackMap. Make the RemoteMediaStreamImpl, which represent remote streams, use the WebRtcMediaStreamTrackMap and AdapterRef classes for the handling of initializing, getting and uninitializing remote tracks. This is one step closer to decoupling streams and tracks. BUG=705901, 700916 ========== to ========== RemoteMediaStreamImpl using WebRtcMediaStreamTrackMap. Make the RemoteMediaStreamImpl, which represent remote streams, use the WebRtcMediaStreamTrackMap and AdapterRef classes for the handling of initializing, getting and uninitializing remote tracks. This is one step closer to decoupling streams and tracks. PeerConnectionDependencyFactory's GetWebRtcSignalingThread is only callable from the main thread, and during initialization of the map this thread does not exist yet. Since the webrtc signaling thread was only used for DCHECKs and getting ahold of the webrtc signaling thread would require significant refactoring, the DCHECKs are updated to check that we are not on the main thread instead of checking that we are on the webrtc signaling thread. This is good enough for our purposes. BUG=705901, 700916 ==========
hbos@chromium.org changed reviewers: + guidou@chromium.org
Please take a look, guidou
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... File content/renderer/media/remote_media_stream_impl.cc (right): https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... content/renderer/media/remote_media_stream_impl.cc:90: AdapterRefMap new_adapter_refs = GetAdapterRefMapFromWebRtcStream( Is there a race if you access track_adapter_map_ from this thread? https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... content/renderer/media/remote_media_stream_impl.cc:112: weak_factory_(this) { If necessary, can you add a check to ensure this runs on a correct thread? https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... File content/renderer/media/remote_media_stream_impl.h (right): https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... content/renderer/media/remote_media_stream_impl.h:32: typedef std::map< using instead of typedef. see http://chromium-cpp.appspot.com/ https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... content/renderer/media/remote_media_stream_impl.h:54: class Observer Can you move the definition of this class to the .cc? https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc (right): https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc:42: DCHECK(!main_thread->BelongsToCurrentThread()); Would a ThreadChecker/SequenceChecker (detached in the constructor) be a better option for this? https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc (right): https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:51: : factory_(factory), main_thread_(base::ThreadTaskRunnerHandle::Get()) { How do you know this is the main thread? Shouldn't the variable have another name? https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:93: return GetOrCreateTrackAdapter( What happens if there are concurrent calls to this from different threads?
PTAL guidou https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... File content/renderer/media/remote_media_stream_impl.cc (right): https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... content/renderer/media/remote_media_stream_impl.cc:90: AdapterRefMap new_adapter_refs = GetAdapterRefMapFromWebRtcStream( On 2017/06/12 14:25:28, Guido Urdaneta wrote: > Is there a race if you access track_adapter_map_ from this thread? No, track_adapter_map_ is thread-safe. https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... content/renderer/media/remote_media_stream_impl.cc:112: weak_factory_(this) { On 2017/06/12 14:25:28, Guido Urdaneta wrote: > If necessary, can you add a check to ensure this runs on a correct thread? Added a DCHECK that we are not on the main thread, that should suffice. https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... File content/renderer/media/remote_media_stream_impl.h (right): https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... content/renderer/media/remote_media_stream_impl.h:32: typedef std::map< On 2017/06/12 14:25:29, Guido Urdaneta wrote: > using instead of typedef. see http://chromium-cpp.appspot.com/ Done. https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... content/renderer/media/remote_media_stream_impl.h:54: class Observer On 2017/06/12 14:25:28, Guido Urdaneta wrote: > Can you move the definition of this class to the .cc? Done. https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc (right): https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc:42: DCHECK(!main_thread->BelongsToCurrentThread()); On 2017/06/12 14:25:29, Guido Urdaneta wrote: > Would a ThreadChecker/SequenceChecker (detached in the constructor) be a better > option for this? I'd like to avoid that because then the test passes on any test that runs on a single thread, and only fails on the subset of tests that would use different threads for different invocations. (This is what other classes did that caused me trouble.) Usually if a method is used the wrong way and called on the wrong thread, it will be called on the wrong thread every time. And if a test is small and only calls it once the detached thread checker is no better than DCHECK(true). I could use a thread checker in combination with !main_thread to ensure that a third thread isn't being used, but I think it would give little value over checking !main_thread. https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc (right): https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:51: : factory_(factory), main_thread_(base::ThreadTaskRunnerHandle::Get()) { On 2017/06/12 14:25:29, Guido Urdaneta wrote: > How do you know this is the main thread? > Shouldn't the variable have another name? It has to be the main thread since it is used by WebRtcMediaStreamTrackAdapter which require it to be the main thread. (Which thread its components is initialized depends on this threading model of main and signaling thread.) Would you prefer if I pass main_thread in as an argument like before, forcing the caller to ensure that it is the main thread? (Code not updated because I have to leave for the day) https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:93: return GetOrCreateTrackAdapter( On 2017/06/12 14:25:29, Guido Urdaneta wrote: > What happens if there are concurrent calls to this from different threads? This is thread safe thanks to use of lock_.
https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc (right): https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:93: return GetOrCreateTrackAdapter( On 2017/06/12 15:05:21, hbos_chromium wrote: > On 2017/06/12 14:25:29, Guido Urdaneta wrote: > > What happens if there are concurrent calls to this from different threads? > > This is thread safe thanks to use of lock_. But the WebRtcMediaStreamTrackAdapter needs this to be on the signaling thread and doesn't allow other threads to invoke it by design.
lgtm % adding a comment about construction on the main thread. https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... File content/renderer/media/remote_media_stream_impl.cc (right): https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... content/renderer/media/remote_media_stream_impl.cc:90: AdapterRefMap new_adapter_refs = GetAdapterRefMapFromWebRtcStream( On 2017/06/12 15:05:20, hbos_chromium wrote: > On 2017/06/12 14:25:28, Guido Urdaneta wrote: > > Is there a race if you access track_adapter_map_ from this thread? > > No, track_adapter_map_ is thread-safe. Acknowledged. https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... content/renderer/media/remote_media_stream_impl.cc:112: weak_factory_(this) { On 2017/06/12 15:05:20, hbos_chromium wrote: > On 2017/06/12 14:25:28, Guido Urdaneta wrote: > > If necessary, can you add a check to ensure this runs on a correct thread? > > Added a DCHECK that we are not on the main thread, that should suffice. Acknowledged. https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc (right): https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc:42: DCHECK(!main_thread->BelongsToCurrentThread()); On 2017/06/12 15:05:21, hbos_chromium wrote: > On 2017/06/12 14:25:29, Guido Urdaneta wrote: > > Would a ThreadChecker/SequenceChecker (detached in the constructor) be a > better > > option for this? > > I'd like to avoid that because then the test passes on any test that runs on a > single thread, and only fails on the subset of tests that would use different > threads for different invocations. (This is what other classes did that caused > me trouble.) Usually if a method is used the wrong way and called on the wrong > thread, it will be called on the wrong thread every time. And if a test is small > and only calls it once the detached thread checker is no better than > DCHECK(true). > > I could use a thread checker in combination with !main_thread to ensure that a > third thread isn't being used, but I think it would give little value over > checking !main_thread. Acknowledged. https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc (right): https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:51: : factory_(factory), main_thread_(base::ThreadTaskRunnerHandle::Get()) { On 2017/06/12 15:05:21, hbos_chromium wrote: > On 2017/06/12 14:25:29, Guido Urdaneta wrote: > > How do you know this is the main thread? > > Shouldn't the variable have another name? > > It has to be the main thread since it is used by WebRtcMediaStreamTrackAdapter > which require it to be the main thread. (Which thread its components is > initialized depends on this threading model of main and signaling thread.) > > Would you prefer if I pass main_thread in as an argument like before, forcing > the caller to ensure that it is the main thread? > > (Code not updated because I have to leave for the day) It's fine this way then. Just add a comment saying that this is intended to be constructed on the main thread. https://codereview.chromium.org/2902733003/diff/140001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:93: return GetOrCreateTrackAdapter( On 2017/06/12 15:07:51, hbos_chromium wrote: > On 2017/06/12 15:05:21, hbos_chromium wrote: > > On 2017/06/12 14:25:29, Guido Urdaneta wrote: > > > What happens if there are concurrent calls to this from different threads? > > > > This is thread safe thanks to use of lock_. > > But the WebRtcMediaStreamTrackAdapter needs this to be on the signaling thread > and doesn't allow other threads to invoke it by design. Acknowledged.
The CQ bit was checked by hbos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from guidou@chromium.org Link to the patchset: https://codereview.chromium.org/2902733003/#ps180001 (title: ""Must be called on the main thread."")
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": 180001, "attempt_start_ts": 1497340827862100,
"parent_rev": "9fc3341ca5afc14bc3ab776a0718e53cce1d49a1", "commit_rev":
"7ff812ea2691302e2307ff0e5c645cce989d420b"}
Message was sent while issue was closed.
Description was changed from ========== RemoteMediaStreamImpl using WebRtcMediaStreamTrackMap. Make the RemoteMediaStreamImpl, which represent remote streams, use the WebRtcMediaStreamTrackMap and AdapterRef classes for the handling of initializing, getting and uninitializing remote tracks. This is one step closer to decoupling streams and tracks. PeerConnectionDependencyFactory's GetWebRtcSignalingThread is only callable from the main thread, and during initialization of the map this thread does not exist yet. Since the webrtc signaling thread was only used for DCHECKs and getting ahold of the webrtc signaling thread would require significant refactoring, the DCHECKs are updated to check that we are not on the main thread instead of checking that we are on the webrtc signaling thread. This is good enough for our purposes. BUG=705901, 700916 ========== to ========== RemoteMediaStreamImpl using WebRtcMediaStreamTrackMap. Make the RemoteMediaStreamImpl, which represent remote streams, use the WebRtcMediaStreamTrackMap and AdapterRef classes for the handling of initializing, getting and uninitializing remote tracks. This is one step closer to decoupling streams and tracks. PeerConnectionDependencyFactory's GetWebRtcSignalingThread is only callable from the main thread, and during initialization of the map this thread does not exist yet. Since the webrtc signaling thread was only used for DCHECKs and getting ahold of the webrtc signaling thread would require significant refactoring, the DCHECKs are updated to check that we are not on the main thread instead of checking that we are on the webrtc signaling thread. This is good enough for our purposes. BUG=705901, 700916 Review-Url: https://codereview.chromium.org/2902733003 Cr-Commit-Position: refs/heads/master@{#478943} Committed: https://chromium.googlesource.com/chromium/src/+/7ff812ea2691302e2307ff0e5c64... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:180001) as https://chromium.googlesource.com/chromium/src/+/7ff812ea2691302e2307ff0e5c64... |
