Created attachment 428266 [details] Screenshot from the test runner fast/mediastream/MediaStream-video-element-video-tracks-disabled.html is flaky image failure. Is this an unitialized read? What I can see in the image doesn't look like an image from my camera.
(In reply to Alicia Boya García from comment #0) > Created attachment 428266 [details] > Screenshot from the test runner > > fast/mediastream/MediaStream-video-element-video-tracks-disabled.html is > flaky image failure. Is this an unitialized read? What I can see in the > image doesn't look like an image from my camera. The tests uses mock audio and video sources. Anyway, I'll check this, thanks for the report.
(In reply to Alicia Boya García from comment #0) > Created attachment 428266 [details] > Screenshot from the test runner > > fast/mediastream/MediaStream-video-element-video-tracks-disabled.html is > flaky image failure. Is this an unitialized read? I don't think so. I can reproduce the failure, the video element associated with the MediaStream renders the mock video track that is disabled, instead of rendering black frames.
I have a patch but the test still flakes: function canplaythrough() { mediaStream.getVideoTracks()[0].enabled = false; window.testRunner.notifyDone(); } Due to the asynchronous nature of the GStreamer pipeline involved, we can't be sure the video is black right after the track was disabled, so we would need to delay the notifyDone call. Would this be acceptable?
Created attachment 428843 [details] Patch
Comment on attachment 428843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428843&action=review > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:252 > + m_needsDiscont = true; If there is no unfortunate technical reason that makes it impossible, we should flush here. That will ensure that the next frame the sink receives is a blank frame. > LayoutTests/fast/mediastream/MediaStream-video-element-video-tracks-disabled.html:45 > + setTimeout(() => { window.testRunner.notifyDone(); }, 200); This is certainly not ideal... Let's see by adding the flush this is no longer necessary, or if it can be reduced to a requestAnimationFrame(() => { window.testRunner.notifyDone(); )
Comment on attachment 428843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428843&action=review >> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:252 >> + m_needsDiscont = true; > > If there is no unfortunate technical reason that makes it impossible, we should flush here. That will ensure that the next frame the sink receives is a blank frame. Yeah I thought about this too but even so, there's no guarantee the test won't flake, unless... see below >> LayoutTests/fast/mediastream/MediaStream-video-element-video-tracks-disabled.html:45 >> + setTimeout(() => { window.testRunner.notifyDone(); }, 200); > > This is certainly not ideal... Let's see by adding the flush this is no longer necessary, or if it can be reduced to a requestAnimationFrame(() => { window.testRunner.notifyDone(); ) the rAF idea is interesting, I can try it. I don't have much hope though because I suspect rAF is not hooked into <video>. There is a spec proposal to have some rAF-equivalent for <video> but it's not implemented in WebKit yet AFAIK.
The rAF idea doesn't work, the video-rAF proposal is https://wicg.github.io/video-rvfc/ BTW.
Created attachment 428927 [details] Patch
Unfortunately flushing doesn't help with the flakiness :(
(In reply to Philippe Normand from comment #7) > The rAF idea doesn't work, the video-rAF proposal is > https://wicg.github.io/video-rvfc/ BTW. > Unfortunately flushing doesn't help with the flakiness :( Sad...(In reply to Philippe Normand from comment #9)
Comment on attachment 428927 [details] Patch I still think the flush should be done but I leave that decision to you.
I'll add the flush support, it's harmless even though it's not helping much :) Thanks for the review!
Committed r277651 (237857@main): <https://commits.webkit.org/237857@main>