-
-
Notifications
You must be signed in to change notification settings - Fork 484
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
Video player example integration one isolate #803
base: minor
Are you sure you want to change the base?
Video player example integration one isolate #803
Conversation
c3fb34d
to
d70ae4e
Compare
@ryanheise could you take a look at this? I have an issue with playing state shown on notification. Although play/pause button on flutter ui and clear button are updated, play/pause button on notification works strange. So I assume PlaybackState.playing works normally but PlaybackState.actions does not. Strange thing also that audio player works correctly. Maybe my implementation is wrong? Or what do you think can be a reason of such behavior: 20210904_173254.mp4 |
Hi @IlyaMax , would be happy to, but can you uncomment the code (or was that intentional?) |
@ryanheise Yes it was intentional. I commented in pubspec that there is a conflict between video player and audio libraries in runtime. So I left commented video player example and video player dependency by default. |
@ryanheise When changed these lines it worked. I've noticed that updateNotification has no reasons to be called every time something in state is changed. Only if actions or compact actions indexes are changed. Or am I wrong? |
You might have a point that it doesn't need to be called every time, but calling it more than necessary shouldn't cause an app to fail because if nothing has changed since the last call, it would effectively replace the notification with an identical one. |
Well, I don't know what exactly but it affects. I will add this hack locally because it solves my issue but can produce issues for other users of package. |
Hmm, I don't seem to be able to reproduce the issue. One thing I noticed is that you're triggering an update more times than is necessary by continuously publishing a new position. Instead, you want to broadcast the position once, and then let the notification automatically calculate its future continuous values. You only need to broadcast a new position if there is a time discontinuity, such as if the user performs a seek and the position is not where the notification expects it to be, or if the track is skipped to the next track, or if the position needs to stop moving due to buffering or a pause. |
But the same is done in simple example of audio player. I haven't noticed significant issues with it. |
My example definitely doesn't do that with updatePosition, although it does update bufferedPosition more often than I'd like. The example pretty much just reflects the behaviour of just_audio which treats updatePosition in the correct way. |
Hm, okay, I will try to bring it to its final form later. |
@ryanheise I've added playback queue example and fixed some bugs but was not able to do like you said. I don't fully understand how to implement it. Maybe documentation is not good enough or just I can't get it. Is it so important in this case? |
The video player plugin has an unfortunate design where it continuously broadcasts state changes whenever the position changes. Will, not exactly continuously but at 500ms intervals. But the media notification should not be continuously updated like this otherwise it overloads the system and causes lag. As I explained above, all you need to do, and all you should do, is update the position once and sent playing to true, then Android will automatically update the seek bar position in the notification where it predicts it should be as time passes. Android can animate the position more smoothly than once every 500ms. Anyway, what you'd have to do is compensate for the video player plugin's for design choice. Maybe by ignoring position updates that were where Android would have expected them to be anyway. E.g. if the position was at 20000 when hitting play, and then 500ms later the next event reports that the position is around 20500, then you can ignore that position update as it's where you would have predicted anyway. You don't want to forward this useless update to Android and overload the system. |
@ryanheise check this version |
Apologies that my latest commit introduced a conflict in But other than that, would you be able to uncomment your example, and comment out whatever conflicts with your example? That will make it a bit easier to work on this. |
FYI I have changed the base of this PR to the |
2e01340
to
1c5d428
Compare
Trying the latest commit, I get this stack trace:
Are you getting the same? |
It was actually the conflicting ExoPlayer in just_audio. Can you comment out the just_audio dependency which as you pointed out conflicts with video_player's linked ExoPlayer? |
Also, I think it would be appropriate to stick these await _controller?.setLooping(true);
await _controller?.initialize();
_controller?.addListener(_broadcastState);
await _controller?.play(); |
The play/pause button in the notification seems to be working correctly. Incidentally, an old version of the Android code in just_audio used to do a similar detection of time discontinuities (if you want to go and chase that down for reference) until I changed it to use the time discontinuity API in ExoPlayer. But the iOS implementation of just_audio still contains that old code to detect time discontinuities (Objective C): - (void)checkForDiscontinuity {
if (!_playing || CMTIME_IS_VALID(_seekPos) || _processingState == completed) return;
int position = [self getCurrentPosition];
if (_processingState == buffering) {
if (position > _lastPosition) {
[self leaveBuffering:@"stall ended"];
[self updatePosition];
[self broadcastPlaybackEvent];
}
} else {
long long now = (long long)([[NSDate date] timeIntervalSince1970] * 1000.0);
long long timeSinceLastUpdate = now - _updateTime;
long long expectedPosition = _updatePosition + (long long)(timeSinceLastUpdate * _player.rate);
long long drift = position - expectedPosition;
//NSLog(@"position: %d, drift: %lld", position, drift);
// Update if we've drifted or just started observing
if (_updateTime == 0L) {
[self broadcastPlaybackEvent];
} else if (drift < -100) {
[self enterBuffering:@"stalling"];
//NSLog(@"Drift: %lld", drift);
[self updatePosition];
[self broadcastPlaybackEvent];
}
}
_lastPosition = position;
} This code serves the dual purpose of both detecting a discontinuity caused by a seek, and also a discontinuity caused by buffering (in which case the appropriate buffering state is broadcast). |
@ryanheise I think that complex logic of position detection should be a part of audio_service package not video_player/any other player what do you think? And I didn't get what you want me to fix in this part. |
@ryanheise I had got this stacktrace but when I reinstalled application, I had no problem with this. |
Regarding making audio_service do this, that has been suggested before although audio_service reflects the underlying native APIs and that is how they work. At the native level these APIs were will thought out, efficient and interoperable. Exoplayer was also designed with the same considerations in mind, so it's just a matter of video_player hiding the useful events from exoplayer and forcing some rather ugly workarounds. Exposing the events that are actually useful here should be a video_player feature request. |
What do you think about instead of writing comment in example, make an issue with temporary workaround after merging this PR? It does not break example of audio and I think if someone face with error, they rather will search for an issue but not for a comment. |
Sorry, which comment are you referring to? This one? /// For efficiency, the [updatePosition] should NOT be updated continuously in
/// real time. Instead, it should be updated only when the normal continuity
/// of time is disrupted, such as during a seek, buffering and seeking. When
/// broadcasting such a position change, the [updateTime] specifies the time
/// of that change, allowing clients to project the realtime value of the
/// position as `position + (DateTime.now() - updateTime)`. As a convenience,
/// this calculation is provided by the [position] getter. |
@ryanheise No, I mean comment about conflict with just_audio. |
Have you tested this example on ios? If you are able to. I reproduce strange behavior. It doesn't show player in control center but no errors are thrown. Maybe you have idea why it can happen. |
That's because audio session is not configured on ios video_player side. In process of fix now |
I would prefer overriding it with the |
yes, I did the same. |
@IlyaMax I managed to make your example work on Android properly but on iOS It doesn't show the player in the control center, as you mentioned. Can you share how you could overcome this issue? |
I set
|
@IlyaMax @ryanheise With both your video_player example and my own implementation, I am facing the same problem with audio pausing when app goes to background on iOS. Did you by any chance encounter the same issue and have some idea how to resolve it? I created a minimal working example based on example with just_audio to replicate the above-mentioned behavior that you can find here: my_example. |
No description provided.