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

[video_player] fix: add missing isPlaybackLikelyToKeepUp check. #3826

Merged
merged 12 commits into from
Jul 17, 2023

Conversation

unknown-undefined
Copy link
Contributor

When watching a live stream, if the playback buffers but the AVPlayerItem is likely to keep up, it is necessary to recheck AVPlayerItem.isPlaybackLikelyToKeepUp. If isPlaybackLikelyToKeepUp, a bufferingEnd event should be immediately triggered. I am encountering an issue with my product where, when watching a live stream, if I seek to the latest time, it continuously bufferingStart and does not come to an bufferingEnd.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@google-cla
Copy link

google-cla bot commented Apr 26, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Copy link
Contributor

@tarrinneal tarrinneal left a comment

Choose a reason for hiding this comment

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

Seems you're still working on this, but I'll just reiterate what the bot already said. You'll need to add some tests to this pr before we can merge it.

@unknown-undefined
Copy link
Contributor Author

Hi @tarrinneal , I've added the necessary test case and all tests have passed, both for replay and livestream.

Copy link
Contributor

@tarrinneal tarrinneal left a comment

Choose a reason for hiding this comment

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

Seems good to me, I'll let @hellohuanlin do final review.

Copy link
Contributor

@hellohuanlin hellohuanlin left a comment

Choose a reason for hiding this comment

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

I'm not sure if isPlaybackLikelyToKeepUp is a good indicator for bufferingStart and bufferingEnd events. From the doc this flag is just a "prediction", which may not be correct.

@@ -319,11 +319,17 @@ - (void)observeValueForKeyPath:(NSString *)path
}
} else if (context == playbackBufferEmptyContext) {
if (_eventSink != nil) {
_eventSink(@{@"event" : @"bufferingStart"});
if ([[_player currentItem] isPlaybackLikelyToKeepUp]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

did you hit this call? When playback buffer is empty, is it possible for isPlaybackLikelyToKeepUp to be true at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tested this with below code and through OBS livestreaming, and have found that it is possible for both the 'isPlaybackLikelyToKeepUp' flag to be true and for the playback buffer to be empty at the same time while watching a livestream.

import AVKit
import UIKit

class ViewController: UIViewController {
    var isPlaybackBufferEmptyListener: NSKeyValueObservation?
    var isPlaybackBufferFullListener: NSKeyValueObservation?
    var isPlaybackLikelyToKeepUpListener: NSKeyValueObservation?

    @IBAction func onClickPlay(_ sender: Any) {
        /// Test with a live stream URL, it's m3u8 has no end tag
        guard let url = URL(string: "Test with a live stream URL") else { return }

        // guard let url = URL(string: "https://flutter.github.io/assets-for-api-docs/assets/videos/hls/bee.m3u8") else { return }

        let item = AVPlayerItem(url: url)

        isPlaybackBufferEmptyListener?.invalidate()
        isPlaybackBufferEmptyListener = item.observe(\.isPlaybackBufferEmpty, options: [.initial, .new]) { _, change in
            guard let new = change.newValue else { return }
            print("isPlaybackBufferEmpty ---> \(new)")
        }

        isPlaybackBufferFullListener?.invalidate()
        isPlaybackBufferFullListener = item.observe(\.isPlaybackBufferFull, options: [.initial, .new]) { _, change in
            guard let new = change.newValue else { return }
            print("isPlaybackBufferFull ---> \(new)")
        }

        isPlaybackLikelyToKeepUpListener?.invalidate()
        isPlaybackLikelyToKeepUpListener = item.observe(\.isPlaybackLikelyToKeepUp, options: [.initial, .new]) { _, change in
            guard let new = change.newValue else { return }
            print("isPlaybackLikelyToKeepUp ---> \(new)")
        }

        let player = AVPlayer(playerItem: item)
        let controller = AVPlayerViewController()
        controller.player = player
        present(controller, animated: true) {
            player.play()
        }
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you post your log here? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. the the log,

isPlaybackBufferFull ---> false
isPlaybackLikelyToKeepUp ---> false
isPlaybackBufferFull ---> true
isPlaybackBufferFull ---> false
isPlaybackLikelyToKeepUp ---> true


>>>>>>> SEEK BEGIN >>>>>>>
isPlaybackLikelyToKeepUp ---> false
<<<<<<<< SEEK END <<<<<<<<
isPlaybackLikelyToKeepUp ---> true


>>>>>>> SEEK BEGIN >>>>>>>
isPlaybackLikelyToKeepUp ---> false
<<<<<<<< SEEK END <<<<<<<<
isPlaybackLikelyToKeepUp ---> true

Based on the aforementioned logs, it is evident that the isPlaybackBufferFull consistently remains false, while the isPlaybackLikelyToKeepUp can be either true or false during seeking. I have uploaded the testing project to AVPlayer_livestream. Please feel free to run it and review the provided logs.

Copy link
Contributor

Choose a reason for hiding this comment

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

i was asking the opposite situation - isPlaybackBufferEmpty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, @hellohuanlin, I realized that I missed including the first line of the log. Here's the revised version:

isPlaybackBufferEmpty ---> true
isPlaybackBufferFull ---> false
isPlaybackLikelyToKeepUp ---> false
isPlaybackBufferFull ---> true
isPlaybackBufferFull ---> false
isPlaybackLikelyToKeepUp ---> true


>>>>>>> SEEK BEGIN >>>>>>>
isPlaybackLikelyToKeepUp ---> false
<<<<<<<< SEEK END <<<<<<<<
isPlaybackLikelyToKeepUp ---> true

Please note that the isPlaybackBufferEmpty always remains true, while the isPlaybackLikelyToKeepUp can be either true or false during seeking.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is surprising. i'd expect isPlaybackBufferEmpty to be false most of the time. Do you have some insight on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like there is a bug in AVFoundation related to the isPlaybackBufferEmpty key-value observing (KVO) not being triggered when its value changes. As a workaround for now, we may check the isPlaybackLikelyToKeepUp property within the playbackBufferEmptyContext and playbackBufferFullContext. refer log:

[KVO] isPlaybackBufferEmpty ---> true
[KVO] isPlaybackBufferFull ---> false
[KVO] isPlaybackLikelyToKeepUp ---> false  (crrent isPlaybackBufferEmpty ---> true)
2023-05-25 13:26:31.528310+0900 Lab_AVPlayer_livestream[1757:465625] Metal API Validation Enabled
[KVO] isPlaybackBufferFull ---> true
[KVO] isPlaybackBufferFull ---> false
[KVO] isPlaybackLikelyToKeepUp ---> true  (crrent isPlaybackBufferEmpty ---> false)

I tested this behavior on iPadOS 16.5 using a real device. You can also check the provided demo AVPlayer_livestream for further reference.

Copy link
Contributor

Choose a reason for hiding this comment

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

This behavior needs to be heavily documented in our code (since it's not mentioned in apple's doc), otherwise it's too hard to understand in the future. Could you add some comments explaining why we need this workaround?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @hellohuanlin Before I proceed with adding comments about the necessity of the isPlaybackLikelyToKeepUp check when observing isPlaybackBufferFull and isPlaybackBufferEmpty using KVO, I would like to seek clarification on the exact meaning of isBuffering within the video_player module. I have raised this question in detail here.

Based on my understanding, the evaluation of isBuffering relies on the Key-Value Observing (KVO) of isPlaybackBufferFull, isPlaybackBufferEmpty, and isPlaybackLikelyToKeepUp.

}
} else if (context == playbackBufferFullContext) {
if (_eventSink != nil) {
_eventSink(@{@"event" : @"bufferingEnd"});
if ([[_player currentItem] isPlaybackLikelyToKeepUp]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

from the doc, when buffer is full, it's still possible for isPlaybackLikelyToKeepUp to return NO. Do we want to emit bufferingStart then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is correct. Since the KVO code related to 'playbackBufferEmptyContext' is already responsible for emitting the 'bufferingStart' event, there is no need to emit it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant, do we need an else statement here (similar to playbackBufferEmptyContext)? I am having some trouble understanding why these 2 are different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe I may have overlooked adding the same logic to the playbackBufferEmptyContext. As stated in the doc the isPlaybackLikelyToKeepUp property is a Boolean value that indicates whether the item will likely play through without stalling. I have already pushed the new commit.

@unknown-undefined
Copy link
Contributor Author

As stated in the doc the isPlaybackLikelyToKeepUp property is a Boolean value that indicates whether the item will likely play through without stalling. While the isBuffering property in the video_player plugin may be the only way to check if the video is currently stalled, it is important to note that it refers to a different aspect of playback than isPlaybackLikelyToKeepUp. So, it may be a separate topic altogether.

@unknown-undefined
Copy link
Contributor Author

Based on the code in FLTVideoPlayerPlugin.m at line 313, the isPlaybackLikelyToKeepUp property is used to indicate the bufferingEnd event. Therefore, using isPlaybackLikelyToKeepUp as an indicator for both the bufferingStart and bufferingEnd events ensures consistent behavior.

@hellohuanlin
Copy link
Contributor

Based on the code in FLTVideoPlayerPlugin.m at line 313, the isPlaybackLikelyToKeepUp property is used to indicate the bufferingEnd event.

The line may have changed. could you link here? Thanks!

@unknown-undefined
Copy link
Contributor Author

@hellohuanlin
Copy link
Contributor

Hello @hellohuanlin, the link FLTVideoPlayerPlugin.m#LL332C4-L339C4

I'm a bit confused here. Simply observing isPlaybackLikelyToKeepUp is not enough? Is it missing notification? If so, it seems like a bug in AVFoundation.

@unknown-undefined
Copy link
Contributor Author

unknown-undefined commented May 25, 2023

@hellohuanlin, based on my testing log, it appears that simply observing isPlaybackLikelyToKeepUp is sufficient for checking if playback is stalling or not. However, the Key-Value Observing (KVO) for isPlaybackBufferEmpty or isPlaybackBufferFull does not always work correctly. It's a bug. There is a related issue: https://developer.apple.com/forums/thread/669541

Here is the log snippet from my test:

[KVO] isPlaybackBufferEmpty ---> true
[KVO] isPlaybackBufferFull ---> false
[KVO] isPlaybackLikelyToKeepUp ---> false  (crrent isPlaybackBufferEmpty ---> true)
2023-05-25 13:26:31.528310+0900 Lab_AVPlayer_livestream[1757:465625] Metal API Validation Enabled
[KVO] isPlaybackBufferFull ---> true
[KVO] isPlaybackBufferFull ---> false
[KVO] isPlaybackLikelyToKeepUp ---> true  (crrent isPlaybackBufferEmpty ---> false)

I conducted this test on iPadOS 16.5 using a real device. You can also refer to the provided demo AVPlayer_livestream for further clarification.

// expected behavior of the value change for `playbackBufferEmpty` is not triggered. This
// issue has been confirmed in iOS 16.5. Refer:
// https://github.com/flutter/packages/pull/3826#discussion_r1204985454 Fortunately, the KVO
// for `playbackBufferFull` is working correctly, so the bug won't affect the event passing
Copy link
Contributor

Choose a reason for hiding this comment

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

Fortunately, the KVO for playbackBufferFull is working correctly,

Sorry for the back forth, but it looks like this comment only explains why we need playbackBufferFull, but it does not explain why we need to check isPlaybackLikelyToKeepUp (i.e. your code change).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @hellohuanlin, thank you for your suggestion. I have added documentation in this commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also rephrase this and add to the comment? #3826 (comment)

I think it's important to know why we can't just listen to isPlaybackLikelyToKeepUp and call it a day.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hellohuanlin It would be preferable for isBuffering to serve a singular purpose, indicating whether the playback is likely to proceed without stalling. I believe that relying solely on isPlaybackLikelyToKeepUp is sufficient, as mentioned in the #3826 (comment), the code listening to isPlaybackBufferEmpty or isPlaybackBufferFull should be removed. This change have a significant impact. I have pushed the commit.

Regarding the Android implementation in the video_player code, the isBuffering status is derived from the playbackState, specifically by checking if it equals Player.STATE_BUFFERING. In the context of Android's ExoPlayer, Player.STATE_BUFFERING signifies that the player is unable to immediately start playing the media but is actively working towards that goal. This state commonly occurs when the player needs to buffer additional data before playback can commence. For further reference, please consult the documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

This change have a significant impact.

Can you elaborate more on the impact? the new code makes more sense to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @hellohuanlin, apologies for the unclear expression in my previous comment. The impact of the commit is significant as it removes a considerable amount of code, and all the test cases have passed successfully. Therefore, it functions as expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just wanna double check before stamping it - now it's just deleting the code. Does it still fix your issue for your project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I have tested the latest commit in my production app and deployed it to my production environment.

…aybackLikelyToKeepUp` for determining `isBuffering` status.

`isBuffering` servea a singular purpose, indicating whether the playback is likely to proceed without stalling. Relying solely on `isPlaybackLikelyToKeepUp` is sufficient, as [discussion](flutter#3826 (comment)).  Therefore, the code listening to `isPlaybackBufferEmpty` or `isPlaybackBufferFull` should be removed.
@stuartmorgan stuartmorgan added autosubmit Merge PR when tree becomes green via auto submit App and removed needs tests labels Jul 13, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jul 13, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Jul 13, 2023

auto label is removed for flutter/packages, pr: 3826, due to - The status or check suite Linux repo_tools_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 17, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jul 17, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Jul 17, 2023

auto label is removed for flutter/packages, pr: 3826, due to - The status or check suite repo_checks has failed. Please fix the issues identified (or deflake) before re-applying this label.

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 17, 2023
@auto-submit auto-submit bot merged commit e113581 into flutter:main Jul 17, 2023
73 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 18, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jul 18, 2023
flutter/packages@6889cca...3e8b813

2023-07-18 jkbturek@gmail.com [video_player] Fix iOS crash with multiple players (flutter/packages#4202)
2023-07-17 stuartmorgan@google.com [pigeon] Enable Android emulator tests in CI (flutter/packages#4484)
2023-07-17 defuncart@gmail.com [video_player] Add optional web options [Platform interface] (flutter/packages#4433)
2023-07-17 rexios@rexios.dev [google_maps_flutter_platform_interface] Platform interface changes for #3258 (flutter/packages#4478)
2023-07-17 yan.outside@gmail.com [video_player] fix: add missing isPlaybackLikelyToKeepUp check. (flutter/packages#3826)
2023-07-17 43054281+camsim99@users.noreply.github.com [camerax] Add flash configuration for image capture (flutter/packages#3800)
2023-07-17 stuartmorgan@google.com Remove `equatable` and `xml` allowances (flutter/packages#4489)
2023-07-17 stuartmorgan@google.com [ci] Switch Linux platform tests to LUCI (flutter/packages#4479)
2023-07-17 stuartmorgan@google.com [ci] Adjust bot configurations (flutter/packages#4485)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
LouiseHsu pushed a commit to LouiseHsu/flutter that referenced this pull request Jul 31, 2023
flutter/packages@6889cca...3e8b813

2023-07-18 jkbturek@gmail.com [video_player] Fix iOS crash with multiple players (flutter/packages#4202)
2023-07-17 stuartmorgan@google.com [pigeon] Enable Android emulator tests in CI (flutter/packages#4484)
2023-07-17 defuncart@gmail.com [video_player] Add optional web options [Platform interface] (flutter/packages#4433)
2023-07-17 rexios@rexios.dev [google_maps_flutter_platform_interface] Platform interface changes for flutter#3258 (flutter/packages#4478)
2023-07-17 yan.outside@gmail.com [video_player] fix: add missing isPlaybackLikelyToKeepUp check. (flutter/packages#3826)
2023-07-17 43054281+camsim99@users.noreply.github.com [camerax] Add flash configuration for image capture (flutter/packages#3800)
2023-07-17 stuartmorgan@google.com Remove `equatable` and `xml` allowances (flutter/packages#4489)
2023-07-17 stuartmorgan@google.com [ci] Switch Linux platform tests to LUCI (flutter/packages#4479)
2023-07-17 stuartmorgan@google.com [ci] Adjust bot configurations (flutter/packages#4485)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: video_player platform-ios
Projects
None yet
6 participants