Bug 237086 - webrtc/video-replace-muted-track.html is unnecessary long to run
Summary: webrtc/video-replace-muted-track.html is unnecessary long to run
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-23 07:47 PST by youenn fablet
Modified: 2022-02-24 04:31 PST (History)
10 users (show)

See Also:


Attachments
Patch (1.63 KB, patch)
2022-02-23 07:51 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (2.85 KB, patch)
2022-02-24 01:28 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2022-02-23 07:47:52 PST
webrtc/video-replace-muted-track.html is unnecessary long to run
Comment 1 youenn fablet 2022-02-23 07:51:17 PST
Created attachment 452980 [details]
Patch
Comment 2 Peng Liu 2022-02-23 10:28:58 PST
Comment on attachment 452980 [details]
Patch

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

> LayoutTests/webrtc/video-replace-muted-track.html:47
> +        return checkVideoBlack(true, canvas, video, "Check we do not receive any black frame", 360).then(assert_unreached, () => { });

To be honest, I was confused by the change at the beginning. Because you change 40 -> 360 here, which is not consistent with the change log. But I got your point after reading the source code. :-)
Comment 3 youenn fablet 2022-02-23 10:37:09 PST
I should probably use a decrementing counter to make it easier to understand.
I'll update the patch tomorrow if it is not changing a lot of tests.
Comment 4 Darin Adler 2022-02-23 13:03:33 PST
Comment on attachment 452980 [details]
Patch

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

> LayoutTests/ChangeLog:11
> +        We were waiting for 360 iterations to validate no black frame is found.
> +        This makes it last around 20 seconds.
> +        Decrease the number of iterations to 40, around 2 seconds.

I see Peng’s comment but I do not understand: Why does this change log comment say the opposite of what the change is?
Comment 5 Peng Liu 2022-02-23 14:22:21 PST
Comment on attachment 452980 [details]
Patch

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

>> LayoutTests/ChangeLog:11
>> +        Decrease the number of iterations to 40, around 2 seconds.
> 
> I see Peng’s comment but I do not understand: Why does this change log comment say the opposite of what the change is?

The checkVideoBlack() function is below:

```
async function checkVideoBlack(expected, canvas, video, errorMessage, counter)
{
    if (isVideoBlack(canvas, video) === expected)
        return Promise.resolve();

    if (counter === undefined)
        counter = 0;
    if (counter > 400) {
        if (!errorMessage)
            errorMessage = "checkVideoBlack timed out expecting " + expected;
        return Promise.reject(errorMessage);
    }

    await waitFor(50);
    return checkVideoBlack(expected, canvas, video, errorMessage, ++counter);
}
```

So the iteration number is changed from (400-40) -> (400 - 360).
Comment 6 youenn fablet 2022-02-24 01:28:40 PST
Created attachment 453084 [details]
Patch for landing
Comment 7 EWS 2022-02-24 04:30:41 PST
Committed r290425 (247733@main): <https://commits.webkit.org/247733@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453084 [details].
Comment 8 Radar WebKit Bug Importer 2022-02-24 04:31:23 PST
<rdar://problem/89411461>