Create IAMF playback test [chromium/src : main]
Syed AbuTalib (Gerrit)
Syed AbuTalib voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Thomas Guilbert
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Guilbert (Gerrit)
Thomas Guilbert added 2 comments![]( "Open in Gerrit")
src="/media/iamf_alternating_sine_waves_stereo.mp4"Doesn't this need to be fed through MSE?
assert_implements_optional(supported, type + ' unsupported');Have you confirmed that this is run and not skipped?
Related details
- Syed AbuTalib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Syed AbuTalib (Gerrit)
Syed AbuTalib added 2 comments![]( "Open in Gerrit")
Doesn't this need to be fed through MSE?
Wow, I can't believe it passed (it actually ran it wasn't a skip) without it. I switched to MSE and it seems to pass/fail correctly.
assert_implements_optional(supported, type + ' unsupported');Have you confirmed that this is run and not skipped?
Okay, the default led to a precondition failed (I assume good?) and when I ran with the enabled flag I got it to pass: `/third_party/blink/tools/run_web_tests.py -t Default --additional-driver-flag=--enable-features=IamfAudioDecoding external/wpt/media-source/mediasource-iamf-playback.html`
I added the expected.txt, but is that how `assert_implements_optional` is used? It seems so according to https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/docs/writing-tests/testharness-api.md
Related details
- Thomas Guilbert
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Guilbert (Gerrit)
Thomas Guilbert added 5 comments![]( "Open in Gerrit")
audio.src = URL.createObjectURL(mediaSource);You can create an element through `let audio_element = document.createElement('audio')` instead of declaring one in the HTML.
const buffer = await response.arrayBuffer();assert `response.Ok` before getting the arrayBuffer.
assert_implements_optional(mediaSource.readyState === 'open', type + ' unsupported by media pipeline');Use a regular assert here.
audio.play().catch(e => {You might run into issues calling `play()` here, due to auto-play policies blocking this without user interaction first.
Here's how another test gets around this:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/html/semantics/embedded-content/media-elements/preserves-pitch.html;l=110;drc=4ce886724455810f79bfb059684b2f56f84ba24d
assert_implements_optional(false, type + ' unsupported by media decoding pipeline');There should only be one `assert_implements_optional` to skip the test if unsupported.
Otherwise, this should be a test failure.
Gemini also thinkgs that wrapping everything in the try/catch is an anti-pattern? I think that if there is an exception, it should bubble up and fail the test correctly. You could verify this locally by throwing any exception and confirming that the test fails with the right messages.
Related details
- Syed AbuTalib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Syed AbuTalib (Gerrit)
Syed AbuTalib added 5 comments![]( "Open in Gerrit")
You can create an element through `let audio_element = document.createElement('audio')` instead of declaring one in the HTML.
Done
assert `response.Ok` before getting the arrayBuffer.
Done
assert_implements_optional(mediaSource.readyState === 'open', type + ' unsupported by media pipeline');Use a regular assert here.
Done
You might run into issues calling `play()` here, due to auto-play policies blocking this without user interaction first.
Here's how another test gets around this:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/html/semantics/embedded-content/media-elements/preserves-pitch.html;l=110;drc=4ce886724455810f79bfb059684b2f56f84ba24d
Done
assert_implements_optional(false, type + ' unsupported by media decoding pipeline');There should only be one `assert_implements_optional` to skip the test if unsupported.
Otherwise, this should be a test failure.Gemini also thinkgs that wrapping everything in the try/catch is an anti-pattern? I think that if there is an exception, it should bubble up and fail the test correctly. You could verify this locally by throwing any exception and confirming that the test fails with the right messages.
Done. The only one is `assert_implements_optional(supported, type + ' unsupported');`.
Related details
- Thomas Guilbert
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Guilbert (Gerrit)
Thomas Guilbert added 3 comments![]( "Open in Gerrit")
const audio_element = document.createElement('audio');Gemini mentions that some browsers throttle out-of-dom audio elements.
Adding the following is its recommendation:
```
document.body.appendChild(audio_element);
t.add_cleanup(() => audio_element.remove());
```
audio_element.src = URL.createObjectURL(mediaSource);Make sure to cleanup the URL, or else it can cause a memory leak if we exit early due to an assert (although, it shouldn't be an issue really for WPTs).
```
const url = URL.createObjectURL(mediaSource);
t.add_cleanup(() => URL.revokeObjectURL(url));
```
audio_element.removeEventListener('timeupdate', onTimeUpdate);How long is the audio clip? There is a flakiness risk if `onended` fires before `timeupdate` does. The test would hang, as there would be no more `timeupdate` events fired afterwards.
Related details
- Syed AbuTalib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Syed AbuTalib (Gerrit)
Syed AbuTalib added 3 comments![]( "Open in Gerrit")
const audio_element = document.createElement('audio');Gemini mentions that some browsers throttle out-of-dom audio elements.
Adding the following is its recommendation:
```
document.body.appendChild(audio_element);
t.add_cleanup(() => audio_element.remove());
```
Done
audio_element.src = URL.createObjectURL(mediaSource);Make sure to cleanup the URL, or else it can cause a memory leak if we exit early due to an assert (although, it shouldn't be an issue really for WPTs).
```
const url = URL.createObjectURL(mediaSource);
t.add_cleanup(() => URL.revokeObjectURL(url));
```
Done
audio_element.removeEventListener('timeupdate', onTimeUpdate);How long is the audio clip? There is a flakiness risk if `onended` fires before `timeupdate` does. The test would hang, as there would be no more `timeupdate` events fired afterwards.
`third_party/blink/web_tests/external/wpt/media/iamf_alternating_sine_waves_stereo.mp4` is `6.01` seconds long. That should be good enough?
Related details
- Thomas Guilbert
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Guilbert (Gerrit)
Thomas Guilbert added 1 comment![]( "Open in Gerrit")
audio_element.removeEventListener('timeupdate', onTimeUpdate);Syed AbuTalibHow long is the audio clip? There is a flakiness risk if `onended` fires before `timeupdate` does. The test would hang, as there would be no more `timeupdate` events fired afterwards.
`third_party/blink/web_tests/external/wpt/media/iamf_alternating_sine_waves_stereo.mp4` is `6.01` seconds long. That should be good enough?
It is long enough, but I still think it would be worth checking the `onended` event. These tests are meant to be run across browsers, on VMs, on old hardware, etc. I feel like we should make these as robust and resilient as possible, given that they are shared with the industry.
If this test starts flaking, this will rule out one possibility.
Related details
- Syed AbuTalib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Syed AbuTalib (Gerrit)
Syed AbuTalib added 1 comment![]( "Open in Gerrit")
audio_element.removeEventListener('timeupdate', onTimeUpdate);Syed AbuTalibHow long is the audio clip? There is a flakiness risk if `onended` fires before `timeupdate` does. The test would hang, as there would be no more `timeupdate` events fired afterwards.
Thomas Guilbert`third_party/blink/web_tests/external/wpt/media/iamf_alternating_sine_waves_stereo.mp4` is `6.01` seconds long. That should be good enough?
It is long enough, but I still think it would be worth checking the `onended` event. These tests are meant to be run across browsers, on VMs, on old hardware, etc. I feel like we should make these as robust and resilient as possible, given that they are shared with the industry.
If this test starts flaking, this will rule out one possibility.
- Thomas Guilbert
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Guilbert (Gerrit)
Thomas Guilbert added 2 comments![]( "Open in Gerrit")
if (audio_element.currentTime > 0 || audio_element.ended) {If this ended but `currentTime == 0`, this should be a test error. Otherwise, it's a success.
I would use a seperate method for checking `ended`.
audio_element.removeEventListener('timeupdate', onTimeUpdate);Syed AbuTalibHow long is the audio clip? There is a flakiness risk if `onended` fires before `timeupdate` does. The test would hang, as there would be no more `timeupdate` events fired afterwards.
Thomas Guilbert`third_party/blink/web_tests/external/wpt/media/iamf_alternating_sine_waves_stereo.mp4` is `6.01` seconds long. That should be good enough?
Syed AbuTalibIt is long enough, but I still think it would be worth checking the `onended` event. These tests are meant to be run across browsers, on VMs, on old hardware, etc. I feel like we should make these as robust and resilient as possible, given that they are shared with the industry.
If this test starts flaking, this will rule out one possibility.
Added check for `ended` event.
- Syed AbuTalib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Syed AbuTalib (Gerrit)
Syed AbuTalib added 1 comment![]( "Open in Gerrit")
if (audio_element.currentTime > 0 || audio_element.ended) {If this ended but `currentTime == 0`, this should be a test error. Otherwise, it's a success.
I would use a seperate method for checking `ended`.
- Thomas Guilbert
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Guilbert (Gerrit)
Thomas Guilbert voted and added 2 comments![]( "Open in Gerrit")
Votes added by Thomas Guilbert
| Code-Review | +1 |
2 comments
LGTM! Would you mind running the tests once more before submitting, and confirming that they are skipped when the kIAMF flag is off?
if (audio_element.currentTime > 0 || audio_element.ended) {Syed AbuTalibIf this ended but `currentTime == 0`, this should be a test error. Otherwise, it's a success.
I would use a seperate method for checking `ended`.
I splitted into two functions, checkProgress and onError.
- Syed AbuTalib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Syed AbuTalib (Gerrit)
Syed AbuTalib voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Syed AbuTalib (Gerrit)
Syed AbuTalib added 2 comments![]( "Open in Gerrit")
LGTM! Would you mind running the tests once more before submitting, and confirming that they are skipped when the kIAMF flag is off?
Just tested with on/off, got `[PRECONDITION FAILED]` in the off case and `PASS` in the enabled case.
assert_implements_optional(supported, type + ' unsupported');Have you confirmed that this is run and not skipped?
Okay, the default led to a precondition failed (I assume good?) and when I ran with the enabled flag I got it to pass: `/third_party/blink/tools/run_web_tests.py -t Default --additional-driver-flag=--enable-features=IamfAudioDecoding external/wpt/media-source/mediasource-iamf-playback.html`
I added the expected.txt, but is that how `assert_implements_optional` is used? It seems so according to https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/docs/writing-tests/testharness-api.md
Done
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/61732.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Create IAMF playback test
This CL introduces a basic test to ensure that iamf audio files can be
played. We confirm 'played' status by checking `timeupdate`.
- A third_party/blink/web_tests/external/wpt/media-source/mediasource-iamf-playback-expected.txt
- A third_party/blink/web_tests/external/wpt/media-source/mediasource-iamf-playback.html
- A third_party/blink/web_tests/external/wpt/media/iamf_alternating_sine_waves_stereo.mp4
Code-Review: +1 by Thomas Guilbert
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/61732
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |