[video_player] Update texture on seekTo #2758
Conversation
…-player-plugin-fix * commit '25a585dfeef6d2dc224fd0dddee327372de63782': [image_picker] iOS: fix bug, sometimes double click cancel button will crash (flutter#2625) [google_maps_flutter] Add liteModeEnabled option (flutter#2449) Update README.md (flutter#2768) [url_launcher_web] Launch mailto urls in same window in Safari (flutter#2740) update README with enableJavaScript info (flutter#2766) Run publish ci check on master (flutter#2764) [image_picker] Add documentation for Android external storage permissions (flutter#2765)
|
Hi @KevinTheGray |
|
@dthuering thanks for the heads up! resolved it. If you want, you can also give this PR a thumbs up and the original issue here a thumbs up: flutter/flutter#43567 I believe that's part of how they prioritize which ones to review and merge in |
|
I’m also eagerly awaiting this PR. |
# Conflicts: # packages/connectivity/connectivity/CHANGELOG.md
…KevinTheGray/plugins into kg-connectivity-ios-permissions
* master: [url_launcher, url_launcher_web]Added webOnlyWindowName parameter to launch() (flutter#2979) [battery_platform_interface] Introduce package. (flutter#2975) [url_launcher_platform_interface] Add webOnlyWindowName parameter to launch() (flutter#2974) [share] Add sharing file support (android & ios) (flutter#970) [integration_test] add support to get timeline (flutter#2947) [battery] Moved battery to its own subdir (flutter#2969) [integration_test] Fix channel name for iOS, docs (flutter#2972) [integration_test] Show stack trace of widget test errors on the platform side (flutter#2967) [video_player] Fix `setWithOthers` test (flutter#2970) [video_player_platform_interface] fix `setMixIWithOthers` test channel (flutter#2968) Add README instructions for enablePendingPurchases (flutter#2921) Rebase previous commits (flutter#2966) Avoid walleye 26 Updating documentation to use isEmpty check. (flutter#2955) [integration_test] Minor cleanup (flutter#2956) [path_provider, video_player] Tweak pubspecs to pass bulk analysis. (flutter#2959) [android_intent] fix platform package dep (flutter#2948) [shared_preferences_linux] update `file` package dep version to be compatible with framework (flutter#2950)
update pubspec version
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
Apologies that it took me so long to get to this. One small test structure comment, but otherwise looks good. (I will make sure to review future updates here promptly.)
| [FLTVideoPlayerPlugin registerWithRegistrar:partialRegistrar]; | ||
| FLTVideoPlayerPlugin<FLTVideoPlayerApi> *videoPlayerPlugin = | ||
| (FLTVideoPlayerPlugin<FLTVideoPlayerApi> *)[registry | ||
| valuePublishedByPlugin:@"TEST_FLTVideoPlayerPlugin"]; |
There was a problem hiding this comment.
This is a convoluted way to create a test object, and has the problem of affecting global state in the test (since the plugin instance will be leaked by the registration, which will in turn extend the life of these mocks and cause problems in other tests). We should instead:
- make
initWithRegistrar:public - call that directly to create a test instance of the plugin
|
Thanks for the review @stuartmorgan ! I'll get that change in as soon as possible |
|
@stuartmorgan I think this should be as you suggested now. Let me know if I didn't get it right, happy to make any changes! |
# Conflicts: # packages/video_player/video_player/CHANGELOG.md
|
I know we're still waiting on a few tweaks to this PR before it can be merged, but can some other people chime in to mention whether this PR fixes the seek behavior in their apps? edit: I was able to verify this PR fixes the issue |
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
I will make sure to review future updates here promptly.
😞
LGTM! @bparrishMines or @cyanglaz for secondary review (per policy).
|
LGTM from my side too. We have been using a version of this PR for quite a while in production. CC @guymclean |
12 participants
Description
@cbenhagen I basically copied your PR: #2397, and opened a new one here with changes after the big 0.10.10 update. It's still a very simple change.
I see that they want XC tests, but I need some clarification on how that is expected to work.@cyanglaz I see the PR you reference on the other PR, (#2445) but I don't understand how to run those tests. I commented on it with the issues I am seeing. If I could understand how I am expected to install the mock dependency and run the test, I could add the tests to this PR.Test added!What this fixes
Explicitly mark texture frame available on seekTo. This is necessary since flutter/engine@6a90b7f has changed the external texture data flow from a producer-consumer model. Now ios_external_texture_gl caches previous opengl texture, if no new frame are available, it does not execute the copyPixelBuffer method, just uses cached opengl texture to draw.
Related Issues
Fixes flutter/flutter#43567
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?