Bug 57969 - Add web audio support to DumpRenderTree (mac port)
Summary: Add web audio support to DumpRenderTree (mac port)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 57977
  Show dependency treegraph
 
Reported: 2011-04-06 12:15 PDT by Chris Rogers
Modified: 2011-04-07 10:02 PDT (History)
5 users (show)

See Also:


Attachments
Patch (6.87 KB, patch)
2011-04-06 12:18 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff
Patch (12.77 KB, patch)
2011-04-06 14:23 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff
Patch (13.10 KB, patch)
2011-04-06 15:07 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff
Patch (7.24 KB, patch)
2011-04-06 15:36 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Rogers 2011-04-06 12:15:23 PDT
Add web audio support to DumpRenderTree (mac port)
Comment 1 Chris Rogers 2011-04-06 12:18:04 PDT
Created attachment 88485 [details]
Patch
Comment 2 Chris Rogers 2011-04-06 12:24:35 PDT
The web audio API is in need of comprehensive layout tests.  The approach taken is for each test to render a short amount of audio (1 to 10 seconds) and compare with a reference .wav file.

This patch adds a new "setEncodedAudioData()" method to the LayoutTestController.  The JS in the test will encode the generated audio result as WAVE file data, then base64 encode it as a string to be passed in to setEncodedAudioData();

This patch currently is only for the mac port, but other ports will follow (chromium next).

I'll put up another patch illustrating an actual layout test using this new system.
Comment 3 Chris Rogers 2011-04-06 13:44:39 PDT
Here's a patch for my first layout test to use the new setEncodedAudioData() method.
Comment 4 Chris Rogers 2011-04-06 13:45:19 PDT
Sorry, and the URL for that patch is:

https://bugs.webkit.org/show_bug.cgi?id=57977
Comment 5 Tony Chang 2011-04-06 14:03:56 PDT
Comment on attachment 88485 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88485&action=review

I think it's OK to put the layout test and the changes to DRT in the same patch.

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:948
> +        if (gLayoutTestController->dumpAsAudio()) {

How big is the -expected.txt?  How horrible would it be to just console.log (or append to a div) the base64 data in the test?  The main benefit would be not having to introduce dumpAsAudio() or remember to call it.  You could also have setEncodedAudioData() write the base64 data via printf (we do this for other delegates like the editing delegate).
Comment 6 Tony Chang 2011-04-06 14:04:29 PDT
BTW, I think the general approach (setEncodedAudioData()) is fine.
Comment 7 Dirk Pranke 2011-04-06 14:08:37 PDT
(In reply to comment #5)
> (From update of attachment 88485 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88485&action=review
> 
> I think it's OK to put the layout test and the changes to DRT in the same patch.
> 
> > Tools/DumpRenderTree/mac/DumpRenderTree.mm:948
> > +        if (gLayoutTestController->dumpAsAudio()) {
> 
> How big is the -expected.txt?  How horrible would it be to just console.log (or append to a div) the base64 data in the test?  The main benefit would be not having to introduce dumpAsAudio() or remember to call it.  You could also have setEncodedAudioData() write the base64 data via printf (we do this for other delegates like the editing delegate).

I think the main downside of printing the audio as base64 text is that it makes it harder to actually do something with it if we get a diff.

My thinking with the separate file was that NRWT could de-base64 it and save it as wav files which can be played directly to compare with the reference.
Comment 8 Chris Rogers 2011-04-06 14:13:59 PDT
The interesting thing is that the expected result will not be *text* but will be a WAVE file.  This will be similar to the approach we take for our pixel tests.  That way people can actually hear the expected and generated results which is very useful when trying to debug problems in the audio code.  Also, re-baselining will be less error prone since the new results can be audibly compared against the old.

I think it's best to have setEncodedAudioData() in DRT so that the 'audio/wav' mime-type can be written in the header.  That way run-webkit-tests can do the right thing (knows to decode64 and compare against a reference .wav file...)
Comment 9 Dirk Pranke 2011-04-06 14:17:05 PDT
Comment on attachment 88485 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88485&action=review

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:950
> +            resultMimeType = @"audio/wav";

I suppose if we were being really picky we'd mark this with a Content-Transfer-Encoding header as well.
Comment 10 Chris Rogers 2011-04-06 14:23:22 PDT
Created attachment 88511 [details]
Patch
Comment 11 Tony Chang 2011-04-06 14:39:02 PDT
From discussion on IRC, the expected result would be test-name-expected.wav and we would have to update ORWT/NRWT to know about these.

Dirk said he plans on adding this to NRWT.

I feel that we shouldn't keep adding tests that only get run by Chromium (like reftests).  This means that either we add this support to ORWT too or we wait on adding audio tests until everyone has switched to NRWT.
Comment 12 Tony Chang 2011-04-06 14:42:50 PDT
*** Bug 57977 has been marked as a duplicate of this bug. ***
Comment 13 Chris Rogers 2011-04-06 15:02:04 PDT
Tony, agreed about not adding a bunch of audio tests until it all works on ORWT, but I'd still to make some forward progress by getting this patch in.
Comment 14 Chris Rogers 2011-04-06 15:07:23 PDT
Created attachment 88523 [details]
Patch
Comment 15 Chris Rogers 2011-04-06 15:08:19 PDT
Latest patch includes:

Content-Transfer-Encoding: base64

in the header
Comment 16 Tony Chang 2011-04-06 15:14:59 PDT
Comment on attachment 88523 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88523&action=review

Sorry, I misunderstood that this required changes to NRWT/ORWT.  Can you re-upload without the test?

> Tools/DumpRenderTree/LayoutTestController.h:271
> +    void setEncodedAudioData(std::string encodedAudioData) { m_encodedAudioData = encodedAudioData; }

const std::string&

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:739
> +    const char* encodedAudioData = gLayoutTestController->encodedAudioData().c_str();

I think in ObjC we put the * next to the variable name.  This file seems like a mix of both styles.

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:741
> +    NSData *data = [NSData dataWithBytes:encodedAudioData length:strlen(encodedAudioData)];

If encodedAudioData has null bytes in it, won't strlen give the wrong value?  Should we use gLayoutTestController->encodedAudioData().length() instead?
Comment 17 Chris Rogers 2011-04-06 15:35:06 PDT
Comment on attachment 88523 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88523&action=review

>> Tools/DumpRenderTree/LayoutTestController.h:271
>> +    void setEncodedAudioData(std::string encodedAudioData) { m_encodedAudioData = encodedAudioData; }
> 
> const std::string&

FIXED

>> Tools/DumpRenderTree/mac/DumpRenderTree.mm:739
>> +    const char* encodedAudioData = gLayoutTestController->encodedAudioData().c_str();
> 
> I think in ObjC we put the * next to the variable name.  This file seems like a mix of both styles.

FIXED

>> Tools/DumpRenderTree/mac/DumpRenderTree.mm:741
>> +    NSData *data = [NSData dataWithBytes:encodedAudioData length:strlen(encodedAudioData)];
> 
> If encodedAudioData has null bytes in it, won't strlen give the wrong value?  Should we use gLayoutTestController->encodedAudioData().length() instead?

because it's base64 data that shouldn't be a problem.  However, your approach seems better anyway - so fixing.
Comment 18 Chris Rogers 2011-04-06 15:36:49 PDT
Created attachment 88533 [details]
Patch
Comment 19 Chris Rogers 2011-04-06 15:37:41 PDT
latest patch addresses comments and removes the actual layout test
Comment 20 Dirk Pranke 2011-04-06 17:49:40 PDT
We need one minor change to make NRWT happy .. we should be sending a Content-Length: header as well  as the Content-Type and Content-Transfer-Encoding headers when we are dumping the audio.

Here's a diff:


layout_tests $ git diff f887b1
diff --git a/Tools/DumpRenderTree/mac/DumpRenderTree.mm b/Tools/DumpRenderTree/mac/DumpRenderTree
index 914fc10..1f485af 100644
--- a/Tools/DumpRenderTree/mac/DumpRenderTree.mm
+++ b/Tools/DumpRenderTree/mac/DumpRenderTree.mm
@@ -971,9 +971,11 @@ void dump()
 
         printf("Content-Type: %s\n", [resultMimeType UTF8String]);
 
-        if (gLayoutTestController->dumpAsAudio())
-            printf("Content-Transfer-Encoding: base64\n");            
-        
+        if (gLayoutTestController->dumpAsAudio()) {
+            printf("Content-Transfer-Encoding: base64\n");
+            printf("Content-Length: %ld\n", [resultData length]);
+        }
+      
         if (resultData) {
             fwrite([resultData bytes], 1, [resultData length], stdout);
Comment 21 Dirk Pranke 2011-04-06 18:19:32 PDT
(In reply to comment #20)
> +            printf("Content-Length: %ld\n", [resultData length]);

Actually, this should be a %lu. We should really be printing two '\n's here, so that there's a blank line between the headers and the data, but it doesn't look like DRT does this for image data, so for consistency it should probably just be one here as well.

Note that the patch to NRWT in bug 57987 has been tested against this patch (with the content-length header added) and verified to work.
Comment 22 WebKit Commit Bot 2011-04-06 20:55:36 PDT
Comment on attachment 88533 [details]
Patch

Clearing flags on attachment: 88533

Committed r83138: <http://trac.webkit.org/changeset/83138>
Comment 23 WebKit Commit Bot 2011-04-06 20:55:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Tony Chang 2011-04-07 10:02:08 PDT
Comment on attachment 88523 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88523&action=review

> LayoutTests/webaudio/audio-testing.js:7
> +function encode64(input) {

Before I forget, base64 encoding is a built in function in javascript: btoa.