MediaRecorder: make sure ondataavailable+onstop events are fired before onerror

MediaRecorder: make sure ondataavailable+onstop events are fired before onerror

This CL makes sure that "error" is the last event to be fired, after
any "dataavailable" and "stop" events have been received, which 
was brought up in [1];

A typical situation triggering an error condition is if the amount
of tracks of the MediaStream being recorded changes.  Checking
for this amount of tracks is moved to the renderer/ 
implementation so all the errors are now sent via onError(), which
forces the recording to stop().

Added a new Layout Test for it (using <canvas>.captureStream() ISO 
getUserMedia() which makes it more upstream-able).

[1] https://github.com/w3c/mediacapture-record/issues/52

BUG=693179
Review-Url: https://codereview.chromium.org/2697703003
Cr-Commit-Position: refs/heads/master@{#451561}
Committed: https://chromium.googlesource.com/chromium/src/+/786184e85335e590fd0dd3b31d267f524633a90b

[mcasas, 2017-02-15 01:38:54]

Description was changed from

==========
MediaRecorder: Force onstop+ondataavailable events before onerror

Force onstop+ondataavailable events before onerror and add layouttest for it;
[1]

use <canvas>.captureStream() ISO getUserMedia() where possible;

Correct indentation of test JS

[1] https://github.com/w3c/mediacapture-record/issues/52
BUG=
==========

to

==========
MediaRecorder: Force onstop+ondataavailable events before onerror


Force firing ondataavailable events before onerror [1], with a
new layouttest for it (using <canvas>.captureStream() ISO 
getUserMedia() which makes it more upstream-able).

Correct indentation of test JS according to style guide [2]

[1] https://github.com/w3c/mediacapture-record/issues/52
[2]
https://google.github.io/styleguide/jsguide.html#formatting-block-indentation

BUG=
==========

[mcasas, 2017-02-15 01:39:37]

Description was changed from

==========
MediaRecorder: Force onstop+ondataavailable events before onerror


Force firing ondataavailable events before onerror [1], with a
new layouttest for it (using <canvas>.captureStream() ISO 
getUserMedia() which makes it more upstream-able).

Correct indentation of test JS according to style guide [2]

[1] https://github.com/w3c/mediacapture-record/issues/52
[2]
https://google.github.io/styleguide/jsguide.html#formatting-block-indentation

BUG=
==========

to

==========
MediaRecorder: make sure ondataavailable event is fired before onerror

Force firing ondataavailable events before onerror [1], with a
new layouttest for it (using <canvas>.captureStream() ISO 
getUserMedia() which makes it more upstream-able).

Correct indentation of test JS according to style guide [2]

[1] https://github.com/w3c/mediacapture-record/issues/52
[2]
https://google.github.io/styleguide/jsguide.html#formatting-block-indentation

BUG=
==========

[mcasas, 2017-02-16 17:36:00]

Patchset #1 (id:1) has been deleted

[mcasas, 2017-02-16 17:36:06]

Patchset #1 (id:20001) has been deleted

[mcasas, 2017-02-16 17:38:35]

Description was changed from

==========
MediaRecorder: make sure ondataavailable event is fired before onerror

Force firing ondataavailable events before onerror [1], with a
new layouttest for it (using <canvas>.captureStream() ISO 
getUserMedia() which makes it more upstream-able).

Correct indentation of test JS according to style guide [2]

[1] https://github.com/w3c/mediacapture-record/issues/52
[2]
https://google.github.io/styleguide/jsguide.html#formatting-block-indentation

BUG=
==========

to

==========
MediaRecorder: make sure ondataavailable+onstop events are fired before onerror

Force firing ondataavailable events before onstop+ onerror [1];
added a new Layout Test for it (using <canvas>.captureStream() ISO 
getUserMedia() which makes it more upstream-able).

[1] https://github.com/w3c/mediacapture-record/issues/52

BUG=
==========

[mcasas, 2017-02-16 19:57:36]

Description was changed from

==========
MediaRecorder: make sure ondataavailable+onstop events are fired before onerror

Force firing ondataavailable events before onstop+ onerror [1];
added a new Layout Test for it (using <canvas>.captureStream() ISO 
getUserMedia() which makes it more upstream-able).

[1] https://github.com/w3c/mediacapture-record/issues/52

BUG=
==========

to

==========
MediaRecorder: make sure ondataavailable+onstop events are fired before onerror

This CL makes sure that "error" is the last event to be fired, after
any "dataavailable" and "stop" events have been received, which 
was brought up in [1];

A typical situation triggering an error condition is if the amount
of tracks of the MediaStream being recorded changes.  Checking
for this amount of tracks is moved to the renderer/ 
implementation so all the errors are now sent via onError(), which
forces the recording to stop().

Added a new Layout Test for it (using <canvas>.captureStream() ISO 
getUserMedia() which makes it more upstream-able).

[1] https://github.com/w3c/mediacapture-record/issues/52

BUG=
==========

[mcasas, 2017-02-16 20:02:01]

Description was changed from

==========
MediaRecorder: make sure ondataavailable+onstop events are fired before onerror

This CL makes sure that "error" is the last event to be fired, after
any "dataavailable" and "stop" events have been received, which 
was brought up in [1];

A typical situation triggering an error condition is if the amount
of tracks of the MediaStream being recorded changes.  Checking
for this amount of tracks is moved to the renderer/ 
implementation so all the errors are now sent via onError(), which
forces the recording to stop().

Added a new Layout Test for it (using <canvas>.captureStream() ISO 
getUserMedia() which makes it more upstream-able).

[1] https://github.com/w3c/mediacapture-record/issues/52

BUG=
==========

to

==========
MediaRecorder: make sure ondataavailable+onstop events are fired before onerror

This CL makes sure that "error" is the last event to be fired, after
any "dataavailable" and "stop" events have been received, which 
was brought up in [1];

A typical situation triggering an error condition is if the amount
of tracks of the MediaStream being recorded changes.  Checking
for this amount of tracks is moved to the renderer/ 
implementation so all the errors are now sent via onError(), which
forces the recording to stop().

Added a new Layout Test for it (using <canvas>.captureStream() ISO 
getUserMedia() which makes it more upstream-able).

[1] https://github.com/w3c/mediacapture-record/issues/52

BUG=693179
==========

[mcasas, 2017-02-16 20:02:58]

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

[mcasas, 2017-02-16 20:02:59]

emircan@ PTAL

[emircan, 2017-02-17 00:19:07]

https://codereview.chromium.org/2697703003/diff/40001/content/renderer/media/...
File content/renderer/media/recorder/media_recorder_handler.cc (right):

https://codereview.chromium.org/2697703003/diff/40001/content/renderer/media/...
content/renderer/media/recorder/media_recorder_handler.cc:150:
media_stream_num_tracks_ = 0;  // Will be updated in start();
Should we set it to 0 in Stop() as well?

https://codereview.chromium.org/2697703003/diff/40001/content/renderer/media/...
content/renderer/media/recorder/media_recorder_handler.cc:337: if
(media_stream_num_tracks_ == current_num_tracks)
What about the case where one is added another is removed?

https://codereview.chromium.org/2697703003/diff/40001/content/renderer/media/...
content/renderer/media/recorder/media_recorder_handler.cc:340:
media_stream_num_tracks_ = current_num_tracks;
Is it necessary to update |media_stream_num_tracks_| here? Can you change this
function to only check that number of tracks is different than the initialize?

https://codereview.chromium.org/2697703003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-error.html
(right):

https://codereview.chromium.org/2697703003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-error.html:46:
drawSomethingOnCanvas(canvas);
Move this after l.56. 

Also, AFAIU first frame is only used to initialize encoders but not encoded. I
am not sure how this works as it only draws once. Maybe something triggers page
repaint and another canvas capture call?

https://cs.chromium.org/chromium/src/content/renderer/media/recorder/video_tr...

[mcasas, 2017-02-17 01:45:34]

Patchset #2 (id:60001) has been deleted

[mcasas, 2017-02-17 01:46:14]

ptal

https://codereview.chromium.org/2697703003/diff/40001/content/renderer/media/...
File content/renderer/media/recorder/media_recorder_handler.cc (right):

https://codereview.chromium.org/2697703003/diff/40001/content/renderer/media/...
content/renderer/media/recorder/media_recorder_handler.cc:150:
media_stream_num_tracks_ = 0;  // Will be updated in start();
On 2017/02/17 00:19:07, emircan wrote:
> Should we set it to 0 in Stop() as well?

Not really  because after stop(), we'd need to 
have to start() again. Actually it's not needed
to reinitialise it/them here.

https://codereview.chromium.org/2697703003/diff/40001/content/renderer/media/...
content/renderer/media/recorder/media_recorder_handler.cc:337: if
(media_stream_num_tracks_ == current_num_tracks)
On 2017/02/17 00:19:07, emircan wrote:
> What about the case where one is added another is removed?

Hmm good point, modified.

https://codereview.chromium.org/2697703003/diff/40001/content/renderer/media/...
content/renderer/media/recorder/media_recorder_handler.cc:340:
media_stream_num_tracks_ = current_num_tracks;
On 2017/02/17 00:19:07, emircan wrote:
> Is it necessary to update |media_stream_num_tracks_| here? Can you change this
> function to only check that number of tracks is different than the initialize?

I still need the update because at the end of the
day from MRHandler we're signalling the error, but
the WebApp might like to continue recording with
the current status (i.e. ignoring the error). You
could perfectly argue that all errors are fatal,
but note that we're not calling stop()...

https://codereview.chromium.org/2697703003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-error.html
(right):

https://codereview.chromium.org/2697703003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-error.html:46:
drawSomethingOnCanvas(canvas);
On 2017/02/17 00:19:07, emircan wrote:
> Move this after l.56. 

Done.
> 
> Also, AFAIU first frame is only used to initialize encoders but not encoded. I
> am not sure how this works as it only draws once. Maybe something triggers
page
> repaint and another canvas capture call?
> 
>
https://cs.chromium.org/chromium/src/content/renderer/media/recorder/video_tr...

When a new MediaStreamVideoSink is connected to a Track
it does a RequestRefreshFrame() :

https://cs.chromium.org/chromium/src/content/renderer/media/media_stream_vide...

[emircan, 2017-02-17 18:59:25]

https://codereview.chromium.org/2697703003/diff/80001/content/renderer/media/...
File content/renderer/media/recorder/media_recorder_handler.cc (right):

https://codereview.chromium.org/2697703003/diff/80001/content/renderer/media/...
content/renderer/media/recorder/media_recorder_handler.cc:151: 
not: Remove empty line.

https://codereview.chromium.org/2697703003/diff/80001/content/renderer/media/...
content/renderer/media/recorder/media_recorder_handler.cc:340:
media_stream_num_audio_tracks_ == audio_tracks.size()) {
As we discussed offline, there can be a case of one video track removed and
another added so we can compare the actual tracks here instead.

https://codereview.chromium.org/2697703003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (left):

https://codereview.chromium.org/2697703003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:341: // is in
muted() state.
Is this TODO still relevant or...

[mcasas, 2017-02-17 20:40:21]

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

[mcasas, 2017-02-17 20:40:29]

ptal

https://codereview.chromium.org/2697703003/diff/80001/content/renderer/media/...
File content/renderer/media/recorder/media_recorder_handler.cc (right):

https://codereview.chromium.org/2697703003/diff/80001/content/renderer/media/...
content/renderer/media/recorder/media_recorder_handler.cc:151: 
On 2017/02/17 18:59:25, emircan wrote:
> not: Remove empty line.

Done.

https://codereview.chromium.org/2697703003/diff/80001/content/renderer/media/...
content/renderer/media/recorder/media_recorder_handler.cc:340:
media_stream_num_audio_tracks_ == audio_tracks.size()) {
On 2017/02/17 18:59:25, emircan wrote:
> As we discussed offline, there can be a case of one video track removed and
> another added so we can compare the actual tracks here instead.

Done.

https://codereview.chromium.org/2697703003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (left):

https://codereview.chromium.org/2697703003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:341: // is in
muted() state.
On 2017/02/17 18:59:25, emircan wrote:
> Is this TODO still relevant or...

No, it was removed from the Spec in 
https://github.com/w3c/mediacapture-record/issues/99

[commit-bot: I haz the power, 2017-02-17 20:41:04]

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-02-17 21:48:11]

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

[commit-bot: I haz the power, 2017-02-17 21:48:12]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[mcasas, 2017-02-17 22:34:39]

Patchset #4 (id:120001) has been deleted

[emircan, 2017-02-20 02:29:21]

lgtm

[emircan, 2017-02-20 02:29:28]

The CQ bit was checked by emircan@chromium.org

[commit-bot: I haz the power, 2017-02-20 02:29:37]

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-02-20 04:09:46]

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

[commit-bot: I haz the power, 2017-02-20 04:09:47]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mcasas, 2017-02-20 04:27:40]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2017-02-20 04:28:01]

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-02-20 05:28:20]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487564860072560,
"parent_rev": "7ff34467488f2eb5e4fa0dc38f14e18a4a17584f", "commit_rev":
"786184e85335e590fd0dd3b31d267f524633a90b"}

[commit-bot: I haz the power, 2017-02-20 05:28:51]

Description was changed from

==========
MediaRecorder: make sure ondataavailable+onstop events are fired before onerror

This CL makes sure that "error" is the last event to be fired, after
any "dataavailable" and "stop" events have been received, which 
was brought up in [1];

A typical situation triggering an error condition is if the amount
of tracks of the MediaStream being recorded changes.  Checking
for this amount of tracks is moved to the renderer/ 
implementation so all the errors are now sent via onError(), which
forces the recording to stop().

Added a new Layout Test for it (using <canvas>.captureStream() ISO 
getUserMedia() which makes it more upstream-able).

[1] https://github.com/w3c/mediacapture-record/issues/52

BUG=693179
==========

to

==========
MediaRecorder: make sure ondataavailable+onstop events are fired before onerror

This CL makes sure that "error" is the last event to be fired, after
any "dataavailable" and "stop" events have been received, which 
was brought up in [1];

A typical situation triggering an error condition is if the amount
of tracks of the MediaStream being recorded changes.  Checking
for this amount of tracks is moved to the renderer/ 
implementation so all the errors are now sent via onError(), which
forces the recording to stop().

Added a new Layout Test for it (using <canvas>.captureStream() ISO 
getUserMedia() which makes it more upstream-able).

[1] https://github.com/w3c/mediacapture-record/issues/52

BUG=693179

Review-Url: https://codereview.chromium.org/2697703003
Cr-Commit-Position: refs/heads/master@{#451561}
Committed:
https://chromium.googlesource.com/chromium/src/+/786184e85335e590fd0dd3b31d26...
==========

[commit-bot: I haz the power, 2017-02-20 05:28:53]

Committed patchset #4 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/786184e85335e590fd0dd3b31d26...