Let WebSourceBufferImpl::setMode() update SourceState::sequence_mode_

Let WebSourceBufferImpl::setMode() update SourceState::sequence_mode_

Connects WebSourceBufferImpl::setMode() to similar ChunkDemuxer
and SourceState methods to enable setting SourceState::sequence_mode_.
Later changes will implement processing of distinct 'segments'
and 'sequence' append modes.

R=acolwell@chromium.org
TEST=On local Linux, all mediasource layout tests and media_unittests pass
BUG=249422
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244741

[wolenetz, 2014-01-10 21:37:32]

acolwell, PTAL @ PS1:

Notes:
1. This change depends on first landing Blink CL
https://codereview.chromium.org/130523002/
2. SourceBuffer.mode setting is restricted currently to
--enable-experimental-web-platform-features

Questions:
1. I was unable to get a layout test to confirm InvalidStateError when setting
mode after appending a partial media segment (likely the parser hasn't escaped
'WAITING_FOR_SEGMENT'). Do you think there is a reliable way to capture
'PARSING_MEDIA_SEGMENT' parser state in a layout test? I at least locally
confirmed that if I changed SourceState::SetAppendMode(...) to return false,
then any change to SourceBuffer.mode throws InvalidStateError, so the high level
blink/chromium plumbing seems correct.
2. I'd like to land this and then dig into sequence mode and appropriate
media_unittests (and layout tests) next. Or would you prefer this CL contain
some of that upcoming work?

[wolenetz, 2014-01-10 22:01:00]

On 2014/01/10 21:37:32, wolenetz wrote:
> acolwell, PTAL @ PS1:
> 
> Notes:
> 1. This change depends on first landing Blink CL
> https://codereview.chromium.org/130523002/
> 2. SourceBuffer.mode setting is restricted currently to
> --enable-experimental-web-platform-features
> 
> Questions:
> 1. I was unable to get a layout test to confirm InvalidStateError when setting
> mode after appending a partial media segment (likely the parser hasn't escaped
> 'WAITING_FOR_SEGMENT'). Do you think there is a reliable way to capture
> 'PARSING_MEDIA_SEGMENT' parser state in a layout test? I at least locally
> confirmed that if I changed SourceState::SetAppendMode(...) to return false,
> then any change to SourceBuffer.mode throws InvalidStateError, so the high
level
> blink/chromium plumbing seems correct.
> 2. I'd like to land this and then dig into sequence mode and appropriate
> media_unittests (and layout tests) next. Or would you prefer this CL contain
> some of that upcoming work?

Chuckle - ignore these initial trybot failures. They're of course due to Note #1
:)

[wolenetz, 2014-01-10 22:51:40]

On 2014/01/10 22:01:00, wolenetz wrote:
> On 2014/01/10 21:37:32, wolenetz wrote:
> > acolwell, PTAL @ PS1:
> > 
> > Notes:
> > 1. This change depends on first landing Blink CL
> > https://codereview.chromium.org/130523002/
> > 2. SourceBuffer.mode setting is restricted currently to
> > --enable-experimental-web-platform-features
> > 
> > Questions:
> > 1. I was unable to get a layout test to confirm InvalidStateError when
setting
> > mode after appending a partial media segment (likely the parser hasn't
escaped
> > 'WAITING_FOR_SEGMENT'). Do you think there is a reliable way to capture
> > 'PARSING_MEDIA_SEGMENT' parser state in a layout test? I at least locally
> > confirmed that if I changed SourceState::SetAppendMode(...) to return false,
> > then any change to SourceBuffer.mode throws InvalidStateError, so the high
> level
> > blink/chromium plumbing seems correct.
> > 2. I'd like to land this and then dig into sequence mode and appropriate
> > media_unittests (and layout tests) next. Or would you prefer this CL contain
> > some of that upcoming work?
> 
> Chuckle - ignore these initial trybot failures. They're of course due to Note
#1
> :)

Per second nit in
https://codereview.chromium.org/130523002/diff/330001/Source/modules/mediasou...,
I'll shortly upload a patch set #2 here that references http://crbug.com/333437.

[wolenetz, 2014-01-10 23:08:08]

Patch set 2 is now ready for your review. Thanks!

[acolwell GONE FROM CHROMIUM, 2014-01-10 23:10:37]

https://codereview.chromium.org/134883002/diff/110001/content/renderer/media/...
File content/renderer/media/websourcebuffer_impl.cc (right):

https://codereview.chromium.org/134883002/diff/110001/content/renderer/media/...
content/renderer/media/websourcebuffer_impl.cc:47: default:
nit: Change switch to a cast and use COMPILE_ASSERTs to verify that the Blink
and ChunkDemuxer contants match. See WebMediaSourceImpl for an example.

https://codereview.chromium.org/134883002/diff/110001/media/filters/chunk_dem...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/134883002/diff/110001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:127: // Returns if the mode was set. Returns
false if the mode could not be updated
s/Returns if/Returns true if/

https://codereview.chromium.org/134883002/diff/110001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:129: bool SetAppendMode(ChunkDemuxer::AppendMode
mode);
How about SetSequenceMode(bool is_sequence_mode) since we only have 2 modes at
this point? That way you don't have to introduce this new enum?

[wolenetz, 2014-01-11 00:55:43]

PTAL @ patch set 3.
It also includes some follow-up for my Note #1:
I added a ChunkDemuxer unit test:
SetSequenceMode(...) should return false mid-parse (similar to existing
mid-parse timestampOffset test).
I have not found a similar layout test for timestampOffset that confirms the
exception is thrown. From a preliminary layout test attempt of mine, there could
be some issue with |parsing_media_segment_| maintenance outside of this CL that
I'm digging deeper on next.

https://codereview.chromium.org/134883002/diff/110001/content/renderer/media/...
File content/renderer/media/websourcebuffer_impl.cc (right):

https://codereview.chromium.org/134883002/diff/110001/content/renderer/media/...
content/renderer/media/websourcebuffer_impl.cc:47: default:
On 2014/01/10 23:10:37, acolwell wrote:
> nit: Change switch to a cast and use COMPILE_ASSERTs to verify that the Blink
> and ChunkDemuxer contants match. See WebMediaSourceImpl for an example.

Done using your other suggestion (use a bool |is_sequence_mode| since there are
only two valid modes). As such, there is no need for COMPILE_ASSERTs, and I
followed the example of using switch from WebMediaSourceImpl::markEndOfStream.

https://codereview.chromium.org/134883002/diff/110001/media/filters/chunk_dem...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/134883002/diff/110001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:127: // Returns if the mode was set. Returns
false if the mode could not be updated
On 2014/01/10 23:10:37, acolwell wrote:
> s/Returns if/Returns true if/

Done.

https://codereview.chromium.org/134883002/diff/110001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:129: bool SetAppendMode(ChunkDemuxer::AppendMode
mode);
On 2014/01/10 23:10:37, acolwell wrote:
> How about SetSequenceMode(bool is_sequence_mode) since we only have 2 modes at
> this point? That way you don't have to introduce this new enum?

Done.

https://codereview.chromium.org/134883002/diff/160001/content/renderer/media/...
File content/renderer/media/websourcebuffer_impl.cc (right):

https://codereview.chromium.org/134883002/diff/160001/content/renderer/media/...
content/renderer/media/websourcebuffer_impl.cc:49: return false;
I suspect we want to expose some exception if we reach here, so I added this to
trigger InvalidStateError exception throw by SourceBuffer::setMode().

[wolenetz, 2014-01-11 01:04:11]

On 2014/01/11 00:55:43, wolenetz wrote:
> PTAL @ patch set 3.
> It also includes some follow-up for my Note #1:
s/Note/Question/

[acolwell GONE FROM CHROMIUM, 2014-01-11 01:24:35]

lgtm % nits

https://codereview.chromium.org/134883002/diff/160001/content/renderer/media/...
File content/renderer/media/websourcebuffer_impl.cc (right):

https://codereview.chromium.org/134883002/diff/160001/content/renderer/media/...
content/renderer/media/websourcebuffer_impl.cc:49: return false;
On 2014/01/11 00:55:44, wolenetz wrote:
> I suspect we want to expose some exception if we reach here, so I added this
to
> trigger InvalidStateError exception throw by SourceBuffer::setMode().

No. Please remove this default section so a compiler error happens if the enum
is changed. The Blink code should be protecting this code from any rogue values.
The enum is changed in the future the compiler error will for the person to look
at this code and update the logic appropriately before landing a Blink change
that will send a new value here.

https://codereview.chromium.org/134883002/diff/160001/media/filters/chunk_dem...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/134883002/diff/160001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:129: bool SetSequenceMode(bool is_sequence_mode);
nit: drop the is_ since you are using sequence_mode_ in the class?

https://codereview.chromium.org/134883002/diff/160001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:430: bool SourceState::SetSequenceMode(bool
is_sequence_mode) {
ditto

https://codereview.chromium.org/134883002/diff/160001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:1551: bool is_sequence_mode) {
ditto

https://codereview.chromium.org/134883002/diff/160001/media/filters/chunk_dem...
File media/filters/chunk_demuxer_unittest.cc (right):

https://codereview.chromium.org/134883002/diff/160001/media/filters/chunk_dem...
media/filters/chunk_demuxer_unittest.cc:2652: TEST_F(ChunkDemuxerTest,
SetSequenceModeMidParse) {
nit: s/MidParse/MidMediaSegment/ here and the test above.

[wolenetz, 2014-01-11 02:15:05]

Thank you. I'll CQ this once Blink prerequisite is rolled
(https://src.chromium.org/viewvc/blink?view=rev&revision=164919)

https://codereview.chromium.org/134883002/diff/160001/content/renderer/media/...
File content/renderer/media/websourcebuffer_impl.cc (right):

https://codereview.chromium.org/134883002/diff/160001/content/renderer/media/...
content/renderer/media/websourcebuffer_impl.cc:49: return false;
On 2014/01/11 01:24:36, acolwell wrote:
> On 2014/01/11 00:55:44, wolenetz wrote:
> > I suspect we want to expose some exception if we reach here, so I added this
> to
> > trigger InvalidStateError exception throw by SourceBuffer::setMode().
> 
> No. Please remove this default section so a compiler error happens if the enum
> is changed. The Blink code should be protecting this code from any rogue
values.
> The enum is changed in the future the compiler error will for the person to
look
> at this code and update the logic appropriately before landing a Blink change
> that will send a new value here.

Done. Also done, per our offline chat, in WebMediaSourceImpl::markEndOfStream(),
which I was using as example.

https://codereview.chromium.org/134883002/diff/160001/media/filters/chunk_dem...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/134883002/diff/160001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:129: bool SetSequenceMode(bool is_sequence_mode);
On 2014/01/11 01:24:36, acolwell wrote:
> nit: drop the is_ since you are using sequence_mode_ in the class?

Done.

https://codereview.chromium.org/134883002/diff/160001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:430: bool SourceState::SetSequenceMode(bool
is_sequence_mode) {
On 2014/01/11 01:24:36, acolwell wrote:
> ditto

Done.

https://codereview.chromium.org/134883002/diff/160001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:1551: bool is_sequence_mode) {
On 2014/01/11 01:24:36, acolwell wrote:
> ditto

Done.

https://codereview.chromium.org/134883002/diff/160001/media/filters/chunk_dem...
File media/filters/chunk_demuxer_unittest.cc (right):

https://codereview.chromium.org/134883002/diff/160001/media/filters/chunk_dem...
media/filters/chunk_demuxer_unittest.cc:2652: TEST_F(ChunkDemuxerTest,
SetSequenceModeMidParse) {
On 2014/01/11 01:24:36, acolwell wrote:
> nit: s/MidParse/MidMediaSegment/ here and the test above.

Done.

[commit-bot: I haz the power, 2014-01-13 18:22:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wolenetz@chromium.org/134883002/260001

[commit-bot: I haz the power, 2014-01-14 00:34:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wolenetz@chromium.org/134883002/260001

[commit-bot: I haz the power, 2014-01-14 16:28:49]

Change committed as 244741