[video_player] Fix pause infinite loop on completed #5920

ToddZeil · 2024-01-18T03:44:36Z

When a video is complete, and not on a loop setting.
Pause() function is called even if video is already paused.

(_applyPlayPause() function is used for play and pause which can lead to playing a video when attempting to restart)

Fixes flutter/flutter#143141

Edited: Also this may only be an issue on web

Pre-launch Checklist

[ y] I read the Contributor Guide and followed the process outlined there for submitting PRs.
[ y] I read the Tree Hygiene wiki page, which explains my responsibilities.
[ y] 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.)
[y ] I signed the CLA.
[y ] The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
[ y] I listed at least one issue that this PR fixes in the description above.
[y ] I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
[y ] I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
[not needed ] I updated/added relevant documentation (doc comments with ///).
[fixed curren, added one more test ] I added new tests to check the change I am making, or this PR is test-exempt.
[Y, one fixed test ] All existing and new tests are passing.

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

flutter-dashboard · 2024-01-18T03:44:39Z

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 "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use 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.

tarrinneal

A couple nits here.

I'm not certain if this will break the work-around for the video completing but not truly reaching the last frame or not. Maybe @stuartmorgan has some insight.

tarrinneal · 2024-01-18T05:24:14Z

packages/video_player/video_player/CHANGELOG.md

 ## NEXT

+* Description of the change.
+


This should be removed

tarrinneal · 2024-01-18T05:25:20Z

packages/video_player/video_player/CHANGELOG.md

 * Updates support matrix in README to indicate that iOS 11 is no longer supported.
 * Clients on versions of Flutter that still support iOS 11 can continue to use this
  package with iOS 11, but will not receive any further updates to the iOS implementation.
+* Fix infinite pause loop caused by on completed to call pause even when video is pause.


Fixes infinite pause loop caused by call to pause even when video is already paused.

ToddZeil · 2024-01-18T05:54:23Z

A couple nits here.

I'm not certain if this will break the work-around for the video completing but not truly reaching the last frame or not. Maybe @stuartmorgan has some insight.

There are a couple of other ways we could do this if your not sure and think they will be better.
Either, split up the play pause into two different functions, or only call the pause part of the function if the timer is not null

stuartmorgan-g · 2024-01-18T17:31:18Z

packages/video_player/video_player/lib/video_player.dart

-          pause().then((void pauseResult) => seekTo(value.duration));
+          if (value.isPlaying) {
+            pause().then((void pauseResult) => seekTo(value.duration));
+          }


This changed logic would definitely not be guaranteed to actually do what this code is supposed to be doing; if the native side reported reaching the end as:

playback stopped

video completed

then the seek code would never run.

@stuartmorgan have updated the logic to account for this

Also, I believe this infinite loop issue is only seen on web for some reason

tarrinneal

This change tracks for me, approved pending @stuartmorgan review.

packages/video_player/video_player/test/video_player_test.dart

stuartmorgan-g

Looking at the code more deeply here, it's not clear to me what the reason for this change is. Why is calling pause while paused a problem? The PR description just says that it's a problem because it calls _applyPlayPause(), but with the paused value state, that will call _videoPlayerPlatform.pause. If calling pause on the platform object will start playing a video, that seems like a platform bug.

Could you file an issue with clear repro steps and an explanation of what exactly is happening?

stuartmorgan-g · 2024-02-07T20:01:32Z

packages/video_player/video_player/test/video_player_test.dart

+  });
+
+  test(
+      'isCompleted seeks position to max duration and pauses the video first if is playing',


Isn't the thing you want to fix that you want to not call pause if it's not playing? That path would also need a test.

stuartmorgan-g · 2024-02-08T18:23:11Z

Thanks for filing the issue. Could you elaborate, either here or (preferably) in the issue about what exactly is going wrong? It's not clear to me how the change in this PR relates to the problem shown in the issue. What exactly is happening in the web implementation that is causing the incorrect behavior?

ToddZeil · 2024-02-08T20:09:16Z

Thanks for filing the issue. Could you elaborate, either here or (preferably) in the issue about what exactly is going wrong? It's not clear to me how the change in this PR relates to the problem shown in the issue. What exactly is happening in the web implementation that is causing the incorrect behavior?

The listener when completed goes into an infinite loop which continuously calls any functions in the onCompleted switch case statement.
Specifically this is repeatedly called
case VideoEventType.completed: // In this case we need to stop _timer, set isPlaying=false, and // position=value.duration. Instead of setting the values directly, // we use pause() and seekTo() to ensure the platform stops playing // and seeks to the last frame of the video. pause().then((void pauseResult) => seekTo(value.duration)); value = value.copyWith(isCompleted: true);

stuartmorgan-g · 2024-02-08T20:34:28Z

The listener when completed goes into an infinite loop which continuously calls any functions in the onCompleted switch case statement.

It's not clear to me why this would be the case. Could you provide the specific call sequence in the infinite loop? In other words, when you say:

Specifically this is repeatedly called

what code is repeatedly calling that?

ToddZeil · 2024-02-08T21:09:12Z

The listener when completed goes into an infinite loop which continuously calls any functions in the onCompleted switch case statement.

It's not clear to me why this would be the case. Could you provide the specific call sequence in the infinite loop? In other words, when you say:

Specifically this is repeatedly called

what code is repeatedly calling that?

This is the code which seems to be repeatedly calling it

/.pub-cache/hosted/pub.dev/video_player_web-2.1.3/lib/src/video_player.dart

_videoElement.onEnded.listen((dynamic _) { setBuffering(false); _eventController.add(VideoEvent(eventType: VideoEventType.completed)); });

stuartmorgan-g · 2024-02-08T21:21:38Z

It seems like more root-causing is still needed here. As far as I can see the only change the current version of the PR makes is not calling pause() when paused, but pause() on web just calls pause() on the <video> element, and according to MDN:

The HTMLMediaElement.pause() method will pause playback of the media, if the media is already in a paused state this method will have no effect.

Are those docs incorrect? Is calling pause() when already paused at the end of the video triggering an onEnded event? That seems very surprising if so, but if not I don't see how this PR would fix the behavior.

ToddZeil · 2024-02-08T21:35:12Z

The infinite loop must be coming from the html code But I believe this issue is coming from the _applyPlayPause function in the video_player Which while in the infinite loop, will actually instantly cancel the timer which is used for the video playing

…

On Fri, 9 Feb 2024, 10:21 am stuartmorgan, ***@***.***> wrote: It seems like more root-causing is still needed here. As far as I can see the only change the current version of the PR makes is not calling pause() when paused, but pause() on web just calls pause() on the <video> element, and according to MDN: The HTMLMediaElement.pause() method will pause playback of the media, if the media is already in a paused state this method will have no effect. Are those docs incorrect? Is calling pause() when already paused at the end of the video triggering an onEnded event? That seems very surprising if so, but if not I don't see how this PR would fix the behavior. — Reply to this email directly, view it on GitHub <#5920 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A4WXA3VIXNG2UQUBXEDBCVTYSU6W5AVCNFSM6AAAAABB7SU7AOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZUHE2TENJWGY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

stuartmorgan-g · 2024-02-08T21:38:43Z

So this PR doesn't fix the infinite loop, just removes one side effect of the infinite loop?

If there's an infinite loop, we should root cause and fix the loop itself, not the side effect.

ToddZeil · 2024-02-08T21:47:18Z

Yes that's correct, Olay I will need to look into that then as from a quick look I couldn't work out where the 'ended' event was getting added to the video event stream in the web package.

…

On Fri, 9 Feb 2024, 10:38 am stuartmorgan, ***@***.***> wrote: So this PR doesn't fix the infinite loop, just removes one side effect of the infinite loop? If there's an infinite loop, we should root cause and fix the loop itself, not the side effect. — Reply to this email directly, view it on GitHub <#5920 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A4WXA3T5FPBHCYDGKU7WF3DYSVAW7AVCNFSM6AAAAABB7SU7AOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZUHE3TINJQG4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

ToddZeil · 2024-02-11T21:53:37Z

The loop is coming from
'flutter/flutter/bin/cache/pkg/sky_engine/lib/html/html_dart2js.dart'
Specifically: ElementStream<Event> get onEnded => endedEvent.forElement(this);

This is used in video_player_web

_videoElement.onEnded.listen((dynamic _) { setBuffering(false); _eventController.add(VideoEvent(eventType: VideoEventType.completed)); });

I can't find the code where the event is added to the stream, I'm not sure this is even in our control ?

stuartmorgan-g · 2024-02-11T22:13:49Z

The creation of the event is done by the browser, but it is likely the result of something the plugin is doing; it's extremely unlikely that browser behavior is to create an infinite stream of onEnded events without any input.

Is seeking to the end when already at the end creating a new onEnded?

ToddZeil · 2024-02-16T02:05:17Z

Thanks, have updated now

ditman · 2024-02-16T18:50:09Z

packages/video_player/video_player/CHANGELOG.md

 * Updates support matrix in README to indicate that iOS 11 is no longer supported.
 * Clients on versions of Flutter that still support iOS 11 can continue to use this
  package with iOS 11, but will not receive any further updates to the iOS implementation.
+* Web, Fix infinite pause loop caused by seekTo marking the video as completed when already completed.


Please, revert all changes to the video_player package, they're not needed.

flutter pub get (or flutter pub upgrade) will take care of fetching the fixed version of video_player_web once the fix is published.

(Sorry I wasn't clearer before!)

ToddZeil · 2024-02-19T21:23:45Z

Thanks for all the help guys, should be all done now!

stuartmorgan-g

Minor comments; I'll leave final approval for @ditman since this is now web-only.

stuartmorgan-g · 2024-02-20T21:08:58Z

packages/video_player/video_player_web/CHANGELOG.md

-## NEXT
+## 2.1.4

+* Fixes infinite pause loop caused by seekTo marking the video as completed when already completed.


Nit: Please use backticks on seekTo to mark it as code.

stuartmorgan-g · 2024-02-20T21:09:54Z

packages/video_player/video_player_web/lib/src/video_player.dart

  void seekTo(Duration position) {
    assert(!position.isNegative);
+    // Don't seek if video is already at position.
+    // As seeking when completed will trigger another completed event ('onEnded')


This is a sentence fragment; please combine it with the line above: "Don't seek if the video is already at the target position, as seeking when completed [...]"

stuartmorgan-g · 2024-02-20T21:10:15Z

packages/video_player/video_player_web/lib/src/video_player.dart

    assert(!position.isNegative);
+    // Don't seek if video is already at position.
+    // As seeking when completed will trigger another completed event ('onEnded')
+    // to avoid potentially firing extra 'onEnded' events when the video is already over


"when the video is already over" is redundant with the earlier "when completed", so should be removed.

stuartmorgan-g · 2024-02-20T21:10:55Z

packages/video_player/video_player_web/lib/src/video_player.dart

    assert(!position.isNegative);
+    // Don't seek if video is already at position.
+    // As seeking when completed will trigger another completed event ('onEnded')
+    // to avoid potentially firing extra 'onEnded' events when the video is already over


This needs a period at the end of the sentence, since comments should be properly punctuated sentences.

stuartmorgan-g

(Fixing review state; I thought I could clear my request without approving, but apparently not.)

ditman · 2024-02-20T22:55:00Z

~~Adding a small commit here to address @stuartmorgan's concerns, and get the branch updated with the latest main.~~ I take this back, it seems @ToddZeil is fixing stuff! Thanks @ToddZeil!

ToddZeil · 2024-02-20T23:00:06Z

Thanks for the review, updated now.
So you can check again when when you can @ditman

ditman

We need a test, I think we can write one easily by listening to the events coming from the videoplayer (two seeks to the same position should only yield a single seek event to the core plugin)

packages/video_player/video_player_web/lib/src/video_player.dart

Co-authored-by: David Iglesias <[email protected]>

ToddZeil · 2024-02-20T23:24:31Z

Okay where do we put tests for this package ?
Will it go in here packages/packages/video_player/video_player/test/video_player_test.dart ?

ditman · 2024-02-20T23:43:29Z

@ToddZeil there's already several tests here:

https://github.com/flutter/packages/tree/main/packages/video_player/video_player_web/example/integration_test

TL;DR: Assuming that chromedriver and dart are in your $PATH, and Chrome is in a standard location, inside the video_player_web directory, you can do something like this:

PLUGIN_TOOLS=/work/flutter/packages/script/tool/bin/flutter_plugin_tools.dart # Adjust this
dart $PLUGIN_TOOLS drive-examples --current-package --web --run-chromedriver

(I normally use Linux)

There's more instructions on how to run tests in the README.md of the example

ToddZeil · 2024-02-21T22:58:58Z

I have set up the integration tests, not sure exactly how these work but the current seekTo doesn't seem to check for anything other than completion ?

testWidgets('can seek to position', (WidgetTester tester) async { expect( VideoPlayerPlatform.instance.seekTo( await textureId, const Duration(seconds: 1), ), completes, ); });

Adding seekTo seems to add the following events to the tests:
` VideoEventType:VideoEventType.bufferingStart,
        VideoEventType:VideoEventType.bufferingUpdate,
        VideoEventType:VideoEventType.isPlayingStateUpdate,
        VideoEventType:VideoEventType.bufferingEnd,
        VideoEventType:VideoEventType.completed`

But when I comment out the new code it doesn't seem to effect anything with in the test?
It doesn't seem to call completed multiple times?
Could I be doing something wrong?

ditman · 2024-02-22T03:54:46Z

Could I be doing something wrong?

@ToddZeil can you share the code of the test that you've written? Or I can try and write one tomorrow :)

My plan is to:

start a video paused
seek to a position
assert the events (probably seeked or some combination of buffering?)
seek to the same position
assert that no new events fired in a reasonable amount of time (500ms or similar).

I that test should fail if we comment out the fix (?)

ToddZeil · 2024-02-22T04:47:32Z

Your test sounds like it should work!
The main issue was happening when looping was off and seekTo was seeking to complete the video

I was just trying to find out what events the seekTo function emitted, but I couldn't get multiple events firing through

This is the test I was trying:

testWidgets('multiple calls to seekTo will emit a seek event',
        (WidgetTester tester) async {
      final int videoPlayerId = await textureId;
      final Stream<VideoEvent> eventStream =
          VideoPlayerPlatform.instance.videoEventsFor(videoPlayerId);

      final Future<List<VideoEvent>> stream = eventStream.timeout(
        const Duration(seconds: 2),
        onTimeout: (EventSink<VideoEvent> sink) {
          sink.close();
        },
      ).toList();

      await VideoPlayerPlatform.instance.setLooping(videoPlayerId, false);
      await VideoPlayerPlatform.instance.setVolume(videoPlayerId, 0);

      await VideoPlayerPlatform.instance.play(videoPlayerId);


      await VideoPlayerPlatform.instance
          .seekTo(await textureId, const Duration(milliseconds: 300));

      await VideoPlayerPlatform.instance
          .seekTo(await textureId, const Duration(milliseconds: 350));

      await VideoPlayerPlatform.instance
          .seekTo(await textureId, const Duration(milliseconds: 350));

      await VideoPlayerPlatform.instance
          .seekTo(await textureId, const Duration(seconds: 99));

      await VideoPlayerPlatform.instance
          .seekTo(await textureId, const Duration(seconds: 99));

      await VideoPlayerPlatform.instance
          .seekTo(await textureId, const Duration(milliseconds: 100));

      // Let the video play, until we stop seeing events for two seconds
      final List<VideoEvent> events = await stream;

      await VideoPlayerPlatform.instance.pause(videoPlayerId);

      // The expected list of event types should look like this:
      // 1. isPlayingStateUpdate (videoElement.onPlaying)
      // 2. bufferingStart,
      // 3. bufferingUpdate (videoElement.onWaiting),
      // 4. initialized (videoElement.onCanPlay),
      // 5. bufferingEnd (videoElement.onCanPlayThrough),
      expect(
          events.map((VideoEvent e) => e.eventType),
          equals(<VideoEventType>[
            VideoEventType.isPlayingStateUpdate,
            VideoEventType.bufferingStart,
            VideoEventType.bufferingUpdate,
            VideoEventType.initialized,
            VideoEventType.bufferingEnd,
          ]));
    });

ToddZeil · 2024-02-28T02:29:08Z

Did get a chance to look into this test ? Or have further hints or advice ? Thanks

ditman · 2024-02-29T03:22:02Z

@ToddZeil I didn't have time to look at the test, but I'm going to work next in migrating the video_player_web to the latest package:web, so I can sneak your fix there :)

The problem is that we don't have any "seek" event that we can check that wasn't propagated:

https://github.com/flutter/packages/blob/main/packages/video_player/video_player_platform_interface/lib/video_player_platform_interface.dart#L279

(We could maybe inject a fake video player and ensure we don't call "seek" multiple times, but I'm not sure... I'll try to add you as a Co-Author in the migration PR!)

ToddZeil · 2024-02-29T22:24:39Z

Okay interesting!

Cool hopefully we can get it in, thanks for that :)

ditman · 2024-03-06T04:23:18Z

This fix has landed alongside #5800! Closing.

ToddZeil · 2024-03-06T20:37:55Z

Great thank you!

ditman · 2024-03-06T20:44:30Z

@ToddZeil thanks for taking the time to report and fix this, keep it coming!

ToddZeil added 5 commits January 18, 2024 15:16

video_player Fix pause loop on completed

990decf

video_player Fix pause infinite loop on completed (fix typo)

cccf9d3

Fix test calling twice when not needed

546e37f

Update version

05d8479

Update changelog

3d71c14

ToddZeil requested a review from tarrinneal as a code owner January 18, 2024 03:44

github-actions bot added the p: video_player label Jan 18, 2024

tarrinneal requested changes Jan 18, 2024

View reviewed changes

Update changelog after review

5842263

stuartmorgan-g reviewed Jan 18, 2024

View reviewed changes

Fix logic after review

d02d031

tarrinneal approved these changes Jan 19, 2024

View reviewed changes

stuartmorgan-g requested changes Jan 23, 2024

View reviewed changes

packages/video_player/video_player/test/video_player_test.dart Outdated Show resolved Hide resolved

Update broken test and add new test as requested

7de2626

ToddZeil requested a review from stuartmorgan-g January 24, 2024 01:48

Merge branch 'main' into video_player_fix_pause_loop_on_completed

1abfd73

stuartmorgan-g requested changes Feb 7, 2024

View reviewed changes

ToddZeil mentioned this pull request Feb 8, 2024

[video_player] [WEB] Video breaks when trying to restart finished video on chrome flutter/flutter#143141

Open

Add extra comments, update function to a get

c4a5c9d

ditman reviewed Feb 16, 2024

View reviewed changes

ToddZeil and others added 2 commits February 20, 2024 10:18

Remove changes to video_player

e9d6fbe

Merge branch 'main' into video_player_fix_pause_loop_on_completed

4f7466e

stuartmorgan-g reviewed Feb 20, 2024

View reviewed changes

stuartmorgan-g self-requested a review February 20, 2024 21:11

stuartmorgan-g requested changes Feb 20, 2024

View reviewed changes

Updates after review

30162ec

ToddZeil and others added 2 commits February 21, 2024 11:57

Update version and changelog

af527eb

Merge branch 'main' into video_player_fix_pause_loop_on_completed

50ee253

ditman reviewed Feb 20, 2024

View reviewed changes

packages/video_player/video_player_web/lib/src/video_player.dart Outdated Show resolved Hide resolved

Update packages/video_player/video_player_web/lib/src/video_player.dart

dda2702

Co-authored-by: David Iglesias <[email protected]>

ditman mentioned this pull request Feb 29, 2024

[video_player_web] migrates to package:web #5800

Merged

11 tasks

ditman added a commit to balvinderz/packages that referenced this pull request Mar 5, 2024

Incorporate flutter#5920

66fac4b

ditman closed this Mar 6, 2024

[video_player] Fix pause infinite loop on completed #5920

[video_player] Fix pause infinite loop on completed #5920

Uh oh!

Conversation

ToddZeil commented Jan 18, 2024 • edited by stuartmorgan-g Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Jan 18, 2024

Uh oh!

tarrinneal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ToddZeil commented Jan 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarrinneal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Feb 8, 2024

Uh oh!

ToddZeil commented Feb 8, 2024

Uh oh!

stuartmorgan-g commented Feb 8, 2024

Uh oh!

ToddZeil commented Feb 8, 2024

Uh oh!

stuartmorgan-g commented Feb 8, 2024

Uh oh!

ToddZeil commented Feb 8, 2024 via email

Uh oh!

stuartmorgan-g commented Feb 8, 2024

Uh oh!

ToddZeil commented Feb 8, 2024 via email

Uh oh!

ToddZeil commented Feb 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartmorgan-g commented Feb 11, 2024

Uh oh!

ToddZeil commented Feb 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ToddZeil commented Feb 19, 2024

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

ditman commented Feb 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ToddZeil commented Feb 20, 2024

Uh oh!

ToddZeil commented Jan 18, 2024 •

edited by stuartmorgan-g

Loading

ToddZeil commented Feb 11, 2024 •

edited

Loading

ditman commented Feb 20, 2024 •

edited

Loading

ToddZeil commented Feb 21, 2024 •

edited

Loading

ditman commented Feb 22, 2024 •

edited

Loading

ToddZeil commented Feb 22, 2024 •

edited by ditman

Loading