Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add audio playback demo to verify audio fingerprint code works as exp… #33

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jonathanKingston
Copy link
Collaborator

…ected

Copy link
Member

@kdzwinel kdzwinel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done! I left couple of small comments.

features/audio-playback/index.html Outdated Show resolved Hide resolved
this.outputElement.addEventListener('click', this);
this.sampleElement.addEventListener('ended', this, false);

this.offlineAudioContext = new OfflineAudioContext(1, 44100 * 40, 44100);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Webkit is still prefixing both OfflineAudioContext and AudioContext, but IMO we should support it :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think... this should work in Safari but I don't have a copy to check. As mentioned this won't be an issue soon given they've added a commit to webkit to add support for some of these things.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, it still doesn't work on Safari (14.0.3) :( renderedBuffer is undefined (see screenshot). If this can work (Safari supports all relevant APIs) you can use jsbin+browserstack to debug.

undefined

features/audio-playback/index.html Outdated Show resolved Hide resolved
features/audio-playback/index.html Outdated Show resolved Hide resolved
@jonathanKingston jonathanKingston force-pushed the audio-playback-check branch 2 times, most recently from fbbb5d6 to decf8f2 Compare March 17, 2021 15:09
@@ -0,0 +1,2 @@
Audio sample: THROW THE SWITCH by Robert Meyers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for nitpicking but it takes a lot of time for the modified sample to start playing. I'm guessing that it'd be faster if we used a shorter input sample (current one is 6min)?

@kdzwinel
Copy link
Member

nudge nudge - you now have access to Safari I hear :)

@kdzwinel
Copy link
Member

kdzwinel commented Jul 16, 2021

OK, to merge this:

  • let's pick a bit shorter file (1-2min instead of 6min). It takes significant time for the current one to be processed and it's confusing to wait that long. We can also add some performance marks for processing duration calculation since we plan to work on that.
  • let's make the UI clearer. The top control is the original? Let's label it "original sound". It's also unclear what the button does. Does it create copies? Maybe let's make them available as more of those players? clicking button "create a copy" would inject another <audio> to the page and label it "sound copy no 1" etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants