Bug 57969 – Add web audio support to DumpRenderTree (mac port)

• Component: New Bugs
• Status: RESOLVED FIXED

[Chris Rogers]

Add web audio support to DumpRenderTree (mac port)

[Chris Rogers]

Created attachment 88485 [details]
Patch

[Chris Rogers]

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.

[Chris Rogers]

Here's a patch for my first layout test to use the new setEncodedAudioData() method.

[Chris Rogers]

Sorry, and the URL for that patch is:

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

[Tony Chang]

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).

[Tony Chang]

BTW, I think the general approach (setEncodedAudioData()) is fine.

[Dirk Pranke]

(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.

[Chris Rogers]

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...)

[Dirk Pranke]

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.

[Chris Rogers]

Created attachment 88511 [details]
Patch

[Tony Chang]

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.

[Tony Chang]

*** Bug 57977 has been marked as a duplicate of this bug. ***

[Chris Rogers]

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.

[Chris Rogers]

Created attachment 88523 [details]
Patch

[Chris Rogers]

Latest patch includes:

Content-Transfer-Encoding: base64

in the header

[Tony Chang]

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?

[Chris Rogers]

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.

[Chris Rogers]

Created attachment 88533 [details]
Patch

[Chris Rogers]

latest patch addresses comments and removes the actual layout test

[Dirk Pranke]

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);

[Dirk Pranke]

(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.

[WebKit Commit Bot]

Comment on attachment 88533 [details]
Patch

Clearing flags on attachment: 88533

Committed r83138: <http://trac.webkit.org/changeset/83138>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

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.