MediaRecorder: correct MIME type parsing

content/renderer/media/media_recorder_handler.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/media/media_recorder_handler.h"
 
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/strings/string_tokenizer.h"
 #include "base/strings/string_util.h"
 #include "content/renderer/media/audio_track_recorder.h"
 #include "content/renderer/media/media_stream_track.h"
 #include "content/renderer/media/video_track_recorder.h"
 #include "content/renderer/media/webrtc_uma_histograms.h"
 #include "media/audio/audio_parameters.h"
 #include "media/base/audio_bus.h"
 #include "media/base/bind_to_current_loop.h"
+#include "media/base/mime_util.h"
 #include "media/base/video_frame.h"
 #include "media/capture/webm_muxer.h"
 #include "third_party/WebKit/public/platform/WebMediaRecorderHandlerClient.h"
 #include "third_party/WebKit/public/platform/WebString.h"
 
 using base::TimeDelta;
 using base::TimeTicks;
 
 namespace content {
 
 MediaRecorderHandler::MediaRecorderHandler()
     : use_vp9_(false),
       recording_(false),
       client_(nullptr),
       weak_factory_(this) {}
 
 MediaRecorderHandler::~MediaRecorderHandler() {
   DCHECK(main_render_thread_checker_.CalledOnValidThread());
   // Send a |last_in_slice| to our |client_|.
   if (client_)
     client_->writeData(nullptr, 0u, true);
 }
 
 bool MediaRecorderHandler::canSupportMimeType(
-    const blink::WebString& mimeType) {
+    const blink::WebString& type, const blink::WebString& codecs) {
   DCHECK(main_render_thread_checker_.CalledOnValidThread());
-  // Ensure we can support each passed MIME type.
-  const std::string input = mimeType.utf8();  // Must outlive tokenizer!
-  base::StringTokenizer tokenizer(input, ",");
-  while (tokenizer.GetNext()) {
-    // Strip whitespace.
-    const std::string token(base::CollapseWhitespaceASCII(
-        tokenizer.token(), true /* trim sequences with line breaks*/));
-    if (!token.empty() &&
-        !base::EqualsCaseInsensitiveASCII(token, "video/vp8") &&
-        !base::EqualsCaseInsensitiveASCII(token, "video/vp9") &&
-        !base::EqualsCaseInsensitiveASCII(token, "audio/opus") &&
-        !base::EqualsCaseInsensitiveASCII(token, "video/webm") &&
-        !base::EqualsCaseInsensitiveASCII(token, "audio/webm")) {
-      return false;
-    }
+  if (type.utf8().empty())

[tommi (sloooow) - chröme, 2015/12/18 10:06:30]

utf8() creates a new std::string every time you call it.  In this function it
looks like we would have a lot of heap allocations just to check if a mime type
could be supported. Please fix the call sites below so that we only create at
most one std::string and none if we can use a WebString method instead.

Here, you can do:

if (type.isEmpty())
  return true;

[mcasas, 2015/12/18 21:27:44]

Done.

+    return true;

[tommi (sloooow) - chröme, 2015/12/18 10:06:29]

Is it right to say we can support an empty mime type?

[mcasas, 2015/12/18 21:27:44]

There is a bit of confusion in the spec about it,
since an empty one in isTypeSupported() gives a 
false and an empty one in MediaRecorder constructor
basically leaves the UA free to choose their
preferred codecs. I think it makes sense to allow
and empty one, and will fix the spec accordingly,
this is just a step ahead. (Note that Gecko
does not implement isTypeSupported(), and that is
fine with an empty mimeType in ctor params).

[philipj_slow, 2016/01/26 04:30:28]

See discussion in
https://github.com/w3c/mediacapture-record/issues/40#issuecomment-174484195

+  // Supported |type|s are {video,audio}/webm. Both support empty |codecs|,
+  // |type| == "video" supports vp8, vp9 or opus; "audio", supports only opus.
+  // http://www.webmproject.org/docs/container/, "HTML5 Video Type Parameters"
+  if ((base::EqualsCaseInsensitiveASCII(type.utf8(), "video/webm") ||
+       base::EqualsCaseInsensitiveASCII(type.utf8(), "audio/webm")) &&
+      codecs.utf8().empty()) {

[tommi (sloooow) - chröme, 2015/12/18 10:06:29]

codecs.isEmpty()
Looks like this will then be the least expensive mandatory check, so it should
be the first one.

I'm wondering if this is the check you want to be doing.  If codecs is empty and
type is neither audio nor video, should we really continue?

[mcasas, 2015/12/18 21:27:44]

Refactored.
No, but with your suggestion further below
  if (!video && !audio)
    return false;
meaning: not supported.

+    return true;
   }
 
-  return true;
+  std::vector<std::string> codecs_list;
+  media::ParseCodecString(codecs.utf8(), &codecs_list, true /* strip */);
+  if (base::EqualsCaseInsensitiveASCII(type.utf8(), "video/webm")) {

[tommi (sloooow) - chröme, 2015/12/18 10:06:30]

this check has already been done before

[mcasas, 2015/12/18 21:27:45]

Refactored.

+    for (const auto& codec : codecs_list) {
+      if (!base::EqualsCaseInsensitiveASCII(codec, "vp8") &&
+          !base::EqualsCaseInsensitiveASCII(codec, "vp9") &&
+          !base::EqualsCaseInsensitiveASCII(codec, "opus")) {
+        return false;
+      }
+    }
+    return true;
+  }
+  if (base::EqualsCaseInsensitiveASCII(type.utf8(), "audio/webm")) {

[tommi (sloooow) - chröme, 2015/12/18 10:06:30]

this check might have been done before

[mcasas, 2015/12/18 21:27:45]

Refactored.

+    for (const auto& codec : codecs_list) {
+      if (!base::EqualsCaseInsensitiveASCII(codec, "opus"))
+        return false;
+    }
+    return true;
+  }
+  return false;

[tommi (sloooow) - chröme, 2015/12/18 10:06:30]

Here's an alternate implementation proposal that has fewer call sites to
EqualsCaseInsensitiveASCII and fewer string copies.  Don't know if it compiles
though :)

bool MediaRecorderHandler::canSupportMimeType(
    const blink::WebString& mime_type, const blink::WebString& codecs) {
  if (mime_type.isEmpty())
    return true;  // not sure about this.

  const std::string type(mime_type.utf8());
  const bool video = base::EqualsCaseInsensitiveASCII(type, "video/webm");
  const bool audio = video ? false : base::EqualsCaseInsensitiveASCII(type,
"audio/webm");
  if (!video && !audio)
    return false;  // I assume we can't support a type that's neither of the
above.

  std::vector<std::string> codecs_list;
  media::ParseCodecString(codecs.utf8(), &codecs_list, true /* strip */);

  static const char* kVideoCodecs[] = { "vp8", "vp9", "opus" };
  static const char* kAudioCodecs[] = { "opus" };

  const char** codecs = video ? &kVideoCodecs[0] : &kAudioCodecs[0];
  int codecs_count =  video ? arraysize(kVideoCodecs) : arraysize(kAudioCodecs);
  for (const auto& codec : codecs_list) {
    const auto found = std::find_if(&codecs[0], &codecs[codecs_count],
        [&codec](const char* name) {
          return base::EqualsCaseInsensitiveASCII(codec, name);
        });
    if (found == &codecs[codecs_count])
      return false;
  }

  return true;
}

[mcasas, 2015/12/18 21:27:45]

Gosh, almost right first time!
Done.

 }
 
 bool MediaRecorderHandler::initialize(
     blink::WebMediaRecorderHandlerClient* client,
     const blink::WebMediaStream& media_stream,
-    const blink::WebString& mimeType) {
+    const blink::WebString& type,
+    const blink::WebString& codecs) {
   DCHECK(main_render_thread_checker_.CalledOnValidThread());
   // Save histogram data so we can see how much MediaStream Recorder is used.
   // The histogram counts the number of calls to the JS API.
   UpdateWebRTCMethodCount(WEBKIT_MEDIA_STREAM_RECORDER);
 
-  if (!canSupportMimeType(mimeType)) {
-    DLOG(ERROR) << "Can't support type " << mimeType.utf8();
+  if (!canSupportMimeType(type, codecs)) {
+    DLOG(ERROR) << "Can't support " << type.utf8()
+                << ";codecs=" << codecs.utf8();
     return false;
   }
-  use_vp9_ = mimeType.utf8().compare("video/vp9") == 0;
+  use_vp9_ = codecs.utf8().find("vp9") != std::string::npos;

[tommi (sloooow) - chröme, 2015/12/18 10:06:29]

this isn't case sensitive like the code in canSupportMimeType.  A check for
"VP9" could pass canSupportMimeType but do the wrong thing here.

[mcasas, 2015/12/18 21:27:45]

Done.

   media_stream_ = media_stream;
   DCHECK(client);
   client_ = client;
 
   return true;
 }
 
 bool MediaRecorderHandler::start() {
   DCHECK(main_render_thread_checker_.CalledOnValidThread());
   DCHECK(!recording_);
@@ skipping 150 matching lines @@
     recorder->OnData(audio_bus, timestamp);
 }
 
 void MediaRecorderHandler::SetAudioFormatForTesting(
     const media::AudioParameters& params) {
   for (auto* recorder : audio_recorders_)
     recorder->OnSetFormat(params);
 }
 
 }  // namespace content