Bug 227258

Summary: [Gstreamer] timeouts in media/media-source/media-source-has-audio-video.html and media/media-source/media-source-seek-unbuffered.html
Product: WebKit Reporter: Arcady Goldmints-Orlov <crzwdjk>
Component: MediaAssignee: Enrique Ocaña <eocanha>
Status: RESOLVED FIXED    
Severity: Normal CC: aboya, calvaris, eocanha, eric.carlson, ews-watchlist, glenn, jer.noble, philipj, pnormand, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Linux   
Bug Depends on: 229072    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Arcady Goldmints-Orlov 2021-06-22 09:55:02 PDT
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.
Comment 1 Arcady Goldmints-Orlov 2021-06-22 10:07:26 PDT
Created attachment 431971 [details]
Patch
Comment 2 EWS 2021-06-22 11:44:52 PDT
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].
Comment 3 Radar WebKit Bug Importer 2021-06-22 11:45:18 PDT
<rdar://problem/79625722>
Comment 4 Philippe Normand 2021-06-22 11:47:26 PDT
Gardening done. Patch needed now :)
Comment 5 Alicia Boya García 2021-06-23 04:31:08 PDT
(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?
Comment 6 Arcady Goldmints-Orlov 2021-06-23 07:32:19 PDT
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.
Comment 7 Alicia Boya García 2021-06-23 09:24:34 PDT
Thanks for the clarification :)
Comment 8 Enrique Ocaña 2021-06-28 08:50:56 PDT
Created attachment 432398 [details]
Patch
Comment 9 Enrique Ocaña 2021-06-28 08:55:04 PDT
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.
Comment 10 Enrique Ocaña 2021-06-29 01:38:42 PDT
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.
Comment 11 Alicia Boya García 2021-06-29 03:10:15 PDT
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.
Comment 12 Enrique Ocaña 2021-06-29 05:02:57 PDT
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.
Comment 13 Enrique Ocaña 2021-08-31 08:44:50 PDT
Created attachment 436889 [details]
Patch
Comment 14 Enrique Ocaña 2021-08-31 08:47:09 PDT
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.
Comment 15 Enrique Ocaña 2021-08-31 09:05:12 PDT
(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.
Comment 16 Enrique Ocaña 2021-08-31 09:09:29 PDT
Created attachment 436892 [details]
Patch
Comment 17 Alicia Boya García 2021-09-06 06:44:26 PDT
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.
Comment 18 Enrique Ocaña 2021-09-08 10:13:31 PDT
Created attachment 437643 [details]
Patch
Comment 19 Alicia Boya García 2021-09-09 01:45:17 PDT
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. 👍
Comment 20 EWS 2021-09-09 03:02:41 PDT
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].