|
|
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. |
DescriptionWebRtcMediaStreamTrackMap 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 #
Messages
Total messages: 62 (45 generated)
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...
Patchset #1 (id:1) has been deleted
Description was changed from ========== WebRtcMediaStreamTrackCollection BUG= ========== to ========== WebRtcMediaStreamTrackCollection added. This maps id <-> WebRtcMediaStreamTrackAdapter and handles creating track adapters if they don't already exists. 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 collection instead of directly creating and owning the sinks/adapters that a WebRtcMediaStreamTrackAdapter consists of. This would allow local and remote streams to to reference tracks that already exist. The collection can also be used to create and own tracks that are not associated with any streams. BUG=705901, 700916 ==========
hbos@chromium.org changed reviewers: + guidou@chromium.org
Description was changed from ========== WebRtcMediaStreamTrackCollection added. This maps id <-> WebRtcMediaStreamTrackAdapter and handles creating track adapters if they don't already exists. 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 collection instead of directly creating and owning the sinks/adapters that a WebRtcMediaStreamTrackAdapter consists of. This would allow local and remote streams to to reference tracks that already exist. The collection can also be used to create and own tracks that are not associated with any streams. BUG=705901, 700916 ========== to ========== WebRtcMediaStreamTrackCollection added. This maps id <-> WebRtcMediaStreamTrackAdapter and handles creating track adapters if they don't already exist. 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 collection 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 collection can also be used to create and own tracks that are not associated with any streams. BUG=705901, 700916 ==========
PTAL guidou
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_media_stream_track_collection.cc (right): https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/... 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/... File content/renderer/media/webrtc/webrtc_media_stream_track_collection.h (right): https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_collection.h:22: class CONTENT_EXPORT WebRtcMediaStreamTrackCollection Would WebRtcMediaStreamTrackMap be a more descriptive name? Also, add a comment explaining what the class does. https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_collection.h:29: scoped_refptr<WebRtcMediaStreamTrackAdapter> GetLocalTrack( Add comment. What happens if the id is not found? https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_collection.h:35: bool RemoveAndUninitializeLocalTrack(const std::string& id); Add a comment explaining the meaning of the result. https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_collection.h:40: // Invoke on the webrtc signaling thread. Newly created tracks are initialized Since this function has no callback, the detail about initialization being in a post to the main thread seems unnecessary. https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_collection.h:47: void ClearAndUninitializeTracks(); Since there are no versions of these methods without the "AndUninitialize" part, can you just remove that part from the method names? https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_collection.h:60: mutable base::Lock lock_; Would having a separate lock for each map have any practical effect on allowing more concurrency? https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_media_stream_track_collection_unittest.cc (right): https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_collection_unittest.cc:184: Add a few extra tests to check what happens if, for example, you look up an invalid ID.
https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_media_stream_track_collection_unittest.cc (right): https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_collection_unittest.cc:184: Add a few extra tests to check what happens if, for example, you look up an invalid ID.
Patchset #3 (id:60001) 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...
PTAL guidou. https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_media_stream_track_collection.cc (right): https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/... 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: > Does this need to acquire the lock? Nice catch, done. https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_media_stream_track_collection.h (right): https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_collection.h:22: class CONTENT_EXPORT WebRtcMediaStreamTrackCollection On 2017/05/22 16:14:47, Guido Urdaneta wrote: > Would WebRtcMediaStreamTrackMap be a more descriptive name? > Also, add a comment explaining what the class does. Done. Changed to WebRtcMediaStreamTrackAdapterMap. https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_collection.h:29: scoped_refptr<WebRtcMediaStreamTrackAdapter> GetLocalTrack( On 2017/05/22 16:14:45, Guido Urdaneta wrote: > Add comment. What happens if the id is not found? Done. https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_collection.h:35: bool RemoveAndUninitializeLocalTrack(const std::string& id); On 2017/05/22 16:14:44, Guido Urdaneta wrote: > Add a comment explaining the meaning of the result. Done. https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_collection.h:40: // Invoke on the webrtc signaling thread. Newly created tracks are initialized On 2017/05/22 16:14:47, Guido Urdaneta wrote: > Since this function has no callback, the detail about initialization being in a > post to the main thread seems unnecessary. It is relevant because it would be an error to try to use the adapter (its web_track or webrtc_track) before it is initialized and this function does not guarantee that it has completed initialization when returned, only that there is an adapter for the track. A callback could be used but considering the initialization is completed in a post and this is documented, any subsequent post to the main thread would occur after it has been initialized. That is, if you want a "callback" you can post one yourself. Updated comment. https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_collection.h:47: void ClearAndUninitializeTracks(); On 2017/05/22 16:14:44, Guido Urdaneta wrote: > Since there are no versions of these methods without the "AndUninitialize" part, > can you just remove that part from the method names? Done. Renamed to Dispose[Local/Remote]TrackAdapter. https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_collection.h:60: mutable base::Lock lock_; On 2017/05/22 16:14:46, Guido Urdaneta wrote: > Would having a separate lock for each map have any practical effect on allowing > more concurrency? Contention is unlikely. I prefer to have a single lock here for simplicity because I don't think there are any practical benefits to having multiple locks, though I don't have a strong preference. https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_media_stream_track_collection_unittest.cc (right): https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_collection_unittest.cc:184: On 2017/05/22 16:14:49, Guido Urdaneta wrote: > Add a few extra tests to check what happens if, for example, you look up an > invalid ID. Done.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_media_stream_track_collection.h (right): https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_collection.h:40: // Invoke on the webrtc signaling thread. Newly created tracks are initialized On 2017/05/29 13:28:50, hbos_chromium wrote: > On 2017/05/22 16:14:47, Guido Urdaneta wrote: > > Since this function has no callback, the detail about initialization being in > a > > post to the main thread seems unnecessary. > > It is relevant because it would be an error to try to use the adapter (its > web_track or webrtc_track) before it is initialized and this function does not > guarantee that it has completed initialization when returned, only that there is > an adapter for the track. > > A callback could be used but considering the initialization is completed in a > post and this is documented, any subsequent post to the main thread would occur > after it has been initialized. That is, if you want a "callback" you can post > one yourself. > > Updated comment. Acknowledged. https://codereview.chromium.org/2887403003/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_collection.h:60: mutable base::Lock lock_; On 2017/05/29 13:28:51, hbos_chromium wrote: > On 2017/05/22 16:14:46, Guido Urdaneta wrote: > > Would having a separate lock for each map have any practical effect on > allowing > > more concurrency? > > Contention is unlikely. I prefer to have a single lock here for simplicity > because I don't think there are any practical benefits to having multiple locks, > though I don't have a strong preference. Acknowledged. https://codereview.chromium.org/2887403003/diff/140001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc (right): https://codereview.chromium.org/2887403003/diff/140001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:84: base::AutoLock cs(lock_); what does cs mean? (no action necessary, unless it's the initials of your cat's name). https://codereview.chromium.org/2887403003/diff/140001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h (right): https://codereview.chromium.org/2887403003/diff/140001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h:46: // the track adapter when this is called. Is the latter precondition something easy to achieve? If not, would it make sense to use unique_ptrs instead? Would failure to achieve this precondition be a source of crashes and security problems in the wild?
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...
Patchset #8 (id:180001) has been deleted
Patchset #8 (id:200001) 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...
PTAL guidou, AdapterRef added. https://codereview.chromium.org/2887403003/diff/140001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc (right): https://codereview.chromium.org/2887403003/diff/140001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:84: base::AutoLock cs(lock_); On 2017/05/30 09:02:41, Guido Urdaneta wrote: > what does cs mean? (no action necessary, unless it's the initials of your cat's > name). Critical section or scope, inspired by rtc::CritScope. I thought lock(lock_) was a bit silly but maybe not :) I'll change it to scoped_lock, that's a common occurrence for this class, along with lock and auto_lock. https://codereview.chromium.org/2887403003/diff/140001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h (right): https://codereview.chromium.org/2887403003/diff/140001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h:46: // the track adapter when this is called. On 2017/05/30 09:02:41, Guido Urdaneta wrote: > Is the latter precondition something easy to achieve? > If not, would it make sense to use unique_ptrs instead? > Would failure to achieve this precondition be a source of crashes and security > problems in the wild? Based on offline discussions, I made an AdapterRef class. Adapters are disposed and removed from the map when all references are destroyed. :)
https://codereview.chromium.org/2887403003/diff/230001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc (right): https://codereview.chromium.org/2887403003/diff/230001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:41: entry()->adapter->Dispose(); Since references are only decreased on the main thread I don't need the lock to do --ref_count, and std::map iterators not being invalidated by the insertion or removal of other elements, I could probably move the scoped_lock to before doing erase.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc (right): https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... 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... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:87: return std::unique_ptr<AdapterRef>( use base::MakeUnique https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:99: auto it = remote_track_adapters_.find(id); Since the logic is the same as for GetLocalTrackAdapter, is there a simple way to make the code more generic? https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:102: return std::unique_ptr<AdapterRef>( use base::MakeUnique https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:109: DCHECK(webrtc_track); Same genericness argument as with simple Get methods. https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:122: return std::unique_ptr<AdapterRef>( use base::MakeUnique https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h (right): https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h:31: struct AdapterEntry { Can you move the definition of this type to the .cc file? Also, move this to the private section below. I think it's against the style guide to define a private section in the beginning of the class. https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h:38: size_t ref_count; Can you reuse the reference count in adapter instead of adding a separate one? If not, can you make WebRtcMediaStreamTrackAdapter not refcounted to avoid the redundant refcount? https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h:47: enum class Type { kLocal, kRemote }; Move to private section. It's used only by a private constructor. https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h:81: std::map<std::string, AdapterEntry>::iterator it_; A private type alias (e.g., MapEntry = std::map<std::string, AdapterEntry>::iterator) would make it easier to see that this field points to an entry in the map. https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h:88: // Gets a new reference to the local track adapter by ID, or null if no such nit: the -> a https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h:94: // by web track. If no adapter exists for the track one is created and nit: by -> for
Description was changed from ========== WebRtcMediaStreamTrackCollection added. This maps id <-> WebRtcMediaStreamTrackAdapter and handles creating track adapters if they don't already exist. 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 collection 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 collection can also be used to create and own tracks that are not associated with any streams. BUG=705901, 700916 ========== to ========== WebRtcMediaStreamTrackMap added. This maps id <-> WebRtcMediaStreamTrackAdapter and handles creating track adapters if they don't already exist. 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 collection 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 collection can also be used to create and own tracks that are not associated with any streams. BUG=705901, 700916 ==========
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 ========== WebRtcMediaStreamTrackMap added. This maps id <-> WebRtcMediaStreamTrackAdapter and handles creating track adapters if they don't already exist. 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 collection 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 collection can also be used to create and own tracks that are not associated with any streams. BUG=705901, 700916 ========== to ========== WebRtcMediaStreamTrackMap added. This maps id <-> WebRtcMediaStreamTrackAdapter and handles creating track adapters if they don't already exist. 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. ==========
PTAL guidou https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc (right): https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... 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 wrote: > use base::MakeUnique base::MakeUnique actually does not work in this case due to the constructor being private. https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:87: return std::unique_ptr<AdapterRef>( On 2017/06/02 13:19:01, Guido Urdaneta wrote: > use base::MakeUnique Ditto. https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:99: auto it = remote_track_adapters_.find(id); On 2017/06/02 13:19:01, Guido Urdaneta wrote: > Since the logic is the same as for GetLocalTrackAdapter, is there a simple way > to make the code more generic? Done. https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:102: return std::unique_ptr<AdapterRef>( On 2017/06/02 13:19:01, Guido Urdaneta wrote: > use base::MakeUnique Ditto. https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:109: DCHECK(webrtc_track); On 2017/06/02 13:19:01, Guido Urdaneta wrote: > Same genericness argument as with simple Get methods. Done. https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:122: return std::unique_ptr<AdapterRef>( On 2017/06/02 13:19:01, Guido Urdaneta wrote: > use base::MakeUnique Ditto. https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h (right): https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h:31: struct AdapterEntry { On 2017/06/02 13:19:01, Guido Urdaneta wrote: > Can you move the definition of this type to the .cc file? > Also, move this to the private section below. I think it's against the style > guide to define a private section in the beginning of the class. It has to be *defined* in the header for AdapterEntry to be defined due to std::map. It can be moved to the private section, but it still needs to be *declared* before AdapterEntry is defined or it will say undeclared identifier. I changed it to: class WebRtcMediaStreamTrackAdapterMap { private: struct AdapterEntry; public: class AdapterRef { ... }; ... private: struct AdapterEntry { ... }; ... }; The only way to move it out of the header is to change AdapterEntry's std::map to use std::unique_ptr<AdapterEntry> but I'll probably still have to forward-declare it before the public section. If you think this is better I can update before landing. https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h:38: size_t ref_count; On 2017/06/02 13:19:02, Guido Urdaneta wrote: > Can you reuse the reference count in adapter instead of adding a separate one? > If not, can you make WebRtcMediaStreamTrackAdapter not refcounted to avoid the > redundant refcount? Done, using HasOneRef() to determine if disposing is necessary. https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h:47: enum class Type { kLocal, kRemote }; On 2017/06/02 13:19:02, Guido Urdaneta wrote: > Move to private section. It's used only by a private constructor. Done. https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h:81: std::map<std::string, AdapterEntry>::iterator it_; On 2017/06/02 13:19:02, Guido Urdaneta wrote: > A private type alias (e.g., MapEntry = std::map<std::string, > AdapterEntry>::iterator) would make it easier to see that this field points to > an entry in the map. Done. https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h:88: // Gets a new reference to the local track adapter by ID, or null if no such On 2017/06/02 13:19:01, Guido Urdaneta wrote: > nit: the -> a Done. https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h:94: // by web track. If no adapter exists for the track one is created and On 2017/06/02 13:19:01, Guido Urdaneta wrote: > nit: by -> for Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h (right): https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h:31: struct AdapterEntry { On 2017/06/05 11:55:40, hbos_chromium wrote: > On 2017/06/02 13:19:01, Guido Urdaneta wrote: > > Can you move the definition of this type to the .cc file? > > Also, move this to the private section below. I think it's against the style > > guide to define a private section in the beginning of the class. > > It has to be *defined* in the header for AdapterEntry to be defined due to > std::map. > It can be moved to the private section, but it still needs to be *declared* > before AdapterEntry is defined or it will say undeclared identifier. > > I changed it to: > > class WebRtcMediaStreamTrackAdapterMap { > private: > struct AdapterEntry; > > public: > class AdapterRef { ... }; > ... > > private: > struct AdapterEntry { ... }; > ... > }; > > The only way to move it out of the header is to change AdapterEntry's std::map > to use std::unique_ptr<AdapterEntry> but I'll probably still have to > forward-declare it before the public section. If you think this is better I can > update before landing. Above I meant forward-declare and declare, not declare and define. Seems like on some bots I have to declare it before AdapterEntry for std::map to work so it's either the old way or the std::unique_ptr way.
https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc (right): https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... 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 2017/06/02 13:19:01, Guido Urdaneta wrote: > > use base::MakeUnique > > base::MakeUnique actually does not work in this case due to the constructor > being private. Acknowledged. https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:87: return std::unique_ptr<AdapterRef>( On 2017/06/05 11:55:40, hbos_chromium wrote: > On 2017/06/02 13:19:01, Guido Urdaneta wrote: > > use base::MakeUnique > > Ditto. Acko. https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h (right): https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h:31: struct AdapterEntry { go for the old way then. https://codereview.chromium.org/2887403003/diff/270001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h (right): https://codereview.chromium.org/2887403003/diff/270001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h:28: private: Add a comment explaining why you need this private section here. https://codereview.chromium.org/2887403003/diff/270001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h:58: typedef std::map<std::string, AdapterEntry>::iterator MapEntryIterator; prefer using to typedef. https://codereview.chromium.org/2887403003/diff/270001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h:118: AdapterEntry(const AdapterEntry&) = delete; Shouldn't you also delete the copy-assignment operator?
Description was changed from ========== WebRtcMediaStreamTrackMap added. This maps id <-> WebRtcMediaStreamTrackAdapter and handles creating track adapters if they don't already exist. 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. ========== to ========== WebRtcMediaStreamTrackMap added. This maps id <-> WebRtcMediaStreamTrackAdapter and handles creating track adapters if they don't already exist. 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 ==========
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...
PTAL guidou. https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h (right): https://codereview.chromium.org/2887403003/diff/250001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h:31: struct AdapterEntry { On 2017/06/05 13:05:56, Guido Urdaneta wrote: > go for the old way then. Done. https://codereview.chromium.org/2887403003/diff/270001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h (right): https://codereview.chromium.org/2887403003/diff/270001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h:28: private: On 2017/06/05 13:05:56, Guido Urdaneta wrote: > Add a comment explaining why you need this private section here. Done. https://codereview.chromium.org/2887403003/diff/270001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h:58: typedef std::map<std::string, AdapterEntry>::iterator MapEntryIterator; On 2017/06/05 13:05:56, Guido Urdaneta wrote: > prefer using to typedef. Done. https://codereview.chromium.org/2887403003/diff/270001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h:118: AdapterEntry(const AdapterEntry&) = delete; On 2017/06/05 13:05:57, Guido Urdaneta wrote: > Shouldn't you also delete the copy-assignment operator? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % nits https://codereview.chromium.org/2887403003/diff/290001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc (right): https://codereview.chromium.org/2887403003/diff/290001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:117: return std::unique_ptr<AdapterRef>(new AdapterRef(this, type, it)); nit: Consider using base::WrapUnique here to avoid repeating the type name twice. https://codereview.chromium.org/2887403003/diff/290001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:137: return std::unique_ptr<AdapterRef>(new AdapterRef(this, type, it)); Ditto.
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...
hbos@chromium.org changed reviewers: + jochen@chromium.org
PTAL jochen for content/renderer/BUILD.gn https://codereview.chromium.org/2887403003/diff/290001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc (right): https://codereview.chromium.org/2887403003/diff/290001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:117: return std::unique_ptr<AdapterRef>(new AdapterRef(this, type, it)); On 2017/06/09 11:28:12, Guido Urdaneta wrote: > nit: Consider using base::WrapUnique here to avoid repeating the type name > twice. Done. https://codereview.chromium.org/2887403003/diff/290001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc:137: return std::unique_ptr<AdapterRef>(new AdapterRef(this, type, it)); On 2017/06/09 11:28:12, Guido Urdaneta wrote: > Ditto. Done.
Description was changed from ========== WebRtcMediaStreamTrackMap added. This maps id <-> WebRtcMediaStreamTrackAdapter and handles creating track adapters if they don't already exist. 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 ========== to ========== 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 ==========
lgtm
The CQ bit was unchecked by hbos@chromium.org
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/2887403003/#ps310001 (title: "WrapUnique")
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": 310001, "attempt_start_ts": 1497256863775640, "parent_rev": "2f151da52049d5db6abd58ddc30551bee99f6ec4", "commit_rev": "ebab8e6156335da0a2f87cfaea1ad8da7f5e47e1"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/ebab8e6156335da0a2f87cfaea1a... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:310001) as https://chromium.googlesource.com/chromium/src/+/ebab8e6156335da0a2f87cfaea1a... |