Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2883023002:
WebRtcMediaStreamTrackAdapter, maps 1 webrtc and 1 blink track (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. |
DescriptionWebRtcMediaStreamTrackAdapter, maps 1 webrtc and 1 blink track.
RemoteMediaStreamImpl creates and owns
Remote[Audio/Video]TrackAdapter for remote audio and video tracks.
These are moved into its own file so that they can be used
independently of remote streams.
Similarly, WebRtcMediaStreamAdapter creates and owns
WebRtcAudioSink/MediaStreamVideoWebRtcSink for local audio and video
tracks.
By creating a new adapter for all kinds of tracks (local/remote x
audio/video) that can be initialized and uninitialized independently
from streams we achieve:
1) An abstraction, we can reference a track adapter without having
different code depending on track type.
2) The ability to handle track adapters independently of stream
adapters. This is important in the decoupling of streams and
tracks. The RTP Media API[1]'s addTrack, removeTrack and
replaceTrack will allow tracks to move between streams and belong
to zero or multiple streams.
In this CL, the new WebRtcMediaStreamTrackAdapter is only used in
unittests. The plan is to update RemoteMediaStreamImpl and
WebRtcMediaStreamAdapter to reference instances of
WebRtcMediaStreamTrackAdapter, having the lifetime of tracks be
independent of their membership of a particular stream.
[1] https://w3c.github.io/webrtc-pc/#rtp-media-api
BUG=705901, 700916
Review-Url: https://codereview.chromium.org/2883023002
Cr-Commit-Position: refs/heads/master@{#475499}
Committed: https://chromium.googlesource.com/chromium/src/+/8e3037fb7ebef14980926800a971f590486d21fd
Patch Set 1 #
Total comments: 23
Patch Set 2 : Rebase with upstream #Patch Set 3 : Addressed guidou's comments #
Total comments: 10
Patch Set 4 : Addressed nits #Dependent Patchsets: Messages
Total messages: 38 (27 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...
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 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
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Decouple MediaStreamTrack from MediaStream WIP BUG= ========== to ========== WebRtcMediaStreamTrackAdapter, mapping of a webrtc and blink track. RemoteMediaStreamImpl creates and owns Remote[Audio/Video]TrackAdapter for remote audio and video tracks. These are moved into its own file so that they can be used independently of remote streams. Similarly, WebRtcMediaStreamAdapter creates and owns WebRtcAudioSink/MediaStreamVideoWebRtcSink for local audio and video tracks. By creating a new adapter for all kinds of tracks (local/remote x audio/video) that can be initialized and uninitialized independently from streams we achieve: 1) An abstraction, we can reference a track adapter without having different code depending on track type. 2) The ability to handle track adapters independently of stream adapters. This is important in the decoupling of streams and tracks. The RTP Media API[1]'s addTrack, removeTrack and replaceTrack will allow tracks to move between streams and belong to zero or multiple streams. In this CL, the new WebRtcMediaStreamTrackAdapter is only used in unittests. The plan is to update RemoteMediaStreamImpl and WebRtcMediaStreamAdapter to reference instances of WebRtcMediaStreamTrackAdapter, having the lifetime of tracks be independent of their membership of a particular stream. [1] https://w3c.github.io/webrtc-pc/#rtp-media-api BUG=705901, 700916 ==========
hbos@chromium.org changed reviewers: + guidou@chromium.org
Please take a look, guidou. https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... File content/renderer/media/remote_media_stream_impl.cc (right): https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... content/renderer/media/remote_media_stream_impl.cc:20: #include "content/renderer/media/remote_media_stream_track_adapter.h" RemoteMediaStreamTrackAdapter, RemoteVideoTrackAdapter and RemoteVideoTrackAdapter are moved into content/renderer/media/remote_media_stream_track_adapter.[h/cc]. https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc (right): https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc:116: const blink::WebMediaStreamTrack& web_track) { This code is based on WebRtcMediaStreamAdapter::AddAudioSinkToTrack. In a follow-up WebRtcMediaStreamAdapter should rely on WebRtcMediaStreamTrackAdapter instead. https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/webrtc_med... https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc:153: const blink::WebMediaStreamTrack& web_track) { This code is based on WebRtcMediaStreamAdapter::AddVideoSinkToTrack. In a follow-up WebRtcMediaStreamAdapter should rely on WebRtcMediaStreamTrackAdapter instead. https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/webrtc_med...
https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h (right): https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h:95: // the webrtc signaling thread. Oops, need to update this comment, it starts and finishes on the main thread in both cases. In the audio case it jumps to the signaling thread and back, in the video case it doesn't have to post at all.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... File content/renderer/media/remote_media_stream_track_adapter.h (right): https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... content/renderer/media/remote_media_stream_track_adapter.h:45: return &webkit_track_; Can you take this opportunity to rename things that have webkit in their name to blink? If it has effects on other files you can postpone that to another CL. https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc (right): https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc:14: scoped_refptr<WebRtcMediaStreamTrackAdapter> static methods usually have a "// static" comment, although it's apparently not part of the style guide. https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc:116: const blink::WebMediaStreamTrack& web_track) { On 2017/05/18 11:41:51, hbos_chromium wrote: > This code is based on WebRtcMediaStreamAdapter::AddAudioSinkToTrack. In a > follow-up WebRtcMediaStreamAdapter should rely on WebRtcMediaStreamTrackAdapter > instead. > https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/webrtc_med... Can you add a TODO for this in WebRtcMediaStreamAdapter in this CL? https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc:142: if (processor && processor->has_audio_processing()) The if (processor) part is redundant since that was already checked on the outer if. https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc:153: const blink::WebMediaStreamTrack& web_track) { On 2017/05/18 11:41:51, hbos_chromium wrote: > This code is based on WebRtcMediaStreamAdapter::AddVideoSinkToTrack. In a > follow-up WebRtcMediaStreamAdapter should rely on WebRtcMediaStreamTrackAdapter > instead. > https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/webrtc_med... Ditto. https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h (right): https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h:40: // thread in a post. |is_initialized| is ensured in posts to the main thread Please update this comment. What is |is_initialized| and what does it mean that it is ensured and why is that important to document here? https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h:50: void Uninitialize(); Does the term Uninitialize come from existing code? If not, perhaps it would be better to rename it to something else. Close(), Dispose()? https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h:95: // the webrtc signaling thread. On 2017/05/18 11:46:04, hbos_chromium wrote: > Oops, need to update this comment, it starts and finishes on the main thread in > both cases. In the audio case it jumps to the signaling thread and back, in the > video case it doesn't have to post at all. Perhaps you should just remove the comment. The comment only had implementation details anyway. https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h:115: std::unique_ptr<WebRtcAudioSink> local_audio_sink_; It seems to me that what is local about this sink is that the track has a local source. Maybe |local_track_audio_sink_| is clearer?
Also, the first paragraph of the description should be adjusted. Saying "mapping of a webrtc and blink track" could be interpreted as a class that manages a map of webrtc->blink tracks. This class maps a single Blink track to a Webrtc-level track.
Description was changed from ========== WebRtcMediaStreamTrackAdapter, mapping of a webrtc and blink track. RemoteMediaStreamImpl creates and owns Remote[Audio/Video]TrackAdapter for remote audio and video tracks. These are moved into its own file so that they can be used independently of remote streams. Similarly, WebRtcMediaStreamAdapter creates and owns WebRtcAudioSink/MediaStreamVideoWebRtcSink for local audio and video tracks. By creating a new adapter for all kinds of tracks (local/remote x audio/video) that can be initialized and uninitialized independently from streams we achieve: 1) An abstraction, we can reference a track adapter without having different code depending on track type. 2) The ability to handle track adapters independently of stream adapters. This is important in the decoupling of streams and tracks. The RTP Media API[1]'s addTrack, removeTrack and replaceTrack will allow tracks to move between streams and belong to zero or multiple streams. In this CL, the new WebRtcMediaStreamTrackAdapter is only used in unittests. The plan is to update RemoteMediaStreamImpl and WebRtcMediaStreamAdapter to reference instances of WebRtcMediaStreamTrackAdapter, having the lifetime of tracks be independent of their membership of a particular stream. [1] https://w3c.github.io/webrtc-pc/#rtp-media-api BUG=705901, 700916 ========== to ========== WebRtcMediaStreamTrackAdapter, maps 1 webrtc and 1 blink track. RemoteMediaStreamImpl creates and owns Remote[Audio/Video]TrackAdapter for remote audio and video tracks. These are moved into its own file so that they can be used independently of remote streams. Similarly, WebRtcMediaStreamAdapter creates and owns WebRtcAudioSink/MediaStreamVideoWebRtcSink for local audio and video tracks. By creating a new adapter for all kinds of tracks (local/remote x audio/video) that can be initialized and uninitialized independently from streams we achieve: 1) An abstraction, we can reference a track adapter without having different code depending on track type. 2) The ability to handle track adapters independently of stream adapters. This is important in the decoupling of streams and tracks. The RTP Media API[1]'s addTrack, removeTrack and replaceTrack will allow tracks to move between streams and belong to zero or multiple streams. In this CL, the new WebRtcMediaStreamTrackAdapter is only used in unittests. The plan is to update RemoteMediaStreamImpl and WebRtcMediaStreamAdapter to reference instances of WebRtcMediaStreamTrackAdapter, having the lifetime of tracks be independent of their membership of a particular stream. [1] https://w3c.github.io/webrtc-pc/#rtp-media-api 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/2883023002/diff/60001/content/renderer/media/... File content/renderer/media/remote_media_stream_track_adapter.h (right): https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... content/renderer/media/remote_media_stream_track_adapter.h:45: return &webkit_track_; On 2017/05/22 14:26:38, Guido Urdaneta wrote: > Can you take this opportunity to rename things that have webkit in their name to > blink? > If it has effects on other files you can postpone that to another CL. Done. https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc (right): https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc:14: scoped_refptr<WebRtcMediaStreamTrackAdapter> On 2017/05/22 14:26:38, Guido Urdaneta wrote: > static methods usually have a "// static" comment, although it's apparently not > part of the style guide. Done. https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc:116: const blink::WebMediaStreamTrack& web_track) { On 2017/05/22 14:26:38, Guido Urdaneta wrote: > On 2017/05/18 11:41:51, hbos_chromium wrote: > > This code is based on WebRtcMediaStreamAdapter::AddAudioSinkToTrack. In a > > follow-up WebRtcMediaStreamAdapter should rely on > WebRtcMediaStreamTrackAdapter > > instead. > > > https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/webrtc_med... > > Can you add a TODO for this in WebRtcMediaStreamAdapter in this CL? Done. https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc:142: if (processor && processor->has_audio_processing()) On 2017/05/22 14:26:38, Guido Urdaneta wrote: > The if (processor) part is redundant since that was already checked on the outer > if. Done. https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc:153: const blink::WebMediaStreamTrack& web_track) { On 2017/05/22 14:26:38, Guido Urdaneta wrote: > On 2017/05/18 11:41:51, hbos_chromium wrote: > > This code is based on WebRtcMediaStreamAdapter::AddVideoSinkToTrack. In a > > follow-up WebRtcMediaStreamAdapter should rely on > WebRtcMediaStreamTrackAdapter > > instead. > > > https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/webrtc_med... > > Ditto. Donno. https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h (right): https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h:40: // thread in a post. |is_initialized| is ensured in posts to the main thread On 2017/05/22 14:26:38, Guido Urdaneta wrote: > Please update this comment. > What is |is_initialized| and what does it mean that it is ensured and why is > that important to document here? Done. https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h:50: void Uninitialize(); On 2017/05/22 14:26:38, Guido Urdaneta wrote: > Does the term Uninitialize come from existing code? > If not, perhaps it would be better to rename it to something else. Close(), > Dispose()? Done. https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h:95: // the webrtc signaling thread. On 2017/05/22 14:26:38, Guido Urdaneta wrote: > On 2017/05/18 11:46:04, hbos_chromium wrote: > > Oops, need to update this comment, it starts and finishes on the main thread > in > > both cases. In the audio case it jumps to the signaling thread and back, in > the > > video case it doesn't have to post at all. > > Perhaps you should just remove the comment. The comment only had implementation > details anyway. Comment updated. I'd like to document this even though it's private because threading can easily become a mess. https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h:115: std::unique_ptr<WebRtcAudioSink> local_audio_sink_; On 2017/05/22 14:26:38, Guido Urdaneta wrote: > It seems to me that what is local about this sink is that the track has a local > source. > Maybe |local_track_audio_sink_| is clearer? 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/2883023002/diff/60001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h (right): https://codereview.chromium.org/2883023002/diff/60001/content/renderer/media/... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h:95: // the webrtc signaling thread. On 2017/05/29 11:24:17, hbos_chromium wrote: > On 2017/05/22 14:26:38, Guido Urdaneta wrote: > > On 2017/05/18 11:46:04, hbos_chromium wrote: > > > Oops, need to update this comment, it starts and finishes on the main thread > > in > > > both cases. In the audio case it jumps to the signaling thread and back, in > > the > > > video case it doesn't have to post at all. > > > > Perhaps you should just remove the comment. The comment only had > implementation > > details anyway. > > Comment updated. I'd like to document this even though it's private because > threading can easily become a mess. Acknowledged. https://codereview.chromium.org/2883023002/diff/100001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_adapter.cc (right): https://codereview.chromium.org/2883023002/diff/100001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_adapter.cc:104: // TODO(hbos): Replace this code with using |WebRtcMediaStreamTrackAdapter|. nit: s/using// ? https://codereview.chromium.org/2883023002/diff/100001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h (right): https://codereview.chromium.org/2883023002/diff/100001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h:48: // is safe to release all external references to the adapter. The last part of the comment should be improved. I interpret it to mean "Dispose() must be called before all external references are released (i.e., before destruction)." Is it safe to call it multiple times? The way the comment is written does not imply that it is unsafe to release external references without calling Dispose() first. https://codereview.chromium.org/2883023002/diff/100001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h:108: // This class is "const" between being initialized (triggered by |Create...|) perhaps immutable is a better term than "const" in this context. up to you, though. https://codereview.chromium.org/2883023002/diff/100001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h:120: // the remote webrtc track and notifies WebKit. WebKit -> Blink
https://codereview.chromium.org/2883023002/diff/100001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc (right): https://codereview.chromium.org/2883023002/diff/100001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc:71: DCHECK(is_initialized_); belated comment: Would it make sense to have if (!is_initialized_) return; ?
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
Please take a look jochen for content/renderer/BUILD.gn. https://codereview.chromium.org/2883023002/diff/100001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_adapter.cc (right): https://codereview.chromium.org/2883023002/diff/100001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_adapter.cc:104: // TODO(hbos): Replace this code with using |WebRtcMediaStreamTrackAdapter|. On 2017/05/30 08:45:35, Guido Urdaneta wrote: > nit: s/using// ? Done. https://codereview.chromium.org/2883023002/diff/100001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc (right): https://codereview.chromium.org/2883023002/diff/100001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc:71: DCHECK(is_initialized_); On 2017/05/30 08:47:37, Guido Urdaneta wrote: > belated comment: Would it make sense to have > if (!is_initialized_) return; ? Done. https://codereview.chromium.org/2883023002/diff/100001/content/renderer/media... File content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h (right): https://codereview.chromium.org/2883023002/diff/100001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h:48: // is safe to release all external references to the adapter. On 2017/05/30 08:45:35, Guido Urdaneta wrote: > The last part of the comment should be improved. > I interpret it to mean "Dispose() must be called before all external references > are released (i.e., before destruction)." > Is it safe to call it multiple times? > > The way the comment is written does not imply that it is unsafe to release > external references without calling Dispose() first. Done. https://codereview.chromium.org/2883023002/diff/100001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h:108: // This class is "const" between being initialized (triggered by |Create...|) On 2017/05/30 08:45:35, Guido Urdaneta wrote: > perhaps immutable is a better term than "const" in this context. up to you, > though. Done. https://codereview.chromium.org/2883023002/diff/100001/content/renderer/media... content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h:120: // the remote webrtc track and notifies WebKit. On 2017/05/30 08:45:35, Guido Urdaneta wrote: > WebKit -> Blink Done.
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...)
lgtm
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/2883023002/#ps120001 (title: "Addressed nits")
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": 120001, "attempt_start_ts": 1496144964674870,
"parent_rev": "b7bbd2b951ba158480bd3817e117f361b899c225", "commit_rev":
"8e3037fb7ebef14980926800a971f590486d21fd"}
Message was sent while issue was closed.
Description was changed from ========== WebRtcMediaStreamTrackAdapter, maps 1 webrtc and 1 blink track. RemoteMediaStreamImpl creates and owns Remote[Audio/Video]TrackAdapter for remote audio and video tracks. These are moved into its own file so that they can be used independently of remote streams. Similarly, WebRtcMediaStreamAdapter creates and owns WebRtcAudioSink/MediaStreamVideoWebRtcSink for local audio and video tracks. By creating a new adapter for all kinds of tracks (local/remote x audio/video) that can be initialized and uninitialized independently from streams we achieve: 1) An abstraction, we can reference a track adapter without having different code depending on track type. 2) The ability to handle track adapters independently of stream adapters. This is important in the decoupling of streams and tracks. The RTP Media API[1]'s addTrack, removeTrack and replaceTrack will allow tracks to move between streams and belong to zero or multiple streams. In this CL, the new WebRtcMediaStreamTrackAdapter is only used in unittests. The plan is to update RemoteMediaStreamImpl and WebRtcMediaStreamAdapter to reference instances of WebRtcMediaStreamTrackAdapter, having the lifetime of tracks be independent of their membership of a particular stream. [1] https://w3c.github.io/webrtc-pc/#rtp-media-api BUG=705901, 700916 ========== to ========== WebRtcMediaStreamTrackAdapter, maps 1 webrtc and 1 blink track. RemoteMediaStreamImpl creates and owns Remote[Audio/Video]TrackAdapter for remote audio and video tracks. These are moved into its own file so that they can be used independently of remote streams. Similarly, WebRtcMediaStreamAdapter creates and owns WebRtcAudioSink/MediaStreamVideoWebRtcSink for local audio and video tracks. By creating a new adapter for all kinds of tracks (local/remote x audio/video) that can be initialized and uninitialized independently from streams we achieve: 1) An abstraction, we can reference a track adapter without having different code depending on track type. 2) The ability to handle track adapters independently of stream adapters. This is important in the decoupling of streams and tracks. The RTP Media API[1]'s addTrack, removeTrack and replaceTrack will allow tracks to move between streams and belong to zero or multiple streams. In this CL, the new WebRtcMediaStreamTrackAdapter is only used in unittests. The plan is to update RemoteMediaStreamImpl and WebRtcMediaStreamAdapter to reference instances of WebRtcMediaStreamTrackAdapter, having the lifetime of tracks be independent of their membership of a particular stream. [1] https://w3c.github.io/webrtc-pc/#rtp-media-api BUG=705901, 700916 Review-Url: https://codereview.chromium.org/2883023002 Cr-Commit-Position: refs/heads/master@{#475499} Committed: https://chromium.googlesource.com/chromium/src/+/8e3037fb7ebef14980926800a971... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001) as https://chromium.googlesource.com/chromium/src/+/8e3037fb7ebef14980926800a971... |
