Bug 57969

Created attachment 88485 [details] Patch

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.

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

Sorry, and the URL for that patch is: https://bugs.webkit.org/show_bug.cgi?id=57977

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

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

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

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

Created attachment 88511 [details] Patch

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.

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

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.

Created attachment 88523 [details] Patch

Latest patch includes: Content-Transfer-Encoding: base64 in the header

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

Created attachment 88533 [details] Patch

latest patch addresses comments and removes the actual layout test

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

(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 on attachment 88533 [details] Patch Clearing flags on attachment: 88533 Committed r83138: <http://trac.webkit.org/changeset/83138>

All reviewed patches have been landed. Closing bug.

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.