MediaRecorder: correct MIME type parsing

MediaRecorder: correct MIME type parsing

Currently the expected/supported MIME types are in the form of
'video/vp8', 'audio/opus' etc, this CL changes that to the
MIME conforming 'video/webm;codecs=vp8', 'video/webm;codecs=vp9',
'audio/webm;codecs=ops' following webm container guidelines [1]

BUG=564950, 262211, 571107
[1] http://www.webmproject.org/docs/container/

TBR=esprehn@chromium.org: Tiny modification in the method
signature in public/platform/WebMediaRecorderHandler.h, where
one argument is splitted into two.

Committed: https://crrev.com/0e327b0dd7ac9cd1bcf7df7177a6f3901f7a3461
Cr-Commit-Position: refs/heads/master@{#366232}

[mcasas, 2015-12-17 02:29:18]

Patchset #1 (id:1) has been deleted

[mcasas, 2015-12-17 18:42:07]

Patchset #1 (id:20001) has been deleted

[mcasas, 2015-12-17 18:42:33]

Patchset #1 (id:40001) has been deleted

[mcasas, 2015-12-17 22:17:46]

mcasas@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[mcasas, 2015-12-17 22:17:47]

dalecurtis@ PTAL
(or fwd appropriately, thx)

[mcasas, 2015-12-18 04:36:24]

mcasas@chromium.org changed reviewers:
+ tommi@chromium.org
- dalecurtis@chromium.org

[mcasas, 2015-12-18 04:36:24]

tommi@ PTAL
(Dale is ooo til Jan 5)

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

https://codereview.chromium.org/1534553003/diff/60001/content/renderer/media/...
File content/renderer/media/media_recorder_handler.cc (right):

https://codereview.chromium.org/1534553003/diff/60001/content/renderer/media/...
content/renderer/media/media_recorder_handler.cc:46: if (type.utf8().empty())
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;

https://codereview.chromium.org/1534553003/diff/60001/content/renderer/media/...
content/renderer/media/media_recorder_handler.cc:47: return true;
Is it right to say we can support an empty mime type?

https://codereview.chromium.org/1534553003/diff/60001/content/renderer/media/...
content/renderer/media/media_recorder_handler.cc:53: codecs.utf8().empty()) {
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?

https://codereview.chromium.org/1534553003/diff/60001/content/renderer/media/...
content/renderer/media/media_recorder_handler.cc:59: if
(base::EqualsCaseInsensitiveASCII(type.utf8(), "video/webm")) {
this check has already been done before

https://codereview.chromium.org/1534553003/diff/60001/content/renderer/media/...
content/renderer/media/media_recorder_handler.cc:69: if
(base::EqualsCaseInsensitiveASCII(type.utf8(), "audio/webm")) {
this check might have been done before

https://codereview.chromium.org/1534553003/diff/60001/content/renderer/media/...
content/renderer/media/media_recorder_handler.cc:76: return false;
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;
}

https://codereview.chromium.org/1534553003/diff/60001/content/renderer/media/...
content/renderer/media/media_recorder_handler.cc:94: use_vp9_ =
codecs.utf8().find("vp9") != std::string::npos;
this isn't case sensitive like the code in canSupportMimeType.  A check for
"VP9" could pass canSupportMimeType but do the wrong thing here.

https://codereview.chromium.org/1534553003/diff/60001/content/renderer/media/...
File content/renderer/media/media_recorder_handler_unittest.cc (right):

https://codereview.chromium.org/1534553003/diff/60001/content/renderer/media/...
content/renderer/media/media_recorder_handler_unittest.cc:54: static const
MediaRecorderTestParams kMediaRecorderTestParams[] = {
Can you document what this array contains? Does it contain supported values
only, video types only, meant for testing valid cases, invalid?

https://codereview.chromium.org/1534553003/diff/60001/content/renderer/media/...
content/renderer/media/media_recorder_handler_unittest.cc:140: const WebString
mime_type_video(base::UTF8ToUTF16("video/webm"));
suggest tests for mixed case mime types as well as testing for not-supported
input.

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

tommi@ PTAL

https://codereview.chromium.org/1534553003/diff/60001/content/renderer/media/...
File content/renderer/media/media_recorder_handler.cc (right):

https://codereview.chromium.org/1534553003/diff/60001/content/renderer/media/...
content/renderer/media/media_recorder_handler.cc:46: if (type.utf8().empty())
On 2015/12/18 10:06:30, tommi ooo until 7th wrote:
> 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;

Done.

https://codereview.chromium.org/1534553003/diff/60001/content/renderer/media/...
content/renderer/media/media_recorder_handler.cc:47: return true;
On 2015/12/18 10:06:29, tommi ooo until 7th wrote:
> Is it right to say we can support an empty mime type?

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).

https://codereview.chromium.org/1534553003/diff/60001/content/renderer/media/...
content/renderer/media/media_recorder_handler.cc:53: codecs.utf8().empty()) {
On 2015/12/18 10:06:29, tommi ooo until 7th wrote:
> codecs.isEmpty()
> Looks like this will then be the least expensive mandatory check, so it should
> be the first one.

Refactored.

> 
> 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?

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

https://codereview.chromium.org/1534553003/diff/60001/content/renderer/media/...
content/renderer/media/media_recorder_handler.cc:59: if
(base::EqualsCaseInsensitiveASCII(type.utf8(), "video/webm")) {
On 2015/12/18 10:06:30, tommi ooo until 7th wrote:
> this check has already been done before

Refactored.

https://codereview.chromium.org/1534553003/diff/60001/content/renderer/media/...
content/renderer/media/media_recorder_handler.cc:69: if
(base::EqualsCaseInsensitiveASCII(type.utf8(), "audio/webm")) {
On 2015/12/18 10:06:30, tommi ooo until 7th wrote:
> this check might have been done before

Refactored.

https://codereview.chromium.org/1534553003/diff/60001/content/renderer/media/...
content/renderer/media/media_recorder_handler.cc:76: return false;
On 2015/12/18 10:06:30, tommi ooo until 7th wrote:
> 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;
> }

Gosh, almost right first time!
Done.

https://codereview.chromium.org/1534553003/diff/60001/content/renderer/media/...
content/renderer/media/media_recorder_handler.cc:94: use_vp9_ =
codecs.utf8().find("vp9") != std::string::npos;
On 2015/12/18 10:06:29, tommi ooo until 7th wrote:
> this isn't case sensitive like the code in canSupportMimeType.  A check for
> "VP9" could pass canSupportMimeType but do the wrong thing here.

Done.

https://codereview.chromium.org/1534553003/diff/60001/content/renderer/media/...
File content/renderer/media/media_recorder_handler_unittest.cc (right):

https://codereview.chromium.org/1534553003/diff/60001/content/renderer/media/...
content/renderer/media/media_recorder_handler_unittest.cc:54: static const
MediaRecorderTestParams kMediaRecorderTestParams[] = {
On 2015/12/18 10:06:30, tommi ooo until 7th wrote:
> Can you document what this array contains? Does it contain supported values
> only, video types only, meant for testing valid cases, invalid?

Done.

https://codereview.chromium.org/1534553003/diff/60001/content/renderer/media/...
content/renderer/media/media_recorder_handler_unittest.cc:140: const WebString
mime_type_video(base::UTF8ToUTF16("video/webm"));
On 2015/12/18 10:06:30, tommi ooo until 7th wrote:
> suggest tests for mixed case mime types as well as testing for not-supported
> input.

Done.
(Note that there were a few negative cases 
EXPECT_FALSE already)

[tommi (sloooow) - chröme, 2015-12-18 21:38:30]

lgtm

[mcasas, 2015-12-18 22:02:08]

Description was changed from

==========
MediaRecorder: correct MIME type parsing

Currently the expected/supported MIME types are in the form of
'video/vp8', 'audio/opus' etc, this CL changes that to the
MIME conforming 'video/webm;codecs=vp8', 'video/webm;codecs=vp9',
'audio/webm;codecs=ops' following webm container guidelines [1]

BUG=564950, 262211
[1] http://www.webmproject.org/docs/container/
==========

to

==========
MediaRecorder: correct MIME type parsing

Currently the expected/supported MIME types are in the form of
'video/vp8', 'audio/opus' etc, this CL changes that to the
MIME conforming 'video/webm;codecs=vp8', 'video/webm;codecs=vp9',
'audio/webm;codecs=ops' following webm container guidelines [1]

BUG=564950, 262211, 571107
[1] http://www.webmproject.org/docs/container/
==========

[mcasas, 2015-12-18 22:14:12]

mcasas@chromium.org changed reviewers:
+ esprehn@chromium.org

[mcasas, 2015-12-18 22:14:12]

esprehn@ Tiny modification in method signature in
third_party/WebKit/public/platform/WebMediaRecorderHandler.h

[mcasas, 2015-12-19 00:35:58]

Description was changed from

==========
MediaRecorder: correct MIME type parsing

Currently the expected/supported MIME types are in the form of
'video/vp8', 'audio/opus' etc, this CL changes that to the
MIME conforming 'video/webm;codecs=vp8', 'video/webm;codecs=vp9',
'audio/webm;codecs=ops' following webm container guidelines [1]

BUG=564950, 262211, 571107
[1] http://www.webmproject.org/docs/container/
==========

to

==========
MediaRecorder: correct MIME type parsing

Currently the expected/supported MIME types are in the form of
'video/vp8', 'audio/opus' etc, this CL changes that to the
MIME conforming 'video/webm;codecs=vp8', 'video/webm;codecs=vp9',
'audio/webm;codecs=ops' following webm container guidelines [1]

BUG=564950, 262211, 571107
[1] http://www.webmproject.org/docs/container/

TBR=esprehn@chromium.org: Tiny modification in the method
signature in public/platform/WebMediaRecorderHandler.h, where
one argument is splitted into two.
==========

[mcasas, 2015-12-19 00:36:05]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2015-12-19 00:37:15]

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

[commit-bot: I haz the power, 2015-12-19 00:43:04]

Description was changed from

==========
MediaRecorder: correct MIME type parsing

Currently the expected/supported MIME types are in the form of
'video/vp8', 'audio/opus' etc, this CL changes that to the
MIME conforming 'video/webm;codecs=vp8', 'video/webm;codecs=vp9',
'audio/webm;codecs=ops' following webm container guidelines [1]

BUG=564950, 262211, 571107
[1] http://www.webmproject.org/docs/container/

TBR=esprehn@chromium.org: Tiny modification in the method
signature in public/platform/WebMediaRecorderHandler.h, where
one argument is splitted into two.
==========

to

==========
MediaRecorder: correct MIME type parsing

Currently the expected/supported MIME types are in the form of
'video/vp8', 'audio/opus' etc, this CL changes that to the
MIME conforming 'video/webm;codecs=vp8', 'video/webm;codecs=vp9',
'audio/webm;codecs=ops' following webm container guidelines [1]

BUG=564950, 262211, 571107
[1] http://www.webmproject.org/docs/container/

TBR=esprehn@chromium.org: Tiny modification in the method
signature in public/platform/WebMediaRecorderHandler.h, where
one argument is splitted into two.
==========

[commit-bot: I haz the power, 2015-12-19 00:43:05]

Committed patchset #2 (id:80001)

[commit-bot: I haz the power, 2015-12-19 00:43:53]

Description was changed from

==========
MediaRecorder: correct MIME type parsing

Currently the expected/supported MIME types are in the form of
'video/vp8', 'audio/opus' etc, this CL changes that to the
MIME conforming 'video/webm;codecs=vp8', 'video/webm;codecs=vp9',
'audio/webm;codecs=ops' following webm container guidelines [1]

BUG=564950, 262211, 571107
[1] http://www.webmproject.org/docs/container/

TBR=esprehn@chromium.org: Tiny modification in the method
signature in public/platform/WebMediaRecorderHandler.h, where
one argument is splitted into two.
==========

to

==========
MediaRecorder: correct MIME type parsing

Currently the expected/supported MIME types are in the form of
'video/vp8', 'audio/opus' etc, this CL changes that to the
MIME conforming 'video/webm;codecs=vp8', 'video/webm;codecs=vp9',
'audio/webm;codecs=ops' following webm container guidelines [1]

BUG=564950, 262211, 571107
[1] http://www.webmproject.org/docs/container/

TBR=esprehn@chromium.org: Tiny modification in the method
signature in public/platform/WebMediaRecorderHandler.h, where
one argument is splitted into two.

Committed: https://crrev.com/0e327b0dd7ac9cd1bcf7df7177a6f3901f7a3461
Cr-Commit-Position: refs/heads/master@{#366232}
==========

[commit-bot: I haz the power, 2015-12-19 00:43:54]

Patchset 2 (id:??) landed as
https://crrev.com/0e327b0dd7ac9cd1bcf7df7177a6f3901f7a3461
Cr-Commit-Position: refs/heads/master@{#366232}

[esprehn, 2015-12-19 00:47:48]

Please don't TBR changes like this, that's not what TBR is for.

lgtm

[philipj_slow, 2016-01-26 04:30:27]

philipj@opera.com changed reviewers:
+ philipj@opera.com

[philipj_slow, 2016-01-26 04:30:29]

https://codereview.chromium.org/1534553003/diff/60001/content/renderer/media/...
File content/renderer/media/media_recorder_handler.cc (right):

https://codereview.chromium.org/1534553003/diff/60001/content/renderer/media/...
content/renderer/media/media_recorder_handler.cc:47: return true;
On 2015/12/18 21:27:44, mcasas wrote:
> On 2015/12/18 10:06:29, tommi ooo until 7th wrote:
> > Is it right to say we can support an empty mime type?
> 
> 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).

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