Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 6 months ago by pbos-webrtc Modified:
5 years, 5 months ago Reviewers:
the sun, hta-webrtc, stefan-webrtc, mflodman CC:
stefan-webrtc, andresp, Andrew MacDonald, interface-changes_webrtc.org, niklas.enbom, perkj_webrtc, qiang.lu, rwolff_gocast.it, tterriberry_mozilla.com, webrtc-reviews_webrtc.org, yujie_mao (webrtc) Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionBase A/V synchronization on sync_labels.
Groups of streams that should be synchronized are signalled through
SDP. These should be used rather than synchronizing the first-added
video stream to the first-added audio stream implicitly.
BUG=webrtc:4667
R=hta@webrtc.org, solenberg@webrtc.org, stefan@webrtc.org
TBR=mflodman@webrtc.org
Committed: https://crrev.com/8fc7fa798f7a36955f1b933980401afad2aff592
Cr-Commit-Position: refs/heads/master@{#9586}
Patch Set 1 #Patch Set 2 : add missing tests + fix bug #
Total comments: 14
Patch Set 3 : rebase #Patch Set 4 : fix win compile error, bah #
Total comments: 15
Patch Set 5 : failing tests #Patch Set 6 : remove sync groups + feedback #Patch Set 7 : more feedback #
Total comments: 8
Patch Set 8 : pull sync into one method #
Total comments: 4
Patch Set 9 : feedback #
Total comments: 25
Patch Set 10 : feedback #
Total comments: 6
Patch Set 11 : rebase #Patch Set 12 : feedback #Patch Set 13 : updated log message #
Total comments: 8
Patch Set 14 : rebase #Patch Set 15 : feedback #Patch Set 16 : rebase #
Created: 5 years, 5 months ago
Messages
Total messages: 51 (7 generated)
add missing tests + fix bug
PTAL, this might require a couple of bounces back and forth.
rebase
fix win compile error, bah
https://codereview.webrtc.org/1181653002/diff/20001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1181653002/diff/20001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:1690: const std::string& sync_label() const { return sync_label_; } Adding the sync label here feels backwards to me. I think going forward we will be better served with a map ssrc->StreamParams, e.g. receive_stream_params_. and to look there for the sync label. https://codereview.webrtc.org/1181653002/diff/20001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:2487: new WebRtcVoiceChannelRenderer(channel, sp.sync_label, audio_transport))); This will never be used in practise? https://codereview.webrtc.org/1181653002/diff/20001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:3630: WebRtcVoiceChannelRenderer* channel) { No need to take the ..Renderer* as argument. We can look it up from the receive_channels_ map. Yes, I know you will say it requires an unnecessary map lookup, to which I will answer it doesn't matter here (1. few objects in the map, 2. code due for refactoring) - keep the interface simple instead! https://codereview.webrtc.org/1181653002/diff/20001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:3634: if (!call_) {} please. https://codereview.webrtc.org/1181653002/diff/20001/webrtc/audio_receive_stre... File webrtc/audio_receive_stream.h (right): https://codereview.webrtc.org/1181653002/diff/20001/webrtc/audio_receive_stre... webrtc/audio_receive_stream.h:48: int channel_id = -1; voe_channel_id https://codereview.webrtc.org/1181653002/diff/20001/webrtc/audio_receive_stre... webrtc/audio_receive_stream.h:51: // TODO(pbos): Synchronize streams in a sync group, not just video streams Is this TODO necessary? Will it be addressed in the foreseeable future? Better add an issue for it instead? https://codereview.webrtc.org/1181653002/diff/20001/webrtc/video/audio_receiv... File webrtc/video/audio_receive_stream.cc (right): https://codereview.webrtc.org/1181653002/diff/20001/webrtc/video/audio_receiv... webrtc/video/audio_receive_stream.cc:51: CHECK(config.channel_id != -1); Why not DCHECK()? If the channel doesn't exist, subsequent calls to VoE will fail anyway. https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/fakeweb... File talk/media/webrtc/fakewebrtccall.cc (right): https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/fakeweb... talk/media/webrtc/fakewebrtccall.cc:247: DCHECK(config.channel_id != -1); Validate config in object ctor instead. https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2_unittest.cc:962: // Test that changing the combined_audio_video_bwe option results in the c/p error https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.h (right): https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.h:450: std::map<int, std::string> sync_labels_; leftovers? https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine_unittest.cc:3357: // Test that changing the combined_audio_video_bwe option results in the c/p error https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine_unittest.cc:3404: EXPECT_TRUE(call.GetAudioReceiveStream(kAudioSsrc1) Is this really style guide compliant formatting? https://codereview.webrtc.org/1181653002/diff/60001/webrtc/video/call.cc File webrtc/video/call.cc (right): https://codereview.webrtc.org/1181653002/diff/60001/webrtc/video/call.cc#newc... webrtc/video/call.cc:68: class SyncGroup { Simplify as discussed offline. https://codereview.webrtc.org/1181653002/diff/60001/webrtc/video/call_perf_te... File webrtc/video/call_perf_tests.cc (right): https://codereview.webrtc.org/1181653002/diff/60001/webrtc/video/call_perf_te... webrtc/video/call_perf_tests.cc:279: dd
failing tests
remove sync groups + feedback
more feedback
phew, PTAL https://codereview.webrtc.org/1181653002/diff/20001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1181653002/diff/20001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:1690: const std::string& sync_label() const { return sync_label_; } On 2015/06/11 11:52:03, the sun wrote: > Adding the sync label here feels backwards to me. I think going forward we will > be better served with a map ssrc->StreamParams, e.g. receive_stream_params_. and > to look there for the sync label. Done. https://codereview.webrtc.org/1181653002/diff/20001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:2487: new WebRtcVoiceChannelRenderer(channel, sp.sync_label, audio_transport))); On 2015/06/11 11:52:03, the sun wrote: > This will never be used in practise? Correct. https://codereview.webrtc.org/1181653002/diff/20001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:3630: WebRtcVoiceChannelRenderer* channel) { On 2015/06/11 11:52:03, the sun wrote: > No need to take the ..Renderer* as argument. We can look it up from the > receive_channels_ map. Yes, I know you will say it requires an unnecessary map > lookup, to which I will answer it doesn't matter here (1. few objects in the > map, 2. code due for refactoring) - keep the interface simple instead! Done. https://codereview.webrtc.org/1181653002/diff/20001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:3634: if (!call_) On 2015/06/11 11:52:03, the sun wrote: > {} please. Gah, done. https://codereview.webrtc.org/1181653002/diff/20001/webrtc/audio_receive_stre... File webrtc/audio_receive_stream.h (right): https://codereview.webrtc.org/1181653002/diff/20001/webrtc/audio_receive_stre... webrtc/audio_receive_stream.h:48: int channel_id = -1; On 2015/06/11 11:52:03, the sun wrote: > voe_channel_id Done. https://codereview.webrtc.org/1181653002/diff/20001/webrtc/audio_receive_stre... webrtc/audio_receive_stream.h:51: // TODO(pbos): Synchronize streams in a sync group, not just video streams On 2015/06/11 11:52:03, the sun wrote: > Is this TODO necessary? Will it be addressed in the foreseeable future? Better > add an issue for it instead? Added an issue, and referenced it here. I think it's good to have it explicit in the header though, since it looks like it should work. https://codereview.webrtc.org/1181653002/diff/20001/webrtc/video/audio_receiv... File webrtc/video/audio_receive_stream.cc (right): https://codereview.webrtc.org/1181653002/diff/20001/webrtc/video/audio_receiv... webrtc/video/audio_receive_stream.cc:51: CHECK(config.channel_id != -1); On 2015/06/11 11:52:03, the sun wrote: > Why not DCHECK()? If the channel doesn't exist, subsequent calls to VoE will > fail anyway. Done. https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/fakeweb... File talk/media/webrtc/fakewebrtccall.cc (right): https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/fakeweb... talk/media/webrtc/fakewebrtccall.cc:247: DCHECK(config.channel_id != -1); On 2015/06/11 at 11:52:04, the sun wrote: > Validate config in object ctor instead. Done. https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2_unittest.cc:962: // Test that changing the combined_audio_video_bwe option results in the On 2015/06/11 at 11:52:04, the sun wrote: > c/p error Removed, done. https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.h (right): https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.h:450: std::map<int, std::string> sync_labels_; On 2015/06/11 11:52:04, the sun wrote: > leftovers? Done. https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine_unittest.cc:3357: // Test that changing the combined_audio_video_bwe option results in the On 2015/06/11 11:52:04, the sun wrote: > c/p error Done. https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine_unittest.cc:3404: EXPECT_TRUE(call.GetAudioReceiveStream(kAudioSsrc1) On 2015/06/11 11:52:04, the sun wrote: > Is this really style guide compliant formatting? Yep, where style == clang-format with Chromium settings. I'm not going to do manual code formatting. https://codereview.webrtc.org/1181653002/diff/60001/webrtc/video/call.cc File webrtc/video/call.cc (right): https://codereview.webrtc.org/1181653002/diff/60001/webrtc/video/call.cc#newc... webrtc/video/call.cc:68: class SyncGroup { On 2015/06/11 11:52:04, the sun wrote: > Simplify as discussed offline. Done. https://codereview.webrtc.org/1181653002/diff/60001/webrtc/video/call_perf_te... File webrtc/video/call_perf_tests.cc (right): https://codereview.webrtc.org/1181653002/diff/60001/webrtc/video/call_perf_te... webrtc/video/call_perf_tests.cc:279: On 2015/06/11 11:52:04, the sun wrote: > dd ddone https://codereview.webrtc.org/1181653002/diff/60001/webrtc/video/call_perf_te... webrtc/video/call_perf_tests.cc:336: Done.
Getting there... https://codereview.webrtc.org/1181653002/diff/120001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1181653002/diff/120001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:2576: receive_stream_params_[ssrc] = sp; DCHECK that there is no entry in map for this ssrc. https://codereview.webrtc.org/1181653002/diff/120001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:3639: config.rtp.extensions = recv_rtp_extensions_; {} https://codereview.webrtc.org/1181653002/diff/120001/webrtc/audio_receive_str... File webrtc/audio_receive_stream.h (right): https://codereview.webrtc.org/1181653002/diff/120001/webrtc/audio_receive_str... webrtc/audio_receive_stream.h:46: // Handle to underlying VoiceEngine handle, used to map AudioReceiveStream Handle to a handle? Rephrase, also include something about it being temporary. https://codereview.webrtc.org/1181653002/diff/120001/webrtc/video/call.cc File webrtc/video/call.cc (right): https://codereview.webrtc.org/1181653002/diff/120001/webrtc/video/call.cc#new... webrtc/video/call.cc:448: void Call::AddAudioReceiveStreamSync(AudioReceiveStream* stream) { I'd like you to spend a few minutes to see if this can be simplified into 1 or 2 functions instead: UpdateAVSync(sync_group), or UpdateAudioAVSync(..)+UpdateVideoAVSync(..). Some work can be done in situ at the call spot instead. If that's 100% impossible/unsuitable, at the very least make the code more readable by pulling stream->config().sync_group into a local const &.
pull sync into one method
PTAL https://codereview.webrtc.org/1181653002/diff/120001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1181653002/diff/120001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:2576: receive_stream_params_[ssrc] = sp; On 2015/06/11 15:38:48, the sun wrote: > DCHECK that there is no entry in map for this ssrc. Done. https://codereview.webrtc.org/1181653002/diff/120001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:3639: config.rtp.extensions = recv_rtp_extensions_; On 2015/06/11 15:38:48, the sun wrote: > {} :Done https://codereview.webrtc.org/1181653002/diff/120001/webrtc/audio_receive_str... File webrtc/audio_receive_stream.h (right): https://codereview.webrtc.org/1181653002/diff/120001/webrtc/audio_receive_str... webrtc/audio_receive_stream.h:46: // Handle to underlying VoiceEngine handle, used to map AudioReceiveStream On 2015/06/11 15:38:48, the sun wrote: > Handle to a handle? Rephrase, also include something about it being temporary. Done. https://codereview.webrtc.org/1181653002/diff/120001/webrtc/video/call.cc File webrtc/video/call.cc (right): https://codereview.webrtc.org/1181653002/diff/120001/webrtc/video/call.cc#new... webrtc/video/call.cc:448: void Call::AddAudioReceiveStreamSync(AudioReceiveStream* stream) { On 2015/06/11 15:38:48, the sun wrote: > I'd like you to spend a few minutes to see if this can be simplified into 1 or 2 > functions instead: UpdateAVSync(sync_group), or > UpdateAudioAVSync(..)+UpdateVideoAVSync(..). Some work can be done in situ at > the call spot instead. > > If that's 100% impossible/unsuitable, at the very least make the code more > readable by pulling stream->config().sync_group into a local const &. Done.
LGTM https://codereview.webrtc.org/1181653002/diff/140001/webrtc/video/call.cc File webrtc/video/call.cc (right): https://codereview.webrtc.org/1181653002/diff/140001/webrtc/video/call.cc#new... webrtc/video/call.cc:457: if (sync_audio_stream == nullptr) { else { https://codereview.webrtc.org/1181653002/diff/140001/webrtc/video/call.cc#new... webrtc/video/call.cc:465: if (sync_audio_stream != nullptr) if (sync_audio_stream)
feedback
https://codereview.webrtc.org/1181653002/diff/140001/webrtc/video/call.cc File webrtc/video/call.cc (right): https://codereview.webrtc.org/1181653002/diff/140001/webrtc/video/call.cc#new... webrtc/video/call.cc:457: if (sync_audio_stream == nullptr) { On 2015/06/15 10:28:50, the sun wrote: > else { Done. https://codereview.webrtc.org/1181653002/diff/140001/webrtc/video/call.cc#new... webrtc/video/call.cc:465: if (sync_audio_stream != nullptr) On 2015/06/15 10:28:50, the sun wrote: > if (sync_audio_stream) Done.
https://codereview.webrtc.org/1181653002/diff/160001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1181653002/diff/160001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:2576: DCHECK(receive_stream_params_.find(ssrc) == receive_stream_params_.end()); Can someone give us a fake sdp and cause a crash here, or is this ssrc always generated locally? https://codereview.webrtc.org/1181653002/diff/160001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:3640: // Only add RTP extensions if we support combined AV BWE. A/V BWE https://codereview.webrtc.org/1181653002/diff/160001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:3645: config.sync_group = receive_stream_params_[ssrc].sync_label; Does this mean A/V BWE depends on sync labels? I would like some explanations on this code. https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc File webrtc/video/call.cc (right): https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new... webrtc/video/call.cc:455: sync_audio_stream = it->second; Shouldn't we fail with an error or warning if two video streams are synced to one audio stream? Or in general if more than 2 streams are synced together? We don't really support that. https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new... webrtc/video/call.cc:467: for (VideoReceiveStream* video_stream : video_receive_streams_) { Is this syncing all video receive streams to the audio stream if one of them had a matching group? Doesn't necessarily have to be all that should be synced, right?
https://codereview.webrtc.org/1181653002/diff/160001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1181653002/diff/160001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:2576: DCHECK(receive_stream_params_.find(ssrc) == receive_stream_params_.end()); On 2015/06/16 15:33:39, stefan-webrtc (holmer) wrote: > Can someone give us a fake sdp and cause a crash here, or is this ssrc always > generated locally? Notice that we look for the ssrc in receive_channels_ a few lines up, and return gracefully if we find it. This assert is just to make sure receive_channels_ and receive_sgtream_params_ are in sync. https://codereview.webrtc.org/1181653002/diff/160001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:3645: config.sync_group = receive_stream_params_[ssrc].sync_label; On 2015/06/16 15:33:39, stefan-webrtc (holmer) wrote: > Does this mean A/V BWE depends on sync labels? I would like some explanations on > this code. No, we create the ARS regardless, but only set rtp extensions on it if we have combined BWE enabled. https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc File webrtc/video/call.cc (right): https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new... webrtc/video/call.cc:455: sync_audio_stream = it->second; On 2015/06/16 15:33:39, stefan-webrtc (holmer) wrote: > Shouldn't we fail with an error or warning if two video streams are synced to > one audio stream? Or in general if more than 2 streams are synced together? We > don't really support that. Since we're literally using the SDP sync group we should fire errors based on what is supported in the relevant spec (https://tools.ietf.org/html/rfc5888 ?). Whenever we can't support the requested configuration, we should log warnings.
A few initial questions, think I need some more background how this should work. I guess I'm misunderstanding the RFC at some point. https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc File webrtc/video/call.cc (right): https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new... webrtc/video/call.cc:112: void ConfigureSync(const std::string& sync_group) Would prefer to include AV in this name. https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new... webrtc/video/call.cc:348: void Call::DestroyVideoReceiveStream( Shouldn't we call ConfigureSync from here too? https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new... webrtc/video/call.cc:447: // Set sync only if there was no previous one. Meaning we can only sync one audio stream with one video stream in a call, not multiple pairs? https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new... webrtc/video/call.cc:467: for (VideoReceiveStream* video_stream : video_receive_streams_) { On 2015/06/16 15:33:39, stefan-webrtc (holmer) wrote: > Is this syncing all video receive streams to the audio stream if one of them had > a matching group? Doesn't necessarily have to be all that should be synced, > right? Agree.
Just to quote the specs a bit: getusermedia spec: http://w3c.github.io/mediacapture-main/getusermedia.html#stream-api Each MediaStream can contain zero or more MediaStreamTrack objects. All tracks in a MediaStream are intended to be synchronized when rendered. This is not a hard requirement, since it might not be possible to synchronize tracks from sources that have different clocks. Different MediaStream objects do not need to be synchronized. draft-ietf-mmusic-msid: https://tools.ietf.org/html/draft-ietf-mmusic-msid-10#section-3 specifies that MSID is used to convey the association of a MediaStreamTrack to a MediaStream.
Thanks Harald, I was looking at RFC 5888 and the LS semantic.
https://codereview.webrtc.org/1181653002/diff/160001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1181653002/diff/160001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:2576: DCHECK(receive_stream_params_.find(ssrc) == receive_stream_params_.end()); On 2015/06/18 08:38:37, the sun wrote: > On 2015/06/16 15:33:39, stefan-webrtc (holmer) wrote: > > Can someone give us a fake sdp and cause a crash here, or is this ssrc always > > generated locally? > > Notice that we look for the ssrc in receive_channels_ a few lines up, and return > gracefully if we find it. This assert is just to make sure receive_channels_ and > receive_sgtream_params_ are in sync. Acknowledged. https://codereview.webrtc.org/1181653002/diff/160001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:3645: config.sync_group = receive_stream_params_[ssrc].sync_label; On 2015/06/18 08:38:37, the sun wrote: > On 2015/06/16 15:33:39, stefan-webrtc (holmer) wrote: > > Does this mean A/V BWE depends on sync labels? I would like some explanations > on > > this code. > > No, we create the ARS regardless, but only set rtp extensions on it if we have > combined BWE enabled. Yes, I have no clue how I read this code. :) https://codereview.webrtc.org/1181653002/diff/160001/webrtc/audio_receive_str... File webrtc/audio_receive_stream.h (right): https://codereview.webrtc.org/1181653002/diff/160001/webrtc/audio_receive_str... webrtc/audio_receive_stream.h:53: // to one of the audio streams. Tracked by issue webrtc:4762. "...not just one video stream to one of the audio streams.", right? We can't sync two video streams to one audio stream AFAIK. https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc File webrtc/video/call.cc (right): https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new... webrtc/video/call.cc:112: void ConfigureSync(const std::string& sync_group) On 2015/06/22 04:53:28, mflodman wrote: > Would prefer to include AV in this name. I'm not sure I agree. We may want to sync video with video as well, right? https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new... webrtc/video/call.cc:455: sync_audio_stream = it->second; On 2015/06/18 08:38:37, the sun wrote: > On 2015/06/16 15:33:39, stefan-webrtc (holmer) wrote: > > Shouldn't we fail with an error or warning if two video streams are synced to > > one audio stream? Or in general if more than 2 streams are synced together? We > > don't really support that. > > Since we're literally using the SDP sync group we should fire errors based on > what is supported in the relevant spec (https://tools.ietf.org/html/rfc5888 ?). I'm not sure what you mean with this. Log errors when an SDP configures something we don't support? > Whenever we can't support the requested configuration, we should log warnings. Agreed. And maybe return an error? Not sure what is expected by a javascript user in this case, but trying to sync 2 or more video streams to a single audio stream will break a/v sync for all.
feedback
PTAL https://codereview.webrtc.org/1181653002/diff/160001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1181653002/diff/160001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:2576: DCHECK(receive_stream_params_.find(ssrc) == receive_stream_params_.end()); On 2015/06/22 14:19:42, stefan-webrtc (holmer) wrote: > On 2015/06/18 08:38:37, the sun wrote: > > On 2015/06/16 15:33:39, stefan-webrtc (holmer) wrote: > > > Can someone give us a fake sdp and cause a crash here, or is this ssrc > always > > > generated locally? > > > > Notice that we look for the ssrc in receive_channels_ a few lines up, and > return > > gracefully if we find it. This assert is just to make sure receive_channels_ > and > > receive_sgtream_params_ are in sync. > > Acknowledged. I moved the storing of the StreamParams to prevent having to erase them, now they are stored at the same place as receive_channels_. https://codereview.webrtc.org/1181653002/diff/160001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:3640: // Only add RTP extensions if we support combined AV BWE. On 2015/06/16 15:33:39, stefan-webrtc (holmer) wrote: > A/V BWE Done. https://codereview.webrtc.org/1181653002/diff/160001/webrtc/audio_receive_str... File webrtc/audio_receive_stream.h (right): https://codereview.webrtc.org/1181653002/diff/160001/webrtc/audio_receive_str... webrtc/audio_receive_stream.h:53: // to one of the audio streams. Tracked by issue webrtc:4762. On 2015/06/22 14:19:42, stefan-webrtc (holmer) wrote: > "...not just one video stream to one of the audio streams.", right? We can't > sync two video streams to one audio stream AFAIK. Correct, done. https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc File webrtc/video/call.cc (right): https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new... webrtc/video/call.cc:112: void ConfigureSync(const std::string& sync_group) On 2015/06/22 14:19:42, stefan-webrtc (holmer) wrote: > On 2015/06/22 04:53:28, mflodman wrote: > > Would prefer to include AV in this name. > > I'm not sure I agree. We may want to sync video with video as well, right? Yes, we want this parameter to be for syncing streams in general, limiting this to a pair of a/v channels is an implementation detail. https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new... webrtc/video/call.cc:348: void Call::DestroyVideoReceiveStream( On 2015/06/22 04:53:28, mflodman wrote: > Shouldn't we call ConfigureSync from here too? Yes, so that the audio stream can be synced to something else, thanks. https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new... webrtc/video/call.cc:447: // Set sync only if there was no previous one. On 2015/06/22 04:53:27, mflodman wrote: > Meaning we can only sync one audio stream with one video stream in a call, not > multiple pairs? Acknowledged. https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new... webrtc/video/call.cc:455: sync_audio_stream = it->second; On 2015/06/16 15:33:39, stefan-webrtc (holmer) wrote: > Shouldn't we fail with an error or warning if two video streams are synced to > one audio stream? Or in general if more than 2 streams are synced together? We > don't really support that. Logging a warning should be appropriate. A SDP is correct in requesting more than two streams to be synchronized, we just don't support it. https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new... webrtc/video/call.cc:467: for (VideoReceiveStream* video_stream : video_receive_streams_) { On 2015/06/22 04:53:27, mflodman wrote: > On 2015/06/16 15:33:39, stefan-webrtc (holmer) wrote: > > Is this syncing all video receive streams to the audio stream if one of them > had > > a matching group? Doesn't necessarily have to be all that should be synced, > > right? > > Agree. Yes, huge bug here. Thanks. I changed it so that it'll only sync if it's the same group, and it will only sync one pair. The rest will get the channel id -1, and remove sync.
rebase
https://codereview.webrtc.org/1181653002/diff/180001/webrtc/video/call.cc File webrtc/video/call.cc (right): https://codereview.webrtc.org/1181653002/diff/180001/webrtc/video/call.cc#new... webrtc/video/call.cc:369: ConfigureSync(receive_stream_impl->config().sync_group); Can't this crash if video_receive_ssrcs_ is empty? Probably shouldn't happen. Maybe move the check from 371 to 369? https://codereview.webrtc.org/1181653002/diff/180001/webrtc/video/call.cc#new... webrtc/video/call.cc:479: LOG(LS_WARNING) << "Attempting to sync more than one audio/video pair. " Isn't it? The following configuration is ok right? {audio1, video1} {audio2, video2} While the following is not: {audio1, audio2, video1} {audio3, video2, video3} I don't think the comment reflects that. https://codereview.webrtc.org/1181653002/diff/180001/webrtc/video/call.cc#new... webrtc/video/call.cc:485: : -1); I'd prefer to move this complex ?: statement out from the function call. Make it a regular if...
feedback
updated log message
PTAL https://codereview.webrtc.org/1181653002/diff/180001/webrtc/video/call.cc File webrtc/video/call.cc (right): https://codereview.webrtc.org/1181653002/diff/180001/webrtc/video/call.cc#new... webrtc/video/call.cc:369: ConfigureSync(receive_stream_impl->config().sync_group); On 2015/06/24 11:57:35, stefan-webrtc (holmer) wrote: > Can't this crash if video_receive_ssrcs_ is empty? Probably shouldn't happen. > Maybe move the check from 371 to 369? Done. https://codereview.webrtc.org/1181653002/diff/180001/webrtc/video/call.cc#new... webrtc/video/call.cc:479: LOG(LS_WARNING) << "Attempting to sync more than one audio/video pair. " On 2015/06/24 11:57:35, stefan-webrtc (holmer) wrote: > Isn't it? The following configuration is ok right? > {audio1, video1} > {audio2, video2} > > While the following is not: > {audio1, audio2, video1} > {audio3, video2, video3} > > I don't think the comment reflects that. Agreed, comment updated. https://codereview.webrtc.org/1181653002/diff/180001/webrtc/video/call.cc#new... webrtc/video/call.cc:485: : -1); On 2015/06/24 11:57:35, stefan-webrtc (holmer) wrote: > I'd prefer to move this complex ?: statement out from the function call. Make it > a regular if... Done.
lgtm
pbos@webrtc.org changed reviewers: + hta@webrtc.org
+R hta@ mflodman@ PTAL
On 2015/06/25 19:23:31, pbos-webrtc wrote: > +R hta@ > > mflodman@ PTAL (hta@ PTAL as well)
LGTM with comments. These are nits. https://codereview.webrtc.org/1181653002/diff/240001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/1181653002/diff/240001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine_unittest.cc:3432: .rtp.extensions.empty()); Drive-by comment: This test doesn't seem very future-proof. The combined BWE option may be the only RTP extension we use *today*, but what happens the day the "mid" RTP option goes live? https://codereview.webrtc.org/1181653002/diff/240001/webrtc/video/bitrate_est... File webrtc/video/bitrate_estimator_tests.cc (right): https://codereview.webrtc.org/1181653002/diff/240001/webrtc/video/bitrate_est... webrtc/video/bitrate_estimator_tests.cc:208: // underlying channel. This comment is not clear. Under what conditions is the voe_channel_id bogus, and is zero always a bogus value? Possible rewording: "We set voe_channel_id to 0, which is bogus but legal. In production, the voe_channel_id is always set to the id of an underlying channel." https://codereview.webrtc.org/1181653002/diff/240001/webrtc/video/call.cc File webrtc/video/call.cc (right): https://codereview.webrtc.org/1181653002/diff/240001/webrtc/video/call.cc#new... webrtc/video/call.cc:481: "the current implementation."; Should there be a bug number filed to track this restriction? https://codereview.webrtc.org/1181653002/diff/240001/webrtc/video/call_perf_t... File webrtc/video/call_perf_tests.cc (right): https://codereview.webrtc.org/1181653002/diff/240001/webrtc/video/call_perf_t... webrtc/video/call_perf_tests.cc:286: CreateStreams(); This seems unclean somehow - can you rewrite this as if (create_audio_first) create audio CreateStreams() else CreateStreams() create_audio ?
rebase
feedback
Won't push this before M45 cuts, it feels risky. https://codereview.webrtc.org/1181653002/diff/240001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/1181653002/diff/240001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine_unittest.cc:3432: .rtp.extensions.empty()); On 2015/07/06 19:04:00, hta wrote: > Drive-by comment: This test doesn't seem very future-proof. The combined BWE > option may be the only RTP extension we use *today*, but what happens the day > the "mid" RTP option goes live? Yes, this isn't meant to be future proof, it's supposed to be non-optional. Before this CL we never created audio receive streams if there wasn't combined BWE, so this was a way to get stream-based A/V sync in at all. https://codereview.webrtc.org/1181653002/diff/240001/webrtc/video/bitrate_est... File webrtc/video/bitrate_estimator_tests.cc (right): https://codereview.webrtc.org/1181653002/diff/240001/webrtc/video/bitrate_est... webrtc/video/bitrate_estimator_tests.cc:208: // underlying channel. On 2015/07/06 19:04:00, hta wrote: > This comment is not clear. Under what conditions is the voe_channel_id bogus, > and is zero always a bogus value? > > Possible rewording: > > "We set voe_channel_id to 0, which is bogus but legal. In production, the > voe_channel_id is always set to the id of an underlying channel." > Rephrased as: // Bogus non-default id to prevent hitting a DCHECK when creating the // AudioReceiveStream. Every receive stream has to correspond to an // underlying channel id. https://codereview.webrtc.org/1181653002/diff/240001/webrtc/video/call.cc File webrtc/video/call.cc (right): https://codereview.webrtc.org/1181653002/diff/240001/webrtc/video/call.cc#new... webrtc/video/call.cc:481: "the current implementation."; On 2015/07/06 19:04:00, hta wrote: > Should there be a bug number filed to track this restriction? https://code.google.com/p/webrtc/issues/detail?id=4762 Put it in a comment as well. https://codereview.webrtc.org/1181653002/diff/240001/webrtc/video/call_perf_t... File webrtc/video/call_perf_tests.cc (right): https://codereview.webrtc.org/1181653002/diff/240001/webrtc/video/call_perf_t... webrtc/video/call_perf_tests.cc:286: CreateStreams(); On 2015/07/06 19:04:00, hta wrote: > This seems unclean somehow - can you rewrite this as > > if (create_audio_first) > create audio > CreateStreams() > else > CreateStreams() > create_audio > > ? Agreed, done.
rebase
The CQ bit was checked by pbos@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, hta@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1181653002/#ps300001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1181653002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/182)
TBR=mflodman@ for:
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
webrtc/audio_receive_stream.h
webrtc/video_receive_stream.h
The CQ bit was checked by pbos@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1181653002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by pbos@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1181653002/300001
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/8fc7fa798f7a36955f1b933980401afad2aff592 Cr-Commit-Position: refs/heads/master@{#9586} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
