Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2505003003:
Let MediaSession generate a FlexFEC SSRC when FlexFEC is active. (Closed)
DescriptionLet MediaSession generate a FlexFEC SSRC when FlexFEC is active.
This CL generates the SSRC that will be exposed in the FEC-FR
group in the SDP.
BUG=webrtc:5654
R=perkj@webrtc.org
CC=stefan@webrtc.org, magjed@webrtc.org
Committed: https://crrev.com/03d5fb1294589293c7a9f5da1b9278b25fc7d270
Cr-Commit-Position: refs/heads/master@{#15187}
Patch Set 1 #Patch Set 2 : Rebase. #
Total comments: 8
Patch Set 3 : Feedback response 1. #Patch Set 4 : Rebase. #
Messages
Total messages: 23 (12 generated)
[Same message for three related CLs.] Please take a look at this change :) To make it easier to review, I have split a set of related changes into three dependent CLs. The CLs have different reviewers, but you are CC'd on the ones where I didn't set you as a reviewer. Review == holmer: config.h magjed: media/ perkj: pc/ Overview == This set of CLs add support for FlexFEC (see https://tools.ietf.org/html/draft-ietf-payload-flexible-fec-scheme-03) in MediaSession, StreamParams, and VideoEngine2. After these changes, FlexFEC can be exposed in the SDP and the corresponding config will be propagated down to the Call level. This is not turned on by default, but instead hidden between a field trial. Changes == StreamParams: Adds support for the FEC-FR grouping semantics. MediaSession: Whenever FlexFEC is added to the list of codecs, a FlexFEC SSRC will be generated, together with a FEC-FR grouping associating it with the primary SSRC. See https://tools.ietf.org/html/rfc5956 for a description of the FEC-FR grouping semantics. Simulcast is not yet supported. VideoEngine2: Interfaces the SDP information with the Call level objects. The FlexFEC payload type is picked up when the Send/Receive parameters are set. The FlexFEC SSRC is picked up when a Send/Receive stream is added, if the FEC-FR information is provided. The parameters are propagated down to a Call level, where the corresponding VideoSendStream config is updated, and possibly a FlexfecReceiveStream object is created.
Rebase.
perkj@webrtc.org changed reviewers: + pthatcher@webrtc.org
lgtm https://codereview.webrtc.org/2505003003/diff/20001/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/2505003003/diff/20001/webrtc/pc/mediasession.cc... webrtc/pc/mediasession.cc:489: // TODO(brandtr): Update when we support multistream protection. No support for Simulcast? https://codereview.webrtc.org/2505003003/diff/20001/webrtc/pc/mediasession.cc... webrtc/pc/mediasession.cc:710: for (it = codecs.begin(); it != codecs.end(); ++it) { const auto& codec : codecs ? https://codereview.webrtc.org/2505003003/diff/20001/webrtc/pc/mediasession_un... File webrtc/pc/mediasession_unittest.cc (right): https://codereview.webrtc.org/2505003003/diff/20001/webrtc/pc/mediasession_un... webrtc/pc/mediasession_unittest.cc:193: static void AddSecondaryCodec(const VideoCodec& secondary_codec, Why not just AddCodec if you want to keep it at all. I dont see the value of it. https://codereview.webrtc.org/2505003003/diff/20001/webrtc/pc/mediasession_un... webrtc/pc/mediasession_unittest.cc:2058: // And should a FEC-FR group for FlexFEC. and should have
Rebase.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
https://codereview.webrtc.org/2505003003/diff/20001/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/2505003003/diff/20001/webrtc/pc/mediasession.cc... webrtc/pc/mediasession.cc:489: // TODO(brandtr): Update when we support multistream protection. On 2016/11/17 20:41:12, perkj_webrtc wrote: > No support for Simulcast? No :( This is not too hard to add though, so it may not be long until we do support it. https://codereview.webrtc.org/2505003003/diff/20001/webrtc/pc/mediasession.cc... webrtc/pc/mediasession.cc:710: for (it = codecs.begin(); it != codecs.end(); ++it) { On 2016/11/17 20:41:12, perkj_webrtc wrote: > const auto& codec : codecs ? Done. https://codereview.webrtc.org/2505003003/diff/20001/webrtc/pc/mediasession_un... File webrtc/pc/mediasession_unittest.cc (right): https://codereview.webrtc.org/2505003003/diff/20001/webrtc/pc/mediasession_un... webrtc/pc/mediasession_unittest.cc:193: static void AddSecondaryCodec(const VideoCodec& secondary_codec, On 2016/11/17 20:41:12, perkj_webrtc wrote: > Why not just AddCodec if you want to keep it at all. I dont see the value of it. > After a second thought, I don't think the the assert on line 195 is necessary for FlexFEC. I'm renaming this function back to AddRtxCodec, and won't use it for FlexFEC. https://codereview.webrtc.org/2505003003/diff/20001/webrtc/pc/mediasession_un... webrtc/pc/mediasession_unittest.cc:2058: // And should a FEC-FR group for FlexFEC. On 2016/11/17 20:41:12, perkj_webrtc wrote: > and should have Done.
Rebase.
The CQ bit was checked by brandtr@webrtc.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.webrtc.org/...
The CQ bit was unchecked by brandtr@webrtc.org
The CQ bit was checked by brandtr@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from perkj@webrtc.org Link to the patchset: https://codereview.webrtc.org/2505003003/#ps100001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_libfuzzer_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by brandtr@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1479814536261720,
"parent_rev": "72b9c0079837b06eff8f53344679d7158bc7e41a", "commit_rev":
"94886dd5ed78d6b3092db644be8d10430be721ed"}
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Let MediaSession generate a FlexFEC SSRC when FlexFEC is active. This CL generates the SSRC that will be exposed in the FEC-FR group in the SDP. BUG=webrtc:5654 R=perkj@webrtc.org CC=stefan@webrtc.org, magjed@webrtc.org ========== to ========== Let MediaSession generate a FlexFEC SSRC when FlexFEC is active. This CL generates the SSRC that will be exposed in the FEC-FR group in the SDP. BUG=webrtc:5654 R=perkj@webrtc.org CC=stefan@webrtc.org, magjed@webrtc.org Committed: https://crrev.com/03d5fb1294589293c7a9f5da1b9278b25fc7d270 Cr-Commit-Position: refs/heads/master@{#15187} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/03d5fb1294589293c7a9f5da1b9278b25fc7d270 Cr-Commit-Position: refs/heads/master@{#15187} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
