88794 – Add layout tests for audio codecs

RESOLVED FIXED 88794

Add layout tests for audio codecs

https://bugs.webkit.org/show_bug.cgi?id=88794

Summary Add layout tests for audio codecs

Raymond Toy

Reported 2012-06-11 11:14:12 PDT

Add layout tests for audio codecs

Attachments
Patch (878.08 KB, patch) 2012-06-11 15:50 PDT, Raymond Toy	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-04 (740.41 KB, application/zip) 2012-06-11 21:16 PDT, WebKit Review Bot	no flags	Details
Patch (677.19 KB, patch) 2012-06-13 09:42 PDT, Raymond Toy	no flags	Details Formatted Diff Diff
Patch (682.68 KB, patch) 2012-06-13 14:11 PDT, Raymond Toy	no flags	Details Formatted Diff Diff
Patch (683.00 KB, patch) 2012-06-14 09:34 PDT, Raymond Toy	no flags	Details Formatted Diff Diff
Patch (1.26 MB, patch) 2012-06-20 10:25 PDT, Raymond Toy	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-02 (469.88 KB, application/zip) 2012-06-20 11:49 PDT, WebKit Review Bot	no flags	Details
Patch (1.26 MB, patch) 2012-06-21 14:05 PDT, Raymond Toy	no flags	Details Formatted Diff Diff
Patch (1.26 MB, patch) 2012-06-22 08:58 PDT, Raymond Toy	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-02 (455.82 KB, application/zip) 2012-06-22 09:59 PDT, WebKit Review Bot	no flags	Details
Patch (1.28 MB, patch) 2012-06-22 14:58 PDT, Raymond Toy	no flags	Details Formatted Diff Diff
Patch (1.28 MB, patch) 2012-06-22 17:09 PDT, Raymond Toy	no flags	Details Formatted Diff Diff
Patch (1.28 MB, patch) 2012-06-22 21:50 PDT, Raymond Toy	no flags	Details Formatted Diff Diff
Patch (1.28 MB, patch) 2012-06-25 10:42 PDT, Raymond Toy	no flags	Details Formatted Diff Diff
Show Obsolete (10) View All Add attachment proposed patch, testcase, etc.

Raymond Toy

Comment 1 2012-06-11 15:50:42 PDT

Created attachment 146940 [details] Patch

Eric Carlson

Comment 2 2012-06-11 15:53:39 PDT

Comment on attachment 146940 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146940&action=review > LayoutTests/ChangeLog:31 > + * webaudio/resources/codec/24bit.wav: Added. > + * webaudio/resources/codec/9874__vixuxx__crow_24bit.wav: Added. > + * webaudio/resources/codec/a_220_00_v2.ogg: Added. > + * webaudio/resources/codec/aac.aac: Added. > + * webaudio/resources/codec/ai_laser1.ogg: Added. > + * webaudio/resources/codec/mp3.mp3: Added. > + * webaudio/resources/codec/ogg.ogg: Added. Not all platforms that implement WebAudio support all of these codecs.

WebKit Review Bot

Comment 3 2012-06-11 21:16:22 PDT

Comment on attachment 146940 [details] Patch Attachment 146940 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12945447 New failing tests: webaudio/audio-codec-mp3-1.html webaudio/audio-codec-aac-1.html

WebKit Review Bot

Comment 4 2012-06-11 21:16:33 PDT

Created attachment 147007 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Chris Rogers

Comment 5 2012-06-12 14:14:06 PDT

(In reply to comment #2) > (From update of attachment 146940 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=146940&action=review > > > LayoutTests/ChangeLog:31 > > + * webaudio/resources/codec/24bit.wav: Added. > > + * webaudio/resources/codec/9874__vixuxx__crow_24bit.wav: Added. > > + * webaudio/resources/codec/a_220_00_v2.ogg: Added. > > + * webaudio/resources/codec/aac.aac: Added. > > + * webaudio/resources/codec/ai_laser1.ogg: Added. > > + * webaudio/resources/codec/mp3.mp3: Added. > > + * webaudio/resources/codec/ogg.ogg: Added. > > Not all platforms that implement WebAudio support all of these codecs. Can we add this as a "skipped" test on these platforms?

Raymond Toy

Comment 6 2012-06-13 09:42:19 PDT

Created attachment 147340 [details] Patch

Raymond Toy

Comment 7 2012-06-13 09:50:59 PDT

Updated patch that compares the actual and expected results with a threshold. The aac and mp3 decoder on mac produces slightly different results from the linux (ffmpeg) decoder. Have not updated TestExpectations on platforms that do not support some of the codecs. Also, these tests cannot pass until crbug http://code.google.com/p/chromium/issues/detail?id=132320 is fixed.

Eric Carlson

Comment 8 2012-06-13 09:52:33 PDT

Comment on attachment 147340 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147340&action=review > LayoutTests/webaudio/audio-codec-24bit-1.html:15 > +<html> > +<head> > +<link rel="stylesheet" href="../fast/js/resources/js-test-style.css"/> > +<script type="text/javascript" src="resources/audio-codec-test.js"></script> > +<script type="text/javascript" src="resources/audio-testing.js"></script> > +<script type="text/javascript" src="resources/buffer-loader.js"></script> > +<script src="../fast/js/resources/js-test-pre.js"></script> > +</head> > +<body> Nit: I think indentation in markup aids readability, just as it does in code. Is there any reason to not add a bit of whitespace in the markup sections of these files? > LayoutTests/webaudio/resources/audio-codec-test.js:5 > +// Maximum allowed difference between the expected and actual output. > +var maxAllowedDiff = 1.1 / 32768; Nit: it would be helpful to future readers to explain what this difference means and why this amount is deemed acceptable. > LayoutTests/webaudio/resources/audio-codec-test.js:11 > +function runTest(testInfo) { Nit: a functions's opening brace should be on a new line. > LayoutTests/webaudio/resources/audio-codec-test.js:58 > + if (maxDiff <= maxAllowedDiff) { > + testPassed(name + " test succeeded."); > + } else { Nit: braces are not needed around a single line condition.

Eric Carlson

Comment 9 2012-06-13 09:53:33 PDT

(In reply to comment #5) > (In reply to comment #2) > > Not all platforms that implement WebAudio support all of these codecs. > > Can we add this as a "skipped" test on these platforms? Definitely!

Raymond Toy

Comment 10 2012-06-13 10:04:50 PDT

Comment on attachment 147340 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147340&action=review >> LayoutTests/webaudio/audio-codec-24bit-1.html:15 >> +<body> > > Nit: I think indentation in markup aids readability, just as it does in code. Is there any reason to not add a bit of whitespace in the markup sections of these files? No particular reason. Is the following acceptable: <html> <head> <link rel="stylesheet" href="../fast/js/resources/js-test-style.css"/> ... <head> <html> <body> <div ...> <script> // Test... </script> </body>

Dirk Pranke

Comment 11 2012-06-13 10:17:03 PDT

Is it possible to do (probably port-specific) runtime checks in new-run-webkit-tests to determine if the codecs are installed? We can then dynamically add SKIP expectations if they aren't, and have them just work right if they are. Also, do you forsee having multiple tests for mp3 and aac? If so, it might make more sense to move them into subdirectories so they are more easily skipped. If not, it's probably fine the way it is.

Raymond Toy

Comment 12 2012-06-13 12:30:36 PDT

(In reply to comment #9) > (In reply to comment #5) > > (In reply to comment #2) > > > Not all platforms that implement WebAudio support all of these codecs. > > > > Can we add this as a "skipped" test on these platforms? > > Definitely! Can I get a list of the platforms that don't support aac/mp3/ogg so I can update the test expectations appropriately?

Raymond Toy

Comment 13 2012-06-13 13:01:02 PDT

(In reply to comment #11) > Is it possible to do (probably port-specific) runtime checks in new-run-webkit-tests to determine if the codecs are installed? We can then dynamically add SKIP expectations if they aren't, and have them just work right if they are. > > Also, do you forsee having multiple tests for mp3 and aac? If so, it might make more sense to move them into subdirectories so they are more easily skipped. If not, it's probably fine the way it is. I don't plan on adding any additional aac or mp3 tests, but it's possible that more will be added in the future. I will move them to a directory.

Chris Rogers

Comment 14 2012-06-13 13:09:05 PDT

(In reply to comment #12) > (In reply to comment #9) > > (In reply to comment #5) > > > (In reply to comment #2) > > > > Not all platforms that implement WebAudio support all of these codecs. > > > > > > Can we add this as a "skipped" test on these platforms? > > > > Definitely! > > Can I get a list of the platforms that don't support aac/mp3/ogg so I can update the test expectations appropriately? If other ports want to use these tests, then it would probably be useful to separate the single test into separate test files per format (one test file each for: MP3, OGG, AAC, WAV, etc.) It's almost certain that if different ports use these tests that they will need separate baselines, so all ports should be disabled until correct baselines can be generated for them. Another note is that some ports will already have extensive audio codec testing done elsewhere (outside of WebKit), and might not need any of these tests.

Raymond Toy

Comment 15 2012-06-13 13:31:30 PDT

(In reply to comment #14) > (In reply to comment #12) > > (In reply to comment #9) > > > (In reply to comment #5) > > > > (In reply to comment #2) > > > > > Not all platforms that implement WebAudio support all of these codecs. > > > > > > > > Can we add this as a "skipped" test on these platforms? > > > > > > Definitely! > > > > Can I get a list of the platforms that don't support aac/mp3/ogg so I can update the test expectations appropriately? > > If other ports want to use these tests, then it would probably be useful to separate the single test into separate test files per format (one test file each for: MP3, OGG, AAC, WAV, etc.) > > It's almost certain that if different ports use these tests that they will need separate baselines, so all ports should be disabled until correct baselines can be generated for them. Currently the test uses a threshold to compare the output against a reference. Hopefully, this will be good enough so we don't have to have separate baselines for each port. I will move all of the tests to a directory so update the test expectations to disable the tests for now.

Raymond Toy

Comment 16 2012-06-13 14:11:58 PDT

Created attachment 147408 [details] Patch

Raymond Toy

Comment 17 2012-06-13 14:14:48 PDT

Comment on attachment 147340 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147340&action=review >> LayoutTests/webaudio/resources/audio-codec-test.js:5 >> +var maxAllowedDiff = 1.1 / 32768; > > Nit: it would be helpful to future readers to explain what this difference means and why this amount is deemed acceptable. Comment updated. >> LayoutTests/webaudio/resources/audio-codec-test.js:11 >> +function runTest(testInfo) { > > Nit: a functions's opening brace should be on a new line. Fixed. >> LayoutTests/webaudio/resources/audio-codec-test.js:58 >> + } else { > > Nit: braces are not needed around a single line condition. Fixed.

Eric Carlson

Comment 18 2012-06-13 17:00:28 PDT

Eric Carlson

Comment 19 2012-06-13 17:02:38 PDT

Comment on attachment 147408 [details] Patch This looks fine to me, but I haven't really reviewed WebAudio bugs so I will let Chris r+ it if he thinks it is OK.

Andrew Scherkus

Comment 20 2012-06-13 18:58:20 PDT

drive-by: do you think the the test clips should be renamed to match the layout test names or do you forsee the clips being used by multiple tests?

Raymond Toy

Comment 21 2012-06-13 20:19:39 PDT

(In reply to comment #20) > drive-by: do you think the the test clips should be renamed to match the layout test names or do you forsee the clips being used by multiple tests? Yes, I will change the names. I don't expect them to be reused.

Raymond Toy

Comment 22 2012-06-14 09:34:14 PDT

Created attachment 147603 [details] Patch

Raymond Toy

Comment 23 2012-06-20 10:25:31 PDT

Created attachment 148595 [details] Patch

Raymond Toy

Comment 24 2012-06-20 10:27:10 PDT

Reworked patch. Tests are now bit-exact, with different expected results for different platforms.

WebKit Review Bot

Comment 25 2012-06-20 11:48:56 PDT

Comment on attachment 148595 [details] Patch Attachment 148595 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12995301 New failing tests: webaudio/codec-tests-proprietary/audio-codec-aac-1.html webaudio/codec-tests-proprietary/audio-codec-mp3-1.html

WebKit Review Bot

Comment 26 2012-06-20 11:49:09 PDT

Created attachment 148615 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Chris Rogers

Comment 27 2012-06-20 12:02:58 PDT

Hi Ray, this looks pretty good overall. But, I'd switch the directory layout from "codecs" and "proprietary-codecs" to a single directory for each codec type "wav", "mp3", "aac", "vorbis". This gives finer grain control over individual codecs which individual ports may or may not support.

Eric Carlson

Comment 28 2012-06-20 12:17:58 PDT

(In reply to comment #27) > Hi Ray, this looks pretty good overall. But, I'd switch the directory layout from "codecs" and "proprietary-codecs" to a single directory for each codec type "wav", "mp3", "aac", "vorbis". This gives finer grain control over individual codecs which individual ports may or may not support. Unless the plan is to have *lots* of tests for each type, I don't think it helps to have separate directories. If a port doesn't support a codec, it isn't a big deal to add the test(s) for that codec to the skipped list, so I would put the "codec" media files in with the rest of the media files. This may also encourage test authors to use the platform-agnostic files in other tests so we test with more source files.

Raymond Toy

Comment 29 2012-06-20 13:01:12 PDT

(In reply to comment #28) > (In reply to comment #27) > > Hi Ray, this looks pretty good overall. But, I'd switch the directory layout from "codecs" and "proprietary-codecs" to a single directory for each codec type "wav", "mp3", "aac", "vorbis". This gives finer grain control over individual codecs which individual ports may or may not support. > > Unless the plan is to have *lots* of tests for each type, I don't think it helps to have separate directories. If a port doesn't support a codec, it isn't a big deal to add the test(s) for that codec to the skipped list, so I would put the "codec" media files in with the rest of the media files. This may also encourage test authors to use the platform-agnostic files in other tests so we test with more source files. I don't expect tons of tests for each type. Could you clarify what you mean by "rest of the media files"? Do you mean to put the audio files in codec-tests and codec-tests-proprietary into LayoutTests/media/content? Or something else?

Raymond Toy

Comment 30 2012-06-20 16:11:31 PDT

(In reply to comment #29) > (In reply to comment #28) > > (In reply to comment #27) > > > Hi Ray, this looks pretty good overall. But, I'd switch the directory layout from "codecs" and "proprietary-codecs" to a single directory for each codec type "wav", "mp3", "aac", "vorbis". This gives finer grain control over individual codecs which individual ports may or may not support. > > > > Unless the plan is to have *lots* of tests for each type, I don't think it helps to have separate directories. If a port doesn't support a codec, it isn't a big deal to add the test(s) for that codec to the skipped list, so I would put the "codec" media files in with the rest of the media files. This may also encourage test authors to use the platform-agnostic files in other tests so we test with more source files. > > I don't expect tons of tests for each type. > > Could you clarify what you mean by "rest of the media files"? Do you mean to put the audio files in codec-tests and codec-tests-proprietary into LayoutTests/media/content? Or something else? After talking this over with Eric, I will move the audio files in codec-tests and codec-tests-proprietary to resources/media.

Chris Rogers

Comment 31 2012-06-20 16:48:23 PDT

Comment on attachment 148595 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148595&action=review > LayoutTests/webaudio/codec-tests-proprietary/audio-codec-aac-1.html:4 > +Test to see if we can load an encoded audio file and play it out correctly. "play it out correctly" doesn't seem right. How about: "Test audio file decoding" or simply remove the comment altogether > LayoutTests/webaudio/codec-tests-proprietary/audio-codec-aac-1.html:16 > + window.onload = init; you might consider shortening all of these to: window.onload = function() { runTest(test); } or do you even need an onload handler and can you just directly call: runTest(test); > LayoutTests/webaudio/resources/audio-codec-test.js:8 > +function runTest(testInfo) "runTest" seems a little too general and could conflict with other functions named the same in other .js files. How about "runDecodingTest"

Raymond Toy

Comment 32 2012-06-21 14:05:20 PDT

Created attachment 148885 [details] Patch

Raymond Toy

Comment 33 2012-06-21 14:07:07 PDT

Comment on attachment 148595 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148595&action=review >> LayoutTests/webaudio/codec-tests-proprietary/audio-codec-aac-1.html:4 >> +Test to see if we can load an encoded audio file and play it out correctly. > > "play it out correctly" doesn't seem right. > > How about: > > "Test audio file decoding" > > or simply remove the comment altogether Removed. >> LayoutTests/webaudio/codec-tests-proprietary/audio-codec-aac-1.html:16 >> + window.onload = init; > > you might consider shortening all of these to: > > window.onload = function() { runTest(test); } > > or do you even need an onload handler and can you just directly call: > > runTest(test); Used your first suggestion. The second didn't work when I tried that earlier, and I didn't investigate why.

Raymond Toy

Comment 34 2012-06-21 14:09:29 PDT

I've moved the media files to resources/media, and also put the tests in a format-specific directory as suggested. I ran this on chromium linux, mac, and windows and the tests pass, including the skipped ones. Before checking this in, I still need to investigate getting DRT to check for symbols so that the proprietary tests won't run if DRT doesn't not have support for those codecs.

Chris Rogers

Comment 35 2012-06-21 14:54:22 PDT

Comment on attachment 148885 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148885&action=review Thanks Ray, this is looking pretty good. Eric wanted these files in the media directory (without sub-directories) and it looks like you haven't yet done this > LayoutTests/webaudio/codec-tests/aac/vbr-128kbps-44khz.html:10 > + var test = ["../../resources/media/vbr-128kbps-44khz.m4a"]; It's odd to create a list with a single element which is a string. Why not simplify this to a simple string. Then you'll have to wrap it up as a list inside the runDecodingTest() impl. > LayoutTests/webaudio/codec-tests/vorbis/vbr-96kbps-44khz.html:9 > + // Test vorbis decoder, ~96kbps. Since your file-names now have all the information in them, many of these comments seem unnecessary. > LayoutTests/webaudio/resources/audio-codec-test.js:9 > +function runDecodingTest(testInfo, optionalRate) "testInfo" seems like an odd name. It's actually the "filename" or "url" of the test file "optionalRate" is inconsistent in naming compared with "defaultSampleRate" above > LayoutTests/webaudio/resources/audio-codec-test.js:29 > +function finishedLoading(bufferList) { WebKit style nit: braces for functions are supposed to go on the next line and the style for this function is inconsistent with the brace-style in the runDecodingTest() function above.

Raymond Toy

Comment 36 2012-06-21 15:07:45 PDT

(In reply to comment #35) > (From update of attachment 148885 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148885&action=review > > Thanks Ray, this is looking pretty good. Eric wanted these files in the media directory (without sub-directories) and it looks like you haven't yet done this Which files are we talking about here? And which media directory? I'll fix up your other comments shortly.

Chris Rogers

Comment 37 2012-06-21 15:36:26 PDT

(In reply to comment #36) > (In reply to comment #35) > > (From update of attachment 148885 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=148885&action=review > > > > Thanks Ray, this is looking pretty good. Eric wanted these files in the media directory (without sub-directories) and it looks like you haven't yet done this > > Which files are we talking about here? And which media directory? Good question. I was thinking that Eric meant the LayoutTests/media/content directory and not in the webaudio directory, but maybe I was confused. Eric? > > I'll fix up your other comments shortly.

Eric Carlson

Comment 38 2012-06-21 16:55:25 PDT

(In reply to comment #37) > (In reply to comment #36) > > (In reply to comment #35) > > > (From update of attachment 148885 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=148885&action=review > > > > > > Thanks Ray, this is looking pretty good. Eric wanted these files in the media directory (without sub-directories) and it looks like you haven't yet done this > > > > Which files are we talking about here? And which media directory? > > Good question. I was thinking that Eric meant the LayoutTests/media/content directory and not in the webaudio directory, but maybe I was confused. > > Eric? > > > > > > I'll fix up your other comments shortly. I was suggesting the approach that Ray has taken: creating a new directory in webaudio/resources/. I think it would be good to move the rest of the media test files (not results) into that directory in a followup patch so they are not mixed in with the tests and results.

Raymond Toy

Comment 39 2012-06-22 08:58:11 PDT

Created attachment 149036 [details] Patch

Raymond Toy

Comment 40 2012-06-22 09:03:50 PDT

Comment on attachment 148885 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148885&action=review >> LayoutTests/webaudio/codec-tests/aac/vbr-128kbps-44khz.html:10 >> + var test = ["../../resources/media/vbr-128kbps-44khz.m4a"]; > > It's odd to create a list with a single element which is a string. Why not simplify this to a simple string. > Then you'll have to wrap it up as a list inside the runDecodingTest() impl. Done. >> LayoutTests/webaudio/codec-tests/vorbis/vbr-96kbps-44khz.html:9 >> + // Test vorbis decoder, ~96kbps. > > Since your file-names now have all the information in them, many of these comments seem unnecessary. Removed everywhere, except for the resampling test (24bit-22khz-resample) where we make it more explicit that resampling is being done. >> LayoutTests/webaudio/resources/audio-codec-test.js:9 >> +function runDecodingTest(testInfo, optionalRate) > > "testInfo" seems like an odd name. It's actually the "filename" or "url" of the test file > > "optionalRate" is inconsistent in naming compared with "defaultSampleRate" above Changed testInfo to url (and renamed from test to rul in the html files). Renamed optionalRate to optionalSampleRate. >> LayoutTests/webaudio/resources/audio-codec-test.js:29 >> +function finishedLoading(bufferList) { > > WebKit style nit: braces for functions are supposed to go on the next line and the style for this function is inconsistent with the brace-style in the runDecodingTest() function above. Fixed.

Raymond Toy

Comment 41 2012-06-22 09:05:00 PDT

(In reply to comment #38) > (In reply to comment #37) > > (In reply to comment #36) > > > (In reply to comment #35) > > > > (From update of attachment 148885 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=148885&action=review > > > > > > > > Thanks Ray, this is looking pretty good. Eric wanted these files in the media directory (without sub-directories) and it looks like you haven't yet done this > > > > > > Which files are we talking about here? And which media directory? > > > > Good question. I was thinking that Eric meant the LayoutTests/media/content directory and not in the webaudio directory, but maybe I was confused. > > > > Eric? > > > > > > > > > > I'll fix up your other comments shortly. > > I was suggesting the approach that Ray has taken: creating a new directory in webaudio/resources/. I think it would be good to move the rest of the media test files (not results) into that directory in a followup patch so they are not mixed in with the tests and results. I'll move the other media files to webaudio/resources/media in a separate patch soon.

WebKit Review Bot

Comment 42 2012-06-22 09:59:13 PDT

Comment on attachment 149036 [details] Patch Attachment 149036 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13085042 New failing tests: webaudio/codec-tests/aac/vbr-128kbps-44khz.html webaudio/codec-tests/mp3/128kbps-44khz.html

WebKit Review Bot

Comment 43 2012-06-22 09:59:20 PDT

Created attachment 149046 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Chris Rogers

Comment 44 2012-06-22 10:15:26 PDT

Comment on attachment 149036 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149036&action=review > LayoutTests/webaudio/resources/audio-codec-test.js:17 > + context = new webkitAudioContext(1, sampleRate * lengthInSeconds, sampleRate); can use regular AudioContext here > LayoutTests/webaudio/resources/audio-codec-test.js:31 > + var bufferSource = context.createBufferSource(); Sorry I didn't catch this earlier, but it's not necesssary to use an offline context here with createBufferSource(), noteOn(), etc. Since you already have the buffer at this point, why go through the hassle of playing it through an AudioBufferSourceNode and capturing the result in another buffer? We should be able to simply call: layoutTestController.setAudioData(bufferList[0]); layoutTestController.notifyDone();

Raymond Toy

Comment 45 2012-06-22 10:32:06 PDT

(In reply to comment #44) > (From update of attachment 149036 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=149036&action=review > > > LayoutTests/webaudio/resources/audio-codec-test.js:17 > > + context = new webkitAudioContext(1, sampleRate * lengthInSeconds, sampleRate); > > can use regular AudioContext here Won't this mean the test is at the mercy of whatever the audio setup is of the machine running the test? As it is now, I think the audiocontext defaults to 48kHz on my linux machine, but all of the test files are 44.1 (or 22.05) kHz, so resampling always happens.

Chris Rogers

Comment 46 2012-06-22 11:45:05 PDT

(In reply to comment #45) > (In reply to comment #44) > > (From update of attachment 149036 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=149036&action=review > > > > > LayoutTests/webaudio/resources/audio-codec-test.js:17 > > > + context = new webkitAudioContext(1, sampleRate * lengthInSeconds, sampleRate); > > > > can use regular AudioContext here > > Won't this mean the test is at the mercy of whatever the audio setup is of the machine running the test? As it is now, I think the audiocontext defaults to 48kHz on my linux machine, but all of the test files are 44.1 (or 22.05) kHz, so resampling always happens. Yes, sorry you're right. We should continue to construct an "offline" context passing in the sample-rate. But then simply use it to decode and not play using AudioBufferSourceNode.

Raymond Toy

Comment 47 2012-06-22 14:58:31 PDT

Created attachment 149111 [details] Patch

Raymond Toy

Comment 48 2012-06-22 15:03:01 PDT

Comment on attachment 149036 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149036&action=review >> LayoutTests/webaudio/resources/audio-codec-test.js:31 >> + var bufferSource = context.createBufferSource(); > > Sorry I didn't catch this earlier, but it's not necesssary to use an offline context here with createBufferSource(), noteOn(), etc. > Since you already have the buffer at this point, why go through the hassle of playing it through an AudioBufferSourceNode and capturing the result in another buffer? > > We should be able to simply call: > layoutTestController.setAudioData(bufferList[0]); > layoutTestController.notifyDone(); Fixed not to use AudioBufferSourceNode.

Raymond Toy

Comment 49 2012-06-22 15:08:51 PDT

Remove dependency on 89706l; just disable proprietary codecs for chromium.

Raymond Toy

Comment 50 2012-06-22 17:09:01 PDT

Created attachment 149143 [details] Patch

Raymond Toy

Comment 51 2012-06-22 21:50:59 PDT

Created attachment 149161 [details] Patch

Raymond Toy

Comment 52 2012-06-25 10:42:45 PDT

Created attachment 149321 [details] Patch

Chris Rogers

Comment 53 2012-06-25 14:17:13 PDT

Comment on attachment 149321 [details] Patch Thanks Ray!

WebKit Review Bot

Comment 54 2012-06-25 14:46:26 PDT

Comment on attachment 149321 [details] Patch Clearing flags on attachment: 149321 Committed r121182: <http://trac.webkit.org/changeset/121182>

WebKit Review Bot

Comment 55 2012-06-25 14:47:02 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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component Tools / Tests

Assignee

Raymond Toy

Reported

2012-06-11 11:14 PDT

Modified

2012-06-25 14:47 PDT History

CC List

9 users Show

URL

Keywords

Depends on

Blocks