Bug 227258 – [Gstreamer] timeouts in media/media-source/media-source-has-audio-video.html and media/media-source/media-source-seek-unbuffered.html

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Arcady Goldmints-Orlov]

Two recently added media tests time out on both GTK and WPE.
media/media-source/media-source-seek-unbuffered.html was added in r278917
media/media-source/media-source-has-audio-video.html was added in r277726
both as a result of fallout from r277116.

[Arcady Goldmints-Orlov]

Created attachment 431971 [details]
Patch

[EWS]

Committed r279131 (239045@main): <https://commits.webkit.org/239045@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431971 [details].

[Radar WebKit Bug Importer]

<rdar://problem/79625722>

[Philippe Normand]

Gardening done. Patch needed now :)

[Alicia Boya García]

(In reply to Arcady Goldmints-Orlov from comment #0)
> Two recently added media tests time out on both GTK and WPE.
> media/media-source/media-source-seek-unbuffered.html was added in r278917
> media/media-source/media-source-has-audio-video.html was added in r277726
> both as a result of fallout from r277116.

What do you mean as a fallout from r277116? That commit preceeds the other commits where the new failing tests were added, right?

[Arcady Goldmints-Orlov]

What I meant is that whatever was done in r277116 seems to have resulted in the other two commits with their changes and tests being necessary for some reason. Mostly I was just trying to be helpful and provide some extra context for whomever goes to actually fix these.

[Alicia Boya García]

Thanks for the clarification :)

[Enrique Ocaña]

Created attachment 432398 [details]
Patch

[Enrique Ocaña]

Please note that this patch fixes only media-source-seek-unbuffered.html.

The remaining media-source-has-audio-video.html can't be fixed until MSE multitrack support (bug 165394) lands, as it appends a multitrack video to a single SourceBuffer and expects to find the audio and video tracks (more than one in total, currently unsupported in the GStreamer MSE implementation).

Therefore, this bug will need to be reopened once the patch to fix media-source-seek-unbuffered.html has landed.

[Enrique Ocaña]

Comment on attachment 432398 [details]
Patch

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

> LayoutTests/media/media-source/media-source-seek-unbuffered.html:47
> +        var interval = setInterval(function checkNotUpdating() {

I must find a different way to append the second media segment, since this way isn't working on the mac-wk2-stress.

[Alicia Boya García]

Comment on attachment 432398 [details]
Patch

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

>> LayoutTests/media/media-source/media-source-seek-unbuffered.html:47
>> +        var interval = setInterval(function checkNotUpdating() {
> 
> I must find a different way to append the second media segment, since this way isn't working on the mac-wk2-stress.

Listen for the "updateend" event. When it's fired, it's safe to append another segment.

[Enrique Ocaña]

Comment on attachment 432398 [details]
Patch

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

>>> LayoutTests/media/media-source/media-source-seek-unbuffered.html:47
>>> +        var interval = setInterval(function checkNotUpdating() {
>> 
>> I must find a different way to append the second media segment, since this way isn't working on the mac-wk2-stress.
> 
> Listen for the "updateend" event. When it's fired, it's safe to append another segment.

That was my first option, but for some reason I still got an error (because the SourceBuffer was "updating") in the code I placed in the "updateend" event handler. I guess I should look more carefully into what's actually happening.

On the other hand, I realized that since the Mac port can actually complete the seek with just the first media segment appended, nothing guarantees that "RUN(sourceBuffer.appendBuffer(loader.mediaSegment(1)))" will happen before "EVENT(seeked)" on that port, no matter if the "periodic check for not updating" or the "updateend" solution is used.

[Enrique Ocaña]

Created attachment 436889 [details]
Patch

[Enrique Ocaña]

In the end, "update" was the right event to listen. Passing "true" as last parameter of both addEventListener() and removeEventListener() was the key to avoid getting the SourceBuffer in an "updating" state, as it prevented other triggerings of the "update" event to be handled by the event listener.

[Enrique Ocaña]

(In reply to Enrique Ocaña from comment #9)

> Please note that this patch fixes only media-source-seek-unbuffered.html.
> 
> The remaining media-source-has-audio-video.html can't be fixed until MSE
> multitrack support (bug 165394) lands, as it appends a multitrack video to a
> single SourceBuffer and expects to find the audio and video tracks (more
> than one in total, currently unsupported in the GStreamer MSE
> implementation).
> 
> Therefore, this bug will need to be reopened once the patch to fix
> media-source-seek-unbuffered.html has landed.

media-source-has-audio-video.html passes on its own now, after bug 229072 landed.

[Enrique Ocaña]

Created attachment 436892 [details]
Patch

[Alicia Boya García]

Comment on attachment 436892 [details]
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.

[Enrique Ocaña]

Created attachment 437643 [details]
Patch

[Alicia Boya García]

Comment on attachment 437643 [details]
Patch

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

> LayoutTests/media/media-source/media-source-loader.js:100
> +    concatenateMediaSegments: function(segmentDataList)

You went the extra mile by adding a function to the library, good Quique. 👍

[EWS]

Committed r282205 (241492@main): <https://commits.webkit.org/241492@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 437643 [details].