Media Capabilities encoding: Blink pass-thru and skeleton renderer/ impl

Media Capabilities encoding: Blink pass-thru and skeleton renderer/ impl

This CL implements MediaCapabilities.encodingInfo() method,
wiring it for the 'record' type and connecting it to MediaRecorder
canRecordMimeType(). A LayoutTest is also added.

Note that there's no distinction about what codec might be
hardware-accelerated or not yet.


Follows https://github.com/WICG/media-capabilities/pull/36.

DD: http://tinyurl.com/media-capabilities-encoding 


BUG=709181
Review-Url: https://codereview.chromium.org/2811103006
Cr-Commit-Position: refs/heads/master@{#466191}
Committed: https://chromium.googlesource.com/chromium/src/+/f7352cd198a784420184136e78cc14f85ff69c3a

[mcasas, 2017-04-12 02:57:09]

Patchset #1 (id:1) has been deleted

[mcasas, 2017-04-12 02:57:17]

Patchset #1 (id:20001) has been deleted

[mcasas, 2017-04-12 02:57:25]

Patchset #1 (id:40001) has been deleted

[mcasas, 2017-04-12 02:57:33]

Patchset #1 (id:60001) has been deleted

[mcasas, 2017-04-12 18:48:41]

Patchset #1 (id:80001) has been deleted

[mcasas, 2017-04-13 23:01:43]

Patchset #1 (id:100001) has been deleted

[mcasas, 2017-04-13 23:01:49]

Patchset #1 (id:120001) has been deleted

[mcasas, 2017-04-13 23:05:57]

Description was changed from

==========
Media Capabilities encoding: Blink pass-thru and skeleton renderer/ impl



first stab at encodingInfo() impl

BUG=709181
==========

to

==========
Media Capabilities encoding: Blink pass-thru and skeleton renderer/ impl

This CL implements MediaCapabilities.encodingInfo() method,
wiring it for the 'record' type and connecting it to MediaRecorder
canRecordMimeType(). A LayoutTest is also added.

Note that there's no distinction about what codec might be
hardware-accelerated or not yet.


Follows https://github.com/WICG/media-capabilities/pull/36.

BUG=709181
==========

[mcasas, 2017-04-13 23:15:42]

Patchset #1 (id:140001) has been deleted

[mcasas, 2017-04-13 23:15:51]

Patchset #1 (id:160001) has been deleted

[mcasas, 2017-04-14 00:22:20]

Patchset #1 (id:180001) has been deleted

[mcasas, 2017-04-14 01:42:13]

mcasas@chromium.org changed reviewers:
+ emircan@chromium.org, mlamouri@chromium.org

[mcasas, 2017-04-14 01:42:14]

mlamouri@ assuming the Spec PR looks good, PTAL

emircan@ RS/FYI/PTAL

[mcasas, 2017-04-14 16:15:07]

The CQ bit was checked by mcasas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-14 16:15:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-14 17:07:19]

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

[commit-bot: I haz the power, 2017-04-14 17:07:19]

Dry run: This issue passed the CQ dry run.

[emircan, 2017-04-14 18:10:38]

looks good. I will wait for mlamouri@s review for RS.

https://codereview.chromium.org/2811103006/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/media_capabilities/encodingInfo.html
(right):

https://codereview.chromium.org/2811103006/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media_capabilities/encodingInfo.html:9: var
createTestForContentType = function(type, isSupported = true) {
Can you use some other param name? "type" is used in two contexts here.

[mlamouri (slow - plz ping), 2017-04-18 12:24:35]

One high level comment: what's the benefit of going trough MediaRecorderHandler
instead of re-using the current plumbing in media/blink/ then re-direct the
calls to media/gpu/ or wherever it would make sense?

https://codereview.chromium.org/2811103006/diff/200001/content/renderer/media...
File content/renderer/media_recorder/media_recorder_handler.cc (right):

https://codereview.chromium.org/2811103006/diff/200001/content/renderer/media...
content/renderer/media_recorder/media_recorder_handler.cc:289: return;
That should not happen.

https://codereview.chromium.org/2811103006/diff/200001/content/renderer/media...
content/renderer/media_recorder/media_recorder_handler.cc:298: content_type =
configuration.audio_configuration->content_type.Ascii();
what if there is audio and video?

https://codereview.chromium.org/2811103006/diff/200001/content/renderer/media...
content/renderer/media_recorder/media_recorder_handler.cc:317: }
I think most of this is going to be moved to Blink.

[mcasas, 2017-04-18 21:41:51]

mlamouri@ PTAL



Inline comment:
> One high level comment: what's the benefit of 
> going trough MediaRecorderHandler instead of 
> re-using the current plumbing in media/blink/ 
> then re-direct the calls to media/gpu/ or 
> wherever it would make sense?

Only content/renderer/.../media_recorder_handler.cc
knows which mime/content types are supported for
encoding, so going to media/blink wouldn't help.

Another question is which of those types are likely
accelerated, which media_recorder_handler.cc will
delegate to video_track_recorder.cc (in subsequent
patch(es)).

https://codereview.chromium.org/2811103006/diff/200001/content/renderer/media...
File content/renderer/media_recorder/media_recorder_handler.cc (right):

https://codereview.chromium.org/2811103006/diff/200001/content/renderer/media...
content/renderer/media_recorder/media_recorder_handler.cc:289: return;
On 2017/04/18 12:24:35, mlamouri wrote:
> That should not happen.

The current code allows for this to happen. What
do you mean "should" ?

Note that :

const configuration = { type : 'record'};

navigator.mediaCapabilities.encodingInfo(configuration)
    .then((result) => {
       console.log('yay!');
    })    
    .catch(() => {
       console.error('boo'); 
    });

is perfectly legal idl-wise. In this case, we want to
reject the promise; what we might want to do is to
reject it in MediaCapabilities.cpp.  

I'm going to assume that you meant the latter, and
I'll DCHECK here and reject in MediaCapabilities.cpp.

https://codereview.chromium.org/2811103006/diff/200001/content/renderer/media...
content/renderer/media_recorder/media_recorder_handler.cc:298: content_type =
configuration.audio_configuration->content_type.Ascii();
On 2017/04/18 12:24:35, mlamouri wrote:
> what if there is audio and video?

TODO :-)

https://codereview.chromium.org/2811103006/diff/200001/content/renderer/media...
content/renderer/media_recorder/media_recorder_handler.cc:317: }
On 2017/04/18 12:24:35, mlamouri wrote:
> I think most of this is going to be moved to Blink.

That'd be great but for that we'd need to split 
Web{Video,Audio}Configuration.type [1] into two
strings: |type| and |parameter| that could be fed
from a ContentType [2] like we do, e.g. in [3].
but those were too many changes for this CL and
the existing code, so decided to split the mime
type here.

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/modul...
[2]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/netwo...
[3]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/mediar...

https://codereview.chromium.org/2811103006/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/media_capabilities/encodingInfo.html
(right):

https://codereview.chromium.org/2811103006/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media_capabilities/encodingInfo.html:9: var
createTestForContentType = function(type, isSupported = true) {
On 2017/04/14 18:10:38, emircan wrote:
> Can you use some other param name? "type" is used in two contexts here.

Done.

[mcasas, 2017-04-18 21:43:22]

Description was changed from

==========
Media Capabilities encoding: Blink pass-thru and skeleton renderer/ impl

This CL implements MediaCapabilities.encodingInfo() method,
wiring it for the 'record' type and connecting it to MediaRecorder
canRecordMimeType(). A LayoutTest is also added.

Note that there's no distinction about what codec might be
hardware-accelerated or not yet.


Follows https://github.com/WICG/media-capabilities/pull/36.

BUG=709181
==========

to

==========
Media Capabilities encoding: Blink pass-thru and skeleton renderer/ impl

This CL implements MediaCapabilities.encodingInfo() method,
wiring it for the 'record' type and connecting it to MediaRecorder
canRecordMimeType(). A LayoutTest is also added.

Note that there's no distinction about what codec might be
hardware-accelerated or not yet.


Follows https://github.com/WICG/media-capabilities/pull/36.

DD: http://tinyurl.com/media-capabilities-encoding 


BUG=709181
==========

[mlamouri (slow - plz ping), 2017-04-20 13:37:13]

lgtm

https://codereview.chromium.org/2811103006/diff/200001/content/renderer/media...
File content/renderer/media_recorder/media_recorder_handler.cc (right):

https://codereview.chromium.org/2811103006/diff/200001/content/renderer/media...
content/renderer/media_recorder/media_recorder_handler.cc:317: }
On 2017/04/18 at 21:41:51, mcasas wrote:
> On 2017/04/18 12:24:35, mlamouri wrote:
> > I think most of this is going to be moved to Blink.
> 
> That'd be great but for that we'd need to split 
> Web{Video,Audio}Configuration.type [1] into two
> strings: |type| and |parameter| that could be fed
> from a ContentType [2] like we do, e.g. in [3].
> but those were too many changes for this CL and
> the existing code, so decided to split the mime
> type here.
> 
> [1]
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/modul...
> [2]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/netwo...
> [3]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/mediar...

chcunningham@ has a CL for that. Worse case I guess we can refactor anyway.

https://codereview.chromium.org/2811103006/diff/220001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/media_capabilities/encodingInfo.html
(right):

https://codereview.chromium.org/2811103006/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media_capabilities/encodingInfo.html:63: //
Rejected MIME types
Just to check: all browsers should fail those, right? (it's obvious for some but
not sure for mpeg4,ogg and webm-vorbis)

[mcasas, 2017-04-20 18:13:48]

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

[mcasas, 2017-04-20 18:13:49]

haraken@ : RS plz the added method in
 third_party/WebKit/public/platform/WebMediaRecorderHandler.h

https://codereview.chromium.org/2811103006/diff/220001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/media_capabilities/encodingInfo.html
(right):

https://codereview.chromium.org/2811103006/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media_capabilities/encodingInfo.html:63: //
Rejected MIME types
On 2017/04/20 13:37:13, mlamouri wrote:
> Just to check: all browsers should fail those, right? (it's obvious for some
but
> not sure for mpeg4,ogg and webm-vorbis)

We talked about this offline: some of the types listed here
are specific to Chrome and some others are potentially 
available to other (all?) implementations.  This file is in 
LayoutTests/, so it's Chrome-specific. I'll move to external/
the tests that apply generally in a follow up CL.
(It's not urgent since this API is behind a flag anyway).

[mcasas, 2017-04-20 18:13:57]

The CQ bit was checked by mcasas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 18:14:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[haraken, 2017-04-20 19:55:19]

https://codereview.chromium.org/2811103006/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/media_capabilities/MediaCapabilities.cpp
(right):

https://codereview.chromium.org/2811103006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_capabilities/MediaCapabilities.cpp:117:
"Either |video| or |audio| needed."));

Add a more developer-friendly message.

https://codereview.chromium.org/2811103006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_capabilities/MediaCapabilities.cpp:125:
DOMException::Create(kInvalidStateError, "Platform error"));

Ditto.

https://codereview.chromium.org/2811103006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_capabilities/MediaCapabilities.cpp:129:
handler->EncodingInfo(

Why do you use a different pattern between decodingInfo and encodingInfo?
decodingInfo is using
Platform::Current()->MediaCapabilitiesClient()->DecodingInfo. If we follow that
pattern, we won't need to add a public API.

[commit-bot: I haz the power, 2017-04-20 20:42:04]

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

[commit-bot: I haz the power, 2017-04-20 20:42:04]

Dry run: This issue passed the CQ dry run.

[mcasas, 2017-04-20 21:12:02]

ptal

https://codereview.chromium.org/2811103006/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/media_capabilities/MediaCapabilities.cpp
(right):

https://codereview.chromium.org/2811103006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_capabilities/MediaCapabilities.cpp:117:
"Either |video| or |audio| needed."));
On 2017/04/20 19:55:18, haraken wrote:
> 
> Add a more developer-friendly message.

Done.

https://codereview.chromium.org/2811103006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_capabilities/MediaCapabilities.cpp:125:
DOMException::Create(kInvalidStateError, "Platform error"));
On 2017/04/20 19:55:18, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/2811103006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_capabilities/MediaCapabilities.cpp:129:
handler->EncodingInfo(
On 2017/04/20 19:55:18, haraken wrote:
> 
> Why do you use a different pattern between decodingInfo and encodingInfo?
> decodingInfo is using
> Platform::Current()->MediaCapabilitiesClient()->DecodingInfo. If we follow
that
> pattern, we won't need to add a public API.
> 

Decoding and Encoding need to go to different places to
retrieve the results of the query: decoding goes to 
media/ whereas encoding has to go to 
content/renderer/media_recorder.

[haraken, 2017-04-20 21:29:10]

On 2017/04/20 21:12:02, mcasas wrote:
> ptal
> 
>
https://codereview.chromium.org/2811103006/diff/220001/third_party/WebKit/Sou...
> File
third_party/WebKit/Source/modules/media_capabilities/MediaCapabilities.cpp
> (right):
> 
>
https://codereview.chromium.org/2811103006/diff/220001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/modules/media_capabilities/MediaCapabilities.cpp:117:
> "Either |video| or |audio| needed."));
> On 2017/04/20 19:55:18, haraken wrote:
> > 
> > Add a more developer-friendly message.
> 
> Done.
> 
>
https://codereview.chromium.org/2811103006/diff/220001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/modules/media_capabilities/MediaCapabilities.cpp:125:
> DOMException::Create(kInvalidStateError, "Platform error"));
> On 2017/04/20 19:55:18, haraken wrote:
> > 
> > Ditto.
> 
> Done.
> 
>
https://codereview.chromium.org/2811103006/diff/220001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/modules/media_capabilities/MediaCapabilities.cpp:129:
> handler->EncodingInfo(
> On 2017/04/20 19:55:18, haraken wrote:
> > 
> > Why do you use a different pattern between decodingInfo and encodingInfo?
> > decodingInfo is using
> > Platform::Current()->MediaCapabilitiesClient()->DecodingInfo. If we follow
> that
> > pattern, we won't need to add a public API.
> > 
> 
> Decoding and Encoding need to go to different places to
> retrieve the results of the query: decoding goes to 
> media/ whereas encoding has to go to 
> content/renderer/media_recorder.

Makes sense. LGTM.

[mcasas, 2017-04-20 21:33:12]

The CQ bit was checked by mcasas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 21:33:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-20 23:03:38]

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

[commit-bot: I haz the power, 2017-04-20 23:03:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mcasas, 2017-04-20 23:05:40]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2017-04-20 23:05:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2811103006/#ps240001
(title: "haraken@ comments")

[commit-bot: I haz the power, 2017-04-20 23:05:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-20 23:51:21]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1492729540459200,
"parent_rev": "6c681f28e11ecf8dd62e3e15cef752c5affc67a8", "commit_rev":
"f7352cd198a784420184136e78cc14f85ff69c3a"}

[commit-bot: I haz the power, 2017-04-20 23:51:31]

Description was changed from

==========
Media Capabilities encoding: Blink pass-thru and skeleton renderer/ impl

This CL implements MediaCapabilities.encodingInfo() method,
wiring it for the 'record' type and connecting it to MediaRecorder
canRecordMimeType(). A LayoutTest is also added.

Note that there's no distinction about what codec might be
hardware-accelerated or not yet.


Follows https://github.com/WICG/media-capabilities/pull/36.

DD: http://tinyurl.com/media-capabilities-encoding 


BUG=709181
==========

to

==========
Media Capabilities encoding: Blink pass-thru and skeleton renderer/ impl

This CL implements MediaCapabilities.encodingInfo() method,
wiring it for the 'record' type and connecting it to MediaRecorder
canRecordMimeType(). A LayoutTest is also added.

Note that there's no distinction about what codec might be
hardware-accelerated or not yet.


Follows https://github.com/WICG/media-capabilities/pull/36.

DD: http://tinyurl.com/media-capabilities-encoding 


BUG=709181

Review-Url: https://codereview.chromium.org/2811103006
Cr-Commit-Position: refs/heads/master@{#466191}
Committed:
https://chromium.googlesource.com/chromium/src/+/f7352cd198a784420184136e78cc...
==========

[commit-bot: I haz the power, 2017-04-20 23:51:32]

Committed patchset #3 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/f7352cd198a784420184136e78cc...