[video_player] Playback speed #3084
[video_player] Playback speed #3084
Conversation
@ditman Can you make sure that #3031, #1400, #860, flutter/flutter#29900, and flutter/flutter#16624 are closed after this is merged? 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to revive this! Let's merge once _web is ready, and CI passes here!
There's a bunch of warnings that seem legit:
(I'll restart the test just in case) |
…back-speed-video-player
Published video_player v0.11.0, which contains the playback speed feature! |
details:nil]); | ||
} | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this PR already merged, but just wanted to bring attention to the fact that the video_player dart code is not built to handle errors properly after initialization.
If someone tries to set the playback speed after the video player has already been initialized, and this sends a flutter error, the video player controller in dart will wipe all its information, including its size, causing exceptions and the video to disappear.
void errorListener(Object obj) {
final PlatformException e = obj;
value = VideoPlayerValue.erroneous(e.message); <======= This is the problem after it's already been initialized
_timer?.cancel();
if (!initializingCompleter.isCompleted) {
initializingCompleter.completeError(obj);
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dannyvalentesonos can you create a new issue for this? That way more people than whoever participated in this PR will see it!
(Please select the "p:video_player" tag, or start the issue with "[video_player]" so the triage knows it's a plugins issue.)
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What other behavior would you expect, though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sending an error to dart is totally acceptable. Nulling out the size so the video is no longer shown though isn't so great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created flutter/flutter#68914. Thanks!
Part
Part 3/3 of #3031.
This is the
video_player
part of that PR.Checklist, full code, et al. can be found in the linked PR.