MSE: Plumb ChunkDemuxer appendData failures into append Error algorithm

MSE: Run appendError on appendData failures

This change lets ChunkDemuxer::AppendData signal parse failure back
to Blink SourceBuffer by its return value. SourceBuffer synchronously
uses that signal to run the append error algorithm (which can enqueue
an 'error' event on the SourceBuffer, among other steps).
In parallel, ChunkDemuxer::ReportError_Locked()
still notifies WMPI and HTMLMediaElement asynchronously via
Pipeline of the error state.

BUG=620903
TEST=Updated mediasource-errors.html, ChunkDemuxerTests, PipelineIntegrationTests

Committed: https://crrev.com/4f0ed2f67852f56ecc8d5ca1605b1ff81a26afd9
Cr-Commit-Position: refs/heads/master@{#401439}

[wolenetz, 2016-06-17 22:46:24]

Description was changed from

==========
WIP MSE: Plumb ChunkDemuxer appendData failures into append Error algorithm

TODO: Full description once I get this working.

WIP: In initial patch set, mediasource-errors.html times out, and the following
media_unittests regress:

8 tests failed:
    ChunkDemuxerTest.AVHeadersWithAudioOnlyType
(../../media/filters/chunk_demuxer_unittest.cc:2396)
    ChunkDemuxerTest.AVHeadersWithVideoOnlyType
(../../media/filters/chunk_demuxer_unittest.cc:2417)
    ChunkDemuxerTest.AudioOnlyHeaderWithAVType
(../../media/filters/chunk_demuxer_unittest.cc:2438)
    ChunkDemuxerTest.Init (../../media/filters/chunk_demuxer_unittest.cc:1417)
    ChunkDemuxerTest.VideoOnlyHeaderWithAVType
(../../media/filters/chunk_demuxer_unittest.cc:2460)
    PipelineIntegrationTest.MediaSource_ConfigChange_ClearThenEncrypted_WebM
(../../media/test/pipeline_integration_test.cc:1299)
    PipelineIntegrationTest.MediaSource_ConfigChange_EncryptedThenClear_MP4_CENC
(../../media/test/pipeline_integration_test.cc:1665)
    PipelineIntegrationTest.MediaSource_ConfigChange_EncryptedThenClear_WebM
(../../media/test/pipeline_integration_test.cc:1331)
3 tests crashed:
    ChunkDemuxerTest.AppendDataBeforeInit
(../../media/filters/chunk_demuxer_unittest.cc:1796)
    ChunkDemuxerTest.BackwardsAndBeforeClusterTimecode
(../../media/filters/chunk_demuxer_unittest.cc:1874)
    ChunkDemuxerTest.NonMonotonicButAboveClusterTimecode
(../../media/filters/chunk_demuxer_unittest.cc:1846)

BUG=620903
TEST=Updated mediasource-errors.html, ChunkDemuxerTests,
PipelineIntegrationTests
==========

to

==========
WIP MSE: Plumb ChunkDemuxer appendData failures into append Error algorithm

TODO: Full description once I get this working.

BUG=620903
TEST=Updated mediasource-errors.html, ChunkDemuxerTests,
PipelineIntegrationTests
==========

[wolenetz, 2016-06-18 00:17:40]

Description was changed from

==========
WIP MSE: Plumb ChunkDemuxer appendData failures into append Error algorithm

TODO: Full description once I get this working.

BUG=620903
TEST=Updated mediasource-errors.html, ChunkDemuxerTests,
PipelineIntegrationTests
==========

to

==========
MSE: Run appendError on appendData failures

This change lets ChunkDemuxer::AppendData signal parse failure back
to Blink SourceBuffer by its return value. SourceBuffer synchronously
uses that signal to run the append error algorithm (which can enqueue
an 'error' event on the SourceBuffer, among other steps).
In parallel, ChunkDemuxer::ReportError_Locked()
still notifies WMPI and HTMLMediaElement asynchronously via
Pipeline of the error state.

BUG=620903
TEST=Updated mediasource-errors.html, ChunkDemuxerTests,
PipelineIntegrationTests
==========

[wolenetz, 2016-06-18 00:17:40]

wolenetz@chromium.org changed reviewers:
+ chcunningham@chromium.org, foolip@chromium.org

[wolenetz, 2016-06-18 00:17:55]

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

[commit-bot: I haz the power, 2016-06-18 00:18:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2076673005/80001

[wolenetz, 2016-06-18 00:27:55]

Please take a look at patch set 5:
chcunningham@: everything
foolip@:  OWNERS review for third_party/WebKit/public/platform/WebSourceBuffer.h

Thanks!

cc+=servolk@ for FYI relative to comments/refactoring of
https://codereview.chromium.org/1678523003/ that will likely depend on this
landing first.

https://codereview.chromium.org/2076673005/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-errors.html
(right):

https://codereview.chromium.org/2076673005/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-errors.html:10:
<div id="log"></div>
aside: MSE layout test cleanup of these obsolete pieces like this and the css
stylesheet inclusion, above, will be done separately from this CL.

https://codereview.chromium.org/2076673005/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-errors.html:35:
test.expectEvent(sourceBuffer, "error", "sourceBuffer error.");
Note to reviewers: This was the critically missing, app-visible, piece. This
even was *not* fired prior to this CL.

https://codereview.chromium.org/2076673005/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-errors.html:188:
test.expectEvent(sourceBuffer, "error", "sourceBuffer error.");
ditto note to reviewers

[commit-bot: I haz the power, 2016-06-18 01:53:37]

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

[commit-bot: I haz the power, 2016-06-18 01:53:38]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-06-18 06:20:05]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2016-06-18 06:20:06]

https://codereview.chromium.org/2076673005/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebSourceBuffer.h (right):

https://codereview.chromium.org/2076673005/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebSourceBuffer.h:40: class WebSourceBuffer {
This is obviously a preexisting class, but do you mind adding a comment that
this is for MSE and what it's role is? It's not immediately obvious from the
name what a "source buffer" is.

[foolip, 2016-06-20 12:04:17]

https://codereview.chromium.org/2076673005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasource/SourceBuffer.h (right):

https://codereview.chromium.org/2076673005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasource/SourceBuffer.h:135: //
|decodeError| is only valid if |runAppendError| is true.
Smells like this should be a 3-state enum?

[chcunningham, 2016-06-21 02:00:01]

Will get to this first thing tomorrow.

[wolenetz, 2016-06-21 17:47:44]

On 2016/06/21 02:00:01, chcunningham wrote:
> Will get to this first thing tomorrow.

Layout test is flakily timing out. I'm investigating at the moment. Feel free to
review in the meantime.

[wolenetz, 2016-06-21 19:38:09]

Please take a look. Same review coverage as before (with 1 additional
public/platform file)

https://codereview.chromium.org/2076673005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasource/SourceBuffer.h (right):

https://codereview.chromium.org/2076673005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasource/SourceBuffer.h:135: //
|decodeError| is only valid if |runAppendError| is true.
On 2016/06/20 12:04:17, Philip Jägenstedt wrote:
> Smells like this should be a 3-state enum?

I was thinking similarly and certainly would have done that already if this
method were public. I changed to using an enum. And will do the same in future
per Blink code style that recommends this :)

I also updated the ::appendError(..) method to take an enum instead of a bool,
following that same code style.

https://codereview.chromium.org/2076673005/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebSourceBuffer.h (right):

https://codereview.chromium.org/2076673005/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebSourceBuffer.h:40: class WebSourceBuffer {
On 2016/06/18 06:20:06, dcheng wrote:
> This is obviously a preexisting class, but do you mind adding a comment that
> this is for MSE and what it's role is? It's not immediately obvious from the
> name what a "source buffer" is.

Done - and I added a small comment to the top of WebSourceBufferClient.h, too.

[chcunningham, 2016-06-21 21:17:30]

lgtm

Nice! Couldn't find any issues.

[foolip, 2016-06-22 12:39:52]

lgtm

https://codereview.chromium.org/2076673005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasource/SourceBuffer.h (right):

https://codereview.chromium.org/2076673005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasource/SourceBuffer.h:135: //
|decodeError| is only valid if |runAppendError| is true.
On 2016/06/21 19:38:09, wolenetz wrote:
> On 2016/06/20 12:04:17, Philip Jägenstedt wrote:
> > Smells like this should be a 3-state enum?
> 
> I was thinking similarly and certainly would have done that already if this
> method were public. I changed to using an enum. And will do the same in future
> per Blink code style that recommends this :)
> 
> I also updated the ::appendError(..) method to take an enum instead of a bool,
> following that same code style.

Yay :)

[wolenetz, 2016-06-22 19:06:52]

On 2016/06/22 12:39:52, Philip Jägenstedt wrote:
> lgtm
> 
>
https://codereview.chromium.org/2076673005/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/mediasource/SourceBuffer.h (right):
> 
>
https://codereview.chromium.org/2076673005/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/mediasource/SourceBuffer.h:135: //
> |decodeError| is only valid if |runAppendError| is true.
> On 2016/06/21 19:38:09, wolenetz wrote:
> > On 2016/06/20 12:04:17, Philip Jägenstedt wrote:
> > > Smells like this should be a 3-state enum?
> > 
> > I was thinking similarly and certainly would have done that already if this
> > method were public. I changed to using an enum. And will do the same in
future
> > per Blink code style that recommends this :)
> > 
> > I also updated the ::appendError(..) method to take an enum instead of a
bool,
> > following that same code style.
> 
> Yay :)

Thanks - I'll CQ this shortly. dcheng@ separately indicated foolip@'s lg*m is
sufficient/dcheng@ was driving-by earlier.

[wolenetz, 2016-06-22 19:07:00]

The CQ bit was checked by wolenetz@chromium.org

[commit-bot: I haz the power, 2016-06-22 19:07:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2076673005/100001

[commit-bot: I haz the power, 2016-06-22 19:10:10]

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

[commit-bot: I haz the power, 2016-06-22 19:10:11]

Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[wolenetz, 2016-06-22 21:24:39]

The CQ bit was checked by wolenetz@chromium.org

[wolenetz, 2016-06-22 21:24:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
chcunningham@chromium.org, foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2076673005/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2016-06-22 21:25:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2076673005/120001

[commit-bot: I haz the power, 2016-06-22 22:55:32]

Description was changed from

==========
MSE: Run appendError on appendData failures

This change lets ChunkDemuxer::AppendData signal parse failure back
to Blink SourceBuffer by its return value. SourceBuffer synchronously
uses that signal to run the append error algorithm (which can enqueue
an 'error' event on the SourceBuffer, among other steps).
In parallel, ChunkDemuxer::ReportError_Locked()
still notifies WMPI and HTMLMediaElement asynchronously via
Pipeline of the error state.

BUG=620903
TEST=Updated mediasource-errors.html, ChunkDemuxerTests,
PipelineIntegrationTests
==========

to

==========
MSE: Run appendError on appendData failures

This change lets ChunkDemuxer::AppendData signal parse failure back
to Blink SourceBuffer by its return value. SourceBuffer synchronously
uses that signal to run the append error algorithm (which can enqueue
an 'error' event on the SourceBuffer, among other steps).
In parallel, ChunkDemuxer::ReportError_Locked()
still notifies WMPI and HTMLMediaElement asynchronously via
Pipeline of the error state.

BUG=620903
TEST=Updated mediasource-errors.html, ChunkDemuxerTests,
PipelineIntegrationTests
==========

[commit-bot: I haz the power, 2016-06-22 22:55:35]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-22 22:57:44]

Description was changed from

==========
MSE: Run appendError on appendData failures

This change lets ChunkDemuxer::AppendData signal parse failure back
to Blink SourceBuffer by its return value. SourceBuffer synchronously
uses that signal to run the append error algorithm (which can enqueue
an 'error' event on the SourceBuffer, among other steps).
In parallel, ChunkDemuxer::ReportError_Locked()
still notifies WMPI and HTMLMediaElement asynchronously via
Pipeline of the error state.

BUG=620903
TEST=Updated mediasource-errors.html, ChunkDemuxerTests,
PipelineIntegrationTests
==========

to

==========
MSE: Run appendError on appendData failures

This change lets ChunkDemuxer::AppendData signal parse failure back
to Blink SourceBuffer by its return value. SourceBuffer synchronously
uses that signal to run the append error algorithm (which can enqueue
an 'error' event on the SourceBuffer, among other steps).
In parallel, ChunkDemuxer::ReportError_Locked()
still notifies WMPI and HTMLMediaElement asynchronously via
Pipeline of the error state.

BUG=620903
TEST=Updated mediasource-errors.html, ChunkDemuxerTests,
PipelineIntegrationTests

Committed: https://crrev.com/4f0ed2f67852f56ecc8d5ca1605b1ff81a26afd9
Cr-Commit-Position: refs/heads/master@{#401439}
==========

[commit-bot: I haz the power, 2016-06-22 22:57:46]

Patchset 7 (id:??) landed as
https://crrev.com/4f0ed2f67852f56ecc8d5ca1605b1ff81a26afd9
Cr-Commit-Position: refs/heads/master@{#401439}