Accept all the media profiles required by JSEP.

webrtc/pc/mediasession.cc

Index: webrtc/pc/mediasession.cc
diff --git a/webrtc/pc/mediasession.cc b/webrtc/pc/mediasession.cc
index 6b05736f0a3f3e74d662bb305e2458b7b7eba8ed..d0daff1ff9d77f9f95cecb758b3c67909a382488 100644
--- a/webrtc/pc/mediasession.cc
+++ b/webrtc/pc/mediasession.cc
@@ -1115,9 +1115,22 @@ static bool CreateMediaContentAnswer(
   return true;
 }
 
+static bool IsProtocolFound(const std::vector<std::string> protocols,

[tommi, 2016/04/14 12:59:09]

Maybe this is by accident - but please don't pass by value.  This creates a copy
of the vector every time the function is called.

[pthatcher1, 2016/04/14 19:31:08]

Good catch.

+                            const std::string& protocol) {
+  return std::find(protocols.begin(), protocols.end(), protocol) !=
+         protocols.end();
+}
+
 static bool IsMediaProtocolSupported(MediaType type,
                                      const std::string& protocol,
                                      bool secure_transport) {
+  // The accepted pattern for media protocol (JSEP Section 5.1.3)
+  const std::vector<std::string> kMediaProtocols = {"RTP/SAVPF", "RTP/SAVP",
+                                                    "RTP/AVPF", "RTP/AVP"};

[tommi, 2016/04/14 12:59:09]

I don't think this needs to be std::vector.  It contains static data which can
be searched without having to copy it into strings every time.

Also, as is, the variable name should not be kMediaProtocols.  kFooNaming is
used for compile-time constants, not runtime, so here using media_protocols
would be appropriate.  Even though this is const, we're still creating this
array every time the function is called.

I'd also not want to change this to be a static vector<> in case that's being
considered, for inefficiency reasons (and that statics are not thread safe on
all platforms).  static char*[] on the other hand, is fine.

Instead, I'd propose something along these lines:

---------
// Since not all applications serialize and deserialize the media protocol,
// we will have to accept |protocol| to be empty.
if (protocol.empty())
  return true;

if (type == MEDIA_TYPE_DATA &&
    (secure_transport ?
     protocol == kMediaProtocolDtlsSctp : protocol == kMediaProtocolSctp))) {
  return true;
}

(see other comment for concern around type == MEDIA_TYPE_DATA at this point)

static const char* kMediaProtocols[] = {...};
static const char* kMediaProtocolsDtls[] = {...};

(the following line may be my misunderstanding, so defer to peter)

return secure_transport ?
    IsProtocolFound(kMediaProtocolsDtls, arraysize(kMediaProtocolsDtls),
protocol) :
    IsProtocolFound(kMediaProtocols, arraysize(kMediaProtocols), protocol);
---------


I suspect that the check for MEDIA_TYPE_DATA ought to be something like this:

if (type == MEDIA_TYPE_DATA) {
  return secure_transport ?
     protocol == kMediaProtocolDtlsSctp : protocol == kMediaProtocolSctp);
}

[pthatcher1, 2016/04/14 19:31:08]

Your idea of using  static const char* is a good optimization.

But the logic/behavior isn't quite right, and I think we could make this even
more readable and correct and just as fast like so:

bool IsDtlsRtp(const std::string& protocol) {
  // Most-likely values first.
  return protocol == "UDP/TLS/RTP/SAVPF" || 
     protocol == "TCP/TLS/RTP/SAVPF" || 
     protocol == "UDP/TLS/RTP/SAVP"  || 
     protocol == "TCP/TLS/RTP/SAVP";
}

bool IsPlainRtp(const std::string& protocol) {
  // Most-likely values first.
  return protocol == "RTP/SAVPF" || 
     protocol == "RTP/AVPF" || 
     protocol == "RTP/SAVP"  || 
     protocol == "RTP/AVP";
}

bool IsDtlsSctp(const std::string& protocol) {
  return protocol == "DTLS/SCTP";
}

bool IsPlainSctp(const std::string& protocol) {
  return protocol == "SCTP";
}




if (protocol.empty()) {
  return true;
}

if (type == MEDIA_TYPE_DATA) {
  // Check for SCTP, but also for RTP for RTP-based data channels.
  // TODO(pthatcher): Remove RTP once RTP-based data channels are gone.
  if (secure_transport) {
    // Most likely scenarios first.
    return IsDtlsSctp(protocol) || IsDtlsRtp(protocol) || IsPlainRtp(protocol);
  } else {
    return IsPlainSctp(protocol) || IsPlainRtp(protocol);
  }
}

// Allow for non-DTLS RTP protocol even when using DTLS because that's what JSEP
specifies.
if (secure_transport) {
  // Most likely scenarios first.
  return IsDtlsRtp(protocol) || IsPlainRtp(protocol);
} else { 
  return IsPlainRtp(protocol);
}

+  const std::vector<std::string> kMediaProtocolsDtls = {
+      "UDP/TLS/RTP/SAVPF", "UDP/TLS/RTP/SAVP", "TCP/TLS/RTP/SAVPF",
+      "TCP/TLS/RTP/SAVP"};
+
   // Data channels can have a protocol of SCTP or SCTP/DTLS.
   if (type == MEDIA_TYPE_DATA &&
       ((protocol == kMediaProtocolSctp && !secure_transport)||

[tommi, 2016/04/14 12:59:09]

check secure_transport first here and on the next line.

@@ -1127,9 +1140,8 @@ static bool IsMediaProtocolSupported(MediaType type,
 
   // Since not all applications serialize and deserialize the media protocol,

[tommi, 2016/04/14 12:59:10]

If type is MEDIA_TYPE_DATA when we get here, should we be returning true if e.g.
protocol is "RTP/SAVPF"?  Does that make sense, or should the above check,
handle MEDIA_TYPE_DATA completely?

[Taylor Brandstetter, 2016/04/14 16:54:28]

This is for RTP data channels, which some downstream applications are still
using (even though not officially supported).

   // we will have to accept |protocol| to be empty.
-  return protocol == kMediaProtocolAvpf || protocol.empty() ||
-      protocol == kMediaProtocolSavpf ||
-      (protocol == kMediaProtocolDtlsSavpf && secure_transport);
+  return protocol.empty() || IsProtocolFound(kMediaProtocols, protocol) ||

[tommi, 2016/04/14 12:59:09]

the |protocol.empty()| check should be done at the top of the function.  As is,
it's done after a lot of unnecessary things have been done, including several
heap allocations.

+         (IsProtocolFound(kMediaProtocolsDtls, protocol) && secure_transport);

[tommi, 2016/04/14 12:59:10]

|secure_transport| should be checked before doing the array lookup.

Also, if secure_transport is true, should we be looking up in kMediaProtocols?

[Taylor Brandstetter, 2016/04/14 16:54:28]

Yes. JSEP says we should accept these protocols as long as there's a fingerprint
attribute.

 }
 
 static void SetMediaProtocol(bool secure_transport,