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.
Created attachment 431971 [details] Patch
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].
<rdar://problem/79625722>
Gardening done. Patch needed now :)
(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?
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.
Thanks for the clarification :)
Created attachment 432398 [details] Patch
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 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 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 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.
Created attachment 436889 [details] Patch
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.
(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.
Created attachment 436892 [details] Patch
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.
Created attachment 437643 [details] Patch
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. 👍
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].