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

Issue 2946063003: WebRtcMediaStreamAdapterMap added. (Closed)

Created:
3 years, 6 months ago by hbos_chromium
Modified:
3 years, 5 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

WebRtcMediaStreamAdapterMap added. This will take care of creating and owning stream adapters, the glue between blink and webrtc layer media streams, independent of any one particular component using a stream. A stream adapter exists for as long as a component is using the stream (as long as it is holding on to a adapter reference). This is the stream equivalent of WebRtcMediaStreamTrackAdapterMap. Initial PS only cares about local streams, a TODO is added to take care of remote streams in the future. This will allow an RTCRtpSender to reference streams that are not part of the local stream set (added with addStream). This will unblock addTrack. The RTCPeerConnectionHandler will make use of this map in a small follow-up CL. BUG=700916, 705901 Review-Url: https://codereview.chromium.org/2946063003 Cr-Commit-Position: refs/heads/master@{#482276} Committed: https://chromium.googlesource.com/chromium/src/+/d6144f20a4e3e525bc3e52d097a6c20c90d615ed

Patch Set 1 : Bots are green #

Patch Set 2 : nits #

Patch Set 3 : Reupload from new machine so that dependent patch sets can be made #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+303 lines, -0 lines) Patch
M content/renderer/BUILD.gn View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M content/renderer/media/webrtc/webrtc_media_stream_adapter.cc View 1 chunk +1 line, -0 lines 0 comments Download
A content/renderer/media/webrtc/webrtc_media_stream_adapter_map.h View 1 1 chunk +109 lines, -0 lines 0 comments Download
A content/renderer/media/webrtc/webrtc_media_stream_adapter_map.cc View 1 1 chunk +88 lines, -0 lines 2 comments Download
A content/renderer/media/webrtc/webrtc_media_stream_adapter_map_unittest.cc View 1 chunk +102 lines, -0 lines 0 comments Download
M content/test/BUILD.gn View 1 2 1 chunk +1 line, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 22 (12 generated)
hbos_chromium
Please take a look, guidou.
3 years, 6 months ago (2017-06-21 10:53:24 UTC) #7
Guido Urdaneta
https://codereview.chromium.org/2946063003/diff/60001/content/renderer/media/webrtc/webrtc_media_stream_adapter_map.cc File content/renderer/media/webrtc/webrtc_media_stream_adapter_map.cc (right): https://codereview.chromium.org/2946063003/diff/60001/content/renderer/media/webrtc/webrtc_media_stream_adapter_map.cc#newcode16 content/renderer/media/webrtc/webrtc_media_stream_adapter_map.cc:16: : adapter(other.adapter.release()), ref_count(other.ref_count) {} why release() and not std::move()?
3 years, 6 months ago (2017-06-26 10:15:17 UTC) #8
hbos_chromium
PTAL guidou https://codereview.chromium.org/2946063003/diff/60001/content/renderer/media/webrtc/webrtc_media_stream_adapter_map.cc File content/renderer/media/webrtc/webrtc_media_stream_adapter_map.cc (right): https://codereview.chromium.org/2946063003/diff/60001/content/renderer/media/webrtc/webrtc_media_stream_adapter_map.cc#newcode16 content/renderer/media/webrtc/webrtc_media_stream_adapter_map.cc:16: : adapter(other.adapter.release()), ref_count(other.ref_count) {} On 2017/06/26 10:15:17, ...
3 years, 5 months ago (2017-06-26 10:52:53 UTC) #9
Guido Urdaneta
On 2017/06/26 10:52:53, hbos_chromium wrote: > PTAL guidou > > https://codereview.chromium.org/2946063003/diff/60001/content/renderer/media/webrtc/webrtc_media_stream_adapter_map.cc > File content/renderer/media/webrtc/webrtc_media_stream_adapter_map.cc (right): ...
3 years, 5 months ago (2017-06-26 10:59:11 UTC) #10
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/2946063003/60001
3 years, 5 months ago (2017-06-26 11:08:10 UTC) #12
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/473779)
3 years, 5 months ago (2017-06-26 11:15:37 UTC) #14
hbos_chromium
Please take a look jochen for content/renderer/BUILD.gn
3 years, 5 months ago (2017-06-26 11:17:53 UTC) #16
jochen (gone - plz use gerrit)
lgtm
3 years, 5 months ago (2017-06-26 13:41:46 UTC) #17
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/2946063003/60001
3 years, 5 months ago (2017-06-26 13:44:58 UTC) #19
commit-bot: I haz the power
3 years, 5 months ago (2017-06-26 15:04:19 UTC) #22
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/d6144f20a4e3e525bc3e52d097a6...

Powered by Google App Engine
This is Rietveld 408576698