Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(75)

Issue 2887403003: WebRtcMediaStreamTrackMap added (Closed)

Created:
3 years, 7 months ago by hbos_chromium
Modified:
3 years, 6 months ago
CC:
chfremer+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

WebRtcMediaStreamTrackMap added. This maps id <-> WebRtcMediaStreamTrackAdapter and handles creating track adapters if they don't already exist. Reference counting ensures adapters exists for as long as they are referenced, and that they are disposed and removed from the map when no longer needed. This can in a follow-up be used by WebRtcMediaStreamAdapter ("local streams") and RemoteMediaStreamImpl ("remote streams") to get references to track adapters "owned" by the map instead of directly creating and owning the sinks/adapters that a WebRtcMediaStreamTrackAdapter consists of. This would allow local and remote streams to reference tracks that already exist. The map can also be used to create and own tracks that are not associated with any streams. BUG=705901, 700916 Review-Url: https://codereview.chromium.org/2887403003 Cr-Commit-Position: refs/heads/master@{#478580} Committed: https://chromium.googlesource.com/chromium/src/+/ebab8e6156335da0a2f87cfaea1ad8da7f5e47e1

Patch Set 1 : Including death tests if trying to uninitialize track adapters with !HasOneRef #

Patch Set 2 : Removed death tests due to warning about death tests being unsafe with multiple threads #

Total comments: 11

Patch Set 3 : Rebase #

Patch Set 4 : Renamed to WebRtcMediaStreamTrackAdapterMapTest #

Patch Set 5 : Addressed guidou's comments #

Patch Set 6 : EXPECT_FALSE( instead of EXPECT_EQ(false fixing android compile error #

Total comments: 4

Patch Set 7 : Rebase #

Patch Set 8 : Rebase with master after dependent CL landed #

Patch Set 9 : AdapterRef with ref count and auto-disposing and removing adapters not referenced #

Total comments: 1

Patch Set 10 : CONTENT_EXPORT AdapterRef for win bots to compile #

Total comments: 29

Patch Set 11 : Addressed guidou's comments #

Total comments: 6

Patch Set 12 : Addressed guidou's comments #

Total comments: 4

Patch Set 13 : WrapUnique #

Unified diffs Side-by-side diffs Delta from patch set Stats (+498 lines, -0 lines) Patch
M content/renderer/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -0 lines 0 comments Download
A content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +145 lines, -0 lines 0 comments Download
A content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +140 lines, -0 lines 0 comments Download
A content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map_unittest.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +210 lines, -0 lines 0 comments Download
M content/test/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 62 (45 generated)
hbos_chromium
PTAL guidou
3 years, 7 months ago (2017-05-19 11:50:56 UTC) #7
Guido Urdaneta
https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/webrtc/webrtc_media_stream_track_collection.cc File content/renderer/media/webrtc/webrtc_media_stream_track_collection.cc (right): https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/webrtc/webrtc_media_stream_track_collection.cc#newcode131 content/renderer/media/webrtc/webrtc_media_stream_track_collection.cc:131: DCHECK(main_thread_->BelongsToCurrentThread()); Does this need to acquire the lock? https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/webrtc/webrtc_media_stream_track_collection.h ...
3 years, 7 months ago (2017-05-22 16:14:54 UTC) #10
Guido Urdaneta
https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/webrtc/webrtc_media_stream_track_collection_unittest.cc File content/renderer/media/webrtc/webrtc_media_stream_track_collection_unittest.cc (right): https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/webrtc/webrtc_media_stream_track_collection_unittest.cc#newcode184 content/renderer/media/webrtc/webrtc_media_stream_track_collection_unittest.cc:184: Add a few extra tests to check what happens ...
3 years, 7 months ago (2017-05-22 16:14:55 UTC) #11
hbos_chromium
PTAL guidou. https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/webrtc/webrtc_media_stream_track_collection.cc File content/renderer/media/webrtc/webrtc_media_stream_track_collection.cc (right): https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/webrtc/webrtc_media_stream_track_collection.cc#newcode131 content/renderer/media/webrtc/webrtc_media_stream_track_collection.cc:131: DCHECK(main_thread_->BelongsToCurrentThread()); On 2017/05/22 16:14:44, Guido Urdaneta wrote: ...
3 years, 6 months ago (2017-05-29 13:28:51 UTC) #15
Guido Urdaneta
https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/webrtc/webrtc_media_stream_track_collection.h File content/renderer/media/webrtc/webrtc_media_stream_track_collection.h (right): https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/webrtc/webrtc_media_stream_track_collection.h#newcode40 content/renderer/media/webrtc/webrtc_media_stream_track_collection.h:40: // Invoke on the webrtc signaling thread. Newly created ...
3 years, 6 months ago (2017-05-30 09:02:42 UTC) #22
hbos_chromium
PTAL guidou, AdapterRef added. https://codereview.chromium.org/2887403003/diff/140001/content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc (right): https://codereview.chromium.org/2887403003/diff/140001/content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc#newcode84 content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:84: base::AutoLock cs(lock_); On 2017/05/30 09:02:41, ...
3 years, 6 months ago (2017-06-02 08:52:19 UTC) #29
hbos_chromium
https://codereview.chromium.org/2887403003/diff/230001/content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc (right): https://codereview.chromium.org/2887403003/diff/230001/content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc#newcode41 content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:41: entry()->adapter->Dispose(); Since references are only decreased on the main ...
3 years, 6 months ago (2017-06-02 08:57:14 UTC) #30
Guido Urdaneta
https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc (right): https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc#newcode68 content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:68: return std::unique_ptr<AdapterRef>( use base::MakeUnique https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc#newcode87 content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:87: return std::unique_ptr<AdapterRef>( use ...
3 years, 6 months ago (2017-06-02 13:19:02 UTC) #33
hbos_chromium
PTAL guidou https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc (right): https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc#newcode68 content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:68: return std::unique_ptr<AdapterRef>( On 2017/06/02 13:19:01, Guido Urdaneta ...
3 years, 6 months ago (2017-06-05 11:55:41 UTC) #38
hbos_chromium
https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h (right): https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h#newcode31 content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h:31: struct AdapterEntry { On 2017/06/05 11:55:40, hbos_chromium wrote: > ...
3 years, 6 months ago (2017-06-05 12:25:00 UTC) #41
Guido Urdaneta
https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc (right): https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc#newcode68 content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:68: return std::unique_ptr<AdapterRef>( On 2017/06/05 11:55:40, hbos_chromium wrote: > On ...
3 years, 6 months ago (2017-06-05 13:05:57 UTC) #42
hbos_chromium
PTAL guidou. https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h (right): https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h#newcode31 content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h:31: struct AdapterEntry { On 2017/06/05 13:05:56, Guido ...
3 years, 6 months ago (2017-06-08 09:47:46 UTC) #46
Guido Urdaneta
lgtm % nits https://codereview.chromium.org/2887403003/diff/290001/content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc (right): https://codereview.chromium.org/2887403003/diff/290001/content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc#newcode117 content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:117: return std::unique_ptr<AdapterRef>(new AdapterRef(this, type, it)); nit: ...
3 years, 6 months ago (2017-06-09 11:28:13 UTC) #49
hbos_chromium
PTAL jochen for content/renderer/BUILD.gn https://codereview.chromium.org/2887403003/diff/290001/content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc (right): https://codereview.chromium.org/2887403003/diff/290001/content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc#newcode117 content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:117: return std::unique_ptr<AdapterRef>(new AdapterRef(this, type, it)); ...
3 years, 6 months ago (2017-06-12 08:12:34 UTC) #53
jochen (gone - plz use gerrit)
lgtm
3 years, 6 months ago (2017-06-12 08:34:56 UTC) #55
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2887403003/310001
3 years, 6 months ago (2017-06-12 08:41:15 UTC) #59
commit-bot: I haz the power
3 years, 6 months ago (2017-06-12 10:17:22 UTC) #62
Message was sent while issue was closed.
Committed patchset #13 (id:310001) as
https://chromium.googlesource.com/chromium/src/+/ebab8e6156335da0a2f87cfaea1a...

Powered by Google App Engine
This is Rietveld 408576698