Bug 180643 - [MSE] SourceBuffer::provideMediaData() may remove sample from decode queue unexpectedly
Summary: [MSE] SourceBuffer::provideMediaData() may remove sample from decode queue un...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alicia Boya García
Keywords: InRadar
Depends on:
Reported: 2017-12-11 04:53 PST by Daniel Zhang
Modified: 2018-09-27 13:11 PDT (History)
4 users (show)

See Also:

Patch (10.77 KB, patch)
2018-09-27 09:02 PDT, Alicia Boya García
no flags Details | Formatted Diff | Diff
Patch (10.63 KB, patch)
2018-09-27 12:44 PDT, Alicia Boya García
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Zhang 2017-12-11 04:53:07 PST
I found in some cases, a sample already in trackBuffer.decodeQueue could be removed unexpectedly when there is gap ahead of it. For example, if samples of the following ranges are already in the trackBuffer.decodeQueue,

    [0 .. 5.0) [10.0, ... 30.0)

when SourceBuffer::provideMediaData() is called on such a decodeQueue, it may remove sample 10.0 (decode time) due to one second gap. Then, if the gap is filled (segment re-appended) later, it will enqueue samples from this decodeQueue but without sample 10.0.

The problem usually happens after playback for a while, e.g. buffered [10.0, ... 30.0), then seeking to the beginning. During re-appending the first segments, provideMediaData() may remove sample 10.0. As the removed sample is usually a sync sample, it results in playback issue if the enqueued samples are sent to decoder (not flushed).

I suggest to move the following removal code to be after the one-second gap checking:


How do you think? Thanks.

Best Regards,
Daniel ZHANG
Comment 1 Alicia Boya García 2018-02-09 13:12:45 PST
I think you are right.

The sample should only be removed from the decodeQueue after it's certain that it will be enqueued to the decoders. (Or if a the entire decodeQueue has to be flushed because of a seek).
Comment 2 Alicia Boya García 2018-09-27 09:02:55 PDT
Created attachment 350964 [details]
Comment 3 Alicia Boya García 2018-09-27 09:04:32 PDT
Better late than never, here is a patch including a test case.
Comment 4 Jer Noble 2018-09-27 12:29:00 PDT
Comment on attachment 350964 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=350964&action=review

> LayoutTests/media/media-source/media-source-append-acb-no-frame-lost.html:42
> +        let updateEnd = waitFor(sourceBuffer, 'updateend');
> +        run('sourceBuffer.appendBuffer(initSegment)');
> +        await updateEnd;

Just a little nit: because events will never be fired before the current run-loop yields, it's safe to restructure this to:

await waitFor(sourceBuffer, 'updateend');

(I wish we could go back in time and change appendBuffer(...) to return a Promise<void>.)
Comment 5 Alicia Boya García 2018-09-27 12:44:44 PDT
Created attachment 350986 [details]
Comment 6 WebKit Commit Bot 2018-09-27 13:10:35 PDT
Comment on attachment 350986 [details]

Clearing flags on attachment: 350986

Committed r236566: <https://trac.webkit.org/changeset/236566>
Comment 7 WebKit Commit Bot 2018-09-27 13:10:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2018-09-27 13:11:25 PDT