| Summary: | webrtc/video-replace-muted-track.html is unnecessary long to run | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||
| Component: | WebRTC | Assignee: | youenn fablet <youennf> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | darin, eric.carlson, ews-watchlist, glenn, jer.noble, peng.liu6, philipj, sergio, webkit-bug-importer, youennf | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
youenn fablet
2022-02-23 07:47:52 PST
Created attachment 452980 [details]
Patch
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. :-) 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 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 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). Created attachment 453084 [details]
Patch for landing
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]. |