New issue
Advanced search Search tips

Issue 616565 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

'sequence' appendMode and remove() calls can spuriously fail subsequent appends

Project Member Reported by wolenetz@chromium.org, Jun 1 2016

Issue description

Repro (we have a private report with more details):
1. Set 'sequence' appendMode on a single mp3 audio-track SourceBuffer.
2. Append some audio, say [0,60)
3. sourceBuffer.abort()
4. Remove [40,sourceBuffer.buffered.end(sourceBuffer.buffered.length - 1))
5. sourceBuffer.abort()
6. Set timeStampOffset = 40
7. Remove [0,sourceBuffer.buffered.end(sourceBuffer.buffered.length - 1))
8. Set timeStampOffset = 0
9. Attempt to re-append the same audio as in step 2.

Expected:
No decode error

Actual:
Decode error from SourceBufferStream due to "Buffers did not monotonically increase).

Note:
If just step #1 is skipped, no repro. This uses default 'segments' appendMode (with current Chromium workaround that emulates 'sequence' appendMode in the engine for mp3).

Note:
If just step #4 is skipped, no repro.
 
Step #5 isn't necessary to either the repro or normal function.

I've narrowed the reason for difference between inclusion (repros) or exclusion (no repro) of step #4 down to SourceBufferStream's |last_appended_buffer_timestamp_| is *not* reset to kNoDecodeTimestamp() in the former case, but it is in the latter. Since it's not reset, the next append sees time going backwards and the error occurs. This code is quite intricate; I'll see about making a fix soon.
And when in 'segments' mode, discontinuity is detected and signalled by the frame processor, side-stepping the issue encountered in 'sequence' mode later in the frames' append processing. Regardless, if last_appended_buffer is removed, then last_appended_buffer_timestamp_ should be reset IIUC.
Hmm. Digging a little further, I believe I found a related 'sequence' mode issue: NotifyStartOfCodedFrameGroup() isn't called in sequence mode, even if there is an associated discontinuity. Possible example:
1. Set 'sequence' AppendMode.
2. Set 'timestampOffset' to 100.
3. Append buffers [100,200) (originally with DTS [0,100))
4. Set 'timestampOffset' to 0.
5. Append buffers [0,10) (originally with DTS [0,10)).

Somewhere betweeen steps 3 and 5, SourceBufferStream needs to know that a new coded frame group is starting. Otherwise, a similar enforcement failure as that in the original repro in this bug will occur (also within the IsMonotonicallyIncreasing within this coded frame group helper method). I haven't tried this second flavor of repro, but I suspect it will also need to be fixed.
Labels: -Pri-1 Pri-2
I found a workaround for this issue, enabling slight deprioritization:

To forcibly indicate to Chromium that it needs to treat a 'sequence' mode append as beginning a new coded frame group:
a) following a prior remove()'s "updateend" such as between steps 4 and 6, and between steps 7 and 8 in comment #1), or
b) if the scenario in comment #3 is also causing trouble, between its steps 3 and 4:

sourceBuffer.mode='segments';
sourceBuffer.abort(); // Chromium's abort() processing in 'segments' mode forces the coded frame processor to begin a new coded frame group.
sourceBuffer.mode='sequence';

For those hitting this issue, please make comment here, especially if the workaround is incomplete or unfeasible for your scenario.
Indeed the scenario in comment #3 leads at least to a DCHECK failure in debug builds (with, for mp3 mimetype, also including an abort() after step 3:

FATAL:source_buffer_stream.cc(234)] Check failed: coded_frame_group_start_time_ <= buffers.front()->GetDecodeTimestamp().

The workaround suggested in comment #4 fixes the scenario in comment #3, too. Specifically, the additional steps needed to make comment #3 work in Chromium currently are:
3.1 Upon 'updateend', sourceBuffer.abort();
3.2 sourceBuffer.mode = 'segments';
3.3 sourceBuffer.abort(); // Yes, again.
3.4 sourceBuffer.mode = 'sequence';

I have the beginnings of a potential fix for this out for review at https://codereview.chromium.org/2035003002/
Project Member

Comment 7 by bugdroid1@chromium.org, Jun 16 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b7982341ce30c1dfb165551b8c641d781eeb0159

commit b7982341ce30c1dfb165551b8c641d781eeb0159
Author: wolenetz <wolenetz@chromium.org>
Date: Thu Jun 16 20:23:40 2016

MSE: Signal new coded frame group more aggressively

For 'sequence' mode coded frame processing, we may need to signal a new
coded frame group because application may override timestampOffset and
cause the processed frame sequence to go backwards in time. Jumps
forward are allowed and kept within the same coded frame group. Jumps
backward are now detected and trigger new coded frame group signalling
so the overlap-append logic in SourceBufferStream handles this situation
more correctly.

TEST=*FrameProcessorTest.AudioVideo_Discontinuity_TimestampOffset*, http/tests/media/media-source/mediasource-crbug-616565.html
BUG= 616565 , 620523 

Review-Url: https://codereview.chromium.org/2035003002
Cr-Commit-Position: refs/heads/master@{#400249}

[modify] https://crrev.com/b7982341ce30c1dfb165551b8c641d781eeb0159/media/filters/frame_processor.cc
[modify] https://crrev.com/b7982341ce30c1dfb165551b8c641d781eeb0159/media/filters/frame_processor.h
[modify] https://crrev.com/b7982341ce30c1dfb165551b8c641d781eeb0159/media/filters/frame_processor_unittest.cc
[add] https://crrev.com/b7982341ce30c1dfb165551b8c641d781eeb0159/third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-sequencemode-crbug-616565.html

Status: Fixed (was: Started)

Sign in to add a comment