Commit-Queue | +1 |
Hi Dale,
As per your suggestion in our offline chat back in June regarding the need for a Web Platform Test (WPT) for the VideoFrame.metadata().rtpTimestamp feature (https://chromestatus.com/feature/5186046555586560), I finally got around to putting one together.
This CL adds a WPT to cover the rtpTimestamp field when the VideoFrameMetadataRtpTimestamp feature is enabled, and also adds virtual test suite configs to run the test with the feature both enabled and disabled. I had to make a small adjustment to the VideoFrame::metadata() implementation to ensure the field is populated even in test environments using fake video sources.
Would appreciate it if you could take a look and let me know if this aligns with what you had in mind for test coverage before the feature can graduate to stable.
Thanks for your help
Ananta
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
// The local frame metadata may not have the rtp timestamp populated.
I think we only want this set if it's actually present. Otherwise we'll end up with a lot of default values in the dict -- which maybe is something the upstream spec should discuss.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
// The local frame metadata may not have the rtp timestamp populated.
I think we only want this set if it's actually present. Otherwise we'll end up with a lot of default values in the dict -- which maybe is something the upstream spec should discuss.
Thanks Dale. I reverted this part of the change and instead added code in the FakeVideoCaptureDevice class to set the rtp timestamp if the InjectFakeRtpTimestamps (newly added media feature) is enabled. This is enabled via the Virtual test config.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ultimately for cross-UA wpts we need the ability to have them run in other browsers without command line flags. Is there a way to set up frames that have a real rtp source through wpt?
// Enables injection of fake RTP timestamps in video capture metadata.
Do we even need the feature? The test source can just always inject these w/o an issue since nothing is relying on them?
// For testing purposes (e.g., in Web Platform Tests), we inject a fake RTP
Instead of generating a nonsense one, can we generate one that looks more appropriate?
const processor = new MediaStreamTrackProcessor({ track });
Is it possible to use VideoTrackGenerator/MediaStreamTrackGenerator to create frames with the metadata you want?
console.log(`Rtp timestamp found in VideoFrame metadata with value:${metadata.rtpTimestamp}`);
No console.log in tests
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ultimately for cross-UA wpts we need the ability to have them run in other browsers without command line flags. Is there a way to set up frames that have a real rtp source through wpt?
Thanks Dale. The WPT test harness doesn’t currently support injecting video frames with RTP level metadata or simulating a media pipeline that includes a real RTP source. Since rtpTimestamp is only exposed in Chromium today via internal hooks (like fake devices or feature flags), the current test uses those mechanisms behind feature guards. We can evolve the test to rely on real RTP based metadata if/when such a standardized path becomes available across UAs.
I left a TODO in the test to this effect
// Enables injection of fake RTP timestamps in video capture metadata.
Do we even need the feature? The test source can just always inject these w/o an issue since nothing is relying on them?
I added the switches to avoid concerns on setting optional data this way. If it is ok to always inject the timestamp, we can remove the switches. (Done)
// For testing purposes (e.g., in Web Platform Tests), we inject a fake RTP
Instead of generating a nonsense one, can we generate one that looks more appropriate?
Thanks for the feedback. I've updated the code to use an appropriate RTP timestamp by initializing last_rtp_timestamp_ with a random 32 bit epoch and ensuring it monotonically increments by 3000 for each 30Â FPS frame. Please let me know if this is acceptable.
const processor = new MediaStreamTrackProcessor({ track });
Is it possible to use VideoTrackGenerator/MediaStreamTrackGenerator to create frames with the metadata you want?
Thanks for the suggestion Dale. I explored using MediaStreamTrackGenerator with synthetic frames, but as per the VideoFrame constructor documentation on MDN
, none of the available constructors currently support setting metadata explicitly:
new VideoFrame(image)
new VideoFrame(image, options)
new VideoFrame(data, options)
Since VideoFrame.metadata() is a read only method and there’s no supported way to inject custom metadata like rtpTimestamp from JavaScript, the generator based approach wouldn’t let us simulate frames with that metadata. It seems this path wouldn't allow testing of rtpTimestamp presence unless it’s injected internally via the capture pipeline.
console.log(`Rtp timestamp found in VideoFrame metadata with value:${metadata.rtpTimestamp}`);
No console.log in tests
Thanks Dale. Since we're still in the experimental phase and the rtpTimestamp metadata is only present when both feature flags are enabled, I found the logs helpful during local testing to confirm behavior (especially in the absence of other test outputs).
Would it be reasonable to leave a TODO comment like the following for now?
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anantanarayanan Iyengar USUltimately for cross-UA wpts we need the ability to have them run in other browsers without command line flags. Is there a way to set up frames that have a real rtp source through wpt?
Thanks Dale. The WPT test harness doesn’t currently support injecting video frames with RTP level metadata or simulating a media pipeline that includes a real RTP source. Since rtpTimestamp is only exposed in Chromium today via internal hooks (like fake devices or feature flags), the current test uses those mechanisms behind feature guards. We can evolve the test to rely on real RTP based metadata if/when such a standardized path becomes available across UAs.
I left a TODO in the test to this effect
The problem we're going to run into here is that the API owners are going to immediately ask why the tests won't work for other browsers.
Is there anything in the existing WebRTC wpt's that might help simulate this using real primitives? https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/webrtc/
// For testing purposes (e.g., in Web Platform Tests), we inject a fake RTP
Anantanarayanan Iyengar USInstead of generating a nonsense one, can we generate one that looks more appropriate?
Thanks for the feedback. I've updated the code to use an appropriate RTP timestamp by initializing last_rtp_timestamp_ with a random 32 bit epoch and ensuring it monotonically increments by 3000 for each 30Â FPS frame. Please let me know if this is acceptable.
Done
console.log(`Rtp timestamp found in VideoFrame metadata with value:${metadata.rtpTimestamp}`);
Anantanarayanan Iyengar USNo console.log in tests
Thanks Dale. Since we're still in the experimental phase and the rtpTimestamp metadata is only present when both feature flags are enabled, I found the logs helpful during local testing to confirm behavior (especially in the absence of other test outputs).
Would it be reasonable to leave a TODO comment like the following for now?
Since this is pushed to an external GitHub repository it's generally better to follow the upstream style requirements. The argument in this case would be that you can just add them when you need to actually do some debugging. They don't need to be printed on every automated test invocation.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit-Queue | +1 |
Anantanarayanan Iyengar USUltimately for cross-UA wpts we need the ability to have them run in other browsers without command line flags. Is there a way to set up frames that have a real rtp source through wpt?
Dale CurtisThanks Dale. The WPT test harness doesn’t currently support injecting video frames with RTP level metadata or simulating a media pipeline that includes a real RTP source. Since rtpTimestamp is only exposed in Chromium today via internal hooks (like fake devices or feature flags), the current test uses those mechanisms behind feature guards. We can evolve the test to rely on real RTP based metadata if/when such a standardized path becomes available across UAs.
I left a TODO in the test to this effect
Anantanarayanan Iyengar USThe problem we're going to run into here is that the API owners are going to immediately ask why the tests won't work for other browsers.
Is there anything in the existing WebRTC wpt's that might help simulate this using real primitives? https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/webrtc/
Thanks Dale. I changed the wpt test to use getNoiseStream using this test as a reference https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/mediacapture-insertable-streams/legacy/MediaStreamTrackGenerator-video.https.html;l=64?q=MediaStreamTrackGenerator&ss=chromium%2Fchromium%2Fsrc:third_party%2Fblink%2Fweb_tests%2Fexternal%2Fwpt%2F and your link above. Additionally the test now sets up a WebRTC sender receiver combination. The sender encodes the video frames which would add the rtp timestamp on that side. On the receiver side we pass the received track to MediaStreamTrackProcessor where we validate the metadata.rtpTimestamp.
Hopefully this addresses your concern.
const processor = new MediaStreamTrackProcessor({ track });
Is it possible to use VideoTrackGenerator/MediaStreamTrackGenerator to create frames with the metadata you want?
Thanks for the suggestion Dale. I explored using MediaStreamTrackGenerator with synthetic frames, but as per the VideoFrame constructor documentation on MDN
, none of the available constructors currently support setting metadata explicitly:
new VideoFrame(image)
new VideoFrame(image, options)
new VideoFrame(data, options)Since VideoFrame.metadata() is a read only method and there’s no supported way to inject custom metadata like rtpTimestamp from JavaScript, the generator based approach wouldn’t let us simulate frames with that metadata. It seems this path wouldn't allow testing of rtpTimestamp presence unless it’s injected internally via the capture pipeline.
We use webrtc sender and receiver to ensure that rtp timestamps are added to the video stream
console.log(`Rtp timestamp found in VideoFrame metadata with value:${metadata.rtpTimestamp}`);
Anantanarayanan Iyengar USNo console.log in tests
Dale CurtisThanks Dale. Since we're still in the experimental phase and the rtpTimestamp metadata is only present when both feature flags are enabled, I found the logs helpful during local testing to confirm behavior (especially in the absence of other test outputs).
Would it be reasonable to leave a TODO comment like the following for now?
Since this is pushed to an external GitHub repository it's generally better to follow the upstream style requirements. The argument in this case would be that you can just add them when you need to actually do some debugging. They don't need to be printed on every automated test invocation.
Acknowledged. The latest iteration only has two console logs for both success and failure. I was planning to take this out when we flip the feature flag to stable in the next few weeks.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anantanarayanan Iyengar USUltimately for cross-UA wpts we need the ability to have them run in other browsers without command line flags. Is there a way to set up frames that have a real rtp source through wpt?
Dale CurtisThanks Dale. The WPT test harness doesn’t currently support injecting video frames with RTP level metadata or simulating a media pipeline that includes a real RTP source. Since rtpTimestamp is only exposed in Chromium today via internal hooks (like fake devices or feature flags), the current test uses those mechanisms behind feature guards. We can evolve the test to rely on real RTP based metadata if/when such a standardized path becomes available across UAs.
I left a TODO in the test to this effect
Anantanarayanan Iyengar USThe problem we're going to run into here is that the API owners are going to immediately ask why the tests won't work for other browsers.
Is there anything in the existing WebRTC wpt's that might help simulate this using real primitives? https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/webrtc/
Thanks Dale. I changed the wpt test to use getNoiseStream using this test as a reference https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/mediacapture-insertable-streams/legacy/MediaStreamTrackGenerator-video.https.html;l=64?q=MediaStreamTrackGenerator&ss=chromium%2Fchromium%2Fsrc:third_party%2Fblink%2Fweb_tests%2Fexternal%2Fwpt%2F and your link above. Additionally the test now sets up a WebRTC sender receiver combination. The sender encodes the video frames which would add the rtp timestamp on that side. On the receiver side we pass the received track to MediaStreamTrackProcessor where we validate the metadata.rtpTimestamp.
Hopefully this addresses your concern.
Great!
I don't think it's worth doing the virtual test suites now that you can do everything inside the harness. Lets just land the test as is w/o the enable/disable suites. The Chromium test harnesses always run with experimental features enabled
console.log(`Rtp timestamp found in VideoFrame metadata with value:${metadata.rtpTimestamp}`);
Anantanarayanan Iyengar USNo console.log in tests
Dale CurtisThanks Dale. Since we're still in the experimental phase and the rtpTimestamp metadata is only present when both feature flags are enabled, I found the logs helpful during local testing to confirm behavior (especially in the absence of other test outputs).
Would it be reasonable to leave a TODO comment like the following for now?
Anantanarayanan Iyengar USSince this is pushed to an external GitHub repository it's generally better to follow the upstream style requirements. The argument in this case would be that you can just add them when you need to actually do some debugging. They don't need to be printed on every automated test invocation.
Acknowledged. The latest iteration only has two console logs for both success and failure. I was planning to take this out when we flip the feature flag to stable in the next few weeks.
Since you have asserts right next to them, I don't see any value to leaving them in?
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
I don't think it's worth doing the virtual test suites now that you can do everything inside the harness. Lets just land the test as is w/o the enable/disable suites. The Chromium test harnesses always run with experimental features enabled
Done
console.log(`Rtp timestamp found in VideoFrame metadata with value:${metadata.rtpTimestamp}`);
Anantanarayanan Iyengar USNo console.log in tests
Dale CurtisThanks Dale. Since we're still in the experimental phase and the rtpTimestamp metadata is only present when both feature flags are enabled, I found the logs helpful during local testing to confirm behavior (especially in the absence of other test outputs).
Would it be reasonable to leave a TODO comment like the following for now?
Anantanarayanan Iyengar USSince this is pushed to an external GitHub repository it's generally better to follow the upstream style requirements. The argument in this case would be that you can just add them when you need to actually do some debugging. They don't need to be printed on every automated test invocation.
Dale CurtisAcknowledged. The latest iteration only has two console logs for both success and failure. I was planning to take this out when we flip the feature flag to stable in the next few weeks.
Since you have asserts right next to them, I don't see any value to leaving them in?
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Revert this change?
Acknowledged. Will remove the CONSOLE log after tracking down which step is failing on mac_rel. We may have to retain the setTimeout usage for the same reason
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anantanarayanan Iyengar USRevert this change?
Acknowledged. Will remove the CONSOLE log after tracking down which step is failing on mac_rel. We may have to retain the setTimeout usage for the same reason
I don't see setTimeout being needed in the other RTC tests. I think we'll want to figure out why the you're seeing hangs here that the other tests aren't.
Note there is a Test.step_wait() which can do similar things to setTimeout here.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anantanarayanan Iyengar USRevert this change?
Dale CurtisAcknowledged. Will remove the CONSOLE log after tracking down which step is failing on mac_rel. We may have to retain the setTimeout usage for the same reason
I don't see setTimeout being needed in the other RTC tests. I think we'll want to figure out why the you're seeing hangs here that the other tests aren't.
Note there is a Test.step_wait() which can do similar things to setTimeout here.
On the mac_rel bot we see these errors. [1010/115430.359460:ERROR:services/network/p2p/socket_manager.cc:137] Failed to resolve address for 23e83a68-d288-41ce-818d-1c3ce4fadd10.local., errorcode: -105
[1010/115430.360164:ERROR:services/network/p2p/socket_manager.cc:137] Failed to resolve address for 23e83a68-d288-41ce-818d-1c3ce4fadd10.local., errorcode: -105
[1010/115430.360770:WARNING:third_party/webrtc/p2p/base/p2p_transport_channel.cc:1302] Failed to resolve ICE candidate hostname 23e83a68-d288-41ce-818d-1c3ce4fadd10.local with error -1
[1010/115430.361788:WARNING:third_party/webrtc/p2p/base/p2p_transport_channel.cc:1302] Failed to resolve ICE candidate hostname 23e83a68-d288-41ce-818d-1c3ce4fadd10.local with error -1.
It appears that ICE candidate exchange fails which then results in no frames coming through on the receiver track.
IIUC Test.step.wait() won't work as we need to check multiple conditions together via promise.race.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anantanarayanan Iyengar USRevert this change?
Dale CurtisAcknowledged. Will remove the CONSOLE log after tracking down which step is failing on mac_rel. We may have to retain the setTimeout usage for the same reason
Anantanarayanan Iyengar USI don't see setTimeout being needed in the other RTC tests. I think we'll want to figure out why the you're seeing hangs here that the other tests aren't.
Note there is a Test.step_wait() which can do similar things to setTimeout here.
On the mac_rel bot we see these errors. [1010/115430.359460:ERROR:services/network/p2p/socket_manager.cc:137] Failed to resolve address for 23e83a68-d288-41ce-818d-1c3ce4fadd10.local., errorcode: -105
[1010/115430.360164:ERROR:services/network/p2p/socket_manager.cc:137] Failed to resolve address for 23e83a68-d288-41ce-818d-1c3ce4fadd10.local., errorcode: -105
[1010/115430.360770:WARNING:third_party/webrtc/p2p/base/p2p_transport_channel.cc:1302] Failed to resolve ICE candidate hostname 23e83a68-d288-41ce-818d-1c3ce4fadd10.local with error -1
[1010/115430.361788:WARNING:third_party/webrtc/p2p/base/p2p_transport_channel.cc:1302] Failed to resolve ICE candidate hostname 23e83a68-d288-41ce-818d-1c3ce4fadd10.local with error -1.It appears that ICE candidate exchange fails which then results in no frames coming through on the receiver track.
IIUC Test.step.wait() won't work as we need to check multiple conditions together via promise.race.
I think this is https://crbug.com/40766743 -- so we may just have to disable as timeout on macos like the other webrtc wpts for now.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anantanarayanan Iyengar USRevert this change?
Dale CurtisAcknowledged. Will remove the CONSOLE log after tracking down which step is failing on mac_rel. We may have to retain the setTimeout usage for the same reason
Anantanarayanan Iyengar USI don't see setTimeout being needed in the other RTC tests. I think we'll want to figure out why the you're seeing hangs here that the other tests aren't.
Note there is a Test.step_wait() which can do similar things to setTimeout here.
Dale CurtisOn the mac_rel bot we see these errors. [1010/115430.359460:ERROR:services/network/p2p/socket_manager.cc:137] Failed to resolve address for 23e83a68-d288-41ce-818d-1c3ce4fadd10.local., errorcode: -105
[1010/115430.360164:ERROR:services/network/p2p/socket_manager.cc:137] Failed to resolve address for 23e83a68-d288-41ce-818d-1c3ce4fadd10.local., errorcode: -105
[1010/115430.360770:WARNING:third_party/webrtc/p2p/base/p2p_transport_channel.cc:1302] Failed to resolve ICE candidate hostname 23e83a68-d288-41ce-818d-1c3ce4fadd10.local with error -1
[1010/115430.361788:WARNING:third_party/webrtc/p2p/base/p2p_transport_channel.cc:1302] Failed to resolve ICE candidate hostname 23e83a68-d288-41ce-818d-1c3ce4fadd10.local with error -1.It appears that ICE candidate exchange fails which then results in no frames coming through on the receiver track.
IIUC Test.step.wait() won't work as we need to check multiple conditions together via promise.race.
I think this is https://crbug.com/40766743 -- so we may just have to disable as timeout on macos like the other webrtc wpts for now.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Code-Review | +1 |
assert_true(true, "Required APIs not supported. Skipping test.");
This should use `assert_implements_optional` I think since VideoFrame and MediaStreamTrackProcessor are both optional features?
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
assert_true(true, "Required APIs not supported. Skipping test.");
This should use `assert_implements_optional` I think since VideoFrame and MediaStreamTrackProcessor are both optional features?
Done
Anantanarayanan Iyengar USExtra line
Done
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anantanarayanan Iyengar USExtra line
Done
Thanks Dale for the +1. Please stamp again as that got erased after the recent update.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |