Fixing some issues with the direction attribute of m-lines in offers.

Fixing some issues with the direction attribute of m-lines in offers.

By default, we'll now offer to receive if already receiving
(meaning that the last remote description contained a track).

Also, m-lines that are neither receiving nor sending are now correctly
marked "inactive".

Also moved some logic relating to default tracks out of webrtcsdp.cc,
such that now the direction seen by upper layers will always be
consistent with the consumed/produced SDP.

BUG=528089
Committed: https://crrev.com/c80741f8957b537e968397ac54ff5b5df8a2c318
Cr-Commit-Position: refs/heads/master@{#10376}

[Taylor Brandstetter, 2015-10-14 23:24:22]

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

[Taylor Brandstetter, 2015-10-14 23:24:22]

https://codereview.webrtc.org/1406803004/diff/1/talk/app/webrtc/webrtcsdp.cc
File talk/app/webrtc/webrtcsdp.cc (left):

https://codereview.webrtc.org/1406803004/diff/1/talk/app/webrtc/webrtcsdp.cc#...
talk/app/webrtc/webrtcsdp.cc:2250: }
This logic is replaced by the "msid_supported" check in peerconnection.cc.
Basically, what we want is to create default streams for peers who say "I'm
sending" and don't have any streams, but ONLY IF they don't support MSID.

[pthatcher1, 2015-10-15 06:02:03]

https://codereview.webrtc.org/1406803004/diff/20001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1406803004/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1052: desc->streams().empty() &&
!remote_desc->msid_supported();
Can you put this one first?  It seems like the most important?  In fact, can you
jut reverse the whole order?  It seems backwards from level of importance.

https://codereview.webrtc.org/1406803004/diff/20001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection.h (right):

https://codereview.webrtc.org/1406803004/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.h:63: bool currently_receiving_video);
A few thoughts:

1.  The out pointer should still go last.
2.  This method should now be called something like CreateOfferOptions.  
3.  The I think instead of passing in currently_receiving_X, we should just pass
in X_recv_streams_, since that's what we're logically doing: create an offer
from existing streams.
4.  Should send streams and recv streams go together?  For example, if send
streams are already populated in session_options before calling this, should
recv streams also?  Or would it make sense to move populating send streams into
this function?

https://codereview.webrtc.org/1406803004/diff/20001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/1406803004/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectioninterface_unittest.cc:648:
MediaConstraintsInterface* constraints = nullptr) {
Usually our style is to not have optional args, but instead to just change all
the call sites.  Is that doable here?

[Taylor Brandstetter, 2015-10-15 18:23:36]

https://codereview.webrtc.org/1406803004/diff/20001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1406803004/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1052: desc->streams().empty() &&
!remote_desc->msid_supported();
On 2015/10/15 06:02:03, pthatcher1 wrote:
> Can you put this one first?  It seems like the most important?  In fact, can
you
> jut reverse the whole order?  It seems backwards from level of importance.

Done.

https://codereview.webrtc.org/1406803004/diff/20001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection.h (right):

https://codereview.webrtc.org/1406803004/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.h:63: bool currently_receiving_video);
On 2015/10/15 06:02:03, pthatcher1 wrote:
> A few thoughts:
> 
> 1.  The out pointer should still go last.
> 2.  This method should now be called something like CreateOfferOptions.  
> 3.  The I think instead of passing in currently_receiving_X, we should just
pass
> in X_recv_streams_, since that's what we're logically doing: create an offer
> from existing streams.
> 4.  Should send streams and recv streams go together?  For example, if send
> streams are already populated in session_options before calling this, should
> recv streams also?  Or would it make sense to move populating send streams
into
> this function?

Actually, I decided on a different approach that keeps this function simpler.
Really its intent WAS just "convert RTC options", mainly for the purpose of
existing unit tests. Hopefully, we can eventually (when talk and webrtc are
merged) just pass RTC options all the way down and this method will be
unnecessary.

https://codereview.webrtc.org/1406803004/diff/20001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/1406803004/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectioninterface_unittest.cc:648:
MediaConstraintsInterface* constraints = nullptr) {
On 2015/10/15 06:02:03, pthatcher1 wrote:
> Usually our style is to not have optional args, but instead to just change all
> the call sites.  Is that doable here?

Done.

[pthatcher1, 2015-10-20 18:22:58]

lgtm

[Taylor Brandstetter, 2015-10-20 20:11:54]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2015-10-20 20:12:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1406803004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1406803004/80001

[commit-bot: I haz the power, 2015-10-20 22:12:20]

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

[commit-bot: I haz the power, 2015-10-20 22:12:21]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[Taylor Brandstetter, 2015-10-22 20:10:46]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2015-10-22 20:10:46]

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/1406803004/#ps100001 (title:
"Fixing a comment.")

[commit-bot: I haz the power, 2015-10-22 20:10:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1406803004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1406803004/100001

[commit-bot: I haz the power, 2015-10-22 20:14:49]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-10-22 20:15:03]

Patchset 6 (id:??) landed as
https://crrev.com/c80741f8957b537e968397ac54ff5b5df8a2c318
Cr-Commit-Position: refs/heads/master@{#10376}

[pthatcher, 2015-11-23 18:00:52]

pthatcher@google.com changed reviewers:
+ pthatcher@google.com

[pthatcher, 2015-11-23 18:00:52]

https://codereview.webrtc.org/1406803004/diff/100001/talk/session/media/media...
File talk/session/media/mediasession.cc (right):

https://codereview.webrtc.org/1406803004/diff/100001/talk/session/media/media...
talk/session/media/mediasession.cc:1044: : MD_SENDRECV);
It looks like this breaks data channels.  If we're answering data channels, we
should probably always go to sendrecv/sendonly.  

This was reported for C++ data channels (native/mobile apps), but we should see
if this affects Chrome as well.  If it is, we'll need to merge a fix ASAP. 

https://bugs.chromium.org/p/webrtc/issues/detail?id=5228#c5