MediaRecorder: bugfix start() means buffer-forever

MediaRecorder: bugfix start() means buffer-forever

MediaRecorder.start(optional timeslice) with |timeslice| == 0
means that MR will have to buffer forever or until stop() or
requestData()  is called;  the current implementation wrongly
assumes undefined |timeslice| to be a very small number.  This
CL corrects this bug.

See
https://bugs.chromium.org/p/chromium/issues/detail?id=567859#c10

and
https://groups.google.com/a/chromium.org/d/msg/blink-dev/REYBYlU_iqE/VaTuPsQcBwAJ
https://groups.google.com/d/msg/discuss-webrtc/HkJF_sm20zU/TDxWIs8kCQAJ

BUG=567859
Committed: https://crrev.com/e2ab4313f9aad6c29633b07c58064052de8a44f6
Cr-Commit-Position: refs/heads/master@{#428738}

[mcasas, 2016-10-24 22:18:14]

Description was changed from

==========
[wip] MediaRecorder: bugfix start() means buffer-forever

BUG=567859
==========

to

==========
[wip] MediaRecorder: bugfix start() means buffer-forever

MediaRecorder.start(optional timeslice) with |timeslice| == 0 
means that MR will have to buffer forever or until stop() or 
requestData()  is called;  the current implementation wrongly 
assumes undefined |timeslice| to be a very small number.  This
CL corrects this bug.

See 
https://bugs.chromium.org/p/chromium/issues/detail?id=567859#c10

BUG=567859
==========

[mcasas, 2016-10-25 16:08:35]

Patchset #1 (id:1) has been deleted

[mcasas, 2016-10-25 16:08:43]

Patchset #1 (id:20001) has been deleted

[mcasas, 2016-10-28 00:51:08]

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

[commit-bot: I haz the power, 2016-10-28 00:51:32]

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, 2016-10-28 04:13:27]

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

[commit-bot: I haz the power, 2016-10-28 04:13:28]

Dry run: This issue passed the CQ dry run.

[mcasas, 2016-10-28 17:26:49]

Description was changed from

==========
[wip] MediaRecorder: bugfix start() means buffer-forever

MediaRecorder.start(optional timeslice) with |timeslice| == 0 
means that MR will have to buffer forever or until stop() or 
requestData()  is called;  the current implementation wrongly 
assumes undefined |timeslice| to be a very small number.  This
CL corrects this bug.

See 
https://bugs.chromium.org/p/chromium/issues/detail?id=567859#c10

BUG=567859
==========

to

==========
MediaRecorder: bugfix start() means buffer-forever

MediaRecorder.start(optional timeslice) with |timeslice| == 0 
means that MR will have to buffer forever or until stop() or 
requestData()  is called;  the current implementation wrongly 
assumes undefined |timeslice| to be a very small number.  This
CL corrects this bug.

See 
https://bugs.chromium.org/p/chromium/issues/detail?id=567859#c10

BUG=567859
==========

[mcasas, 2016-10-28 17:27:24]

Description was changed from

==========
MediaRecorder: bugfix start() means buffer-forever

MediaRecorder.start(optional timeslice) with |timeslice| == 0 
means that MR will have to buffer forever or until stop() or 
requestData()  is called;  the current implementation wrongly 
assumes undefined |timeslice| to be a very small number.  This
CL corrects this bug.

See 
https://bugs.chromium.org/p/chromium/issues/detail?id=567859#c10

BUG=567859
==========

to

==========
MediaRecorder: bugfix start() means buffer-forever

MediaRecorder.start(optional timeslice) with |timeslice| == 0 
means that MR will have to buffer forever or until stop() or 
requestData()  is called;  the current implementation wrongly 
assumes undefined |timeslice| to be a very small number.  This
CL corrects this bug.

See 
https://bugs.chromium.org/p/chromium/issues/detail?id=567859#c10

and
https://groups.google.com/a/chromium.org/d/msg/blink-dev/REYBYlU_iqE/VaTuPsQc...

BUG=567859
==========

[mcasas, 2016-10-28 17:28:42]

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

[mcasas, 2016-10-28 17:28:42]

niklase@ RS plz

(will wait to ship it until next week JIC).

[Niklas Enbom, 2016-10-28 17:43:43]

Is this the final solution or a quick fix? If final, is
WebMediaRecorderHandler::start() needed any longer? 


On 2016/10/28 17:28:42, mcasas wrote:
> niklase@ RS plz
> 
> (will wait to ship it until next week JIC).

[mcasas, 2016-10-28 19:06:36]

On 2016/10/28 17:43:43, Niklas Enbom wrote:
> Is this the final solution or a quick fix? If final, is
> WebMediaRecorderHandler::start() needed any longer? 
> 
> 
> On 2016/10/28 17:28:42, mcasas wrote:
> > niklase@ RS plz
> > 
> > (will wait to ship it until next week JIC).

This is a final solution.

Probably WMRH::start() can be removed indeed.

[mcasas, 2016-10-28 19:27:45]

Patchset #2 (id:60001) has been deleted

[mcasas, 2016-10-28 20:18:06]

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

[mcasas, 2016-10-28 20:18:07]

niklase@ PTAL

foolip@ plz RS the nuking of one method
in public/platform/WebMediaRecorderHandler.h

[Niklas Enbom, 2016-10-28 20:36:56]

lgtm
On 2016/10/28 20:18:07, mcasas wrote:
> niklase@ PTAL
> 
> foolip@ plz RS the nuking of one method
> in public/platform/WebMediaRecorderHandler.h

[mcasas, 2016-10-28 23:57:29]

Description was changed from

==========
MediaRecorder: bugfix start() means buffer-forever

MediaRecorder.start(optional timeslice) with |timeslice| == 0 
means that MR will have to buffer forever or until stop() or 
requestData()  is called;  the current implementation wrongly 
assumes undefined |timeslice| to be a very small number.  This
CL corrects this bug.

See 
https://bugs.chromium.org/p/chromium/issues/detail?id=567859#c10

and
https://groups.google.com/a/chromium.org/d/msg/blink-dev/REYBYlU_iqE/VaTuPsQc...

BUG=567859
==========

to

==========
MediaRecorder: bugfix start() means buffer-forever

MediaRecorder.start(optional timeslice) with |timeslice| == 0 
means that MR will have to buffer forever or until stop() or 
requestData()  is called;  the current implementation wrongly 
assumes undefined |timeslice| to be a very small number.  This
CL corrects this bug.

See 
https://bugs.chromium.org/p/chromium/issues/detail?id=567859#c10

and
https://groups.google.com/a/chromium.org/d/msg/blink-dev/REYBYlU_iqE/VaTuPsQc...
https://groups.google.com/d/msg/discuss-webrtc/HkJF_sm20zU/TDxWIs8kCQAJ

BUG=567859
==========

[foolip, 2016-10-31 13:52:31]

lgtm % test nit

https://codereview.chromium.org/2444153002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-framerate-0.html
(right):

https://codereview.chromium.org/2444153002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-framerate-0.html:48:
recorder.start(1);
If recorder.start(0) is exactly the previous behavior, keeping that and changing
the behavior of the tests would be good. Or, if the tests are changed, I'd
expect some other change as well, since otherwise the test isn't actually
exercising the difference between 0 and 1.

[mcasas, 2016-10-31 14:45:58]

Patchset #3 (id:100001) has been deleted

[mcasas, 2016-10-31 15:34:43]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2016-10-31 15:34:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
niklase@chromium.org, foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2444153002/#ps120001
(title: "s/start(1)/start(0)/")

[commit-bot: I haz the power, 2016-10-31 15:34:56]

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

[mcasas, 2016-10-31 16:17:19]

https://codereview.chromium.org/2444153002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-framerate-0.html
(right):

https://codereview.chromium.org/2444153002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-framerate-0.html:48:
recorder.start(1);
On 2016/10/31 13:52:31, foolip wrote:
> If recorder.start(0) is exactly the previous behavior, keeping that and
changing
> the behavior of the tests would be good. Or, if the tests are changed, I'd
> expect some other change as well, since otherwise the test isn't actually
> exercising the difference between 0 and 1.

Done.

[commit-bot: I haz the power, 2016-10-31 16:33:14]

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

[commit-bot: I haz the power, 2016-10-31 16:33:15]

Failed to apply the patch.
On branch working_branch
Your branch is up-to-date with 'origin/refs/pending/heads/master'.
nothing to commit, working tree clean

[commit-bot: I haz the power, 2016-10-31 16:45:59]

Description was changed from

==========
MediaRecorder: bugfix start() means buffer-forever

MediaRecorder.start(optional timeslice) with |timeslice| == 0 
means that MR will have to buffer forever or until stop() or 
requestData()  is called;  the current implementation wrongly 
assumes undefined |timeslice| to be a very small number.  This
CL corrects this bug.

See 
https://bugs.chromium.org/p/chromium/issues/detail?id=567859#c10

and
https://groups.google.com/a/chromium.org/d/msg/blink-dev/REYBYlU_iqE/VaTuPsQc...
https://groups.google.com/d/msg/discuss-webrtc/HkJF_sm20zU/TDxWIs8kCQAJ

BUG=567859
==========

to

==========
MediaRecorder: bugfix start() means buffer-forever

MediaRecorder.start(optional timeslice) with |timeslice| == 0
means that MR will have to buffer forever or until stop() or
requestData()  is called;  the current implementation wrongly
assumes undefined |timeslice| to be a very small number.  This
CL corrects this bug.

See
https://bugs.chromium.org/p/chromium/issues/detail?id=567859#c10

and
https://groups.google.com/a/chromium.org/d/msg/blink-dev/REYBYlU_iqE/VaTuPsQc...
https://groups.google.com/d/msg/discuss-webrtc/HkJF_sm20zU/TDxWIs8kCQAJ

BUG=567859

Committed: https://crrev.com/e2ab4313f9aad6c29633b07c58064052de8a44f6
Cr-Commit-Position: refs/heads/master@{#428738}
==========

[commit-bot: I haz the power, 2016-10-31 16:46:00]

Patchset 3 (id:??) landed as
https://crrev.com/e2ab4313f9aad6c29633b07c58064052de8a44f6
Cr-Commit-Position: refs/heads/master@{#428738}