Bug 92620 – Add test for decodeAudioData

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Li Yin]

Spec: https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html#AudioContext-section

[Li Yin]

Created attachment 155749 [details]
Patch

[Li Yin]

The purpose of designing this test is to check if callback function can be invoked correctly.
For the successCallback scenario, I can't find an audio file which can be decoded successfully in all platforms.

The test can work correctly in chromium-linux platform, but I don't verify the result in chromium-win.
In addition, the test is time out for chromium on MAC, I think the root cause is that it doesn't support ogg decoding.(If I am wrong, please feel free to correct me.)

Hi Raymond,
   You designed codec tests, could you please give some advices? Thanks in advance.

[Li Yin]

Created attachment 155753 [details]
Patch

[Li Yin]

Created attachment 155792 [details]
Patch

[Li Yin]

(In reply to comment #2)
> The purpose of designing this test is to check if callback function can be invoked correctly.
> For the successCallback scenario, I can't find an audio file which can be decoded successfully in all platforms.

Sorry for the incorrect information, ogg format file should be decoded correctly for chromium on Linux, MAC and Windows.

> 
> The test can work correctly in chromium-linux platform, but I don't verify the result in chromium-win.
> In addition, the test is time out for chromium on MAC, I think the root cause is that it doesn't support ogg decoding.(If I am wrong, please feel free to correct me.)

Sorry, the test can work correctly for chromium on MAC.

[Raymond Toy]

Comment on attachment 155792 [details]
Patch

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

Just a few minor comments on implementation.  Otherwise, the test looks good.

> LayoutTests/ChangeLog:14
> +        * webaudio/resources/media/error-format.txt: Added.

Perhaps a more obvious name would be good.  Something like invalid-audio-file.txt.  

Might also be useful to have a real ogg (or mp3 or aac) file that we've manually corrupted in some way.  There are also some Microsoft ADPCM wav files that we cannot decode because it's not supported by chromium.

> LayoutTests/webaudio/decode-audio-data-basic.html:36
> +    debug("The " + index + " test: " + decodeCaseArray[index][0]);

Is this debug needed?  I think it should be removed.  You can add this information in the testPassed/testFailed messages.

> LayoutTests/webaudio/decode-audio-data-basic.html:47
> +                testPassed("successCallback has been called correctly.");

As mentioned above, the message can include information about the test.  I think that's better than the debug call above.

> LayoutTests/webaudio/decode-audio-data-basic.html:48
> +                runDecodeTest(++index);

Do you need to call runDecodeTest here?  Can you just call runDecodeTest(1) at the end of the file?

> LayoutTests/webaudio/decode-audio-data-basic.html:51
> +                finishedJSTest();

Can finishedJSTest() be called at the end of the file, after calling runDecodeTest(1)?

> LayoutTests/webaudio/decode-audio-data-basic.html:58
> +                finishJSTest();

Same comment as for line 51.

> LayoutTests/webaudio/decode-audio-data-basic.html:61
> +                runDecodeTest(++index);

Same comment as for line 48.

> LayoutTests/webaudio/decode-audio-data-basic.html:68
> +runDecodeTest(0);

Can we just do

runDecodeTest(0);
runDecodeTest(1);
finishJSTest();

I think this makes it clearer which tests are being run.  It looks like with the current code, if runDecodeTest(0) fails, the remaining (one) test isn't run.  I think it's beneficial to see all the test run, independent of whether a previous tests fails or not.

[Li Yin]

Created attachment 155944 [details]
Patch

[Li Yin]

(In reply to comment #6)
> (From update of attachment 155792 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155792&action=review
> 
> Just a few minor comments on implementation.  Otherwise, the test looks good.
> 
> > LayoutTests/ChangeLog:14
> > +        * webaudio/resources/media/error-format.txt: Added.
> 
> Perhaps a more obvious name would be good.  Something like invalid-audio-file.txt. 

Fixed.

> 
> Might also be useful to have a real ogg (or mp3 or aac) file that we've manually corrupted in some way.  There are also some Microsoft ADPCM wav files that we cannot decode because it's not supported by chromium.
> 
> > LayoutTests/webaudio/decode-audio-data-basic.html:36
> > +    debug("The " + index + " test: " + decodeCaseArray[index][0]);
> 
> Is this debug needed?  I think it should be removed.  You can add this information in the testPassed/testFailed messages.
> 
> > LayoutTests/webaudio/decode-audio-data-basic.html:47
> > +                testPassed("successCallback has been called correctly.");
> 
> As mentioned above, the message can include information about the test.  I think that's better than the debug call above.
> 

Fixed.

> > LayoutTests/webaudio/decode-audio-data-basic.html:48
> > +                runDecodeTest(++index);
> 
> Do you need to call runDecodeTest here?  Can you just call runDecodeTest(1) at the end of the file?
> 
> > LayoutTests/webaudio/decode-audio-data-basic.html:51
> > +                finishedJSTest();
> 
> Can finishedJSTest() be called at the end of the file, after calling runDecodeTest(1)?
> 
> > LayoutTests/webaudio/decode-audio-data-basic.html:58
> > +                finishJSTest();
> 
> Same comment as for line 51.
> 
> > LayoutTests/webaudio/decode-audio-data-basic.html:61
> > +                runDecodeTest(++index);
> 
> Same comment as for line 48.
> 
> > LayoutTests/webaudio/decode-audio-data-basic.html:68
> > +runDecodeTest(0);
> 
> Can we just do
> 
> runDecodeTest(0);
> runDecodeTest(1);
> finishJSTest();
> 
> I think this makes it clearer which tests are being run.  It looks like with the current code, if runDecodeTest(0) fails, the remaining (one) test isn't run.  I think it's beneficial to see all the test run, independent of whether a previous tests fails or not.

Fixed, I agree with you that it is better to run all the test no matter what is failed. In addition, I want to let the tests run one by one, and another test can be run after the callback is invoked.
So I will keep it unchanged. And I think it is better for the expandablity.
And in some other module such as WebSocket, this style is often used.

[Chris Rogers]

Li, great test!  I haven't looked fully at the patch yet, but would highly recommend using a .wav file instead of .ogg as the success case.  Not all ports (like the mac port) support .ogg, but most would support .wav

[Li Yin]

Created attachment 155953 [details]
Patch

[Li Yin]

(In reply to comment #9)
> Li, great test!  I haven't looked fully at the patch yet, but would highly recommend using a .wav file instead of .ogg as the success case.  Not all ports (like the mac port) support .ogg, but most would support .wav

Fixed. Thanks for your comments.

[Raymond Toy]

Comment on attachment 155953 [details]
Patch

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

Looks good to me, except for a couple of really minor nits.  Don't have to fix them if you don't want to.

> LayoutTests/webaudio/decode-audio-data-basic.html:47
> +                testFailed("The " + decodeCaseArray[index][0] + " test: successCallback should be called.");

Maybe be more forceful and say 

successCallback was not called.

> LayoutTests/webaudio/decode-audio-data-basic.html:54
> +                testFailed("The " + decodeCaseArray[index][0] + " test: errorCallback should not be called.");

Maybe be more forceful:

errorCallback was incorrectly called.

[Li Yin]

Created attachment 156221 [details]
Patch

[Li Yin]

(In reply to comment #12)
> (From update of attachment 155953 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155953&action=review
> 
> Looks good to me, except for a couple of really minor nits.  Don't have to fix them if you don't want to.
> 
> > LayoutTests/webaudio/decode-audio-data-basic.html:47
> > +                testFailed("The " + decodeCaseArray[index][0] + " test: successCallback should be called.");
> 
> Maybe be more forceful and say 
> 
> successCallback was not called.
> 
> > LayoutTests/webaudio/decode-audio-data-basic.html:54
> > +                testFailed("The " + decodeCaseArray[index][0] + " test: errorCallback should not be called.");
> 
> Maybe be more forceful:
> 
> errorCallback was incorrectly called.

Fixed. Thanks for your comments.

[Chris Rogers]

Comment on attachment 156221 [details]
Patch

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

Looks good overall:

> LayoutTests/webaudio/decode-audio-data-basic.html:28
> +                       ["resources/media/invalid-audio-file.txt", false]];

I think a JSON format might be better, something like:

[[url:"resources/media/24bit-44khz.wav", result:true], ...

Because it will make the test code below easier to read

[Li Yin]

Created attachment 156240 [details]
Patch

[Li Yin]

(In reply to comment #15)
> (From update of attachment 156221 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156221&action=review
> 
> Looks good overall:
> 
> > LayoutTests/webaudio/decode-audio-data-basic.html:28
> > +                       ["resources/media/invalid-audio-file.txt", false]];
> 
> I think a JSON format might be better, something like:
> 
> [[url:"resources/media/24bit-44khz.wav", result:true], ...
> 
> Because it will make the test code below easier to read

Fixed. They indeed make test be easier to read. Thanks.

[Li Yin]

Hi Chris,
   Could you please give an official r+ and c+ if there are no issues? Please feel free to let me know if something is still needed to improved. Thanks in advance.

[Chris Rogers]

Comment on attachment 156240 [details]
Patch

Thanks Li!

[WebKit Review Bot]

Comment on attachment 156240 [details]
Patch

Clearing flags on attachment: 156240

Committed r126458: <http://trac.webkit.org/changeset/126458>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.