If startRecorder fails, the recorder is left in an invalid, unrecoverable state

[jbaudanza]

It's common for either MediaRecorder.start() or MediaRecorder.prepare() to fail if the device doesn't support the requested codec, or there is some problem with the arguments.

Currently, if this happens, the recorder is left in an invalid state:

index.ts

If an error occurs, this._isRecording is still set to true, which is incorrect. This PR will catch an error and reset this boolean to false

RNAudioRecorderPlayerModule.kt

If an error occurs, this.mediaRecorder will be set to partially or incorrectly initialized object. The next time startRecord() is called, this stale object will be reused, which caused a secondary (and more confusing) error.

To fix this, startRecord will catch any initialization error and reset this.mediaRecorder to null.

[isekovanic]

Just did a bunch of debugging and came to the same conclusion, for the index.ts bit at least. It also happens on iOS whenever you decline the microphone capability permission and then immediately try recording again (without unmounting whatever's wrapping the recorder so that the internal state gets reset). this._isRecording is true forever and the permission check is not done again for this.startRecorder to fail properly and handled downstream. It is then left in a state where it thinks it's recording and you can't do anything about it, since:

• this.stopRecorder fails on the native level (because of this check, self.audioRecorder is nil irrecoverably)
• The JS wrapper thinks a recording is in progress and is never going to propagate the promise rejection that's supposed to be returned from this.startRecorder

Not sure if this is useful to anyone, but as a workaround I'm doing something along these lines currently:

try {  
  // some logic
} catch (err) {
  // some error handling logic
  audioRecorderPlayer._isRecording = false;
  audioRecorderPlayer._hasPausedRecord = false;
}

which I'm not particularly a fan of since I'm essentially messing with local state of the class.

Also, I'm pretty certain that this and this method will also likely produce the same issue if something fails on the native side.

So yeah, huge +1 for this @jbaudanza !

PS: Does it make sense to pull these changes out in a separate PR ? Would be happy to do it if so !

[jbaudanza]

Also, I'm pretty certain that this and this method will also likely produce the same issue if something fails on the native side.

So yeah, huge +1 for this @jbaudanza !

PS: Does it make sense to pull these changes out in a separate PR ? Would be happy to do it if so !

Yeah I agree. Any asynchronous operation should have a fallback to reset the state if there is an error.

Maybe cleaning up the js should be done in a separate PR, since this one already has a lot of native changes.

[hyochan]

Sorry for the late review!
LGTM!

[hyochan]

@jbaudanza Could you kindly check why reset should be removed?

[hyochan]

I'll leave this part as remaining question and will merge this PR! Thanks @jbaudanza

[hyochan]

@isekovanic I'll look into this! Thanks for the comment!

[efstathiosntonas]

Hey folks, does anyone noticed if the audio message being recorded sounds robotic or tinny?

I have several reports from users about it. Downgrading to 3.6.10 fixes the issue so I guess something is probably broken in this PR.

Find attached 2 audio files sharing exactly the same sound source. The good file is 174KB recorded on 3.6.10 while the "bad" is 20KB on 3.6.11 using the same config,codecs.

audio_files.zip

 const audioSet: AudioSet = {
          AudioEncoderAndroid: AudioEncoderAndroidType.AAC,
          AudioSamplingRateAndroid: 44100,
          AudioSourceAndroid: AudioSourceAndroidType.MIC,
          AVEncoderAudioQualityKeyIOS: AVEncoderAudioQualityIOSType.medium,
          AVFormatIDKeyIOS: AVEncodingOption.aac,
          AVNumberOfChannelsKeyIOS: 2,
          OutputFormatAndroid: OutputFormatAndroidType.AAC_ADTS
        };
        meteringEnabled = true;
        stopPlayer().catch((e) =>
          console.log(`error while stopping player of messages bubbles, error: ${e}`)
        );
        await audioRecorderPlayer.startRecorder(path, audioSet, meteringEnabled);

[efstathiosntonas]

After inspecting the changes in this PR it seems some default values are stripped out if not found on audioSet. This does not align with the optional types though, needs to be fixed or add fallback values like it used to be.

The audio quality went back to normal when I added these 2 options in the audioSet:

          AudioEncodingBitRateAndroid: 128000,
          AudioChannelsAndroid: 2,

@hyochan what shall we do here? Make the AudioSet types mandatory to avoid situations like mine or update the RNAudioRecorderPlayerModule.kt with fallback values like the old times?

[jbaudanza]

@efstathiosntonas Thanks for spotting that. I think setting default fallback values would be a good approach