[webkit-reviews] review granted: [Bug 227258] [Gstreamer] timeouts in media/media-source/media-source-has-audio-video.html and media/media-source/media-source-seek-unbuffered.html : [Attachment 436892] Patch

[bugzilla-daemon at webkit.org]

Alicia Boya García <aboya at igalia.com> has granted Enrique Ocaña
<eocanha at igalia.com>'s request for review:
Bug 227258: [Gstreamer] timeouts in
media/media-source/media-source-has-audio-video.html and
media/media-source/media-source-seek-unbuffered.html
https://bugs.webkit.org/show_bug.cgi?id=227258

Attachment 436892: Patch

https://bugs.webkit.org/attachment.cgi?id=436892&action=review




--- Comment #17 from Alicia Boya García <aboya at igalia.com> ---
Comment on attachment 436892
  --> https://bugs.webkit.org/attachment.cgi?id=436892
Patch

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

Other than the commented, LGTM.

> LayoutTests/media/media-source/media-source-seek-unbuffered.html:53
>	   run('sourceBuffer.appendBuffer(loader.mediaSegment(0))');
> +
> +	   // Silently append a second segment after the first one. This
unblocks the seek on glib ports,
> +	   // where the h264 decoder needs more than one second of data to
start producing output.
> +	   sourceBuffer.addEventListener('update', function secondAppend() {
> +	       sourceBuffer.removeEventListener('update', secondAppend, true);
> +	       sourceBuffer.appendBuffer(loader.mediaSegment(1));
> +	   }, true);

Instead of adding them separately, I would just combine them in the same append
for simplicity. Unfortunately there is no built-in function to concatenate
array buffers, but you can add one, as it's done in another test
(media/media-source/media-source-error-crash.html):

function concatArrayBuffers(buffer1, buffer2) {
  var view = new Uint8Array(buffer1.byteLength + buffer2.byteLength);
  view.set(new Uint8Array(buffer1), 0);
  view.set(new Uint8Array(buffer2), buffer1.byteLength);
  return view.buffer;
}

run('sourceBuffer.appendBuffer(concatArrayBuffers(loader.mediaSegment(0),
loader.mediaSegment(1)))');

> LayoutTests/media/media-source/media-source-seek-unbuffered.html:60
> +	   // On Apple ports this may run before the second append. Still, we
try to remove the ranges
> +	   // of the second append (2) too, even if they haven't been buffered
yet.

This comment wouldn't be needed if you concatenated the segments.

> LayoutTests/media/media-source/media-source-seek-unbuffered.html:65
> +	   testExpected('oldCurrentTime <= video.currentTime' , true);

Good catch.