Bug 88794 - Add layout tests for audio codecs
Summary: Add layout tests for audio codecs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Raymond Toy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-11 11:14 PDT by Raymond Toy
Modified: 2012-06-25 14:47 PDT (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Raymond Toy 2012-06-11 11:14:12 PDT
Add layout tests for audio codecs
Comment 1 Raymond Toy 2012-06-11 15:50:42 PDT
Created attachment 146940 [details]
Patch
Comment 2 Eric Carlson 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.
Comment 3 WebKit Review Bot 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
Comment 4 WebKit Review Bot 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
Comment 5 Chris Rogers 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?
Comment 6 Raymond Toy 2012-06-13 09:42:19 PDT
Created attachment 147340 [details]
Patch
Comment 7 Raymond Toy 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.
Comment 8 Eric Carlson 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.
Comment 9 Eric Carlson 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!
Comment 10 Raymond Toy 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>
Comment 11 Dirk Pranke 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.
Comment 12 Raymond Toy 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?
Comment 13 Raymond Toy 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.
Comment 14 Chris Rogers 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.
Comment 15 Raymond Toy 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.
Comment 16 Raymond Toy 2012-06-13 14:11:58 PDT
Created attachment 147408 [details]
Patch
Comment 17 Raymond Toy 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.
Comment 18 Eric Carlson 2012-06-13 17:00:28 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?

Apple ports don't support ogg (by default at least), but the other formats should be OK.
Comment 19 Eric Carlson 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.
Comment 20 Andrew Scherkus 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?
Comment 21 Raymond Toy 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.
Comment 22 Raymond Toy 2012-06-14 09:34:14 PDT
Created attachment 147603 [details]
Patch
Comment 23 Raymond Toy 2012-06-20 10:25:31 PDT
Created attachment 148595 [details]
Patch
Comment 24 Raymond Toy 2012-06-20 10:27:10 PDT
Reworked patch.  Tests are now bit-exact, with different expected results for different platforms.
Comment 25 WebKit Review Bot 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
Comment 26 WebKit Review Bot 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
Comment 27 Chris Rogers 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.
Comment 28 Eric Carlson 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.
Comment 29 Raymond Toy 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?
Comment 30 Raymond Toy 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.
Comment 31 Chris Rogers 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"
Comment 32 Raymond Toy 2012-06-21 14:05:20 PDT
Created attachment 148885 [details]
Patch
Comment 33 Raymond Toy 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.
Comment 34 Raymond Toy 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.
Comment 35 Chris Rogers 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.
Comment 36 Raymond Toy 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.
Comment 37 Chris Rogers 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.
Comment 38 Eric Carlson 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.
Comment 39 Raymond Toy 2012-06-22 08:58:11 PDT
Created attachment 149036 [details]
Patch
Comment 40 Raymond Toy 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.
Comment 41 Raymond Toy 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.
Comment 42 WebKit Review Bot 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
Comment 43 WebKit Review Bot 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
Comment 44 Chris Rogers 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();
Comment 45 Raymond Toy 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.
Comment 46 Chris Rogers 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.
Comment 47 Raymond Toy 2012-06-22 14:58:31 PDT
Created attachment 149111 [details]
Patch
Comment 48 Raymond Toy 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.
Comment 49 Raymond Toy 2012-06-22 15:08:51 PDT
Remove dependency on 89706l; just disable proprietary codecs for chromium.
Comment 50 Raymond Toy 2012-06-22 17:09:01 PDT
Created attachment 149143 [details]
Patch
Comment 51 Raymond Toy 2012-06-22 21:50:59 PDT
Created attachment 149161 [details]
Patch
Comment 52 Raymond Toy 2012-06-25 10:42:45 PDT
Created attachment 149321 [details]
Patch
Comment 53 Chris Rogers 2012-06-25 14:17:13 PDT
Comment on attachment 149321 [details]
Patch

Thanks Ray!
Comment 54 WebKit Review Bot 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>
Comment 55 WebKit Review Bot 2012-06-25 14:47:02 PDT
All reviewed patches have been landed.  Closing bug.