1331763 - AudioContext#createMediaElementSource does not support EME content

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P2)

Description

[Andrew Petersen]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170110004021

Steps to reproduce:

- Take an EME-enabled audio tag (mp4+aac, any CDM like widevine)
- call `AudioContext#createMediaElementSource(audio)`


Actual results:

Firefox throws a "NotSupportedError: Operation is not supported" exception. No further message details.


Expected results:

In Chrome, createMediaElementSource would return a MediaElementSourceAudioNode instance. I can see that the intended behavior in mozilla-central (https://hg.mozilla.org/mozilla-central/file/9c1c90ad617b/dom/media/webaudio/MediaElementAudioSourceNode.cpp#l28) is to throw, but it would be really nice to allow these APIs to be compatible. The spec (https://www.w3.org/TR/encrypted-media/#media-element-restictions) leaves this up to browser vendors, why doesn't Firefox support EME SourceNodes? Are there any prior discussions about why this choice was made?

Comment 1

[Paul Adenot (:padenot)]

Chris, I think that's by design, right ? This would allow capturing the audio output of an `HTMLMediaElement`.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

This was indeed by design, and we made this decision when the only DRM scheme we supported was Adobe's. Given that we're now using the same DRM scheme as Chrome, and that DRM scheme is owned by Google, it doesn't make sense to be more restrictive than Chrome.

So we should revert this behaviour by undoing that part of bug 1071482 that restricts audio capture and remove test_eme_stream_capture_blocked_case*.html.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Blake: Is there someone on your team who can look into this? It basically involves a partial revert of the changes made in bug 1071482, so it should be pretty easy.

Comment 4

[Blake Wu [:bwu][:blakewu]]

(In reply to Chris Pearce (:cpearce) from comment #3)
> Blake: Is there someone on your team who can look into this? It basically
> involves a partial revert of the changes made in bug 1071482, so it should
> be pretty easy.
Yeah. It looks like easy. :-)
Chunmin,
Would you like to try?

Comment 5

[C.M.Chang[:chunmin] | PTO]

OK

Comment 6

[C.M.Chang[:chunmin] | PTO]

Attachment: [WIP] Return a MediaElementSourceAudioNode instance from AudioContext::CreateMediaElementSource with EME-enabled media eleme

Comment 7

[C.M.Chang[:chunmin] | PTO]

Attachment: [WIP] Draft for test

Comment 8

[C.M.Chang[:chunmin] | PTO]

I am not familiar with these code, so I need more information to continue.

(In reply to [PTO until 26 April] Chris Pearce (:cpearce) from comment #2)
> So we should revert this behaviour by undoing that part of bug 1071482 that
> restricts audio capture 
By testing with attachment 8859088 [details] [diff] [review], In current patch(attachment 8859087 [details] [diff] [review]), AudioContext::CreateMediaElementSource is allowed to get the restricted content. For other cases(e.g. mozCaptureStream), the restricted content is still blocked.

> and remove test_eme_stream_capture_blocked_case*.html.

From Chrome's document[0], mediaElement.captureStream() will throw an exception if mediaElement is protected by EME:
> Attempting to use captureStream() with a media element that implements content protection via Encrypted Media Extensions will throw an exception.

Does it mean that our test_eme_stream_capture_blocked_case3.html, verifying an exception will be threw from mozCaptureStreamUntilEnded, should keep serving? I thinks it's a pending issue[1], so there is no spec. Do we need to keep compatible with Chrome?

Currently, we have three test cases:
1. setting MediaKeys on an element captured by MediaElementSource should fail, and
2. creating a MediaElementSource on a media element with a MediaKeys should fail, and
3. capturing a media element with mozCaptureStream that has a MediaKeys should fail.

With the expected modification, what are the expected results of the above tests?


Hi Paul,
I was wondering if you could give me some hints. Thanks!

[0] https://developers.google.com/web/updates/2016/10/capture-stream#the_small_print
[1] https://github.com/w3c/mediacapture-fromelement/issues/20

Comment 9

[Paul Adenot (:padenot)]

(In reply to Chun-Min Chang[:chunmin] from comment #8)
> I am not familiar with these code, so I need more information to continue.
> 
> (In reply to [PTO until 26 April] Chris Pearce (:cpearce) from comment #2)
> > So we should revert this behaviour by undoing that part of bug 1071482 that
> > restricts audio capture 
> By testing with attachment 8859088 [details] [diff] [review], In current
> patch(attachment 8859087 [details] [diff] [review]),
> AudioContext::CreateMediaElementSource is allowed to get the restricted
> content. For other cases(e.g. mozCaptureStream), the restricted content is
> still blocked.
> 
> > and remove test_eme_stream_capture_blocked_case*.html.
> 
> From Chrome's document[0], mediaElement.captureStream() will throw an
> exception if mediaElement is protected by EME:
> > Attempting to use captureStream() with a media element that implements content protection via Encrypted Media Extensions will throw an exception.
> 
> Does it mean that our test_eme_stream_capture_blocked_case3.html, verifying
> an exception will be threw from mozCaptureStreamUntilEnded, should keep
> serving? I thinks it's a pending issue[1], so there is no spec. Do we need
> to keep compatible with Chrome?

Our implementation of https://w3c.github.io/mediacapture-fromelement/ is not very spec compliant anyways.

> Currently, we have three test cases:
> 1. setting MediaKeys on an element captured by MediaElementSource should
> fail, and
> 2. creating a MediaElementSource on a media element with a MediaKeys should
> fail, and
> 3. capturing a media element with mozCaptureStream that has a MediaKeys
> should fail.
> 
> With the expected modification, what are the expected results of the above
> tests?

I don't really know. I **think** capturing an audio-only `HTMLMediaElement` that is using EME is fine. Capturing only the audio of an `HTMLMediaElement` that is using EME and has both an audio and video track is fine. Capturing only the video or the video and the audio of an `HTMLMediaElement` that is using EME should fail (by throwing an exception).

There seem to be a disagreement, that is not expressed in the spec, about Google's implementation of things, here, we should have a chat.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Capturing audio from an audio-only HTMLMediaElement using EME is fine. Capturing only audio from a HTMLMediaElement using EME that has audio and video is fine too. Google told me that this is Chrome's behaviour, but I have not tested it. Capturing video from a HTMLMediaElement is not fine; we should continue to block video being captured from an HTMLMediaElement using EME, as Chrome does.

Chumin: does that make sense?

Comment 11

[C.M.Chang[:chunmin] | PTO]

(In reply to Chris Pearce (:cpearce) from comment #10)
> Chumin: does that make sense?
Yes. I'll revise patches for these spec.

Comment 12

[C.M.Chang[:chunmin] | PTO]

Attachment: Bug 1331763 - part1: Support EME content for AudioContext::CreateMediaElementSource;

Review commit: https://reviewboard.mozilla.org/r/137002/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/137002/

Comment 13

[C.M.Chang[:chunmin] | PTO]

Attachment: Bug 1331763 - part2: Test cases;

Review commit: https://reviewboard.mozilla.org/r/137004/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/137004/

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8865351 [details]
Bug 1331763 - part1: Support EME content for AudioContext::CreateMediaElementSource;

https://reviewboard.mozilla.org/r/137002/#review140822

With your patch applied, I was able to write a test case that records audio and video data from an HTMLMediaElement playing EME video:

https://people-mozilla.org/~cpearce/mse-clearkey/capture.html
(click "start" to start recording for 5 seconds, a recorded video will appear and be able to be downloaded )

We should not be able to record video from a media element playing EME video; the above test case should fail to work.

It would be good if you could make a test case based on the above example to ensure that the above example fails.

::: dom/html/HTMLMediaElement.cpp:3333
(Diff revision 1)
>  
>    nsPIDOMWindowInner* window = OwnerDoc()->GetInnerWindow();
>    if (!window) {
>      return nullptr;
>    }
> -  if (ContainsRestrictedContent()) {
> +  if (!HasAudio() && ContainsRestrictedContent()) {

I think this needs to be:

  if (HasVideo() && ContainsRestrictedContent())

as it's access to the video we're restricting. That change makes my test page pass at least.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8865352 [details]
Bug 1331763 - part2: Test cases;

https://reviewboard.mozilla.org/r/137004/#review140828

::: dom/media/test/test_eme_stream_capture_blocked_case1.html:19
(Diff revision 1)
>  
>  function startTest(test, token)
>  {
>    // Three cases:
>    // 1. setting MediaKeys on an element captured by MediaElementSource should fail, and
> -  // 2. creating a MediaElementSource on a media element with a MediaKeys should fail, and
> +  // 2. creating a MediaElementSource on a media element with a MediaKeys should

We don't care about restricting access to audio; we only want to block access to video.

MediaElementSource can only capture audio. So we don't care whether it's attached to a media element with restricted content or not.

So this test case should in fact test that capturing audio via MediaElementSource always works.

::: dom/media/test/test_eme_stream_capture_blocked_case1.html:24
(Diff revision 1)
> -  // 2. creating a MediaElementSource on a media element with a MediaKeys should fail, and
> -  // 3. capturing a media element with mozCaptureStream that has a MediaKeys should fail.
> +  // 2. creating a MediaElementSource on a media element with a MediaKeys should
> +  //    - fail, if the media element is video-only
> +  //    - succeed, if the media element containing audio
> +  // , and
> +  // 3. capturing a media element with mozCaptureStream that has a MediaKeys should
> +  //    - fail, if the media element is video-only

mozCaptureStream should fail if the media element  *has* video, not if it *only* has video.

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Chris Pearce (:cpearce) from comment #14)
> Comment on attachment 8865351 [details]
> Bug 1331763 - part1: Support EME content for
> AudioContext::CreateMediaElementSource;
> 
> https://reviewboard.mozilla.org/r/137002/#review140822
> 
> With your patch applied, I was able to write a test case that records audio
> and video data from an HTMLMediaElement playing EME video:
> 
> https://people-mozilla.org/~cpearce/mse-clearkey/capture.html
> (click "start" to start recording for 5 seconds, a recorded video will
> appear and be able to be downloaded )
> 
> We should not be able to record video from a media element playing EME
> video; the above test case should fail to work.
> 
> It would be good if you could make a test case based on the above example to
> ensure that the above example fails.

Actually looking closer at dom/media/test/test_eme_stream_capture_blocked_case3.html I think this test case covers what is happening in my demo. So don't bother about adding a new test case. That should make things easier. :)

You will however need to change test_eme_stream_capture_blocked_case3.html to expect the capture to fail if the media element has video, rather than if it's video-only.

Comment 17

[C.M.Chang[:chunmin] | PTO]

Hi cpearce,
Thanks for your review. I have one question:
If the media element has both video and audio, could the audio part of the media element be captured? Or we should throw an error when mediaElement.captureStream() is called?

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

I think it's best if we just match Chrome's behaviour here, if a media element is using EME, just throw in captureStream, regardless of whether it's got audio or video or something else.

Google document this behaviour here:
https://developers.google.com/web/updates/2016/10/capture-stream

And enforce that in Chromium here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/mediacapturefromelement/HTMLMediaElementCapture.cpp?l=107&rcl=faace60759ac73ae60b5d4ef84fb85c65c937b4a

So we should just throw in HTMLMediaElement.captureStream() if EME is in use on the media element. No need to check if there's a video track. Sorry I should have got that right the first time.

Comment 19

[C.M.Chang[:chunmin] | PTO]

Comment on attachment 8865351 [details]
Bug 1331763 - part1: Support EME content for AudioContext::CreateMediaElementSource;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137002/diff/1-2/

Comment 20

[C.M.Chang[:chunmin] | PTO]

Comment on attachment 8865352 [details]
Bug 1331763 - part2: Test cases;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137004/diff/1-2/

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8865352 [details]
Bug 1331763 - part2: Test cases;

https://reviewboard.mozilla.org/r/137004/#review142348

::: dom/media/test/test_eme_stream_capture_blocked_case1.html:17
(Diff revision 2)
>  <script class="testbody" type="text/javascript">
>  var manager = new MediaTestManager;
>  
>  function startTest(test, token)
>  {
>    // Three cases:

I don't think we need to list the three cases in all tests; we're duplicating comments. This should have been changed when we split the test into three separate sub tests. So please remove lines [17,21] in all three test cases.

::: dom/media/test/test_eme_stream_capture_blocked_case2.html:24
(Diff revision 2)
> -  // 2. creating a MediaElementSource on a media element with a MediaKeys should fail, and
> +  // 2. creating a MediaElementSource on a media element should always succeed
> +  //    (no matter it's restricted content or not), and
>    // 3. capturing a media element with mozCaptureStream that has a MediaKeys should fail.
>  
> -  // Case 2. creating a MediaElementSource on a media element with a MediaKeys should fail.
> +  // Case 2. creating a MediaElementSource on a media element should always succeed
> +  //         (no matter it's restricted content or not), and

Grammar: "no matter whether it's...", i.e. insert "whether" between "matter" and "it's".

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8865351 [details]
Bug 1331763 - part1: Support EME content for AudioContext::CreateMediaElementSource;

https://reviewboard.mozilla.org/r/137002/#review142350

Comment 23

[C.M.Chang[:chunmin] | PTO]

Comment on attachment 8865351 [details]
Bug 1331763 - part1: Support EME content for AudioContext::CreateMediaElementSource;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137002/diff/2-3/

Comment 24

[C.M.Chang[:chunmin] | PTO]

Comment on attachment 8865352 [details]
Bug 1331763 - part2: Test cases;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137004/diff/2-3/

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/61d6ee9aa5bb
part1: Support EME content for AudioContext::CreateMediaElementSource; r=cpearce
https://hg.mozilla.org/integration/autoland/rev/8f2387be7e2d
part2: Test cases; r=cpearce

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/61d6ee9aa5bb
https://hg.mozilla.org/mozilla-central/rev/8f2387be7e2d