[video_player] Flaky integration test failure on web

[stuartmorgan-g]

While converting web platform tests to LUCI, I'm seeing frequent failures in one test:

Failure in method: stay paused when seeking after video completed
══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞═════════════════
The following TestFailure was thrown running a test:
Expected: Duration:<0:00:07.534000>
  Actual: Duration:<0:00:07.544000>

When the exception was thrown, this was the stack:
dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 294:49  throw_
packages/matcher/src/expect/prints_matcher.dart.js 433:22                     fail
packages/matcher/src/expect/prints_matcher.dart.js 430:12                     _expect
packages/matcher/src/expect/prints_matcher.dart.js 365:12                     expect$
packages/flutter_test/src/test_text_input_key_handler.dart.js 7284:12         expect$
video_player_test.dart.js 257:23                                              <fn>
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 45:50            <fn>
...

It wasn't happening in Cirrus, but I could repro it locally in 2/4 runs. Either Cirrus for some reason didn't trip the flake, or the flake only happens in newer versions of Chrome (I believe the LUCI config has a newer Chrome version than our last update to the one pulled in our Cirrus tests).

I'm going to mark this as flaky to unblock the migration.

[stuartmorgan-g]

@ditman FYI

[ditman]

I'll get this test deflaked ASAP. I remember there was some other time-dependent code in the test that would flake more (this seems... parsing video metadata?), but I'll tackle this soon.

[ditman]

This might be a bug/race condition on the web implementation (?).

At first I thought that "10ms" was too low resolution for a web browser to do anything meaningful with, so I bumped the rewind up to "500ms", but that didn't help; in some cases it seems that:

• the rewind is not applied (hence why the play head remains at the end)
• the video actually plays from the beginning

I need to step through the test case manually and see what's actually happening.

---

The following TestFailure was thrown running a test:
Expected: Duration:<0:00:07.544000>
  Actual: Duration:<0:00:00.974000>
Play head should have reached the end of the video (duration)

It seems that the video buffering is affecting the results of the test. I'm going to let the video fully play on the web before starting to move the playhead around, to ensure that the buffering is not interfering with the positioning of the playhead (note in the failure above how we'd expect the video to be at 7.544 seconds, but it's actually at 0.974 seconds after playing for 1 second!)

---

Unsure why currentTime is being set at 0. This doesn't happen in vanilla JS (demo). Maybe the seek code on the web is wrong?

[ditman]

TL;DR: I've looked at the player code and the test a little bit, and there are a few problems with the plugin (that we already know about; my upcoming rant is related to: #90080).

---

The plugin attempts to keep track of the play head position with a Timer that only refreshes every 500ms, but the test is operating in much shorter increments. The web might not fully respect those timeouts for many reasons, but the length of those intervals is probably not the source of issues.

There's also a couple of seekTo calls liberally applied to the code to manipulate the play head position in certain moments, like "when the video ends, jump to the last frame":

• https://github.com/flutter/packages/blob/main/packages/video_player/video_player/lib/video_player.dart#L463-L469

Or "when the user clicks play, if you're in the last frame, rewind to 0":

• https://github.com/flutter/packages/blob/main/packages/video_player/video_player/lib/video_player.dart#L534-L542

The seekTo calls above get sent to the native implementation, and in the web (at least, I haven't looked at how the defensive is the code in the mobile implementations) may cause trouble (at least, they made debugging much more spicy).

Third, and possibly the ACTUAL problem, is that the web implementation has a bug where the autoplay attribute is always set to true, even though we try very hard to make it false (because it's an HTML Boolean attribute). This probably caused the "seek to zero" behavior that I was seeing while debugging above.)

---

In any case, I think a proper solution to this is to send the progress updates from the native implementation to the plugin, simplify the event handling coming from the native side to ONLY update the controller.value, and stop using seekTo "internally" for logic which should probably live in the native code. The Flutter side of the plugin should be super simple, in the web, for example, we just need to choose what events we care about.

---

As for why this started failing with us updating the browser, and I haven't verified this, but this change looks sus (and somewhat related to the failing test) ⁉️

[ditman]

After this lands, we might try to re-enable the flaky test. I've ran it with the package overridden locally, and it has passed several (8) consecutive attempts.

[stuartmorgan-g]

FWIW, revamping how video_player works internally so it's less "two sources of truth fighting for dominance" is something I am hoping will be a 2024H1 project.

[ditman]

I think we're going to be able to close this with only the fix to the autoplay issue on the web, so I guess we're going to be fine until then (but yeah the added complexity is super frustrating when debugging!)

Nope, I've just got a failure even with the new code; yikes!

Failure Details:
Failure in method: stay paused when seeking after video completed
══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞═════════════════
The following TestFailure was thrown running a test:
Expected: Duration:<0:00:07.534000>
  Actual: Duration:<0:00:07.544000>

When the exception was thrown, this was the stack:
dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 294:49  throw_
packages/matcher/src/expect/prints_matcher.dart.js 436:22                     fail
packages/matcher/src/expect/prints_matcher.dart.js 433:12                     _expect
packages/matcher/src/expect/prints_matcher.dart.js 368:12                     expect$
packages/flutter_test/src/test_text_input_key_handler.dart.js 9367:12         expect$
video_player_test.dart.js 259:23                                              <fn>
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 45:50            <fn>
dart-sdk/lib/async/zone.dart 1407:47                                          _rootRunUnary
dart-sdk/lib/async/zone.dart 1308:19                                          runUnary
dart-sdk/lib/async/future_impl.dart 162:18                                    handleValue
dart-sdk/lib/async/future_impl.dart 846:44                                    handleValueCallback
dart-sdk/lib/async/future_impl.dart 875:13                                    _propagateToListeners
dart-sdk/lib/async/future_impl.dart 647:5                                     [_completeWithValue]
dart-sdk/lib/async/future_impl.dart 721:7                                     <fn>
dart-sdk/lib/async/zone.dart 1399:13                                          _rootRun
dart-sdk/lib/async/zone.dart 1301:19                                          run
dart-sdk/lib/async/zone.dart 1209:7                                           runGuarded
dart-sdk/lib/async/zone.dart 1249:23                                          callback
dart-sdk/lib/async/schedule_microtask.dart 40:11                              _microtaskLoop
dart-sdk/lib/async/schedule_microtask.dart 49:5                               _startMicrotaskLoop
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 181:15           <fn>

The test description was:
  stay paused when seeking after video completed
═════════════════════════════════════════════════════════════════

end of failure 1

[flutter-triage-bot]

This issue is assigned to @ditman but has had no recent status updates. Please consider unassigning this issue if it is not going to be addressed in the near future. This allows people to have a clearer picture of what work is actually planned. Thanks!

[ditman]

Haven't had time to work on this one anymore. I suspect this is hard to fix because we're trying to keep in sync the state of the plugin with the internal state of the player, buuut I haven't rewritten this to verify, so: don't know.