[webkit-reviews] review granted: [Bug 124422] [MSE] Support fastSeek() in MediaSource. : [Attachment 217177] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 124422: [MSE] Support fastSeek() in MediaSource.
https://bugs.webkit.org/show_bug.cgi?id=124422

Attachment 217177: Patch
https://bugs.webkit.org/attachment.cgi?id=217177&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
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).