Accept all the media profiles required by JSEP.

webrtc/pc/mediasession.cc

 /*
  *  Copyright 2004 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 1097 matching lines @@
                                 : MD_SENDRECV);
       break;
     default:
       RTC_DCHECK(false && "MediaContentDescription has unexpected direction.");
       break;
   }
 
   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.

        (protocol == kMediaProtocolDtlsSctp && secure_transport))) {
     return true;
   }
 
   // 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,
                              MediaContentDescription* desc) {
   if (!desc->cryptos().empty())
     desc->set_protocol(kMediaProtocolSavpf);
   else if (secure_transport)
     desc->set_protocol(kMediaProtocolDtlsSavpf);
   else
     desc->set_protocol(kMediaProtocolAvpf);
@@ skipping 861 matching lines @@
       GetFirstMediaContentDescription(sdesc, MEDIA_TYPE_VIDEO));
 }
 
 const DataContentDescription* GetFirstDataContentDescription(
     const SessionDescription* sdesc) {
   return static_cast<const DataContentDescription*>(
       GetFirstMediaContentDescription(sdesc, MEDIA_TYPE_DATA));
 }
 
 }  // namespace cricket