Bug 225651 - [MediaStream][GStreamer] Flaky fast/mediastream/MediaStream-video-element-video-tracks-disabled.html
Summary: [MediaStream][GStreamer] Flaky fast/mediastream/MediaStream-video-element-vid...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-05-11 05:44 PDT by Alicia Boya García
Modified: 2021-05-18 06:46 PDT (History)
10 users (show)

See Also:


Attachments
Screenshot from the test runner (50.14 KB, image/png)
2021-05-11 05:44 PDT, Alicia Boya García
no flags Details
Patch (7.39 KB, patch)
2021-05-17 10:22 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (7.33 KB, patch)
2021-05-18 04:10 PDT, Philippe Normand
aboya: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alicia Boya García 2021-05-11 05:44:28 PDT
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.
Comment 1 Philippe Normand 2021-05-17 08:11:23 PDT
(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.
Comment 2 Philippe Normand 2021-05-17 08:24:48 PDT
(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.
Comment 3 Philippe Normand 2021-05-17 09:31:45 PDT
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?
Comment 4 Philippe Normand 2021-05-17 10:22:31 PDT
Created attachment 428843 [details]
Patch
Comment 5 Alicia Boya García 2021-05-18 03:06:42 PDT
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 6 Philippe Normand 2021-05-18 03:57:58 PDT
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.
Comment 7 Philippe Normand 2021-05-18 04:04:05 PDT
The rAF idea doesn't work, the video-rAF proposal is https://wicg.github.io/video-rvfc/ BTW.
Comment 8 Philippe Normand 2021-05-18 04:10:25 PDT
Created attachment 428927 [details]
Patch
Comment 9 Philippe Normand 2021-05-18 04:11:10 PDT
Unfortunately flushing doesn't help with the flakiness :(
Comment 10 Alicia Boya García 2021-05-18 04:59:51 PDT
(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 11 Alicia Boya García 2021-05-18 05:10:34 PDT
Comment on attachment 428927 [details]
Patch

I still think the flush should be done but I leave that decision to you.
Comment 12 Philippe Normand 2021-05-18 05:42:59 PDT
I'll add the flush support, it's harmless even though it's not helping much :)
Thanks for the review!
Comment 13 Philippe Normand 2021-05-18 06:46:48 PDT
Committed r277651 (237857@main): <https://commits.webkit.org/237857@main>