Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1956343002:
Initial asymmetric codec support in MediaSessionDescription (Closed)
DescriptionInitial asymmetric codec support in MediaSessionDescription
Added initial support for MediaSessionDescriptionFactory to pick different codecs based on communications direction (sendrecv, sendonly, recvonly, inactive) specifically for audio.
This adds some more degradation options for the answer: depending on answer options, it's now possible to degrade to INACTIVE from any offer, as well as to either RECVONLY or SENDONLY from a SENDRECV offer.
The set of "codecs" used for testing the answer was compiled using this spreadsheet:
https://docs.google.com/a/google.com/spreadsheets/d/1nVIfZLsFo5YK10_e80BCAADZnnRQ1devwwwAGmqJPow/edit?usp=sharing
I should probably condense it into a smaller table and put in the source.
BUG=webrtc:5805
Committed: https://crrev.com/075af927309cea5420d06798c389c27cc6b34e61
Cr-Commit-Position: refs/heads/master@{#13126}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Added unit tests #
Total comments: 1
Patch Set 3 : Parameterized tests #Patch Set 4 : More intense testing! #
Total comments: 1
Patch Set 5 : Check stream type to decide on whether or not to receive. #
Total comments: 15
Patch Set 6 : Added RtpTranscieverDirection; removed old set_audio_codecs. #
Total comments: 2
Patch Set 7 : Added a comment describing how the codec sets were chosen. #
Total comments: 4
Patch Set 8 : Added { } #
Total comments: 2
Patch Set 9 : Fix MediaContetDirectionToString warning. #
Total comments: 13
Patch Set 10 : Fixed codec choice during negotiation. #
Total comments: 3
Patch Set 11 : Added TODO. #
Dependent Patchsets: Messages
Total messages: 44 (14 generated)
https://codereview.webrtc.org/1956343002/diff/1/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/1956343002/diff/1/webrtc/pc/mediasession.cc#new... webrtc/pc/mediasession.cc:1270: for (const auto& sc : audio_send_codecs_) { This clearly needs testing and should probably be placed somewhere else.
Description was changed from ========== WIP: Initial assymetric codec support in MediaSessionDescription Added initial support for MediaSessionDescriptionFactory to pick different codecs based on communications direction (sendrecv, sendonly, recvonly, inactive) specifically for audio. There are some TODOs and the added functionality needs new unit tests. BUG=webrtc:5801 ========== to ========== WIP: Initial asymmetric codec support in MediaSessionDescription Added initial support for MediaSessionDescriptionFactory to pick different codecs based on communications direction (sendrecv, sendonly, recvonly, inactive) specifically for audio. There are some TODOs and the added functionality needs new unit tests. BUG=webrtc:5801 ==========
ossu@webrtc.org changed reviewers: + kwiberg@webrtc.org
Please have a look at this. I've a second step on its way, that plumbs all the way down to WebRtcVoiceEngine. Not sure if I should put that up separately, or as another patch set to this. Thoughts?
https://codereview.webrtc.org/1956343002/diff/1/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/1956343002/diff/1/webrtc/pc/mediasession.cc#new... webrtc/pc/mediasession.cc:1293: const AudioCodecs *supported_audio_codecs = nullptr; You set this in all branches below, so the default value is unnecessary.
On 2016/05/16 12:44:01, ossu wrote: > Please have a look at this. I've a second step on its way, that plumbs all the > way down to WebRtcVoiceEngine. Not sure if I should put that up separately, or > as another patch set to this. Thoughts? Looks good, provided you add tests. I think a separate CL for the rest would be best.
Description was changed from ========== WIP: Initial asymmetric codec support in MediaSessionDescription Added initial support for MediaSessionDescriptionFactory to pick different codecs based on communications direction (sendrecv, sendonly, recvonly, inactive) specifically for audio. There are some TODOs and the added functionality needs new unit tests. BUG=webrtc:5801 ========== to ========== Initial asymmetric codec support in MediaSessionDescription Added initial support for MediaSessionDescriptionFactory to pick different codecs based on communications direction (sendrecv, sendonly, recvonly, inactive) specifically for audio. BUG=webrtc:5805 ==========
Added tests for the codec merging separately, as well as for the sets of codecs presented when creating offers and answers. https://codereview.webrtc.org/1956343002/diff/1/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/1956343002/diff/1/webrtc/pc/mediasession.cc#new... webrtc/pc/mediasession.cc:1293: const AudioCodecs *supported_audio_codecs = nullptr; On 2016/05/17 12:22:36, kwiberg-webrtc wrote: > You set this in all branches below, so the default value is unnecessary. Acknowledged. https://codereview.webrtc.org/1956343002/diff/20001/webrtc/pc/mediasession_un... File webrtc/pc/mediasession_unittest.cc (right): https://codereview.webrtc.org/1956343002/diff/20001/webrtc/pc/mediasession_un... webrtc/pc/mediasession_unittest.cc:2672: TEST_F(MediaSessionDescriptionFactoryTest, TestAudioSendOnlyCodecsAnswer) { I grouped them by the expected resulting direction in the answer. It's a bit messy, though, and makes it hard to find where things go wrong, if they do. I pondering separating them into one test per call, or turning it into a parameterized test.
Parameterized the offer/answer tests, to see what it'd look like. The testing output isn't as good (since we only get values for parameters and not names), but it's easier to maintain if, for some reason, other combinations appear.
Parameterized the offer/answer tests, to see what it'd look like. The testing output isn't as good (since we only get values for parameters and not names), but it's easier to maintain if, for some reason, other combinations appear.
Patchset #4 (id:60001) has been deleted
I believe the offer/answer tests need four different sets of codecs to test the system properly. I'm working on that now - don't review this one too intensely yet. :)
Description was changed from ========== Initial asymmetric codec support in MediaSessionDescription Added initial support for MediaSessionDescriptionFactory to pick different codecs based on communications direction (sendrecv, sendonly, recvonly, inactive) specifically for audio. BUG=webrtc:5805 ========== to ========== Initial asymmetric codec support in MediaSessionDescription Added initial support for MediaSessionDescriptionFactory to pick different codecs based on communications direction (sendrecv, sendonly, recvonly, inactive) specifically for audio. This adds some more degradation options for the answer: depending on answer options, it's now possible to degrade to INACTIVE from any offer, as well as to either RECVONLY or SENDONLY from a SENDRECV offer. The set of "codecs" used for testing the answer was compiled using this spreadsheet: https://docs.google.com/a/google.com/spreadsheets/d/1nVIfZLsFo5YK10_e80BCAADZ... I should probably condense it into a smaller table and put in the source. BUG=webrtc:5805 ==========
ossu@webrtc.org changed reviewers: + deadbeef@webrtc.org
Improved testing of the answer by using more specialized sets of codecs. I've added a link in the description showing how I decided on these sets. I'll probably remove it before committing, since it's only internally viewable. Please have a look to see I didn't make a mistake in the lists.
https://codereview.webrtc.org/1956343002/diff/80001/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/1956343002/diff/80001/webrtc/pc/mediasession.cc... webrtc/pc/mediasession.cc:1041: answer->set_direction(options.recv_audio ? MD_RECVONLY : MD_INACTIVE); This is clearly wrong: it should only check recv_audio if doing audio; this method is templated to deal with all kinds of streams. Will update.
Fixed the check for whether to receive or not: it now checks different flags depending on media type. https://codereview.webrtc.org/1956343002/diff/100001/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/1956343002/diff/100001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1037: (answer->type() == cricket::MEDIA_TYPE_AUDIO ? options.recv_audio : true); I expect that since there's no flag for data, we'll always want to receive if asked. This should equal to how it used to work before checking the recv_* flags.
ossu@webrtc.org changed reviewers: + pthatcher@webrtc.org - kwiberg@webrtc.org
Removed Karl as reviewer and added pthatcher, being an owner.
Taylor, Can you check the logic here and the unit tests? https://codereview.webrtc.org/1956343002/diff/100001/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/1956343002/diff/100001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1034: !IsRtpProtocol(answer->protocol()) || !answer->streams().empty(); This would be more clear as: const bool is_data = !IsRtpProtocol(answer->protocol()); const bool has_send_streams = !answer->streams().empty(); const bool wants_send = has_send_streams || is_data; const bool recv_video = (answer->type() == cricket::MEDIA_TYPE_VIDEO && options.recv_video); const bool recv_audio = (answer->type() == cricket::MEDIA_TYPE_AUDIO && options.recv_audio); const bool wants_recv = recv_audio || recv_video || is_data; https://codereview.webrtc.org/1956343002/diff/100001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1053: break; I think this would be more clear if we made an RtpTransceiverDirection struct (RtpTransceiverDirection is the enum recently introduced into the standard): struct RtpTransceiverDirection { bool send; bool recv; RtpTransceiverDirection(bool send, bool recv) : send(send), recv(recv) {} bool operator==(const RtpTransceiverDirection& o) const { return send == o.send && recv == o.recv; } bool operator!=(const RtpTransceiverDirection& o) const { return !(*this == 0); } static RtpTransceiverDirection FromMediaContentDirection(MediaContentDirection md) { bool send = (md == MD_SENDRECV || md == MD_SENDONLY); bool recv = (md == MD_SENDRECV || md == MD_RECVONLY); return RtpTransceiverDirection(send, recv); } MediaContentDescription ToMediaContentDescription() { if (send && recv) { return MD_SENDRECV; } if (send) { return MD_SENDONLY; } if (recv) { return MD_RECVONLY; } return MD_INACTIVE; } } RtpTransceiverDirection NegotiateRtpTransceiverDirection(RtpTransceiverDirection offer, RtpTranscevierDirection wants) { return RtpTransceiverDirection(offer.send && wants_to.send, offer.recv && wants_to.recv); } The, finally, the code in question could just be: auto offer_rtd = RtpTransceiverDirection::FromMediaContentDirection(offer->direction()); auto wants_rtd = RtpTransceiverDirection(wants_send, wants_recv); answer->set_direction(NegotiateRtpTranscevierDirection(offer_rtd, wants_rtd).ToMediaContentDirection()); Eventually, we could even replace MediaContentDescription with it (hopefully we can make a global RTD_SENDRECV = RtpTransceiverDirection(true, true) without a static initializer). https://codereview.webrtc.org/1956343002/diff/100001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1264: const AudioCodecs& MediaSessionDescriptionFactory::audio_codecs() const { This could be renamed to audio_sendrecv_codecs(). https://codereview.webrtc.org/1956343002/diff/100001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1339: } Would it make sense to make a method like this? const AudioCodecs& GetAudioCodecsForDirection(MediaContentDirection md) { switch (md) { case MD_SENDONLY: return audio_send_codecs; case MD_RECVONLY: return audio_recv_codecs; } return audio_sendrecv_codecs; } Then this code could just be: bool send_audio = options.HasSendMediaStream(MEDIA_TYPE_AUDIO) || add_legacy_; bool recv_audio = options.recv_audio; const AudioCodecs& supported_audio_codecs = GetAudioCodecsForDirection( RtpTransceiverDirection(send_audio, recv_audio).ToMediaContentDescription())); https://codereview.webrtc.org/1956343002/diff/100001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1832: const AudioContentDescription* audio_content_description = A better name would be offer_audio. https://codereview.webrtc.org/1956343002/diff/100001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1873: } This could be more simple also: auto offer_rtd = RtpTransceiverDirection::FromMediaContentDirection(offer_audio->direction()); bool wants_send = options.HasSendMediaStream(MEDIA_TYPE_AUDIO) || add_legacy_; bool wants_recv = options.recv_audio; auto wants_rtd = RtpTransceiverDirection(wants_send, wants_recv); auto answer_rtd = NegotiateRtpTransceiverDirection(offer_rtd, wants_rtd); answer_audio_codecs = GetAudioCodecs(answer_rtd.ToMediaContentDescription())); https://codereview.webrtc.org/1956343002/diff/100001/webrtc/pc/mediasession.h File webrtc/pc/mediasession.h (right): https://codereview.webrtc.org/1956343002/diff/100001/webrtc/pc/mediasession.h... webrtc/pc/mediasession.h:400: const AudioCodecs& audio_codecs() const; I'd prefer audio_sendrecv_codecs() https://codereview.webrtc.org/1956343002/diff/100001/webrtc/pc/mediasession.h... webrtc/pc/mediasession.h:405: const AudioCodecs& recv_codecs); This should probably both be SetAudioCodecs. Also, why do we need the first version? Can we just get rid of it? It would be much more clear to always provide send and recv and let this class do the merging rather than get in a mix of letting the caller do the merge or letting the class do the merge.
Patchset #6 (id:120001) has been deleted
Addressed the issues in your comments. Most of RtpTranscieverDirection is taken directly from them, with some small adjustments to the logic. https://codereview.webrtc.org/1956343002/diff/100001/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/1956343002/diff/100001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1034: !IsRtpProtocol(answer->protocol()) || !answer->streams().empty(); On 2016/05/31 21:52:36, pthatcher1 wrote: > This would be more clear as: > > const bool is_data = !IsRtpProtocol(answer->protocol()); > const bool has_send_streams = !answer->streams().empty(); > const bool wants_send = has_send_streams || is_data; > > const bool recv_video = (answer->type() == cricket::MEDIA_TYPE_VIDEO && > options.recv_video); > const bool recv_audio = (answer->type() == cricket::MEDIA_TYPE_AUDIO && > options.recv_audio); > const bool wants_recv = recv_audio || recv_video || is_data; Acknowledged. https://codereview.webrtc.org/1956343002/diff/100001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1053: break; On 2016/05/31 21:52:36, pthatcher1 wrote: > I think this would be more clear if we made an RtpTransceiverDirection struct > (RtpTransceiverDirection is the enum recently introduced into the standard): I've put an RtpTranscieverDirection in and changed to using it. I also updated the offer side of audio to use it, rather than replicating the logic there. https://codereview.webrtc.org/1956343002/diff/100001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1339: } On 2016/05/31 21:52:36, pthatcher1 wrote: > Would it make sense to make a method like this? > > const AudioCodecs& GetAudioCodecsForDirection(MediaContentDirection md) { > switch (md) { > case MD_SENDONLY: > return audio_send_codecs; > case MD_RECVONLY: > return audio_recv_codecs; > } > return audio_sendrecv_codecs; > } > > Then this code could just be: > > bool send_audio = options.HasSendMediaStream(MEDIA_TYPE_AUDIO) || add_legacy_; > bool recv_audio = options.recv_audio; > const AudioCodecs& supported_audio_codecs = GetAudioCodecsForDirection( > RtpTransceiverDirection(send_audio, recv_audio).ToMediaContentDescription())); Why not have it take an RtpTranscieverDirection directly? https://codereview.webrtc.org/1956343002/diff/100001/webrtc/pc/mediasession.h File webrtc/pc/mediasession.h (right): https://codereview.webrtc.org/1956343002/diff/100001/webrtc/pc/mediasession.h... webrtc/pc/mediasession.h:400: const AudioCodecs& audio_codecs() const; On 2016/05/31 21:52:36, pthatcher1 wrote: > I'd prefer audio_sendrecv_codecs() Sure! I've addressed it in the second CL. https://codereview.webrtc.org/1956343002/diff/100001/webrtc/pc/mediasession.h... webrtc/pc/mediasession.h:405: const AudioCodecs& recv_codecs); On 2016/05/31 21:52:36, pthatcher1 wrote: > This should probably both be SetAudioCodecs. > > Also, why do we need the first version? Can we just get rid of it? It would be > much more clear to always provide send and recv and let this class do the > merging rather than get in a mix of letting the caller do the merge or letting > the class do the merge. I've kept it while we're redoing the rest of the code to provide two lists and separate factories for encode and decode. However, looking more closely these setters are only called by unit tests. I'll take a look at changing them to use the two-parameter version. https://codereview.webrtc.org/1956343002/diff/140001/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/1956343002/diff/140001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:92: return RtpTransceiverDirection(offer.recv && wants.send, offer and wants mirror each other here: if the offer is to send, we must want to receive, etc.
lgtm, assuming you add the {}s :).
https://codereview.webrtc.org/1956343002/diff/100001/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (right):
https://codereview.webrtc.org/1956343002/diff/100001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:1339: }
On 2016/06/02 13:28:02, ossu wrote:
> On 2016/05/31 21:52:36, pthatcher1 wrote:
> > Would it make sense to make a method like this?
> >
> > const AudioCodecs& GetAudioCodecsForDirection(MediaContentDirection md) {
> > switch (md) {
> > case MD_SENDONLY:
> > return audio_send_codecs;
> > case MD_RECVONLY:
> > return audio_recv_codecs;
> > }
> > return audio_sendrecv_codecs;
> > }
> >
> > Then this code could just be:
> >
> > bool send_audio = options.HasSendMediaStream(MEDIA_TYPE_AUDIO) ||
add_legacy_;
> > bool recv_audio = options.recv_audio;
> > const AudioCodecs& supported_audio_codecs = GetAudioCodecsForDirection(
> > RtpTransceiverDirection(send_audio,
> recv_audio).ToMediaContentDescription()));
>
> Why not have it take an RtpTranscieverDirection directly?
Sounds like a great idea.
https://codereview.webrtc.org/1956343002/diff/140001/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (right):
https://codereview.webrtc.org/1956343002/diff/140001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:92: return RtpTransceiverDirection(offer.recv &&
wants.send,
On 2016/06/02 13:28:02, ossu wrote:
> offer and wants mirror each other here: if the offer is to send, we must want
to
> receive, etc.
Ah, yes. You are correct. I forgot about that.
https://codereview.webrtc.org/1956343002/diff/160001/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (right):
https://codereview.webrtc.org/1956343002/diff/160001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:84: return MD_RECVONLY;
{}s please
https://codereview.webrtc.org/1956343002/diff/160001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:1508: return audio_recv_codecs_;
{}s please
https://codereview.webrtc.org/1956343002/diff/160001/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/1956343002/diff/160001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:84: return MD_RECVONLY; On 2016/06/02 22:29:17, pthatcher1 wrote: > {}s please Oh, alright! https://codereview.webrtc.org/1956343002/diff/160001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1508: return audio_recv_codecs_; On 2016/06/02 22:29:17, pthatcher1 wrote: > {}s please Acknowledged.
Not going to submit this right now (Friday afternoon and all). Did you have a look over the logic in the unit tests, Taylor? Did it look alright? :)
A few small observations, but otherwise this looks very good. https://codereview.webrtc.org/1956343002/diff/200001/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/1956343002/diff/200001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1320: sc.feedback_params == rc.feedback_params) { Can this comparison be replaced by a comparison method on AudioCodec? https://codereview.webrtc.org/1956343002/diff/200001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1512: return audio_sendrecv_codecs_; The rule in rfc 3264 for "inactive" is a bit more complex; it takes into account both the offer and answer direction. I think this function should take the offer and answer direction and do something like the following: RtpTransceiverDirection effective_direction = answer_direction; if (!effective_direction.send && !effective_direction.recv) { effective_direction.send = offer_direction.recv; effective_direction.recv = offer_direction.send; } https://codereview.webrtc.org/1956343002/diff/200001/webrtc/pc/mediasession_u... File webrtc/pc/mediasession_unittest.cc (right): https://codereview.webrtc.org/1956343002/diff/200001/webrtc/pc/mediasession_u... webrtc/pc/mediasession_unittest.cc:2570: * codec|send recv sr | send recv sr | s/r s/sr r/s r/sr sr/s sr/r sr/sr I'm confused about s/sr and r/sr. How can the answer upgrade from sendonly or recvonly to sendrecv? How are these tests passing?
https://codereview.webrtc.org/1956343002/diff/200001/webrtc/pc/mediasession_u... File webrtc/pc/mediasession_unittest.cc (right): https://codereview.webrtc.org/1956343002/diff/200001/webrtc/pc/mediasession_u... webrtc/pc/mediasession_unittest.cc:2654: static_cast<const AudioContentDescription*>(ac->description); Another small afterthought: It may be valuable to test that acd->direction() and the equivalent direction in the offer are what you expect. Otherwise this test may end up not covering one code path if some changes are made in the future, and we wouldn't realize.
https://codereview.webrtc.org/1956343002/diff/180001/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/1956343002/diff/180001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1848: AudioCodecs audio_codecs = GetAudioCodecsForDirection(answer_rtd); There's some code duplication between this and CreateMediaContentAnswer. Could this be eliminated by determining the answer direction in CreateAnswer and passing it down through the other methods?
https://codereview.webrtc.org/1956343002/diff/180001/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/1956343002/diff/180001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1848: AudioCodecs audio_codecs = GetAudioCodecsForDirection(answer_rtd); Probably. I'll look into it! https://codereview.webrtc.org/1956343002/diff/200001/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/1956343002/diff/200001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1320: sc.feedback_params == rc.feedback_params) { On 2016/06/03 17:13:20, Taylor Brandstetter wrote: > Can this comparison be replaced by a comparison method on AudioCodec? There are currently two ways to compare AudioCodecs: operator== and AudioCodec.Matches(). The former does not do case insensitive matching of the names, and does require the payload types (ids) be the same. Matches, on the other hand, compares payload types if they are one of the static payload types, or names (case-insensitively) if they are not. It also allows zeroes in several of the parameters to match anything. I don't think I could modify any of them to work like this code, without breaking something, but I'll take a look at it. Just adding a third one doesn't seem right. It's already unclear enough why there are two, IMO. https://codereview.webrtc.org/1956343002/diff/200001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1512: return audio_sendrecv_codecs_; On 2016/06/03 17:13:20, Taylor Brandstetter wrote: > The rule in rfc 3264 for "inactive" is a bit more complex; it takes into account > both the offer and answer direction. I think this function should take the offer > and answer direction and do something like the following: > > RtpTransceiverDirection effective_direction = answer_direction; > if (!effective_direction.send && !effective_direction.recv) { > effective_direction.send = offer_direction.recv; > effective_direction.recv = offer_direction.send; > } Ah, that's correct! I missed that when reading the RFC, but I see that now. I will revise. https://codereview.webrtc.org/1956343002/diff/200001/webrtc/pc/mediasession_u... File webrtc/pc/mediasession_unittest.cc (right): https://codereview.webrtc.org/1956343002/diff/200001/webrtc/pc/mediasession_u... webrtc/pc/mediasession_unittest.cc:2570: * codec|send recv sr | send recv sr | s/r s/sr r/s r/sr sr/s sr/r sr/sr On 2016/06/03 17:13:20, Taylor Brandstetter wrote: > I'm confused about s/sr and r/sr. How can the answer upgrade from sendonly or > recvonly to sendrecv? How are these tests passing? I should flesh out this description: s/sr means the offer was a sendonly stream, and the answerer was configured with recv_audio and with a stream to send, which would degrade to a receive on the answerer side, but with only the codecs available for both sending and receiving. However, thinking about it now, this seems like the wrong conclusion - if we end up with only sending (for example), we should include all formats that we're able to send, provided they're also in the offer. Right? https://codereview.webrtc.org/1956343002/diff/200001/webrtc/pc/mediasession_u... webrtc/pc/mediasession_unittest.cc:2654: static_cast<const AudioContentDescription*>(ac->description); On 2016/06/03 17:18:58, Taylor Brandstetter wrote: > Another small afterthought: It may be valuable to test that acd->direction() and > the equivalent direction in the offer are what you expect. Otherwise this test > may end up not covering one code path if some changes are made in the future, > and we wouldn't realize. I intended this test to only cover that the correct codecs are chosen, based on the direction that other parts of the codec decide to pick. I rely on preexisting tests to make sure that the right direction is picked in the answer, given a certain offer and a certain answerer configuration.
https://codereview.webrtc.org/1956343002/diff/220001/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/1956343002/diff/220001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:795: negotiated.name = theirs.name; Essentially I added this because it broke my SetAudioCodecs test. However, I think it makes sense in general: if the offer spells the codec one way, and we're able to understand that, why not use the same spelling as in the offer, rather than correct it. It seems rude! :) (and could possibly confuse the other side, if it's implemented poorly). https://codereview.webrtc.org/1956343002/diff/220001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1309: NegotiateCodecs(recv_codecs, send_codecs, &audio_sendrecv_codecs_); I repurposed NegotiateCodecs to merge these codecs lists as well. The operation is exactly the same, as far as I can see. https://codereview.webrtc.org/1956343002/diff/220001/webrtc/pc/mediasession_u... File webrtc/pc/mediasession_unittest.cc (right): https://codereview.webrtc.org/1956343002/diff/220001/webrtc/pc/mediasession_u... webrtc/pc/mediasession_unittest.cc:2574: * codec|send recv sr | send recv sr | s/r r/s sr/s sr/r sr/sr Cleaned out special handling of the answerer wanting sendrecv. The list of codecs could possibly be reduced by two (rows 0 and 5 are identical in the result, as are 2 and 4) but I've kept it as is for now. I'm happy to do so, though.
https://codereview.webrtc.org/1956343002/diff/200001/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/1956343002/diff/200001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1320: sc.feedback_params == rc.feedback_params) { On 2016/06/10 09:10:04, ossu wrote: > On 2016/06/03 17:13:20, Taylor Brandstetter wrote: > > Can this comparison be replaced by a comparison method on AudioCodec? > > There are currently two ways to compare AudioCodecs: operator== and > AudioCodec.Matches(). The former does not do case insensitive matching of the > names, and does require the payload types (ids) be the same. > Matches, on the other hand, compares payload types if they are one of the static > payload types, or names (case-insensitively) if they are not. It also allows > zeroes in several of the parameters to match anything. > I don't think I could modify any of them to work like this code, without > breaking something, but I'll take a look at it. Just adding a third one doesn't > seem right. It's already unclear enough why there are two, IMO. Understood. My only worry is that an additional attribute may be added to AudioCodec and this comparison would break. Though I guess that's unlikely; new things will probably just go in `params`. https://codereview.webrtc.org/1956343002/diff/200001/webrtc/pc/mediasession_u... File webrtc/pc/mediasession_unittest.cc (right): https://codereview.webrtc.org/1956343002/diff/200001/webrtc/pc/mediasession_u... webrtc/pc/mediasession_unittest.cc:2570: * codec|send recv sr | send recv sr | s/r s/sr r/s r/sr sr/s sr/r sr/sr On 2016/06/10 09:10:04, ossu wrote: > On 2016/06/03 17:13:20, Taylor Brandstetter wrote: > > I'm confused about s/sr and r/sr. How can the answer upgrade from sendonly or > > recvonly to sendrecv? How are these tests passing? > > I should flesh out this description: s/sr means the offer was a sendonly stream, > and the answerer was configured with recv_audio and with a stream to send, which > would degrade to a receive on the answerer side, but with only the codecs > available for both sending and receiving. However, thinking about it now, this > seems like the wrong conclusion - if we end up with only sending (for example), > we should include all formats that we're able to send, provided they're also in > the offer. Right? Yes, I think that's right. Similarly, suppose the offerer offers "recvonly". Even if the answerer is configured to receive audio (which is the default), since it ends up having nothing to receive, there's no reason to restrict the list of codecs to the "sendrecv" ones. https://codereview.webrtc.org/1956343002/diff/200001/webrtc/pc/mediasession_u... webrtc/pc/mediasession_unittest.cc:2654: static_cast<const AudioContentDescription*>(ac->description); On 2016/06/10 09:10:04, ossu wrote: > On 2016/06/03 17:18:58, Taylor Brandstetter wrote: > > Another small afterthought: It may be valuable to test that acd->direction() > and > > the equivalent direction in the offer are what you expect. Otherwise this test > > may end up not covering one code path if some changes are made in the future, > > and we wouldn't realize. > > I intended this test to only cover that the correct codecs are chosen, based on > the direction that other parts of the codec decide to pick. I rely on > preexisting tests to make sure that the right direction is picked in the answer, > given a certain offer and a certain answerer configuration. Acknowledged. I don't think we *have* full coverage in the existing tests for choosing a direction, though. There are only four tests I see: sr/sr, s/r, r/s, and i/i. Could you leave a TODO for me to add tests for the missing combinations?
I've added the TODO for deadbeef. Are you satisfied with the rest of the changes? https://codereview.webrtc.org/1956343002/diff/200001/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/1956343002/diff/200001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1320: sc.feedback_params == rc.feedback_params) { On 2016/06/10 17:39:13, Taylor Brandstetter wrote: > On 2016/06/10 09:10:04, ossu wrote: > > On 2016/06/03 17:13:20, Taylor Brandstetter wrote: > > > Can this comparison be replaced by a comparison method on AudioCodec? > > > > There are currently two ways to compare AudioCodecs: operator== and > > AudioCodec.Matches(). The former does not do case insensitive matching of the > > names, and does require the payload types (ids) be the same. > > Matches, on the other hand, compares payload types if they are one of the > static > > payload types, or names (case-insensitively) if they are not. It also allows > > zeroes in several of the parameters to match anything. > > I don't think I could modify any of them to work like this code, without > > breaking something, but I'll take a look at it. Just adding a third one > doesn't > > seem right. It's already unclear enough why there are two, IMO. > > Understood. My only worry is that an additional attribute may be added to > AudioCodec and this comparison would break. Though I guess that's unlikely; new > things will probably just go in `params`. Ah, yes. Well, I guess it's a moot point now that I've changed to using NegotiateCodecs instead. https://codereview.webrtc.org/1956343002/diff/200001/webrtc/pc/mediasession_u... File webrtc/pc/mediasession_unittest.cc (right): https://codereview.webrtc.org/1956343002/diff/200001/webrtc/pc/mediasession_u... webrtc/pc/mediasession_unittest.cc:2654: static_cast<const AudioContentDescription*>(ac->description); On 2016/06/10 17:39:13, Taylor Brandstetter wrote: > On 2016/06/10 09:10:04, ossu wrote: > > On 2016/06/03 17:18:58, Taylor Brandstetter wrote: > > > Another small afterthought: It may be valuable to test that acd->direction() > > and > > > the equivalent direction in the offer are what you expect. Otherwise this > test > > > may end up not covering one code path if some changes are made in the > future, > > > and we wouldn't realize. > > > > I intended this test to only cover that the correct codecs are chosen, based > on > > the direction that other parts of the codec decide to pick. I rely on > > preexisting tests to make sure that the right direction is picked in the > answer, > > given a certain offer and a certain answerer configuration. > > Acknowledged. I don't think we *have* full coverage in the existing tests for > choosing a direction, though. There are only four tests I see: sr/sr, s/r, r/s, > and i/i. Could you leave a TODO for me to add tests for the missing > combinations? Acknowledged.
lgtm, thanks!
The CQ bit was checked by ossu@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1956343002/#ps240001 (title: "Added TODO.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1956343002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/14144)
The CQ bit was checked by ossu@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1956343002/240001
Message was sent while issue was closed.
Description was changed from ========== Initial asymmetric codec support in MediaSessionDescription Added initial support for MediaSessionDescriptionFactory to pick different codecs based on communications direction (sendrecv, sendonly, recvonly, inactive) specifically for audio. This adds some more degradation options for the answer: depending on answer options, it's now possible to degrade to INACTIVE from any offer, as well as to either RECVONLY or SENDONLY from a SENDRECV offer. The set of "codecs" used for testing the answer was compiled using this spreadsheet: https://docs.google.com/a/google.com/spreadsheets/d/1nVIfZLsFo5YK10_e80BCAADZ... I should probably condense it into a smaller table and put in the source. BUG=webrtc:5805 ========== to ========== Initial asymmetric codec support in MediaSessionDescription Added initial support for MediaSessionDescriptionFactory to pick different codecs based on communications direction (sendrecv, sendonly, recvonly, inactive) specifically for audio. This adds some more degradation options for the answer: depending on answer options, it's now possible to degrade to INACTIVE from any offer, as well as to either RECVONLY or SENDONLY from a SENDRECV offer. The set of "codecs" used for testing the answer was compiled using this spreadsheet: https://docs.google.com/a/google.com/spreadsheets/d/1nVIfZLsFo5YK10_e80BCAADZ... I should probably condense it into a smaller table and put in the source. BUG=webrtc:5805 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Initial asymmetric codec support in MediaSessionDescription Added initial support for MediaSessionDescriptionFactory to pick different codecs based on communications direction (sendrecv, sendonly, recvonly, inactive) specifically for audio. This adds some more degradation options for the answer: depending on answer options, it's now possible to degrade to INACTIVE from any offer, as well as to either RECVONLY or SENDONLY from a SENDRECV offer. The set of "codecs" used for testing the answer was compiled using this spreadsheet: https://docs.google.com/a/google.com/spreadsheets/d/1nVIfZLsFo5YK10_e80BCAADZ... I should probably condense it into a smaller table and put in the source. BUG=webrtc:5805 ========== to ========== Initial asymmetric codec support in MediaSessionDescription Added initial support for MediaSessionDescriptionFactory to pick different codecs based on communications direction (sendrecv, sendonly, recvonly, inactive) specifically for audio. This adds some more degradation options for the answer: depending on answer options, it's now possible to degrade to INACTIVE from any offer, as well as to either RECVONLY or SENDONLY from a SENDRECV offer. The set of "codecs" used for testing the answer was compiled using this spreadsheet: https://docs.google.com/a/google.com/spreadsheets/d/1nVIfZLsFo5YK10_e80BCAADZ... I should probably condense it into a smaller table and put in the source. BUG=webrtc:5805 Committed: https://crrev.com/075af927309cea5420d06798c389c27cc6b34e61 Cr-Commit-Position: refs/heads/master@{#13126} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/075af927309cea5420d06798c389c27cc6b34e61 Cr-Commit-Position: refs/heads/master@{#13126} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
