search

google / blockly-android

Blockly for Android
Apache License 2.0
672 stars 209 forks source link

Adds audio files and polyphonic audio player to the CodeLab starter. #634

Closed AnmAtAnm closed 6 years ago

AnmAtAnm commented 6 years ago

Adds audio files and polyphonic audio player (i.e., multiple files played at the same time).

This change is Reviewable

RoboErikG commented 6 years ago

Review status: 0 of 14 files reviewed at latest revision, 2 unresolved discussions.

Codelab/codelab-starter/src/main/java/com/google/blockly/codelab/AudioFilePlayer.java, line 119 at r1 (raw file):

     */
    private void findOrCreatePlayer(
            final String filename, @Nullable final MediaPlayer.OnPreparedListener callback)

callback should be @NonNull since there's no other way to get a reference to the MediaPlayer that was returned.

Codelab/codelab-starter/src/main/java/com/google/blockly/codelab/AudioFilePlayer.java, line 128 at r1 (raw file):


        if (list.isEmpty()) {
            MediaPlayer player = new MediaPlayer();

You're missing an error listener. You might also be interested in a MediaPlayerManager class I wrote a while back if you're using MediaPlayer. https://googleplex-android-review.git.corp.google.com/#/c/324160/3/Tuq/src/com/google/android/aah/tuq/player/MediaPlayerManager.java

A better option might be to use SoundPool since you're playing multiple short sounds. https://developer.android.com/reference/android/media/SoundPool.html

Comments from Reviewable

AnmAtAnm commented 6 years ago

PTAL Complete rewrite, now based on SoundPool.

RoboErikG commented 6 years ago

:lgtm_strong: One nit

Review status: 0 of 15 files reviewed at latest revision, 3 unresolved discussions.

Codelab/codelab-starter/src/main/java/com/google/blockly/codelab/MultiFileAudioPlayer.java, line 50 at r2 (raw file):

        mSoundPool = new SoundPool.Builder()
                .setAudioAttributes(audioAttrs)
                .setMaxStreams(9)  // One per button

Since you're overlapping notes I think you want this to be two per button for the worst case that won't really happen. ;)

Comments from Reviewable

AnmAtAnm commented 6 years ago

Review status: 0 of 15 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

Codelab/codelab-starter/src/main/java/com/google/blockly/codelab/MultiFileAudioPlayer.java, line 50 at r2 (raw file):

Previously, RoboErikG wrote…
Since you're overlapping notes I think you want this to be two per button for the worst case that won't really happen. ;)

Good catch. Fixed.

Comments from Reviewable