Bug 124422 – [MSE] Support fastSeek() in MediaSource.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

[MSE] Support fastSeek() in MediaSource.

[Jer Noble]

Created attachment 217088 [details]
Patch

[WebKit Commit Bot]

Attachment 217088 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/media-source/media-source-fastseek-expected.txt', u'LayoutTests/media/media-source/media-source-fastseek.html', u'LayoutTests/media/media-source/mock-media-source.js', u'Source/WebCore/ChangeLog', u'Source/WebCore/Modules/mediasource/MediaSource.cpp', u'Source/WebCore/Modules/mediasource/MediaSource.h', u'Source/WebCore/Modules/mediasource/SampleMap.cpp', u'Source/WebCore/Modules/mediasource/SampleMap.h', u'Source/WebCore/Modules/mediasource/SourceBuffer.cpp', u'Source/WebCore/Modules/mediasource/SourceBuffer.h', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/HTMLMediaSource.h', u'Source/WebCore/platform/MediaSample.h', u'Source/WebCore/platform/graphics/SourceBufferPrivate.h', u'Source/WebCore/platform/graphics/SourceBufferPrivateClient.h', u'Source/WebCore/platform/mock/mediasource/MockMediaPlayerMediaSource.cpp', u'Source/WebCore/platform/mock/mediasource/MockMediaPlayerMediaSource.h', u'Source/WebCore/platform/mock/mediasource/MockMediaSourcePrivate.cpp', u'Source/WebCore/platform/mock/mediasource/MockMediaSourcePrivate.h', u'Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.cpp', u'Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.h']" exit_code: 1
Source/WebCore/Modules/mediasource/SampleMap.h:43:  reverse_iterator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 217088 [details]
Patch

Attachment 217088 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/24448006

New failing tests:
media/media-source/media-source-play.html

[Build Bot]

Created attachment 217093 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 217088 [details]
Patch

Attachment 217088 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/23658247

New failing tests:
media/media-source/media-source-play.html

[Build Bot]

Created attachment 217095 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Jer Noble]

Created attachment 217154 [details]
Patch

[WebKit Commit Bot]

Attachment 217154 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/media-source/media-source-fastseek-expected.txt', u'LayoutTests/media/media-source/media-source-fastseek.html', u'LayoutTests/media/media-source/mock-media-source.js', u'Source/WebCore/ChangeLog', u'Source/WebCore/Modules/mediasource/MediaSource.cpp', u'Source/WebCore/Modules/mediasource/MediaSource.h', u'Source/WebCore/Modules/mediasource/SampleMap.cpp', u'Source/WebCore/Modules/mediasource/SampleMap.h', u'Source/WebCore/Modules/mediasource/SourceBuffer.cpp', u'Source/WebCore/Modules/mediasource/SourceBuffer.h', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/HTMLMediaSource.h', u'Source/WebCore/platform/MediaSample.h', u'Source/WebCore/platform/graphics/SourceBufferPrivate.h', u'Source/WebCore/platform/graphics/SourceBufferPrivateClient.h', u'Source/WebCore/platform/mock/mediasource/MockMediaPlayerMediaSource.cpp', u'Source/WebCore/platform/mock/mediasource/MockMediaPlayerMediaSource.h', u'Source/WebCore/platform/mock/mediasource/MockMediaSourcePrivate.cpp', u'Source/WebCore/platform/mock/mediasource/MockMediaSourcePrivate.h', u'Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.cpp', u'Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.h']" exit_code: 1
Source/WebCore/Modules/mediasource/SampleMap.h:43:  reverse_iterator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[kov's GTK+ EWS bot]

Comment on attachment 217154 [details]
Patch

Attachment 217154 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/25638068

[Jer Noble]

Created attachment 217161 [details]
Patch

[Jer Noble]

Created attachment 217163 [details]
Patch

Add default implementations to new SourceBufferPrivate virtual methods in order to not break the GStreamer implementation.

[WebKit Commit Bot]

Attachment 217163 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/media-source/media-source-fastseek-expected.txt', u'LayoutTests/media/media-source/media-source-fastseek.html', u'LayoutTests/media/media-source/mock-media-source.js', u'Source/WebCore/ChangeLog', u'Source/WebCore/Modules/mediasource/MediaSource.cpp', u'Source/WebCore/Modules/mediasource/MediaSource.h', u'Source/WebCore/Modules/mediasource/SampleMap.cpp', u'Source/WebCore/Modules/mediasource/SampleMap.h', u'Source/WebCore/Modules/mediasource/SourceBuffer.cpp', u'Source/WebCore/Modules/mediasource/SourceBuffer.h', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/HTMLMediaSource.h', u'Source/WebCore/platform/MediaSample.h', u'Source/WebCore/platform/graphics/SourceBufferPrivate.h', u'Source/WebCore/platform/graphics/SourceBufferPrivateClient.h', u'Source/WebCore/platform/mock/mediasource/MockMediaPlayerMediaSource.cpp', u'Source/WebCore/platform/mock/mediasource/MockMediaPlayerMediaSource.h', u'Source/WebCore/platform/mock/mediasource/MockMediaSourcePrivate.cpp', u'Source/WebCore/platform/mock/mediasource/MockMediaSourcePrivate.h', u'Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.cpp', u'Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.h']" exit_code: 1
Source/WebCore/Modules/mediasource/SampleMap.h:43:  reverse_iterator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[kov's GTK+ EWS bot]

Comment on attachment 217163 [details]
Patch

Attachment 217163 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/27228057

[Philippe Normand]

Created attachment 217177 [details]
Patch

GTK build fix

[Philippe Normand]

Oh webkit-patch obsoleted Jer's patch, sorry about that

[WebKit Commit Bot]

Attachment 217177 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/media-source/media-source-fastseek-expected.txt', u'LayoutTests/media/media-source/media-source-fastseek.html', u'LayoutTests/media/media-source/mock-media-source.js', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Modules/mediasource/MediaSource.cpp', u'Source/WebCore/Modules/mediasource/MediaSource.h', u'Source/WebCore/Modules/mediasource/SampleMap.cpp', u'Source/WebCore/Modules/mediasource/SampleMap.h', u'Source/WebCore/Modules/mediasource/SourceBuffer.cpp', u'Source/WebCore/Modules/mediasource/SourceBuffer.h', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/HTMLMediaSource.h', u'Source/WebCore/platform/MediaSample.h', u'Source/WebCore/platform/graphics/SourceBufferPrivate.h', u'Source/WebCore/platform/graphics/SourceBufferPrivateClient.h', u'Source/WebCore/platform/mock/mediasource/MockMediaPlayerMediaSource.cpp', u'Source/WebCore/platform/mock/mediasource/MockMediaPlayerMediaSource.h', u'Source/WebCore/platform/mock/mediasource/MockMediaSourcePrivate.cpp', u'Source/WebCore/platform/mock/mediasource/MockMediaSourcePrivate.h', u'Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.cpp', u'Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.h']" exit_code: 1
Source/WebCore/Modules/mediasource/SampleMap.h:43:  reverse_iterator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Comment on attachment 217177 [details]
Patch

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

> Source/WebCore/Modules/mediasource/SampleMap.cpp:125
> +    return --reverse_iterator(findSampleWithDecodeTime(time));

Is it possible for reverse_iterator to return the first item? If so, what will "--" do?

> Source/WebCore/Modules/mediasource/SampleMap.cpp:136
> +    ASSERT(reverseDecodeEnd() == reverseDecodeEnd());

???

> Source/WebCore/Modules/mediasource/SampleMap.cpp:157
> +    ASSERT(decodeEnd() == decodeEnd());

Huh?

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:258
> +        auto currentSamplePTSIter = trackBuffer.samples.findSampleContainingPresentationTime(time);

Nit: why don't you burn a few bytes and spell out "Iterator".

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:269
> +        auto reverseCurrentSampleIter = --SampleMap::reverse_iterator(currentSampleDTSIter);

Ditto the above question about "--".

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:304
> +        auto futureSyncSampleIter = trackBuffer.samples.findSyncSampleAfterPresentationTime(targetTime, positiveThreshold);
> +        auto pastSyncSampleIter = trackBuffer.samples.findSyncSamplePriorToPresentationTime(targetTime, negativeThreshold);

Nit: "Iter" -> "Iterator"

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:314
> +            futureSeekTime = sample->presentationTime() + millisecond;

Nit: a comment about the magic 1 ms value would be helpful.

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:872
>          MediaTime microsecond(1, 1000000);

Why use a microsecond here and a millisecond above?

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:944
> +

Nit: extra blank line.

> Source/WebCore/platform/mock/mediasource/MockMediaSourcePrivate.cpp:148
> +

Nit: extra blank line.

> LayoutTests/media/media-source/media-source-fastseek.html:23
> +        var videoSource = document.createElement('source')

Nit: missing semi-colon.

> LayoutTests/media/media-source/media-source-fastseek.html:62
> +    function seeked1() {
> +        waitForEventOnce('seeked', seeked2);
> +        run('video.fastSeek(2)');
> +    }

Nit: is it worth checking the time to make sure 'seeked' is fired at the appropriate time (here and the other seeked methods)?

> LayoutTests/media/media-source/mock-media-source.js:36
> +function concatenateSamples(samples) {

Nit: a function's opening brace should be on a new line (here and the rest of the file).

[Jer Noble]

Comment on attachment 217177 [details]
Patch

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

>> Source/WebCore/Modules/mediasource/SampleMap.cpp:125
>> +    return --reverse_iterator(findSampleWithDecodeTime(time));
> 
> Is it possible for reverse_iterator to return the first item? If so, what will "--" do?

It is.  We need to check the return of findSampleWithDecodeTime, and return rend() if it returns end().

>> Source/WebCore/Modules/mediasource/SampleMap.cpp:136
>> +    ASSERT(reverseDecodeEnd() == reverseDecodeEnd());
> 
> ???

I'll remove this.

>> Source/WebCore/Modules/mediasource/SampleMap.cpp:157
>> +    ASSERT(decodeEnd() == decodeEnd());
> 
> Huh?

Ditto.

>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:269
>> +        auto reverseCurrentSampleIter = --SampleMap::reverse_iterator(currentSampleDTSIter);
> 
> Ditto the above question about "--".

This should always be safe, because the same sample will always be in both maps, the decode map and the presentation map.  I'll add an ASSERT here to make this explicit.

>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:872
>>          MediaTime microsecond(1, 1000000);
> 
> Why use a microsecond here and a millisecond above?

"Microsecond" is in the spec.  See line 897 (and 1.14.2.2 of the MSE spec).

>> LayoutTests/media/media-source/media-source-fastseek.html:62
>> +    }
> 
> Nit: is it worth checking the time to make sure 'seeked' is fired at the appropriate time (here and the other seeked methods)?

sure.

[Jer Noble]

Created attachment 217237 [details]
Patch for landing.

[WebKit Commit Bot]

Attachment 217237 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/media-source/media-source-fastseek-expected.txt', u'LayoutTests/media/media-source/media-source-fastseek.html', u'LayoutTests/media/media-source/mock-media-source.js', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Modules/mediasource/MediaSource.cpp', u'Source/WebCore/Modules/mediasource/MediaSource.h', u'Source/WebCore/Modules/mediasource/SampleMap.cpp', u'Source/WebCore/Modules/mediasource/SampleMap.h', u'Source/WebCore/Modules/mediasource/SourceBuffer.cpp', u'Source/WebCore/Modules/mediasource/SourceBuffer.h', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/HTMLMediaSource.h', u'Source/WebCore/platform/MediaSample.h', u'Source/WebCore/platform/graphics/SourceBufferPrivate.h', u'Source/WebCore/platform/graphics/SourceBufferPrivateClient.h', u'Source/WebCore/platform/mock/mediasource/MockMediaPlayerMediaSource.cpp', u'Source/WebCore/platform/mock/mediasource/MockMediaPlayerMediaSource.h', u'Source/WebCore/platform/mock/mediasource/MockMediaSourcePrivate.cpp', u'Source/WebCore/platform/mock/mediasource/MockMediaSourcePrivate.h', u'Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.cpp', u'Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.h']" exit_code: 1
Source/WebCore/Modules/mediasource/SampleMap.h:43:  reverse_iterator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 217237 [details]
Patch for landing.

Attachment 217237 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/24088030

New failing tests:
media/media-source/media-source-fastseek.html

[Build Bot]

Created attachment 217255 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 217237 [details]
Patch for landing.

Attachment 217237 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/23438198

New failing tests:
media/media-source/media-source-fastseek.html

[Build Bot]

Created attachment 217256 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 217237 [details]
Patch for landing.

Attachment 217237 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/28078030

New failing tests:
media/media-source/media-source-fastseek.html

[Build Bot]

Created attachment 217259 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Jer Noble]

Committed r159519: <http://trac.webkit.org/changeset/159519>