[video_player_web] Stop buffering when browser canPlayThrough.

[ditman]

The plugin used to remove the "buffering" state of the player when the browser reported canPlay. That event means that the browser can start playback, but doesn't guarantee (with the current network conditions) that the video can be successfully played to completion without additional buffering.

This change removes the "buffering" state when the browser reports canPlayThrough, which ensures that the browser will be able to playback the whole video without additional buffering.

In order to properly test the above, this change separates the old _VideoPlayer private class into its own file under src, and adds specific "unit" tests for it, by controlling the internal state of the VideoPlayer under test, and mocking video player events from a fake Video element.

Fixes flutter/flutter#94630

Tests

• Ran all the automated tests locally, and they pass.
• Added unit test for the expected order of player events.

Pre-launch Checklist

• 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/plugins repo does use dart format.)
• I signed the CLA.
• The title of the PR starts with the name of the plugin 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.

[ditman]

/cc @NicolasDionB

[stuartmorgan-g]

The logic change LGTM in the sense that it's less wrong, but I don't think this is reporting the same thing as on other platforms; I believe they are reporting whether any buffering is taking place, rather than whether enough buffering has happened. But it's not clear to me from a quick look at the video element APIs that we can tell that; I see that we can get the buffered regions, but I don't see how to tell if that's changing. Maybe poll until the buffer region includes everything from the current position to the end?

But that could be a follow-up later at lower priority even if it is possible.

[NicolasDionB]

The logic change LGTM in the sense that it's less wrong, but I don't think this is reporting the same thing as on other platforms; I believe they are reporting whether any buffering is taking place, rather than whether enough buffering has happened. But it's not clear to me from a quick look at the video element APIs that we can tell that; I see that we can get the buffered regions, but I don't see how to tell if that's changing. Maybe poll until the buffer region includes everything from the current position to the end?

But that could be a follow-up later at lower priority even if it is possible.

@stuartmorgan The idea is not to be able to know if the whole content is buffered but if enough is loaded to ensure a playback with no interruption. So validating that everything from the current position to the end is buffered is not what's needed here.

[stuartmorgan-g]

@stuartmorgan The idea is not to be able to know if the whole content is buffered but if enough is loaded to ensure a playback with no interruption.

I don't know what "the idea" refers to here. My comment was about the cross-platform behavior of the video_player API surface, as I said in my comment.

If "the idea" means the request you made in the issue, what you requested and what the plugin's established behavior is aren't necessarily the same thing. And I believe they are not, as explained in my comment.

So validating that everything from the current position to the end is buffered is not what's needed here.

Are you saying that you've tested ExoPlayer and AVPlayer and they don't report buffering the way I have described? Because what this plugin implementation is supposed to do is be coherent with the other implementations to the degree that is possible.

[ditman]

ExoPlayer seems to not be "buffering enough to play continuously until the end". Here's where it's created:

• plugins/packages/video_player/video_player_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayer.java

  Line 74 in bb9dc22

  exoPlayer = new ExoPlayer.Builder(context).build();

It doesn't specify any special LoadControl strategy, meaning it uses the DefaultLoadControl. See some suggested tuning by Akamai.

There's an interesting bit, published by Akamai here:

"viewers will start abandoning a video if startup takes longer than two seconds to begin playing and for every additional second of delay, roughly an additional 6% of the audience leaves. [...] with a 10 second delay, nearly half the audience has left."

So maybe, we should drop this PR, and let the browser start playing a video ASAP? It seems users prefer rebuffering events than the video taking longer to start (which is what this change does).

[stuartmorgan-g]

So maybe, we should drop this PR, and let the browser start playing a video ASAP? It seems users prefer rebuffering events than the video taking longer to start (which is what this change does).

Is the web implementation of video_player forcibly linking play/pause status to whether we report buffering? I wasn't expecting this PR to directly affect playback.

[ditman]

Is the web implementation of video_player forcibly linking play/pause status to whether we report buffering? I wasn't expecting this PR to directly affect playback.

You're right, it seems that the only thing used by the plugin is the "buffered" state (to render the current buffered status in the VideoProgressIndicator widget). The isBuffering boolean value isn't used at all.

So this change can't be helping much in playing the video until enough of it has been loaded.

(Maybe postponing the "initialized" event on web would help code, as long as we have a way to check it from the outside to render a "disabled"/"enabled" play button or similar, so users can only click play once the video is ready.)

[ditman]

Closing for now, this change isn't solving what it's supposed to.

[NicolasDionB]

Well I missed the big picture it seems. So this all sounds good. Maybe a little detail though, postponing the initialized event could work for the initial buffering but when you seek in the video, you fall back in buffering mode again so that strategy would not work in that case.

In my understanding, both buffered and canPlayThrough can be useful, as some users might want to trigger different actions using the buffered state but the canPlayThrough event provided by the browser is more useful as it kind of garantees we can safely play the video. Anyways, thanks for all that. Hope you can find a way to implement this.

[stuartmorgan-g]

Closing for now, this change isn't solving what it's supposed to.

Isn't it still something we should land? It seems like it changes us from "reports that buffering is over way too early" to "reports that buffering is over somewhat too early", which is still better. And by just removing code.

[ditman]

"reports that buffering is over way too early" to "reports that buffering is over somewhat too early", which is still better. And by just removing code.

Yeah, the thing is that even if "canPlay" is true and we mark as not buffering, the moment the browser starts buffering again and we'll set buffering to true again. So we don't earn much by not removing the buffering state a little bit too early.

@stuartmorgan We can land this to remove code and add some tests that weren't there before, but knowing that this doesn't solve the linked issue of "start playing only when enough video has buffered so it won't be interrupted again".

[stuartmorgan-g]

The logic change LGTM in the sense that it's less wrong, but I don't think this is reporting the same thing as on other platforms; I believe they are reporting whether any buffering is taking place, rather than whether enough buffering has happened.

This was wrong for iOS too; it's using playbackLikelyToKeepUp or a full buffer to indicate the end of buffering. So I think this change actually does make it match other platforms (or at least iOS).

[stuartmorgan-g]

We can land this to remove code and add some tests that weren't there before, but knowing that this doesn't solve the linked issue of "start playing only when enough video has buffered so it won't be interrupted again".

It allows clients of the plugin to more reliably control playback themselves though, so they can choose to start playing only when it's likely to work. Isn't that what the issue is requesting?

[ditman]

This was wrong for iOS too; it's using playbackLikelyToKeepUp or a full buffer to indicate the end of buffering. So I think this change actually does make it match other platforms (or at least iOS).

All right, let's land this then! :)

[ditman]

PTAL! I added a bunch of test coverage that I think is interesting. (Also "discovered" a fail-fast way of dealing with streams in tests that makes me want to revisit every other web test that uses streams differently 🤣)

[stuartmorgan-g]

Nice! LGTM