[Web Audio] Added Audio API tests: MediaElementAudioSource interface

[amitayd]

• Test with source output to OfflineAudioContext
• Test with source output to ScriptProcessor
• Added trimEmptyElements() to helpers.js

[hoppipolla-critic-bot]

Critic review: https://critic.hoppipolla.co.uk/r/386

This is an external review system which you may optionally use for the code review of your pull request.

[amitayd]

Needs review (possibly by @chrislo)

1. Implemented testing the MediaElementAudioSource node output by two ways:

   • OfflineAudioContext - script is less straight forward, but some implementations (Firefox) do no support createMediaElementSource on an offline context
   • ScriptProcessor node

   It should be possible to combine two methods to a single test file.

2. Timing precisely the start/end time of the source node samples is problematic. Using the media events (i.e. playing/eneded) is not precise enough and might miss samples.
   Achieved it by trimming expected/actual sample arrays for trailing/ending empty samples.

3. I'm not sure the behavior of a MediaElementAudioSource in relation with OfflineAudioContext is well defined. Should the output be rendered live or sample by sample (i.e. if there's a pause in the streaming, what should be the input for context.destination)?
   I think this might be the reason that Chrome implementation sometimes fails and sometimes passes.

4. Included tests for just the beginning of the recorded data, since some (all?) implementations contain some unexplained noise in the end of the output using OfflineAudioContext.

5. Not using the harness setup() method, since it hides errors and considering the setup phase optional.

6. Not using the harness test.step, but separate test(), since it is valuable to see which steps failed separately, and using step(), they are all under a single test (i.e. one fail => all fail)

Thanks,
Amitay

[chrislo]

Yep, I'll take a look at this. Might take me a few days, but I'll get back to you. Thanks for the PR!

[amitayd]

Great, whenever you have the time.

[chrislo]

I think I see why this function is required - it's because calling play on an audio element doesn't happen at a predictable time?

In the case of these tests I think it's required, but we should be careful not to use this function as a crutch.

[amitayd]

Correct, couldn't get enough precision (per sample) with the media events such as play. Would be happy to get rid of this function, but couldn't find a better way to compare the actual play/stop of the media.

[chrislo]

This looks really good, thank you. I've made a few comments, but primarily

• remove the script processor-based test, I think it's ok for this to fail on FF at the moment if they don't support mediaelementsourcenode in an offlineAudioContext but it is specified. If you have a particular concern about FF's support, maybe we could re-use your code to raise a ticket with their implementation?
• switch to single-channel wav file if possible, and document or add a script showing how the wav file was generated.

I really appreciate your contribution, thank you.

[amitayd]

Thanks for the comments! will work on that and update the pull request.

Regarding the first main issue:

remove the script processor-based test, I think it's ok for this to fail on FF at the moment if they don't support mediaelementsourcenode in an offlineAudioContext but it is specified".

As I wrote in my notes:

I'm not sure the behavior of a MediaElementAudioSource in relation with OfflineAudioContext is well defined. Should the output be rendered live or sample by sample (i.e. if there's a pause in the streaming, what should be the input for context.destination)?
I think this might be the reason that Chrome implementation sometimes fails and sometimes passes.

Also asked it in the public-audio list. How should a real-time streaming source behave in regards to an offlineAudioContext rendering?

I'm probably missing it in the spec, but could you point that out for me?

thanks,
Amitay

[amitayd]

Updated the pull request with a new commit. Addressed the following issues:

1. Wave file is generated using an included browser based utlitiy
2. Wave file and tests are all single channel (mono).
3. Removed some dead code.

I didn't remove the test based on ScriptProcessor, as it exposes other failures in the implementations (compared to the OfflineAudioContext test). For example, in that test under Chrome, the scriptProcessor gets some additional data, not from the wave file, in its processaudio event input buffer.

If you'd insist, of course I can remove that test, but I'm not sure why wouldn't we want to have as much tests as possible, especially when its demonstrated that those test results are not redundant.

Thanks,
Amitay

[amitayd]

@chrislo
Any update on this? Let me know if you need anything in addition to the last commit (see the last notes)

[chrislo]

@amitayd there was some discussion about whether offlineAudioContext should even allow the creation of mediaElementAudioSource node and the streaming source and destination nodes. I've re-raised that discussion on the mailing list. Thank you for bringing it to our attention again!

In the meantime, I'll add the ScriptProcessor based test you wrote to the test suite.

[chrislo]

Merged into master, thank you.