Fix possible buffer emission during an abort.

Fix possible buffer emission during an abort.

From the MSE spec - Reset Parser state:
If the append state equals PARSING_MEDIA_SEGMENT and
the input buffer contains some complete coded frames,
then run the coded frame processing algorithm until
all of these complete coded frames have been processed.

BUG=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268529

[damienv1, 2014-04-25 02:03:22]

I don't know what is the expectation if the JS app is doing:
1- append()
2- set timestamp offset
3- abort

If there are some pending buffers, should ChunkDemuxer use the timestamp offset
that was used during append ? or use the timestamp offset provided in 2)
I implemented the second approach but I realize that it might make more sense to
use the timeoffset that was used during the append (the pending buffers are
logically part of the previous append).

[wolenetz, 2014-04-25 20:56:59]

On 2014/04/25 02:03:22, damienv1 wrote:
> I don't know what is the expectation if the JS app is doing:
> 1- append()
> 2- set timestamp offset
> 3- abort
> 
> If there are some pending buffers, should ChunkDemuxer use the timestamp
offset
> that was used during append ? or use the timestamp offset provided in 2)
> I implemented the second approach but I realize that it might make more sense
to
> use the timeoffset that was used during the append (the pending buffers are
> logically part of the previous append).

Thankfully, the spec prevents concurrent asynchronous append processing and js
setting timestampOffset:
https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.....
Your #2 should throw JS INVALID_STATE_ERR if async update hasn't completed yet
(== if 'updating' is true).

[wolenetz, 2014-04-25 21:00:32]

Thank you for sharing this change out. A few comments from my first pass are
below. Also, I would like to see a test that verifies abort() when parser hasn't
yet emitted all buffers causes the parser to emit them. Is this possible to
deterministically test for at least the mp2ts stream parser? Bonus points for
including verification that append_window_{start,end} are verified also to
constrain abort()'s induced coded frame processing.

https://codereview.chromium.org/256583006/diff/1/content/renderer/media/webso...
File content/renderer/media/websourcebuffer_impl.cc (right):

https://codereview.chromium.org/256583006/diff/1/content/renderer/media/webso...
content/renderer/media/websourcebuffer_impl.cc:91: demuxer_->Abort(id_,
timestamp_offset_);
append_window_{start,end}_ should also be part of processing any buffers the
parser emits as a result of abort() flushing the parser. For example, if an
emitted buffer is outside the append window, then it should not be buffered (and
a discontinuity should be triggered in case subsequent buffers fall within the
append window).
Also, |timestamp_offset_| might be updated by the coded frame processing
algorithm's processing of buffers emitted by the abort(). Pass it by reference
here?

https://codereview.chromium.org/256583006/diff/1/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/256583006/diff/1/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:667: DCHECK(timestamp_offset_during_append_);
Now would be a good time to add DCHECK(parsing_media_segment_) here to ensure
correct stream parser behavior: append and abort should only induce stream
parser to emit buffers iff OnNewMediaSegment() has been called most recently
(i.e., in PARSING_MEDIA_SEGMENT sourcebuffer state). If stream parsers violate
this, they need to be fixed.

https://codereview.chromium.org/256583006/diff/1/media/filters/pipeline_integ...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/256583006/diff/1/media/filters/pipeline_integ...
media/filters/pipeline_integration_test.cc:325: base::TimeDelta
timestamp_offset;
Use |last_timestamp_offset_|, passed by reference, and test that it is
updated/used correctly (along with append_window start/end) by Abort()?

[damienv1, 2014-05-05 22:39:12]

Two remarks:
- the unit test won't be enabled by default (as the Mpeg2 TS stream parser is
not enabled in the StreamParserFactory).
- I'll try to come up with a test to also test the window append range.

https://codereview.chromium.org/256583006/diff/1/content/renderer/media/webso...
File content/renderer/media/websourcebuffer_impl.cc (right):

https://codereview.chromium.org/256583006/diff/1/content/renderer/media/webso...
content/renderer/media/websourcebuffer_impl.cc:91: demuxer_->Abort(id_,
timestamp_offset_);
On 2014/04/25 21:00:32, wolenetz wrote:
> append_window_{start,end}_ should also be part of processing any buffers the
> parser emits as a result of abort() flushing the parser. For example, if an
> emitted buffer is outside the append window, then it should not be buffered
(and
> a discontinuity should be triggered in case subsequent buffers fall within the
> append window).
> Also, |timestamp_offset_| might be updated by the coded frame processing
> algorithm's processing of buffers emitted by the abort(). Pass it by reference
> here?

Done. Somehow, I was thinking that the timestamp offset was reset after an abort
(that's why I passed it by value).

https://codereview.chromium.org/256583006/diff/1/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/256583006/diff/1/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:667: DCHECK(timestamp_offset_during_append_);
On 2014/04/25 21:00:32, wolenetz wrote:
> Now would be a good time to add DCHECK(parsing_media_segment_) here to ensure
> correct stream parser behavior: append and abort should only induce stream
> parser to emit buffers iff OnNewMediaSegment() has been called most recently
> (i.e., in PARSING_MEDIA_SEGMENT sourcebuffer state). If stream parsers violate
> this, they need to be fixed.

Done.

https://codereview.chromium.org/256583006/diff/1/media/filters/pipeline_integ...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/256583006/diff/1/media/filters/pipeline_integ...
media/filters/pipeline_integration_test.cc:325: base::TimeDelta
timestamp_offset;
On 2014/04/25 21:00:32, wolenetz wrote:
> Use |last_timestamp_offset_|, passed by reference, and test that it is
> updated/used correctly (along with append_window start/end) by Abort()?

Updated to use last_timestamp_offset_.
However, MockMediaSource::AppendData does not seem correct in that case: it does
not use the |last_timestamp_offset_|. So 2 consecutive AppendData would possibly
not give the correct result.

[damienv1, 2014-05-05 23:34:06]

Using the append window does not work with the Mpeg2 TS parser, since the Mpeg2
TS parser triggers several calls to OnNewBuffers. This is fine in the regular
case. However, when the append window is used, some buffers might be discarded
and then the next call to OnNewBuffers expect the first buffer to be a key frame
which is not necessarily the case.

I would rather address this issue in a different CL.

[wolenetz, 2014-05-06 00:04:32]

acolwell@: Please especially review the media.gyp change.
damienv1@: I'm unclear if your message about append windows indicates there is a
problem with previous implementation (outside of this abort() CL), or if it is
specific to abort(). Please clarify.
Otherwise, looking pretty good:

https://codereview.chromium.org/256583006/diff/10001/content/renderer/media/w...
File content/renderer/media/websourcebuffer_impl.cc (right):

https://codereview.chromium.org/256583006/diff/10001/content/renderer/media/w...
content/renderer/media/websourcebuffer_impl.cc:90: void
WebSourceBufferImpl::abort() {
Upon further inspection of the MSE spec, and given the abort() for at least
mp2ts streams could cause further buffers to be processed by the coded frame
processing algorithm, it looks like the Blink interface for WSB::abort() needs
to pass a double* timestamp_offset, and we need to (conditionally) update it
only if we get a non-NULL ptr from Blink AND the abort() changed the TimeDelta
version of it. See ::append(), above, for a clear example of what I mean. Since
coded frame processing can change the timestamp offset, and since everything
below WSBI uses TimeDeltas (lower precision than Blink's double), we need to do
this. I've filed crbug.com/370229 accordingly. Please add a TODO(wolenetz) for
this here, referencing that bug.

https://codereview.chromium.org/256583006/diff/10001/media/filters/chunk_demu...
File media/filters/chunk_demuxer_unittest.cc (right):

https://codereview.chromium.org/256583006/diff/10001/media/filters/chunk_demu...
media/filters/chunk_demuxer_unittest.cc:2754:
EXPECT_EQ(AddIdForMp2tSource(kSourceId), ChunkDemuxer::kOk);
nit: swap parameters to EXPECT_EQ please

https://codereview.chromium.org/256583006/diff/10001/media/filters/pipeline_i...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/256583006/diff/10001/media/filters/pipeline_i...
media/filters/pipeline_integration_test.cc:326: chunk_demuxer_->Abort(
nit: add similar TODO(wolenetz): referencing http://crbug.com/370229 here.

[damienv1, 2014-05-06 00:28:59]

The append window issue is not related to the abort() issue.
In the Mpeg2 TS parser, there is no guarantee OnNewBuffers will be called on a
key frame boundary: e.g. since OnNewBuffers is called with a pair of list of
audio *and* video buffers, an audio config change will trigger a new call to
OnNewBuffers regardless of whether the next video frame is a key frame (you
don't have the same issue with un-muxed streams).

https://codereview.chromium.org/256583006/diff/10001/content/renderer/media/w...
File content/renderer/media/websourcebuffer_impl.cc (right):

https://codereview.chromium.org/256583006/diff/10001/content/renderer/media/w...
content/renderer/media/websourcebuffer_impl.cc:90: void
WebSourceBufferImpl::abort() {
On 2014/05/06 00:04:32, wolenetz wrote:
> Upon further inspection of the MSE spec, and given the abort() for at least
> mp2ts streams could cause further buffers to be processed by the coded frame
> processing algorithm, it looks like the Blink interface for WSB::abort() needs
> to pass a double* timestamp_offset, and we need to (conditionally) update it
> only if we get a non-NULL ptr from Blink AND the abort() changed the TimeDelta
> version of it. See ::append(), above, for a clear example of what I mean.
Since
> coded frame processing can change the timestamp offset, and since everything
> below WSBI uses TimeDeltas (lower precision than Blink's double), we need to
do
> this. I've filed crbug.com/370229 accordingly. Please add a TODO(wolenetz) for
> this here, referencing that bug.

Added a comment which references this bug.

https://codereview.chromium.org/256583006/diff/10001/media/filters/chunk_demu...
File media/filters/chunk_demuxer_unittest.cc (right):

https://codereview.chromium.org/256583006/diff/10001/media/filters/chunk_demu...
media/filters/chunk_demuxer_unittest.cc:2754:
EXPECT_EQ(AddIdForMp2tSource(kSourceId), ChunkDemuxer::kOk);
On 2014/05/06 00:04:32, wolenetz wrote:
> nit: swap parameters to EXPECT_EQ please

Done.

https://codereview.chromium.org/256583006/diff/10001/media/filters/pipeline_i...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/256583006/diff/10001/media/filters/pipeline_i...
media/filters/pipeline_integration_test.cc:326: chunk_demuxer_->Abort(
On 2014/05/06 00:04:32, wolenetz wrote:
> nit: add similar TODO(wolenetz): referencing http://crbug.com/370229 here.

Why ? Doesn't seem to be the same issue.
Are you rather referring to http://crbug.com/249422 instead ?

[wolenetz, 2014-05-06 00:32:05]

https://codereview.chromium.org/256583006/diff/10001/media/filters/pipeline_i...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/256583006/diff/10001/media/filters/pipeline_i...
media/filters/pipeline_integration_test.cc:326: chunk_demuxer_->Abort(
On 2014/05/06 00:29:00, damienv1 wrote:
> On 2014/05/06 00:04:32, wolenetz wrote:
> > nit: add similar TODO(wolenetz): referencing http://crbug.com/370229 here.
> 
> Why ? Doesn't seem to be the same issue.
> Are you rather referring to http://crbug.com/249422 instead ?

You're right. Use 249422 please.

[wolenetz, 2014-05-06 00:44:28]

On 2014/05/06 00:32:05, wolenetz wrote:
>
https://codereview.chromium.org/256583006/diff/10001/media/filters/pipeline_i...
> File media/filters/pipeline_integration_test.cc (right):
> 
>
https://codereview.chromium.org/256583006/diff/10001/media/filters/pipeline_i...
> media/filters/pipeline_integration_test.cc:326: chunk_demuxer_->Abort(
> On 2014/05/06 00:29:00, damienv1 wrote:
> > On 2014/05/06 00:04:32, wolenetz wrote:
> > > nit: add similar TODO(wolenetz): referencing http://crbug.com/370229 here.
> > 
> > Why ? Doesn't seem to be the same issue.
> > Are you rather referring to http://crbug.com/249422 instead ?
> 
> You're right. Use 249422 please.

Also, as I mentioned in chat, the append window testing can be a separate CL
(probably best after crbug.com/249422 is fixed). It is expected that
non-keyframes will be dropped once a discontinuity is detected in a particular
track, on a per track basis, but the current legacy_frame_processor is incorrect
especially when processing frames from a mux'ed stream that either contains such
discontinuities or has them introduced by append_window clipping.

[wolenetz, 2014-05-06 00:58:10]

lgtm % nits (I'm sorry, I think we didn't understand each other for the portion
of our chat related to these nits.)

https://codereview.chromium.org/256583006/diff/30001/media/filters/pipeline_i...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/256583006/diff/30001/media/filters/pipeline_i...
media/filters/pipeline_integration_test.cc:330: base::TimeDelta(),
kInfiniteDuration(), &timestamp_offset);
nit: Let's use &last_timestamp_offset_ here and drop the local
|timestamp_offset|. Abort during an mpeg2ts seek could emit frames that need
processing, and such processing uses this timestamp offset as input even now
(though I don't see any such mpeg2ts unit test seeking at the moment).

https://codereview.chromium.org/256583006/diff/30001/media/filters/pipeline_i...
media/filters/pipeline_integration_test.cc:350: base::TimeDelta(),
kInfiniteDuration(), &timestamp_offset);
nit: ditto. Similarly, there are not two AppendData() in a row currently that
use a non-zero timestamp offset (there is AppendData followed by AppendAtTime in
a couple test cases), but let's not confuse future test writers :)

[damienv1, 2014-05-06 01:21:43]

https://codereview.chromium.org/256583006/diff/30001/media/filters/pipeline_i...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/256583006/diff/30001/media/filters/pipeline_i...
media/filters/pipeline_integration_test.cc:330: base::TimeDelta(),
kInfiniteDuration(), &timestamp_offset);
On 2014/05/06 00:58:11, wolenetz wrote:
> nit: Let's use &last_timestamp_offset_ here and drop the local
> |timestamp_offset|. Abort during an mpeg2ts seek could emit frames that need
> processing, and such processing uses this timestamp offset as input even now
> (though I don't see any such mpeg2ts unit test seeking at the moment).

Done.

https://codereview.chromium.org/256583006/diff/30001/media/filters/pipeline_i...
media/filters/pipeline_integration_test.cc:350: base::TimeDelta(),
kInfiniteDuration(), &timestamp_offset);
On 2014/05/06 00:58:11, wolenetz wrote:
> nit: ditto. Similarly, there are not two AppendData() in a row currently that
> use a non-zero timestamp offset (there is AppendData followed by AppendAtTime
in
> a couple test cases), but let's not confuse future test writers :)

Done.

[acolwell GONE FROM CHROMIUM, 2014-05-06 01:43:09]

lgtm

[damienv1, 2014-05-06 16:06:58]

The CQ bit was checked by damienv@chromium.org

[commit-bot: I haz the power, 2014-05-06 16:07:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/damienv@chromium.org/256583006/50001

[commit-bot: I haz the power, 2014-05-06 16:21:19]

Change committed as 268529