Issue 2991693002: Adding support for Unified Plan offer/answer negotiation.

Zhi Huang

Description was changed from ========== Unified plan SDP in mediasession layer. BUG= ========== to ========== ...

3 years, 5 months ago (2017-07-26 04:01:25 UTC) #1

Zhi Huang

zhihuang@webrtc.org changed reviewers: + deadbeef@webrtc.org

3 years, 4 months ago (2017-07-27 00:15:58 UTC) #7

Zhi Huang

PTAL. More tests could be added but I think this should be OK for the ...

3 years, 4 months ago (2017-07-27 00:15:58 UTC) #8

Taylor Brandstetter

I've only reviewed the mediasession part so far, but I'm sending out my comments now ...

3 years, 4 months ago (2017-07-28 01:19:40 UTC) #9

Taylor Brandstetter

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession.cc#newcode1374 webrtc/pc/mediasession.cc:1374: // This restriction doesn't exist before. On 2017/07/28 01:19:38, ...

3 years, 4 months ago (2017-07-28 01:23:14 UTC) #10

Taylor Brandstetter

Finished reviewing rest of files. I'm pretty confused by the WebRtcSession test infrastructure, but I ...

3 years, 4 months ago (2017-07-28 19:00:27 UTC) #11

Zhi Huang

Description was changed from ========== Adding support for Unified Plan offer/answer negotiation to the mediasession ...

3 years, 4 months ago (2017-07-28 22:24:30 UTC) #12

Zhi Huang

Description was changed from ========== Adding support for Unified Plan offer/answer negotiation to the mediasession ...

3 years, 4 months ago (2017-07-28 22:25:36 UTC) #13

Zhi Huang

Description was changed from ========== Adding support for Unified Plan offer/answer negotiation to the mediasession ...

3 years, 4 months ago (2017-07-28 22:26:30 UTC) #14

Zhi Huang

PTAL. https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (left): https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession.cc#oldcode449 webrtc/pc/mediasession.cc:449: if (streams.empty() && add_legacy_stream) { On 2017/07/28 01:19:38, ...

3 years, 4 months ago (2017-08-02 04:38:37 UTC) #17

PTAL.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (left):

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:449: if (streams.empty() && add_legacy_stream) {
On 2017/07/28 01:19:38, Taylor Brandstetter wrote:
> Somewhere in the CL description, can you note that the "add_legacy_stream"
> option is going away? Just in case anyone is relying on it and we didn't
> realize.

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (right):

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:938: ext_to_match.uri);
On 2017/07/28 01:19:39, Taylor Brandstetter wrote:
> This doesn't seem like it would work... If "extensions" contains both the
> encrypted and non-encrypted versions, FindHeaderExtensionByUri would just
return
> the first one found.

Agree. I'll update this.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:1003: NULL)) {
On 2017/07/28 01:19:38, Taylor Brandstetter wrote:
> nullptr?

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:1006: if
(FindByUriAndEncryption(*encrypted_extensions, reference_extension,
On 2017/07/28 01:19:39, Taylor Brandstetter wrote:
> How could an extension be in encrypted_extensions but not offered_extensions?
I
> probably am overlooking something (sorry).

In current use case, both the regular and encrypted extensions are part of
offered extensions and this branch will never be executed.

It might still make sense when we use this method to merge the
|reference_extensions| to new |offered_extensions| with previously extracted
|regular_extensions| and |encrypted_extensions|.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:1098: // TODO: combine these two params into
mediadescriptionoptions
On 2017/07/28 01:19:38, Taylor Brandstetter wrote:
> Can this be done, since there's now MediaDescriptionOptions? I don't think
there
> was when I wrote this TODO to myself.

Done. I should be able be fix this before.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:1325: all_audio_codecs_.clear();
On 2017/07/28 01:19:38, Taylor Brandstetter wrote:
> Maybe the "clear" should happen inside ComputeAudioCodecsIntersectionAndUnion?

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:1355: // Must have options for each existing section.
On 2017/07/28 01:19:38, Taylor Brandstetter wrote:
> This conflicts with the comment in the header file that says:
> 
> // The m= section with index X will use media_description_options[X]. If no
such
> // MediaDescriptionOptions exists, the m= section will just be rejected (port
of
> // 0).
> 
> I think that comment is just out of date.

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:1362: // description in |current_description|.
On 2017/07/28 01:19:38, Taylor Brandstetter wrote:
> "descriptions", plural

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:1374: // This restriction doesn't exist before.
On 2017/07/28 01:23:14, Taylor Brandstetter wrote:
> On 2017/07/28 01:19:38, Taylor Brandstetter wrote:
> > Hmmm... We may want to remove this restriction. The MID is allowed to change
> > when an m= section is recycled. And existing apps may be relying on this
being
> > allowed.
> 
> In case you're not familiar with the terminology, when I say "recycled", I
mean
> when it's rejected in one offer/answer, and its index is used for something
else
> in a subsequent offer/answer. Look for the word "recycle" in JSEP.

Thanks for clarifying it.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:1484: // Media types and mids must match.
On 2017/07/28 01:19:38, Taylor Brandstetter wrote:
> nit: When referring to MIDs in comments, we should use "MID", all-caps. Like
> "RTP" or "SDP".

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:1493: // Media types and mids must match.
On 2017/07/28 01:19:38, Taylor Brandstetter wrote:
> This is the same as the comment above; maybe the comments should say:
> 
> // Media types and MIDs must match between the remote offer and the
> MediaDescriptionOptions.
> 
> And then:
> 
> // Media types and MIDs must match between the existing local description, if
> one exists, and the MediaDescriptionOptions.
> 
> This second DCHECK may also be too strict for the "recycling" case though, so
we
> may want to remove it.

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:1744: if (IsMediaContentOfType(&content,
MEDIA_TYPE_DATA)) {
On 2017/07/28 01:19:38, Taylor Brandstetter wrote:
> This "if (data) { continue; }" block can actually be removed, since nothing
> happens in this loop outside the "if (audio) {} else if (video) {}" blocks.

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:1843: AudioCodecs supported_audio_codecs =
On 2017/07/28 01:19:38, Taylor Brandstetter wrote:
> This can be a const ref to avoid a copy.

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession.h
File webrtc/pc/mediasession.h (right):

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession.h...
webrtc/pc/mediasession.h:120: // sender per transceiver and this can be
simplified.
On 2017/07/28 01:19:39, Taylor Brandstetter wrote:
> nit: Can change this comment to say "per media description". In an earlier
> version of my CL I called this struct "TransceiverOptions".

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession.h...
webrtc/pc/mediasession.h:133: // don't use MID.
On 2017/07/28 01:19:39, Taylor Brandstetter wrote:
> Can this TODO be removed? Are the tests for "endpoint that doesn't implement
> MID" succeeding? (I hope those tests exist...)

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession.h...
webrtc/pc/mediasession.h:139: // stream information goes in local descriptions.
On 2017/07/28 01:19:39, Taylor Brandstetter wrote:
> nit: Add "the" before "local descriptions".

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession.h...
webrtc/pc/mediasession.h:153: // generated in the next available space.
On 2017/07/28 01:19:39, Taylor Brandstetter wrote:
> Is this comment about data_channel_type still valid? If so, has_data seems
> incorrect.

The comments should be updated.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession.h...
webrtc/pc/mediasession.h:164: // TODO: make index-based
On 2017/07/28 01:19:39, Taylor Brandstetter wrote:
> Is this method even still used?

Since MID is not the identification, I think we can remove it.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession_u...
File webrtc/pc/mediasession_unittest.cc (left):

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:2874: }
On 2017/07/28 01:19:39, Taylor Brandstetter wrote:
> What this test is really verifying is that MIDs are generated that match the
> existing offer, rather than the default MIDs (CN_AUDIO, etc.).
> 
> So, updating this test to the new API, I'd suggest passing non-default MIDs
into
> AddMediaSection.
> 
> And there could also be a test that ensures a DCHECK is triggered if the MIDs
> don't match between the MediaSessionOptions and existing offer (you can do
this
> with gtest death tests; there are a couple examples in the webrtc codebase).
> This may be overkill though.

I'll use some non-default MIDs for this tests. 
But we have removed the restriction on the MID for m-section recycling.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession_u...
File webrtc/pc/mediasession_unittest.cc (right):

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:246: bool stopped,
On 2017/07/28 01:19:39, Taylor Brandstetter wrote:
> nit: I'd replace "send/recv" with a MediaContentDirection, and use constants
> like "constexpr bool kStopped = true;" for "stopped". Then you can write code
> like this:
> 
> AddMediaSection(MEDIA_TYPE_AUDIO,
>                 "audio",
>                 MD_SENDRECV,
>                 kNotStopped,
>                 &session_options);

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:247: MediaSessionOptions& session_options) {
On 2017/07/28 01:19:39, Taylor Brandstetter wrote:
> nit: Since this an out parameter it should be passed by pointer instead of by
> reference.

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:532: // to |direction_in_offer|.
On 2017/07/28 01:19:39, Taylor Brandstetter wrote:
> I'd add to this comment: "and the answerer is willing to both send and
receive".

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:540: direction_in_offer ==
cricket::MD_RECVONLY);
On 2017/07/28 01:19:39, Taylor Brandstetter wrote:
> If "send/recv" were replaced with a MediaContentDirection this would become
> simpler too.

Yes, thanks for the suggestion.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:546: MediaSessionOptions answer_opts;
On 2017/07/28 01:19:39, Taylor Brandstetter wrote:
> nit: I'd put the answer opts above CreateAnswer.

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:714: false /*stopped*/, opts);
On 2017/07/28 01:19:39, Taylor Brandstetter wrote:
> A helper method for adding an audio and video section may be helpful. For
> example:
> 
> AddAudioAndVideoMediaSections(MediaContentDirection direction, 
>                               MediaSessionOptions* opts);
> 
> As long as the test doesn't depend on one of the values set inside the helper
> method (like "audio" being the MID), this improves the test's readability.

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:717: false /*stopped*/, opts);
On 2017/07/28 01:19:39, Taylor Brandstetter wrote:
> Setting opts.data_channel_type to DCT_RTP and adding a data section is pretty
> common; maybe add a helper method for that too, like:
> 
> AddDataSection(cricket::DataChannelType type, MediaSessionOptions* opts);

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:911: // SessionDescription is preserved in
the new SessionDescription.
On 2017/07/28 01:19:39, Taylor Brandstetter wrote:
> These tests for ordering are a bit obsolete now that the ordering is explicit,
> but we can worry about cleaning up these tests later. May want to add a TODO
> though. Maybe a blanket TODO at the top of this file, like:
> 
> TODO(deadbeef): Most of these tests were written while MediaSessionOptions was
> designed for Plan B SDP, where only one audio "m=" section and one video "m="
> section could be generated, and ordering couldn't be controlled. Many of these
> tests may be obsolete as a result, and should be refactored or removed.

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:1564: false /*stopped*/, offer_opts);
On 2017/07/28 01:19:39, Taylor Brandstetter wrote:
> If these options are identical, could just use the same struct. Or at least,
> order it as "offer options for all media types", then "answer options for all
> media types".

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:1644: MediaSessionOptions opts;
On 2017/07/28 01:19:39, Taylor Brandstetter wrote:
> nit: rename to "offer_opts", to match "answer_opts" here and elsewhere?

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:3304: if (send_audio) {
On 2017/07/28 01:19:39, Taylor Brandstetter wrote:
> To extract the send/recv direction easily, you can use:
> 
> RtpTransceiverDirection::FromMediaContentDirection(direction).send

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/peerconnection.cc
File webrtc/pc/peerconnection.cc (right):

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection.cc:325: // TODO: delete, it's simpler without a
function
On 2017/07/28 19:00:25, Taylor Brandstetter wrote:
> Can this TODO be addressed in this CL? Also, as we discovered when looking at
> this code, the "kUndefined" direction gets overwritten later, so this code is
> misleading.

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection.cc:352: // m= sections (in other words, nothign that
involves a map/array).
On 2017/07/28 19:00:25, Taylor Brandstetter wrote:
> nothign -> nothing

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection.cc:1628: bool send_video =
HasRtpSender(cricket::MEDIA_TYPE_VIDEO);
On 2017/07/28 19:00:25, Taylor Brandstetter wrote:
> nit: Some whitespace in between different logical groups of statements could
> help readability here.

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection.cc:1774: // By default, generate sendrecv/recvonly m=
sections.
On 2017/07/28 19:00:25, Taylor Brandstetter wrote:
> I'd clarify that the generated direction will also be restricted by the
> direction in the offer.

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection.cc:1858: :
&session_options->media_description_options[data_index];
On 2017/07/28 19:00:25, Taylor Brandstetter wrote:
> nit: Something could probably be done to reduce duplication (between
> audio/video/data, and between offer/answer). But I don't have any great ideas,
> so unless you have one, I'm fine with this as-is.

I think maybe the code in the for-loop can be optimized.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/peerconnection.h
File webrtc/pc/peerconnection.h (right):

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection.h:44: PeerConnectionInterface::RTCOfferAnswerOptions*
offer_answer_options);
On 2017/07/28 19:00:25, Taylor Brandstetter wrote:
> Unless the tests still rely on these, the declarations could be moved to the
CC
> file.

Might still need one of them for WebRtcSession tests.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/peerconnection...
File webrtc/pc/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/peerconnection...
webrtc/pc/peerconnectioninterface_unittest.cc:1176: const RTCOfferAnswerOptions&
offer_answer_options) {
On 2017/07/28 19:00:25, Taylor Brandstetter wrote:
> Can this just return the unique_ptr, returning null for failure?

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/peerconnection...
webrtc/pc/peerconnectioninterface_unittest.cc:1212: bool VerifyNoCNCodecs(const
cricket::ContentInfo* content) {
On 2017/07/28 19:00:25, Taylor Brandstetter wrote:
> I think it would make more sense to call this "HasCNCodec", and then in the
test
> do:
> 
> EXPECT_FALSE(HasCNCodec(content));
> 
> Or do the "EXPECT" in this method itself. The word "verify" somewhat implies
> that it does that.

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/peerconnection...
webrtc/pc/peerconnectioninterface_unittest.cc:3466:
TEST_F(PeerConnectionInterfaceTest, GetOptionsForOfferWithInvalidAudioOption) {
On 2017/07/28 19:00:25, Taylor Brandstetter wrote:
> Since these tests are no longer calling "GetOptionsForOffer", can they be
> renamed appropriately? For example,
> "CreateOfferFailsWithInvalidOfferToReceiveAudio".

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/peerconnection...
webrtc/pc/peerconnectioninterface_unittest.cc:3472:
EXPECT_FALSE(CreateOfferWithOptions(&offer, rtc_options));
On 2017/07/28 19:00:25, Taylor Brandstetter wrote:
> These tests could use some comments. For example:
> 
> // Setting offer_to_receive_audio to a value lower than kUndefined or greater
> than kMaxOfferToReceiveMedia should be treated as invalid.

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/peerconnection...
webrtc/pc/peerconnectioninterface_unittest.cc:3504: EXPECT_NE(nullptr,
offer->description()->GetContentByName(cricket::CN_VIDEO));
On 2017/07/28 19:00:25, Taylor Brandstetter wrote:
> nit: To be foolproof, we shouldn't rely on "GetContentByName", since the API
> doesn't guarantee anything about the name. Can use
> "GetFirstAudioContent"/"GetFirstVideoContent" instead.
> 
> (I know it's not your fault the tests did this, but as long as we're rewriting
> them, we might as well fix things like this)

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/peerconnection...
webrtc/pc/peerconnectioninterface_unittest.cc:3505:
EXPECT_TRUE(offer->description()->HasGroup(cricket::GROUP_TYPE_BUNDLE));
On 2017/07/28 19:00:26, Taylor Brandstetter wrote:
> I don't think the BUNDLE group assertion is relevant to this test. There could
> be another test that does this assertion specifically.

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/peerconnection...
webrtc/pc/peerconnectioninterface_unittest.cc:3523: // Test that the
|vad_enabled| can be set correctly in MediaSessionOptions.
On 2017/07/28 19:00:26, Taylor Brandstetter wrote:
> This comment should be updated since this test doesn't deal with
> MediaSessionOptions directly any more.

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/peerconnection...
webrtc/pc/peerconnectioninterface_unittest.cc:3590:
CreateOfferWithOptionsAsLocalDescription(&offer, rtc_options);
On 2017/07/28 19:00:26, Taylor Brandstetter wrote:
> So this is also testing that if ice_restart is false, the ufrag *doesn't*
> change. I think that could go in a separate test. But if stays here, can you
add
> a comment that mentions that?

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/peerconnection...
webrtc/pc/peerconnectioninterface_unittest.cc:3617:
TEST_F(PeerConnectionInterfaceTest, MediaConstraintsInAnswer) {
On 2017/07/28 19:00:26, Taylor Brandstetter wrote:
> I think this test could be written in terms of PeerConnectionInterface as
well.
> It's not obvious, but what this is really testing is: "If
> SetMandatoryReceiveAudio(false) is called for the answer constraints, but an
> audio section was offered, there will still be an audio section in the
answer."
> It could also verify that the section is rejected (I think that's the expected
> behavior, currently).

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/webrtcsession_...
File webrtc/pc/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/webrtcsession_...
webrtc/pc/webrtcsession_unittest.cc:515: recv_video_ = true;
On 2017/07/28 19:00:26, Taylor Brandstetter wrote:
> It's not obvious why "SendAudioVideoStream1" would set "recv_audio_" to true.
> Can you add some comments explaining? Or change the name of this method?

I realized that we don't need to set recv_X actually.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/webrtcsession_...
webrtc/pc/webrtcsession_unittest.cc:567: bool stopped,
On 2017/07/28 19:00:26, Taylor Brandstetter wrote:
> Similar comment for this as for AddMediaSection in mediasession_unittest.cc;
> would make tests more readable to use MediaContentDirection and
> "kStopped"/"kNotStopped" constants, and session_options should be a pointer.

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/webrtcsession_...
webrtc/pc/webrtcsession_unittest.cc:575: // creating an answer or a new offer.
On 2017/07/28 19:00:27, Taylor Brandstetter wrote:
> Can you add a comment mentioning that this duplicates a lot of logic from
> PeerConnection, but that this will be fixed when PeerConnection and
> WebRtcSession are merged?

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/webrtcsession_...
webrtc/pc/webrtcsession_unittest.cc:636: data_section_added_ = false;
On 2017/07/28 19:00:26, Taylor Brandstetter wrote:
> nit: Using member variables here seems error-prone; they could be output
> parameters for AddExistingMediaSectionsAndSendersToOptions, or this method
could
> just use "has_audio" (couldn't it?).

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/webrtcsession_...
webrtc/pc/webrtcsession_unittest.cc:695: bool recv_audio =
On 2017/07/28 19:00:26, Taylor Brandstetter wrote:
> Couldn't this just be:
> 
> bool recv_audio = rtc_options.offer_to_receive_audio != 0;

Done.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/webrtcsession_...
webrtc/pc/webrtcsession_unittest.cc:802: options.bundle_enabled = true;
On 2017/07/28 19:00:26, Taylor Brandstetter wrote:
> It seems odd to set bundle_enabled to true here, but not in CreateOffer. I'd
> suggest having all the helper methods set "bundle_enabled" to true by default,
> and if there needs to be a test that sets it to false, it can use a different
or
> lower level helper method.

The helper methods here are different. For answer, the method takes a default
MediaSessionOptions with default bundle_enabled = false; for offer, the method
take a default RTCOfferAnswerOptions with default use_rtp_mux is true. So I only
set the bundle_enabled = true for answer.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/webrtcsession_...
webrtc/pc/webrtcsession_unittest.cc:3830: auto it =
options.FindMediaDescription(content.name);
On 2017/07/28 19:00:26, Taylor Brandstetter wrote:
> "ASSERT_NE(options.media_description_options.end(), it);"?

We can change this to index based iteration.

Taylor Brandstetter

It looks like all my comments were addressed, so I think all that's left to ...

3 years, 4 months ago (2017-08-03 00:24:48 UTC) #18

It looks like all my comments were addressed, so I think all that's left to do
is add new tests for some of the new functionality that's possible (in
mediasession_unittest.cc).

Some suggestions for test cases:

1. Create an offer with two m= sections of the same media type, each with a
SenderOptions, and verify that both are generated, they're not rejected, and
they each contain a stream.
2. Create an offer (and/or answer) with a "stopped" MediaDescriptionOptions, and
verify the generated m= section is rejected/stopped.
3. Create an offer that orders the video MediaDescriptionOptions before the
audio options, and verify that the generated m= sections have the same order.

Maybe you have some more ideas, but I think those are the main new things that
are now possible.

There could also be tests that cover the new logic for creating payload
type->codec mappings across multiple m= sections. I don't know how far we want
to go, but a basic test could be "Create offer with two m= sections of the same
media type, and verify they use the same payload types for the same codecs." And
maybe the same thing for an answer.

A step further would involve cases where the default payload type (from the
"reference codec") is used by a different codec (in either the remote offer or a
previous local description), and a new one needs to be chosen. But it may make
sense to just wait until PeerConnection supports unified plan SDP, and write
these more advanced tests at the PeerConnection level.

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/webrtcsession_...
File webrtc/pc/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/2991693002/diff/120001/webrtc/pc/webrtcsession_...
webrtc/pc/webrtcsession_unittest.cc:802: options.bundle_enabled = true;
On 2017/08/02 04:38:37, Zhi Huang wrote:
> On 2017/07/28 19:00:26, Taylor Brandstetter wrote:
> > It seems odd to set bundle_enabled to true here, but not in CreateOffer. I'd
> > suggest having all the helper methods set "bundle_enabled" to true by
default,
> > and if there needs to be a test that sets it to false, it can use a
different
> or
> > lower level helper method.
> 
> The helper methods here are different. For answer, the method takes a default
> MediaSessionOptions with default bundle_enabled = false; for offer, the method
> take a default RTCOfferAnswerOptions with default use_rtp_mux is true. So I
only
> set the bundle_enabled = true for answer.

Acknowledged; sorry for glossing over that.

https://codereview.webrtc.org/2991693002/diff/180001/webrtc/pc/mediasession_u...
File webrtc/pc/mediasession_unittest.cc (right):

https://codereview.webrtc.org/2991693002/diff/180001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:244: // MIDs are not the identification of
the MediaDescriptionOptions.
Can you explain what this comment means?

https://codereview.webrtc.org/2991693002/diff/180001/webrtc/pc/peerconnection.cc
File webrtc/pc/peerconnection.cc (right):

https://codereview.webrtc.org/2991693002/diff/180001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection.cc:1694: GenerateMediaDescrptionOptions(
nit: "Description" is missing an "i".

Zhi Huang

PTAL. https://codereview.webrtc.org/2991693002/diff/180001/webrtc/pc/mediasession_unittest.cc File webrtc/pc/mediasession_unittest.cc (right): https://codereview.webrtc.org/2991693002/diff/180001/webrtc/pc/mediasession_unittest.cc#newcode244 webrtc/pc/mediasession_unittest.cc:244: // MIDs are not the identification of the ...

3 years, 4 months ago (2017-08-04 18:38:26 UTC) #21

Taylor Brandstetter

Did one last full review. Just some nits and minor comments about test helper methods. ...

3 years, 4 months ago (2017-08-05 00:39:10 UTC) #22

Zhi Huang

PTAL. Thanks. https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession.cc#newcode194 webrtc/pc/mediasession.cc:194: } On 2017/08/05 00:39:08, Taylor Brandstetter wrote: ...

3 years, 4 months ago (2017-08-09 00:54:52 UTC) #25

PTAL. Thanks.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (right):

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:194: }
On 2017/08/05 00:39:08, Taylor Brandstetter wrote:
> This now assumes that content->description can safely be cast to
> MediaContentDescription, so it would make sense to add:
> 
> RTC_DCHECK(IsMediaContent(content));

Done.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:721: // Create a media content to be offered in a
session-initiate for the
On 2017/08/05 00:39:08, Taylor Brandstetter wrote:
> nit: We can remove "in a session-initiate"; I think that's referring to SIP,
but
> this code isn't SIP-specific.

Done.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:995: // |encrypted_extensions|, its ID is used.
On 2017/08/05 00:39:08, Taylor Brandstetter wrote:
> nit: Just to be even more clear, we could say "its ID is marked as used in
> |used_ids|".

Done.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:1000: UsedRtpHeaderExtensionIds* used_ids) {
On 2017/08/05 00:39:08, Taylor Brandstetter wrote:
> I still think it's possible to simplify this code. If |offered_extensions| is
> always the union of |regular_extensions| and |encrypted_extensions|, let's
just
> get rid of those parameters, and update "AddEncryptedVersionsOfHdrExts" if we
> need to.

offered_extension is not always the union of reg and encrypted.
I'll leave a comment here.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:1092: // session-initiate.  If the negotiation fails,
this method returns false.  The
On 2017/08/05 00:39:08, Taylor Brandstetter wrote:
> Again, we should remove the reference to session-initiate.

Done.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:1217: GetTransportDescription(content->name,
current_description);
On 2017/08/05 00:39:08, Taylor Brandstetter wrote:
> This still relies on the "content name" aka MID. I don't think this code path
is
> actually exercised in chromium, which is probably why it hasn't been a
problem,
> but we should match the ContentInfo with TransportDescription by index
instead.

Done.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:1484: media_description_options.type));
On 2017/08/05 00:39:08, Taylor Brandstetter wrote:
> Weren't we going to remove this restriction, for the "recycling" case?

Done.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:1846: }
On 2017/08/05 00:39:08, Taylor Brandstetter wrote:
> I don't understand this logic now that I'm looking at it again. "audio_codecs"
> will contain all supported audio codecs, regardless of whether
> "current_description" is null, right? So I think we just need the code inside
> the "if" block.

Done

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:1897: }
On 2017/08/05 00:39:08, Taylor Brandstetter wrote:
> In AddAudioContentForOffer, this step was necessary to filter codecs by
> direction, but since we don't do that for video, does this actually do
anything?

Maybe we should do the per-media section thing for video and data as well?

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:2038: }
On 2017/08/05 00:39:08, Taylor Brandstetter wrote:
> Same comment as above.

Done.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession.h
File webrtc/pc/mediasession.h (right):

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession.h...
webrtc/pc/mediasession.h:105: struct SenderOptions {
On 2017/08/05 00:39:08, Taylor Brandstetter wrote:
> Let's add a comment explaining that a "SenderOptions" represents the options
for
> an RtpSender contained with an m= section.

Done.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession.h...
webrtc/pc/mediasession.h:120: // sender per media descritption and this can be
simplified.
On 2017/08/05 00:39:09, Taylor Brandstetter wrote:
> nit: "descritption" has an extra 't'

Done.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession.h...
webrtc/pc/mediasession.h:149: // must be an option for each existing section.
On 2017/08/05 00:39:08, Taylor Brandstetter wrote:
> Can you clarify this comment slightly? "There must be an option for each
> existing section if creating an answer, or a subsequent offer."

Done.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession.h...
webrtc/pc/mediasession.h:589: SecurePolicy secure_ = SEC_DISABLED;
On 2017/08/05 00:39:08, Taylor Brandstetter wrote:
> Not really related to this CL, but something that's always confused me is how
> "SecurePolicy" only refers to SDES, not DTLS-SRTP. Can we rename this to
> "sdes_policy_" or something similar, or add a TODO to do so?

I think it also make sense to rename the getter and setter as well so I would
prefer to make a separate CL for it.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession_u...
File webrtc/pc/mediasession_unittest.cc (right):

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:213: static constexpr bool kStopped = true;
On 2017/08/05 00:39:09, Taylor Brandstetter wrote:
> May want to comment that this is used to make code using "AddMediaSection"
more
> readable. Also, "kActive" may be more readable than "!kStopped"

Done.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:981: AddMediaSection(MEDIA_TYPE_AUDIO,
"audio", cricket::MD_RECVONLY, kStopped,
On 2017/08/05 00:39:09, Taylor Brandstetter wrote:
> Meant to be "!kStopped"? Could also use CreatePlanBMediaSessionOptions here,
for
> consistency.

Done.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:1599:
AddAudioVideoSections(cricket::MD_RECVONLY, &offer_opts);
On 2017/08/05 00:39:09, Taylor Brandstetter wrote:
> Since this test needs the offer/answer options to match, this is a case where
it
> would be better to not use the AddAudioVideoSections helper method.
> 
> Could do this:
> 
> MediaSessionOptions opts;
> // Add media sections
> // Create offer
> opts.media_description_options[1].stopped = true;
> // Create answer

Done.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:1621: AddDataSection(cricket::DCT_RTP,
cricket::MD_RECVONLY, &offer_opts);
On 2017/08/05 00:39:09, Taylor Brandstetter wrote:
> Same thing here.

Done.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:1827: AttachSenderToMediaSection("video",
MEDIA_TYPE_VIDEO, kVideoTrack1,
On 2017/08/05 00:39:09, Taylor Brandstetter wrote:
> Similar comment for this test; it relies on specific values inside the
> AddAudioVideoSections helper method, so it would be better to not use it.

Done.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:1854:
AddAudioVideoSections(cricket::MD_RECVONLY, &offer_opts);
On 2017/08/05 00:39:09, Taylor Brandstetter wrote:
> Same thing here.

Done.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:2636: FindFirstMediaDescriptionByMid("audio",
&options);
On 2017/08/05 00:39:09, Taylor Brandstetter wrote:
> nit: Don't really have to look up by MID, since this test knows that it only
> added one MediaDescriptionOptions.

Done.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:2693: FindFirstMediaDescriptionByMid("audio",
&options);
On 2017/08/05 00:39:09, Taylor Brandstetter wrote:
> Same here.

Done.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:3030: MediaSessionOptions opts =
CreateUnifiedPlanMediaSessionOptions();
On 2017/08/05 00:39:09, Taylor Brandstetter wrote:
> This test has assertions that rely on specific things that happen in
> "CreateUnifiedPlanMediaSessionOptions", like the track names being
> "kAudioTrack1"/etc., there being exactly 2 audio sections and 2 video
sections,
> and them being ordered "audio, video, audio, video".
> 
> So it would be better to put that code in this test itself, so that the test
can
> be fully understood without looking at what the helper method does (among
other
> reasons). It's worth the cost of the test being larger and having some
> duplication between the test below.
> 
> See the "do"s and "don't"s here:
>
https://engdoc.corp.google.com/eng/doc/devguide/coding/testing/unit-testing-b...

Good to know this. Thanks for the link.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:3126: // Create an answer based on the offer.
On 2017/08/05 00:39:09, Taylor Brandstetter wrote:
> nit: I think "answer rejects m= section if rejected in offer" could be a
> separate test.

Done.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:3153:
EXPECT_FALSE(offer->contents()[1].rejected);
On 2017/08/05 00:39:09, Taylor Brandstetter wrote:
> nit: Since these aren't the main thing this test is testing, I'd change them
to
> "ASSERT_FALSE"s.

Done.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:3170:
TEST_F(MediaSessionDescriptionFactoryTest, TestMediaSectionsOrder) {
On 2017/08/05 00:39:09, Taylor Brandstetter wrote:
> nit: I think having the name "Test" in a test name is somewhat redundant. So
I'd
> just call this "MediaSectionsOrder", or something more descriptive like
> "CreateOfferRespectsMediaDescriptionOptionsOrder"

Done.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:3172: AddMediaSection(MEDIA_TYPE_VIDEO,
"video", cricket::MD_SENDRECV, !kStopped,
On 2017/08/05 00:39:09, Taylor Brandstetter wrote:
> Can you comment that this test puts "video" first, because normally audio
comes
> first by default?

Done.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:3187:
TestPayloadTypeOfSameCodecInOfferAnswer) {
On 2017/08/05 00:39:09, Taylor Brandstetter wrote:
> nit: Again, I think "Test" can be removed from the name, and the name could be
> more descriptive, like "PayloadTypesSharedByMediaSectionsOfSameType"

Done.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/mediasession_u...
webrtc/pc/mediasession_unittest.cc:3451: &offer_opts);
On 2017/08/05 00:39:09, Taylor Brandstetter wrote:
> Does this test need a video section at all?

It can be removed (and the one for answer below).

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/peerconnection.h
File webrtc/pc/peerconnection.h (right):

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection.h:35: void ExtractSharedMediaSessionOptions(
On 2017/08/05 00:39:09, Taylor Brandstetter wrote:
> Could add TODO to move this to the CC file when there aren't WebRtcSession
tests
> that use it.

Done.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/peerconnection...
File webrtc/pc/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/peerconnection...
webrtc/pc/peerconnectioninterface_unittest.cc:3648:
TEST_F(PeerConnectionInterfaceTest, CreateOfferWithVideoOnlyOptions) {
On 2017/08/05 00:39:09, Taylor Brandstetter wrote:
> nit: Would make sense to move this next to CreateOfferWithAudioOnlyOptions

Done.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/peerconnection...
webrtc/pc/peerconnectioninterface_unittest.cc:3730: // answer.
On 2017/08/05 00:39:09, Taylor Brandstetter wrote:
> nit: This comment should be updated since the test uses both audio and video.

Done.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/peerconnection...
webrtc/pc/peerconnectioninterface_unittest.cc:3731:
TEST_F(PeerConnectionInterfaceTest, CreateAnswerWithConstraintsAndRemoteOffer) {
On 2017/08/05 00:39:09, Taylor Brandstetter wrote:
> nit: This test could use a better name, like
> "RejectAudioAndVideoInAnswerWithConstraints"

Done.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/webrtcsession_...
File webrtc/pc/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/webrtcsession_...
webrtc/pc/webrtcsession_unittest.cc:577: for (auto media_description_options :
offered_media_sections_) {
On 2017/08/05 00:39:09, Taylor Brandstetter wrote:
> It would be ideal if instead of using extra state to remember the existing
> options (offered_media_sections_), they were just generated from
> WebRtcSession::local_description.

There are some test cases would create answer before setting local/remote
descriptions (e.g test creating answer in the wrong state would fail) so I added
this extra vector.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/webrtcsession_...
webrtc/pc/webrtcsession_unittest.cc:727: void
GetOptionsForRemoteOffer(cricket::MediaSessionOptions* session_options) {
On 2017/08/05 00:39:10, Taylor Brandstetter wrote:
> This method is what still confuses me; it uses a lot of state that's used for
> creating a local offer, so I'm surprised it even works. Can the tests just
> explicitly add to the MediaSessionOptions instead, using helper methods if
> necessary for readability? For example, "AddAudioVideoSections", as in
> mediasession_unittest.cc?

I modified these tests a bit to make it easier to understand.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/webrtcsession_...
webrtc/pc/webrtcsession_unittest.cc:1071:
EXPECT_TRUE(session_->SetRemoteDescription(desc, NULL));
I would change this to ASSERT so that the test wouldn't have undefined behavior
because of this failure.

https://codereview.webrtc.org/2991693002/diff/240001/webrtc/pc/webrtcsession_...
webrtc/pc/webrtcsession_unittest.cc:1115: GetOptionsForAnswer(&answer_options);
On 2017/08/05 00:39:10, Taylor Brandstetter wrote:
> I also don't see how GetOptionsForAnswer works for remote answers. It seems
like
> methods that use "WebRtcSessionTest" state (like recv_audio_, send_stream_1_)
> are only valid for creating a "local" offer or answer, since otherwise all the
> directions would end up reversed.

I added another helper method for remote answer options.

Taylor Brandstetter

lgtm, though I still have minor concerns about the code that builds the audio codec ...

3 years, 4 months ago (2017-08-09 17:05:12 UTC) #26

Zhi Huang

I have some updates on the CL. You might want to take a look. https://codereview.webrtc.org/2991693002/diff/300001/webrtc/p2p/base/sessiondescription.h ...

3 years, 4 months ago (2017-08-09 21:51:02 UTC) #28

I have some updates on the CL. You might want to take a look.

https://codereview.webrtc.org/2991693002/diff/300001/webrtc/p2p/base/sessiond...
File webrtc/p2p/base/sessiondescription.h (right):

https://codereview.webrtc.org/2991693002/diff/300001/webrtc/p2p/base/sessiond...
webrtc/p2p/base/sessiondescription.h:59: size_t msection_index = -1;
On 2017/08/09 17:05:11, Taylor Brandstetter wrote:
> It's redundant to store this inside ContentInfo, since it's implied by the
> ContentInfo's index in SessionDescription::contents().
> 
> Where this is used in IsDtlsActive, you could do this instead:
> 
> int msection_index = content - &current_description->contents()[0];
> 
> Since subtraction of pointers gives you the number of elements that fit
between
> the two addresses.
> 
> Or you could just pass the index into the method.

This is a good point!

https://codereview.webrtc.org/2991693002/diff/300001/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (right):

https://codereview.webrtc.org/2991693002/diff/300001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:1822: const AudioCodecs& audio_codecs,
On 2017/08/09 17:05:12, Taylor Brandstetter wrote:
> As we talked about earlier, it would be helpful to rename these codec list
> variables to make it more clear what they're used for, or add comments.
> 
> audio_codecs = set of all possible codecs that can be used, with correct
payload
> type mappings
> 
> supported_audio_codecs = set of codecs that are supported for the direction of
> this m= section
> 
> acd->codecs() = set of previously negotiated codecs for this m= section
> 
> The payload types should come from audio_codecs, but the order should come
from
> acd->codecs() and then supported_codecs, to ensure that re-offers don't change
> existing codec priority, and that new codecs are added with the right
priority.

Done.

https://codereview.webrtc.org/2991693002/diff/300001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:1840: codec, &existing_codec)) {
On 2017/08/09 17:05:12, Taylor Brandstetter wrote:
> Based on the description of FindMatchingCodec, I think this should be:
> 
> if (FindMatchingCodec(acd->codecs(), supported_audiocodecs, codec, nullptr)) {
>   filtered_codecs.push_back(codec);
> }
> 
> In other words, for each existing codec, add it to the new offer if it's
> supported (based on the direction).

Done.

https://codereview.webrtc.org/2991693002/diff/300001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:1841: if (current_description) {
On 2017/08/09 17:05:12, Taylor Brandstetter wrote:
> I still don't understand the "if (current_description)" logic. Maybe with my
> above change it wouldn't be necessary. But if it is necessary, can you add
> comments explaining it?

Done.

https://codereview.webrtc.org/2991693002/diff/300001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:1852: codec, &existing_codec) &&
On 2017/08/09 17:05:12, Taylor Brandstetter wrote:
> nit: "existing_codec" isn't a great name, since it doesn't necessarily come
from
> the current description. It could come from the "reference codecs".

Maybe just call it "found_codec"? I'm not good at naming.

https://codereview.webrtc.org/2991693002/diff/300001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:1856: filtered_codecs.push_back(existing_codec);
On 2017/08/09 17:05:12, Taylor Brandstetter wrote:
> Again, why is the "if (current_description)" needed? Why not always do
> filtered_codecs.push_back(existing_codec);?

Done.

https://codereview.webrtc.org/2991693002/diff/300001/webrtc/pc/webrtcsession_...
File webrtc/pc/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/2991693002/diff/300001/webrtc/pc/webrtcsession_...
webrtc/pc/webrtcsession_unittest.cc:3014: // transport is not currently
supported.
On 2017/08/09 17:05:12, Taylor Brandstetter wrote:
> Could link to the bug here:
> https://bugs.chromium.org/p/webrtc/issues/detail?id=6704

Done.

Zhi Huang

The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run

3 years, 4 months ago (2017-08-09 22:08:20 UTC) #29

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2991693002/340001

3 years, 4 months ago (2017-08-09 22:08:31 UTC) #30

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 4 months ago (2017-08-10 00:36:41 UTC) #32

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 4 months ago (2017-08-10 00:36:42 UTC) #33

Zhi Huang

The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run

3 years, 4 months ago (2017-08-11 20:13:17 UTC) #34

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2991693002/360001

3 years, 4 months ago (2017-08-11 20:13:22 UTC) #35

Zhi Huang

The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run

3 years, 4 months ago (2017-08-14 17:25:48 UTC) #38

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2991693002/420001

3 years, 4 months ago (2017-08-14 17:25:55 UTC) #39

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 4 months ago (2017-08-14 17:29:24 UTC) #40

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_more_configs on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_more_configs/builds/7986)

3 years, 4 months ago (2017-08-14 17:29:25 UTC) #41

Zhi Huang

The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run

3 years, 4 months ago (2017-08-14 17:41:24 UTC) #42

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2991693002/440001

3 years, 4 months ago (2017-08-14 17:41:34 UTC) #43

Zhi Huang

The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run

3 years, 4 months ago (2017-08-14 20:02:14 UTC) #46

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2991693002/460001

3 years, 4 months ago (2017-08-14 20:02:24 UTC) #47

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 4 months ago (2017-08-14 20:27:14 UTC) #48

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/builds/6716)

3 years, 4 months ago (2017-08-14 20:27:16 UTC) #49

Zhi Huang

The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run

3 years, 4 months ago (2017-08-14 20:40:56 UTC) #50

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2991693002/480001

3 years, 4 months ago (2017-08-14 20:41:07 UTC) #52

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 4 months ago (2017-08-14 21:14:15 UTC) #53

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_clang_dbg/builds/25743)

3 years, 4 months ago (2017-08-14 21:14:16 UTC) #54

Zhi Huang

The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run

3 years, 4 months ago (2017-08-14 21:34:59 UTC) #56

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2991693002/500001

3 years, 4 months ago (2017-08-14 21:35:05 UTC) #57

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 4 months ago (2017-08-14 21:42:55 UTC) #58

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/builds/6497)

3 years, 4 months ago (2017-08-14 21:42:56 UTC) #59

Zhi Huang

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org Link to the ...

3 years, 4 months ago (2017-08-15 00:43:56 UTC) #61

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/2991693002/500001

3 years, 4 months ago (2017-08-15 00:44:07 UTC) #62

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 4 months ago (2017-08-15 00:48:55 UTC) #63

commit-bot: I haz the power

Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/20160)

3 years, 4 months ago (2017-08-15 00:48:56 UTC) #64

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/2991693002/500001

3 years, 4 months ago (2017-08-15 00:55:55 UTC) #66

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 500001, "attempt_start_ts": 1502758547689740, "parent_rev": "1d44550ddc8c412652c75a2dfb69ec861066592b", "commit_rev": "a77e6bbd30276bdc5b30f2cbc1e92ca181ae76f0"}

3 years, 4 months ago (2017-08-15 01:17:49 UTC) #67

commit-bot: I haz the power

Description was changed from ========== Adding support for Unified Plan offer/answer negotiation to the mediasession ...

3 years, 4 months ago (2017-08-15 01:18:16 UTC) #68

commit-bot: I haz the power

Committed patchset #9 (id:500001) as https://chromium.googlesource.com/external/webrtc/+/a77e6bbd30276bdc5b30f2cbc1e92ca181ae76f0

3 years, 4 months ago (2017-08-15 01:18:18 UTC) #69

olka_webrtc

A revert of this CL (patchset #9 id:500001) has been created in https://codereview.webrtc.org/3001083002/ by olka@webrtc.org. ...

3 years, 4 months ago (2017-08-17 11:33:16 UTC) #70

Zhi Huang

Description was changed from ========== Adding support for Unified Plan offer/answer negotiation to the mediasession ...

3 years, 4 months ago (2017-08-17 16:20:33 UTC) #71

Message was sent while issue was closed.

Description was changed from

==========
Adding support for Unified Plan offer/answer negotiation to the mediasession
layer.

This layer takes in a simplified "options" struct and the current local
description,
and generates a new offer/answer. Previously the options struct assumed there
would
only be one media description per media type (audio/video), but it now supports
N number of audio/video descriptions.

The |add_legacy_stream| options is removed from the mediasession.cc/.h 
in this CL.

The next step is to add the ability for PeerConnection/WebRtcSession to create
"options" to represent multiple RtpTransceivers, and apply the Unified Plan
descriptions correctly. Right now, only Plan B descriptions will be
generated in unit tests.

BUG=chromium:465349

Review-Url: https://codereview.webrtc.org/2991693002
Cr-Commit-Position: refs/heads/master@{#19343}
Committed:
https://chromium.googlesource.com/external/webrtc/+/a77e6bbd30276bdc5b30f2cbc...
==========

to

==========
Adding support for Unified Plan offer/answer negotiation to the mediasession
layer.

This layer takes in a simplified "options" struct and the current local
description,
and generates a new offer/answer. Previously the options struct assumed there
would
only be one media description per media type (audio/video), but it now supports
N number of audio/video descriptions.

The |add_legacy_stream| options is removed from the mediasession.cc/.h 
in this CL.

The next step is to add the ability for PeerConnection/WebRtcSession to create
"options" to represent multiple RtpTransceivers, and apply the Unified Plan
descriptions correctly. Right now, only Plan B descriptions will be
generated in unit tests.

BUG=chromium:465349

Review-Url: https://codereview.webrtc.org/2991693002
Cr-Commit-Position: refs/heads/master@{#19343}
Committed:
https://chromium.googlesource.com/external/webrtc/+/a77e6bbd30276bdc5b30f2cbc...
==========

Zhi Huang

Description was changed from ========== Adding support for Unified Plan offer/answer negotiation to the mediasession ...

3 years, 4 months ago (2017-08-17 16:21:06 UTC) #72

Zhi Huang

The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run

3 years, 4 months ago (2017-08-17 16:30:27 UTC) #73

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2991693002/520001

3 years, 4 months ago (2017-08-17 16:30:32 UTC) #74

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 4 months ago (2017-08-17 16:34:21 UTC) #75

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/builds/6864) ios64_sim_ios9_dbg on ...

3 years, 4 months ago (2017-08-17 16:34:23 UTC) #76

Zhi Huang

The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run

3 years, 4 months ago (2017-08-17 19:55:50 UTC) #78

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2991693002/540001

3 years, 4 months ago (2017-08-17 19:55:58 UTC) #79

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 4 months ago (2017-08-17 20:24:57 UTC) #80

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 4 months ago (2017-08-17 20:24:59 UTC) #81

Zhi Huang

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org Link to the ...

3 years, 4 months ago (2017-08-17 21:08:20 UTC) #83

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/2991693002/540001

3 years, 4 months ago (2017-08-17 21:08:28 UTC) #84

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 540001, "attempt_start_ts": 1503004099937710, "parent_rev": "825f65e9d2285e69467acd6449549f10f9e1df6b", "commit_rev": "1c378ed83b5b32a00368bbfc3ce2ee7687691abe"}

3 years, 4 months ago (2017-08-17 21:10:51 UTC) #85

commit-bot: I haz the power

Description was changed from ========== Relanding: Adding support for Unified Plan offer/answer negotiation to the ...

3 years, 4 months ago (2017-08-17 21:10:58 UTC) #86

Message was sent while issue was closed.

Description was changed from

==========
Relanding: Adding support for Unified Plan offer/answer negotiation to the
mediasession layer.

This layer takes in a simplified "options" struct and the current local
description,
and generates a new offer/answer. Previously the options struct assumed there
would
only be one media description per media type (audio/video), but it now supports
N number of audio/video descriptions.

The |add_legacy_stream| options is removed from the mediasession.cc/.h 
in this CL.

The next step is to add the ability for PeerConnection/WebRtcSession to create
"options" to represent multiple RtpTransceivers, and apply the Unified Plan
descriptions correctly. Right now, only Plan B descriptions will be
generated in unit tests.

BUG=chromium:465349

Review-Url: https://codereview.webrtc.org/2991693002
Cr-Commit-Position: refs/heads/master@{#19343}
Committed:
https://chromium.googlesource.com/external/webrtc/+/a77e6bbd30276bdc5b30f2cbc...
==========

to

==========
Relanding: Adding support for Unified Plan offer/answer negotiation to the
mediasession layer.

This layer takes in a simplified "options" struct and the current local
description,
and generates a new offer/answer. Previously the options struct assumed there
would
only be one media description per media type (audio/video), but it now supports
N number of audio/video descriptions.

The |add_legacy_stream| options is removed from the mediasession.cc/.h 
in this CL.

The next step is to add the ability for PeerConnection/WebRtcSession to create
"options" to represent multiple RtpTransceivers, and apply the Unified Plan
descriptions correctly. Right now, only Plan B descriptions will be
generated in unit tests.

BUG=chromium:465349

Review-Url: https://codereview.webrtc.org/2991693002
Cr-Original-Commit-Position: refs/heads/master@{#19343}
Committed:
https://chromium.googlesource.com/external/webrtc/+/a77e6bbd30276bdc5b30f2cbc...
Review-Url: https://codereview.webrtc.org/2991693002
Cr-Commit-Position: refs/heads/master@{#19394}
Committed:
https://chromium.googlesource.com/external/webrtc/+/1c378ed83b5b32a00368bbfc3...
==========

commit-bot: I haz the power

3 years, 4 months ago (2017-08-17 21:11:00 UTC) #87

Message was sent while issue was closed.

Committed patchset #10 (id:540001) as
https://chromium.googlesource.com/external/webrtc/+/1c378ed83b5b32a00368bbfc3...

Issue 2991693002: Adding support for Unified Plan offer/answer negotiation. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Clean up. #

Patch Set 3 : address review comments #

Patch Set 4 : More tests. #

Patch Set 5 : Review comments. #

Patch Set 6 : Responded the comments. #

Patch Set 7 : Fix the tests in Chrommium. #

Patch Set 8 : Fix the internal issue. #

Patch Set 9 : Minor fix. #

Patch Set 10 : Fix the chromium win-clang bulid by replacing int with Optional<size_t>. #

Messages