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

Issue 2806173002: Fix RtpReceiver.GetParameters when SSRCs aren't signaled. (Closed)

Created:
3 years, 8 months ago by Taylor Brandstetter
Modified:
3 years, 8 months ago
Reviewers:
pthatcher1
CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

Fix RtpReceiver.GetParameters when SSRCs aren't signaled. When SSRCs aren't signaled, an SSRC of 0 is used internally to mean "the default receive stream". But this wasn't working with the implementation of GetRtpReceiveParameters in the audio/video engines. This was breaking RtpReceiver.GetParameters in this situation, as well as the new getStats implementation (which relies on GetParameters). The new implementation will fail if *no* default receive stream is configured (meaning no default sink is set), and otherwise will return a default RtpEncodingParameters (later it will be filled with relevant SDP parameters as they're implemented). BUG=webrtc:6971 Review-Url: https://codereview.webrtc.org/2806173002 Cr-Commit-Position: refs/heads/master@{#17803} Committed: https://chromium.googlesource.com/external/webrtc/+/3bc15103aea1dd2fdfc01b833b62f34171c82e81

Patch Set 1 #

Total comments: 2

Patch Set 2 : Changing behavior slightly in response to comment on https://github.com/w3c/webrtc-pc/issues/1116 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+167 lines, -27 lines) Patch
M webrtc/media/base/mediachannel.h View 1 3 chunks +11 lines, -1 line 0 comments Download
M webrtc/media/engine/webrtcvideoengine2.h View 1 chunk +3 lines, -0 lines 0 comments Download
M webrtc/media/engine/webrtcvideoengine2.cc View 1 2 chunks +37 lines, -13 lines 0 comments Download
M webrtc/media/engine/webrtcvideoengine2_unittest.cc View 1 1 chunk +43 lines, -0 lines 0 comments Download
M webrtc/media/engine/webrtcvoiceengine.cc View 1 3 chunks +36 lines, -13 lines 0 comments Download
M webrtc/media/engine/webrtcvoiceengine_unittest.cc View 1 1 chunk +37 lines, -0 lines 0 comments Download

Messages

Total messages: 12 (6 generated)
Taylor Brandstetter
3 years, 8 months ago (2017-04-10 04:05:24 UTC) #2
the sun
https://codereview.webrtc.org/2806173002/diff/1/webrtc/media/engine/webrtcvoiceengine.cc File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2806173002/diff/1/webrtc/media/engine/webrtcvoiceengine.cc#newcode1761 webrtc/media/engine/webrtcvoiceengine.cc:1761: if (!default_sink_) { Checking !unsignaled_recv_ssrcs_.empty() should be enough, if ...
3 years, 8 months ago (2017-04-18 18:27:20 UTC) #4
Taylor Brandstetter
Pinging Peter. https://codereview.webrtc.org/2806173002/diff/1/webrtc/media/engine/webrtcvoiceengine.cc File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2806173002/diff/1/webrtc/media/engine/webrtcvoiceengine.cc#newcode1761 webrtc/media/engine/webrtcvoiceengine.cc:1761: if (!default_sink_) { On 2017/04/18 18:27:20, the ...
3 years, 8 months ago (2017-04-20 07:16:22 UTC) #5
pthatcher1
lgtm
3 years, 8 months ago (2017-04-21 01:14:31 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2806173002/20001
3 years, 8 months ago (2017-04-21 02:01:45 UTC) #9
commit-bot: I haz the power
3 years, 8 months ago (2017-04-21 02:25:12 UTC) #12
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/external/webrtc/+/3bc15103aea1dd2fdfc01b833...

Powered by Google App Engine
This is Rietveld 408576698