Add a test for ReplaceTrack that verifies video track content. #22779
Conversation
| doSignalingHandshake(pc1, pc2); | ||
| await metadataToBeLoaded; | ||
| await detectSignal(t, v, 20); | ||
| await sender.replaceTrack(null); |
There was a problem hiding this comment.
What is the purpose of await sender.replaceTrack(null) in the code?
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-2141913
branch
from
2dd257b
to
bedfd54
Compare
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-2141913
branch
from
bedfd54
to
5f88979
Compare
|
Failed both chrome and firefox stability checks. Firefox was a timeout, but Chrome was: There's no previously recorded flake for that test on wpt.fyi, but this PR also doesn't obvious touch it. I'll need to look closer at the changes to the helper js files. |
| if (signal !== null) { | ||
| ctx.fillStyle = `rgb(${signal}, ${signal}, ${signal})`; | ||
| ctx.fillRect(10, 10, 20, 20); | ||
| let pixel = ctx.getImageData(15, 15, 1, 1); |
There was a problem hiding this comment.
What is the purpose of let pixel = ctx.getImageData(15, 15, 1, 1)?
There was a problem hiding this comment.
This does look like there's no good reason for it. I'll send a PR to delete this line once this PR is landed.
| pc2.ontrack = (e) => { | ||
| v.srcObject = new MediaStream([e.track]); | ||
| }; | ||
| const metadataToBeLoaded = new Promise((resolve) => { |
There was a problem hiding this comment.
One issue is metadataToBeLoaded Promise never fulfills.
There was a problem hiding this comment.
https://wpt.fyi/results/webrtc/RTCRtpSender-replaceTrack.https.html?label=pr_head&max-count=1&pr=22779 are the results from running this test in Chrome, Firefox and Safari.
The failure due to OffscreenCanvas is inside detectSignal, so this promise did resolve.
If you see some other behavior locally it's probably because differences in rules around autoplaying video. In CI we pass some additional flags to browsers to allow that, since so many tests depend on this.
|
As indicated at #22779 (comment) One example of observable differences between Firefox and Chromium implementation of either and later we have the result is that, more than not, Nightly 77 will not fire Chromium 83 Nightly 77 |
|
Another issue is both Firefox and Chromium appear to return an At Chromium Nightly 77 First the Chromium 83, same code Using |
|
Comparing the is intended to handle that case? Or, if the test should fail if |
|
A test for Chromium that uses Results after 5 runs where the left column is the number of colors tested (148) and the right column is the number of strict equal results for every index of the respective as is clear from this version of the test every value at every index match exactly an averge of 10% for the 5 runs. What is the exact criteria of the test? How is pass and fail determined? What is the expected result? |
|
For Nightly, substituting |
|
Re using
Reconstruct original 16-bit Raw pixel data from the HTML5 canvas https://stackoverflow.com/a/43413784
Is canvas getImageData method machine/browser dependent? https://stackoverflow.com/a/26615864
Errata Chromium behaviour of a captured Nightly 76 Chromium 82 notice that Chromium 82 took over 4 minutes to render and capture each image of a 33 second video. Transmittal of MediaStreamTrack data at Firefox has issues too, is not an immediate occurrence. Firefox and Nightly have some form of a delay before transmission of the MediaStream occurs https://gist.github.com/guest271314/c38042935db4e0131c1e0b68ca59f4ac#gistcomment-3241753
Writing For the tests demonstrating how fast Firefox is at processing images and video, or conversely, how slow Chromium is at processing images, if the tab does not freeze or crash, see https://bugs.chromium.org/p/chromium/issues/detail?id=1065675 and Issue 1063681: Using video.requestAnimationFrame() with OffscreenCanvas and createImageBitmap() freezes video presentation https://bugs.chromium.org/p/chromium/issues/detail?id=1063681 and Issue 1065720: Chromium never sets ended attribute or fires ended event on HTMLMediaElement when video is produced by Chromium MediaRecorder - Nightly does get duration, set ended attribute, fires ended event with same file https://bugs.chromium.org/p/chromium/issues/detail?id=1065720, where a user will never get the last frame of a video at Firefox might throw an error as well when attempting to connect to a remote track, e.g., https://bugzilla.mozilla.org/show_bug.cgi?id=1212237. Chromium will output a In short, as the author of the test is probably well aware of, it is non-trivial, yet illuminating, to compose WebRTC, or
|
|
Note also, |
This verifies that replaceTrack() actually replaces the track. Adds a new helper function to add a "signal" square into a noise track. Bug: none Change-Id: Ia90535c984d65adcdf2c63a5700b08d7c1e384c0 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2141913 Commit-Queue: Harald Alvestrand <[email protected]> Reviewed-by: Guido Urdaneta <[email protected]> Cr-Commit-Position: refs/heads/master@{#758062}
foolip
force-pushed
the
chromium-export-cl-2141913
branch
from
5f88979
to
499a34a
Compare
|
wpt-firefox-nightly-stability timed out and failed after 2 hours. This is because of #7660. I'm going to admin merge this. |
|
Actually, there's a regression of a test in Safari that we should look at first: |
|
@guest271314 PRs with the label |
| if (v.videoWidth < 21 || v.videoHeight < 21) { | ||
| return null; | ||
| } | ||
| const canvas = new OffscreenCanvas(v.videoWidth, v.videoHeight); |
There was a problem hiding this comment.
@alvestrand is it necessary to use OffscreenCanvas here? In https://wpt.fyi/results/webrtc/RTCRtpSender-replaceTrack.https.html?label=pr_head&max-count=1&pr=22779 it looks like Firefox and Safari fail this test because of this. Could a regular canvas be used just as well?
There was a problem hiding this comment.
Even if canvas is used the expectation of the test is not clear where the codec used is lossy: why is a specific pixel expected to be in an exact coordinate when using a lossy video codec.
The timeout is probably related to loadedmetadata never being fired in the code in this PR see w3c/webrtc-pc#2506 (comment)
There was a problem hiding this comment.
detectSignal allows for an error margin of 1, but good point that this may not always hold.
I don't see a timeout in the test results so there isn't anything to investigate there I think.
There was a problem hiding this comment.
The test paints a box in a specific color, and then checks that the middle of that box is the expected color (or close to it). The margin should be big enough that even edge-blurring codecs don't mess things up; if the video width and height are the same (no resize), the box should be in the same place.
The amount of color disparity one can expect should be tested; good point. Later CL.
|
Note: The RTCDataChannel flake is a real flake, but it's completely unrelated to this thread. |
|
Thanks for taking a look, @alvestrand! I'll go ahead and merge this now, so that the follow-up fixes can be made more easily. I'll send a PR for the one thing I think I can fix myself. I checked the seeming regression in Safari mentioned in #22779 (comment), but using the "Show History" button on https://wpt.fyi/results/webrtc/RTCPeerConnection-setRemoteDescription.html I can see this test was already flaky on master: |
…eData()` call, a=testonly Automatic update from web-platform-tests [webrtc] remove unnecessary `ctx.getImageData()` call (#23337) See web-platform-tests/wpt#22779 (comment) -- wpt-commits: a9f1e8c01979f0415e4f18396fa7b46d257a675a wpt-pr: 23337
…eData()` call, a=testonly Automatic update from web-platform-tests [webrtc] remove unnecessary `ctx.getImageData()` call (#23337) See web-platform-tests/wpt#22779 (comment) -- wpt-commits: a9f1e8c01979f0415e4f18396fa7b46d257a675a wpt-pr: 23337
…eData()` call, a=testonly Automatic update from web-platform-tests [webrtc] remove unnecessary `ctx.getImageData()` call (#23337) See web-platform-tests/wpt#22779 (comment) -- wpt-commits: a9f1e8c01979f0415e4f18396fa7b46d257a675a wpt-pr: 23337
…eData()` call, a=testonly Automatic update from web-platform-tests [webrtc] remove unnecessary `ctx.getImageData()` call (#23337) See web-platform-tests/wpt#22779 (comment) -- wpt-commits: a9f1e8c01979f0415e4f18396fa7b46d257a675a wpt-pr: 23337
This verifies that replaceTrack() actually replaces the track.
Adds a new helper function to add a "signal" square into a noise track.
Bug: none
Change-Id: Ia90535c984d65adcdf2c63a5700b08d7c1e384c0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2141913
Commit-Queue: Harald Alvestrand <[email protected]>
Reviewed-by: Guido Urdaneta <[email protected]>
Cr-Commit-Position: refs/heads/master@{#758062}