Bug 216371

Summary:

Some WebAudio tests give different output on different machines

Product:

WebKit

Reporter:

Chris Dumez <cdumez>

Component:

Web Audio

Assignee:

Chris Dumez <cdumez>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, calvaris, eric.carlson, ews-watchlist, ggaren, glenn, jer.noble, philipj, pnormand, sergio, vjaquez, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

216425

Bug Blocks:

212611

Attachments:

Description	Flags
Patch	none
Patch	none

Chris Dumez

Reported 2020-09-10 09:50:45 PDT

Some WebAudio tests give different output on different machines, because they rely on the hardware's preferred sample rate.

Attachments
Patch (34.03 KB, patch) 2020-09-10 09:56 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (25.44 KB, patch) 2020-09-14 09:18 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Chris Dumez

Comment 1 2020-09-10 09:56:11 PDT

Created attachment 408453 [details] Patch

Alex Christensen

Comment 2 2020-09-10 10:10:03 PDT

Comment on attachment 408453 [details] Patch Could you pass it through the WebPageCreationParameters instead of making new bundle SPI?

Chris Dumez

Comment 3 2020-09-10 10:13:32 PDT

(In reply to Alex Christensen from comment #2) > Comment on attachment 408453 [details] > Patch > > Could you pass it through the WebPageCreationParameters instead of making > new bundle SPI? I was trying to reduce the amount of plumbing necessary. Also, this hardwareSampleRate() is a global static so I did not really want this to be per page.

Alex Christensen

Comment 4 2020-09-10 10:17:39 PDT

Comment on attachment 408453 [details] Patch Why do you pass a WKBundleRef then? I think we should not be expanding the bundle SPI, even for testing, if we can get away with not expanding it. In this case we can.

Chris Dumez

Comment 5 2020-09-10 10:23:47 PDT

(In reply to Alex Christensen from comment #4) > Comment on attachment 408453 [details] > Patch > > Why do you pass a WKBundleRef then? > I think we should not be expanding the bundle SPI, even for testing, if we > can get away with not expanding it. In this case we can. Can you clarify your proposal? You want the SPI to be on the WKWebView even though the sample rate is process-wide? Also, how about when the view swaps process? I would need to add logic to forward the sample rate to the new process? Doing it like you suggest adds more code and complexity.

Alex Christensen

Comment 6 2020-09-10 10:41:35 PDT

Comment on attachment 408453 [details] Patch I think we should make a WKWebViewConfigurationPrivateForTesting.h like we have WKWebViewPrivateForTesting.h, add an override there, then copy it to an Optional on WebPageCreationParameters, then in the WebPage constructor if it's not nullopt we set it globally in that process. When we process swap, a new WebPageCreationParameters is already sent to the new process. It's not perfect, but it's good enough for test-only SPI. If you really think that's not a good direction and would prefer increasing our dependence on the bundle, r=me

Chris Dumez

Comment 7 2020-09-10 11:20:41 PDT

Committed r266842: <https://trac.webkit.org/changeset/266842>

Radar WebKit Bug Importer

Comment 8 2020-09-10 11:21:18 PDT

<rdar://problem/68653909>

Alex Christensen

Comment 9 2020-09-10 11:29:07 PDT

I think the best solution would have been to modify the web platform tests to print out a deterministic output.

Chris Dumez

Comment 10 2020-09-10 15:57:56 PDT

Reverted r266842 and r266883 for reason: Causes some assertions to be hit in debug builds Committed r266895: <https://trac.webkit.org/changeset/266895>

Chris Dumez

Comment 11 2020-09-14 09:18:24 PDT

Created attachment 408710 [details] Patch

Chris Dumez

Comment 12 2020-09-14 09:18:45 PDT

(In reply to Chris Dumez from comment #11) > Created attachment 408710 [details] > Patch New approach so requesting review again.

Alex Christensen

Comment 13 2020-09-14 09:20:48 PDT

Comment on attachment 408710 [details] Patch For some reason I don't understand, I think this is better than bundle SPI. I think it would be even better to modify the test to have different output (just PASS/FAIL instead of printing audio sample rates) but this is fine too.

Chris Dumez

Comment 14 2020-09-14 10:01:00 PDT

Comment on attachment 408710 [details] Patch Clearing flags on attachment: 408710 Committed r267018: <https://trac.webkit.org/changeset/267018>

Chris Dumez

Comment 15 2020-09-14 10:01:03 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.