[BUG]: onProgress stream never completes, not sure if intended

[cedvdb]

Flutter sound onProgress stream never completes. Looking at the source, it seem to have been intended this way.

However I believe semantically speaking it should complete. It would allow things like this:

  Stream<RecordingDisposition> record(String path) {
    assert(_recorder.isStopped);
    return Stream.fromFuture(_doRecord(path))
        .switchMap((_) => _recorder.onProgress!)
        .doOnDone(() => _closeRecorderSession());
  }

  Future<void> _doRecord(String path) async {
    await _openRecorderSession();
    await _recorder.setSubscriptionDuration(const Duration(milliseconds: 100));
    await _recorder.startRecorder(
      toFile: path,
      codec: kIsWeb ? Codec.opusWebM : Codec.aacADTS,
    );
  }

It also has potential for unnecessary active subscriptions and might be the source of other bugs.

[Larpoux]

Hi @cedvdb ,

I am going to look to your issue. I am not sure, actually, to understand it. (Flutter Sound used this stream of event before I began to work on FS). I do not like that very much streams, and I would prefer that the App will receive a call-back and do what it wants with this call back. For example, streams itself those events in its own stream if this is what it wants.

I hope to clean the API soon. Please tell me if you agree that a callback would be better than a stream of events. Also : Flutter Sound handles many kind of callbacks. I think that the App would prefer sometimes to subclass Flutter Sound player/recorder, and override the onXXX events, instead of providing a procedure callback. This is exactly same, but maybe more elegant.

For example :

class MyRecorder extends FlutterSoundRecorder
{
      @override
      void onProgress(Duration duration, int dbLevel) { // The Callback of the App
              .. // do something with this event
      }

      Future<void>  _doRecord(String path) async {
            await _openRecorderSession();
            await _recorder.setSubscriptionDuration(const Duration(milliseconds: 100),);
            await _recorder.startRecorder(
              toFile: path,
              codec: kIsWeb ? Codec.opusWebM : Codec.aacADTS,
            );
      }
}

or

      Future<void>  _doRecord(String path) async {
            await _openRecorderSession();
            await _recorder.setSubscriptionDuration(const Duration(milliseconds: 100),
              (Duration duration, int dbLevel) { // The call back
                   .. // do something with this event
               }
            );
            await _recorder.startRecorder(
              toFile: path,
              codec: kIsWeb ? Codec.opusWebM : Codec.aacADTS,
            );
      }

I think also, that it could be better to have a startRecorder() parameter for the duration, instead of using a separate procedure "setSubscriptionDuration()". Then the App will never forget to call setSubscriptionDuration().

      Future<void>  _doRecord(String path) async {
            await _openRecorderSession();
            await _recorder.startRecorder(
               toFile: path,
               codec: kIsWeb ? Codec.opusWebM : Codec.aacADTS,
               subscribeEvery: Duration(milliseconds: 100),
               onProgress:
                   (Duration duration, int dbLevel) { // The call back
                       .. // do something with this event
                   }
             );
      }

Or if the App decides to subclass FlutterSoundRecorder

class MyRecorder extends FlutterSoundRecorder
{
      @override
      void onProgress(Duration duration, int dbLevel) { // The Callback of the App
              .. // do something with this event
      }

      Future<void>  _doRecord(String path) async {
            await _openRecorderSession();
            await _recorder.startRecorder(
               toFile: path,
               codec: kIsWeb ? Codec.opusWebM : Codec.aacADTS,
               subscribeEvery: Duration(milliseconds: 100),
             );
      }

}

These two last examples, will permit Flutter Sound to check that the App will never provide a subscriptionDuration and no callback, or the opposite : provide a Callback but no subscriptionDuration

[TonyTNguyen]

I was stumbled on a separate issue that relates to record duration then I found this: I agree to

to have a startRecorder() parameter for the duration, instead of using a separate procedure "setSubscriptionDuration()"

where we both can set the duration and callback function as parameters in startRecorder(). I myself initially did not know I have to call setSubscriptionDuration(), and wondered why the listen progress function never get call!

Or at least we have the duration setting and callback in the same function as in the second solution @Larpoux proposed.

I am kinda against subclassing. Subclass and override are for generic purpose classes. Most of cases if not always, we cannot use these generic classes out of the box. Flutter_sound is a wonderful tool that we can use it out of the box.

[Larpoux]

@TonyTNguyen ,

thank you for your feedback. I am/was not sure that subclassing was a good idea. It is more difficult to check that the App overridden the onProgress method, but forgot to specify the duration/frequency. Its overriden function onProgress() will never be called, and the developer will not understand why.

But ... it seems to me that all those closure functions are not always readable and subclassing can be more elegant. Any way, I will certainly take into account your point of view.

@cedvdb : what do you think ?

[cedvdb]

I'm used to stream so I think they are fine. If you prefer callbacks then by all means, use what you are more comfortable with.

I think stream offer the following advantage if rxdart is used:

onProgress.throttleTime(Duration(seconds: 1)).listen(() {})

which is the same as setSubscriptionDuration which you'd not need.

In any case I would not use the parameter name subscribeEvery as it makes it look like a new subscription is made every x, which would be a memory leak. On the same token setSubscriptionDuration is also not a descriptive name imo. This is more a onProgressUpdateRate.

[cedvdb]

And what I meant with this issue is that I believe that the onProgress should be a stream that completes when the recording stops. Same goes for the player. Currently it's not completing when it stops.

That's an advantage of stream vs callback as well, that you know when it completes. However it can be achieved with multiple callbacks as well but at that point you might as well use a stream

What do you think ?

[cedvdb]

You said you prefer callbacks it might be a wiser choice to use that though, you are the main maintainer after all. There is also things in the recorder that you commented as being unnecessary because it is used by the UI library (which I'm doubtful is worth maintaining, because most people end up building their own UI). It could be in its own package too but I'm going on a tangant

[Larpoux]

Would you accept a PR that changes it so the stream returned starts when the recording starts and ends when the recording ends ?

Of course. 👍 I always like very much Pull Requests. PR are something that I always put on top of my TODO list. When I receive a PR, I stop everything I am doing, to handle the PR. Unfortunately PR are something not frequent, and there are no many people contributing to Flutter Sound

You said you prefer callbacks it might be a wiser choice to use that though, you are the main maintainer after all.

I maintain FS, but I am not a dictator. Taking care of what developers need, or want is not an option. It is necessary.

My feeling is that Flutter Sound receives Callbacks from the Platform. Instead of feeding a stream managed by FS, maybe better to transmit the callback to the App. Then, the App can manage its own stream if it needs that.

That's an advantage of stream vs callback as well, that you know when it completes

This is a good point. Knowing when the stream is closed can be very useful for the App.

There is also things in the recorder that you commented as being unnecessary because it is used by the UI library (which I'm doubtful is worth maintaining, because most people end up building their own UI).

I will be frank with you, I do not like at all the FS UI widgets. I do not maintain them because I do not feel concern. Actually those widgets are buggy and not documented, but I am waiting that other developers will contribute to maintain them.

[Larpoux]

Today, I am re-opening #493. I want a clean API before porting τ on other frameworks and before coding enhancements like support of PCM stream with various parameters (integer, floating point, mono/stereo, bigendian/littleendian).

I do not know if I can achieve this task soon, because I am actually busy with the imminent release of my own App. But probably better to do your Pull Request when the new API will be stable and released.

[cedvdb]

Actually those widgets are buggy and not documented, but I am waiting that other developers will contribute to maintain them.

Imo, it would be better to put them in a separate package and forget about them. If people really use the UI widgets, then they can submit PR on that separate package to make it production ready. Do you think people will use that in a production app ? Do you think it's production ready ? If you answer no to any of those I don't think it's worth being in the main lib.

For the PR I'll take a look this week and think about your callback idea as well

[Larpoux]

For the PR I'll take a look this week and think about your callback idea as well

If you really insist to do your PR without waiting my new API, then probably better to do your PR against the v8 branch.

The master branch is the branch that I began to work on. It will be unstable during several days (or weeks) and will be perhaps a V9 (or a v 8.4.x, I have not yet decided). I will do my best to keep backward compatibility, without any breaking change. The classes FlutterSoundPlayer, FlutterSoundRecorder and FlutterSoundHelper will be kept deprecated and will be replaced by 3 new modules : TauPlayer, TauRecorder and TauHelper.

Later, I will code a new module (something like TauNodePlayer ). This new module will permit to assemble blocs (nodes) to do audio processing. This module will replace startPlayerFromStream() and startRecorderToStream() that I do not like very much.

[cedvdb]

How about returning a stream from the startRecord/startPlaying instead ? Or is that too much breaking change ?

   Stream<PlaybackDisposition> startPlayer({
    String? fromURI,
    Uint8List? fromDataBuffer,
    Codec codec = Codec.aacADTS,
    int sampleRate = 16000, // Used only with codec == Codec.pcm16
    int numChannels = 1, // Used only with codec == Codec.pcm16
    TWhenFinished? whenFinished,
  }) {
    _progressController = StreamController();
    _lock.synchronized(() async {
      await _startPlayer(
        fromURI: fromURI,
        fromDataBuffer: fromDataBuffer,
        codec: codec,
        sampleRate: sampleRate,
        numChannels: numChannels,
        whenFinished: whenFinished,
      );
    });
    return _progressController!.stream.asBroadcastStream();
  }

I'm not sure what the lock is, if I understood correctly it's to be sure the callback is executed only once at a time, if that's the case then the way the progress is initialized is not entirely correct. his.

usage

_player.play().listen(
    (progress) => print(progress), 
    onDone: (duration) => _playNextTrack(), 
    onError: (e) => _displayError(e)
 );

[AR553]

Any update on this issue? Can we alternatively check if the player has completed playing the audio?

[github-actions[bot]]

This issue is stale because it has been open 90 days with no activity. Leave a comment or this will be closed in 7 days.