Bug 124422 - [MSE] Support fastSeek() in MediaSource.
Summary: [MSE] Support fastSeek() in MediaSource.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
Depends on:
Blocks: 123378
  Show dependency tree / graph
 
Reported: 2013-11-15 10:22 PST by Jer Noble
Modified: 2013-11-19 13:37 PST (History)
11 users (show)

See Also:

Attachments
Patch (60.56 KB, patch)
2013-11-15 14:49 PST, Jer Noble 		no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (461.28 KB, application/zip)
2013-11-15 15:48 PST, Build Bot 		no flags Details
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (847.05 KB, application/zip)
2013-11-15 16:21 PST, Build Bot 		no flags Details
Patch (61.21 KB, patch)
2013-11-17 15:01 PST, Jer Noble 		no flags Details | Formatted Diff | Diff
Patch (61.21 KB, patch)
2013-11-17 17:20 PST, Jer Noble 		no flags Details | Formatted Diff | Diff
Patch (61.20 KB, patch)
2013-11-17 17:26 PST, Jer Noble 		no flags Details | Formatted Diff | Diff
Patch (61.89 KB, patch)
2013-11-18 01:21 PST, Philippe Normand 		eric.carlson: review+
Details | Formatted Diff | Diff
Patch for landing. (62.06 KB, patch)
2013-11-18 14:44 PST, Jer Noble 		buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (449.00 KB, application/zip)
2013-11-18 17:18 PST, Build Bot 		no flags Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (501.28 KB, application/zip)
2013-11-18 17:21 PST, Build Bot 		no flags Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (490.54 KB, application/zip)
2013-11-18 17:51 PST, Build Bot 		no flags Details
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2013-11-15 10:22:15 PST 
[MSE] Support fastSeek() in MediaSource.
Comment 1 Jer Noble 2013-11-15 14:49:55 PST 
Created attachment 217088 [details]
Patch
Comment 2 WebKit Commit Bot 2013-11-15 14:52:27 PST 
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.
Comment 3 Build Bot 2013-11-15 15:48:06 PST 
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
Comment 4 Build Bot 2013-11-15 15:48:08 PST 
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
Comment 5 Build Bot 2013-11-15 16:21:04 PST 
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
Comment 6 Build Bot 2013-11-15 16:21:07 PST 
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
Comment 7 Jer Noble 2013-11-17 15:01:13 PST 
Created attachment 217154 [details]
Patch
Comment 8 WebKit Commit Bot 2013-11-17 15:02:47 PST 
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.
Comment 9 kov's GTK+ EWS bot 2013-11-17 16:17:43 PST 
Comment on attachment 217154 [details]
Patch

Attachment 217154 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/25638068
Comment 10 Jer Noble 2013-11-17 17:20:50 PST 
Created attachment 217161 [details]
Patch
Comment 11 Jer Noble 2013-11-17 17:26:26 PST 
Created attachment 217163 [details]
Patch

Add default implementations to new SourceBufferPrivate virtual methods in order to not break the GStreamer implementation.
Comment 12 WebKit Commit Bot 2013-11-17 17:28:24 PST 
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.
Comment 13 kov's GTK+ EWS bot 2013-11-17 17:40:17 PST 
Comment on attachment 217163 [details]
Patch

Attachment 217163 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/27228057
Comment 14 Philippe Normand 2013-11-18 01:21:55 PST 
Created attachment 217177 [details]
Patch

GTK build fix
Comment 15 Philippe Normand 2013-11-18 01:23:05 PST 
Oh webkit-patch obsoleted Jer's patch, sorry about that
Comment 16 WebKit Commit Bot 2013-11-18 01:24:43 PST 
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.
Comment 17 Eric Carlson 2013-11-18 10:55:23 PST 
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).
Comment 18 Jer Noble 2013-11-18 11:44:08 PST 
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.
Comment 19 Jer Noble 2013-11-18 14:44:10 PST 
Created attachment 217237 [details]
Patch for landing.
Comment 20 WebKit Commit Bot 2013-11-18 15:22:10 PST 
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.
Comment 21 Build Bot 2013-11-18 17:18:20 PST 
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
Comment 22 Build Bot 2013-11-18 17:18:22 PST 
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
Comment 23 Build Bot 2013-11-18 17:21:14 PST 
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
Comment 24 Build Bot 2013-11-18 17:21:17 PST 
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
Comment 25 Build Bot 2013-11-18 17:51:46 PST 
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
Comment 26 Build Bot 2013-11-18 17:51:50 PST 
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
Comment 27 Jer Noble 2013-11-19 13:37:41 PST 
Committed r159519: <http://trac.webkit.org/changeset/159519>