Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[video_player]: reduce video player position update interval from 500ms to 100ms #8346

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

abdelaziz-mahdy
Copy link
Contributor

@abdelaziz-mahdy abdelaziz-mahdy commented Dec 26, 2024

Description
reduce video player position update interval from 500ms to 100ms to fix the issue of fast captions not showing for their intended time

List which issues are fixed by this PR. You must list at least one issue.
flutter/flutter#160855

Pre-launch Checklist

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

@stuartmorgan
Copy link
Contributor

Per discussion linked from flutter/flutter#90080 (comment) I'm skeptical that this is API we want to add. If subtitles don't work reliably at the current update frequency, and higher update frequencies don't have noticeable performance impact, then we should simply increase the frequency. If it does have performance issues, then we should figure out a different way of triggering subtitle changes. Either way, I would expect that we should be able to resolve the issue without new public API and without requiring extra configuration.

@abdelaziz-mahdy
Copy link
Contributor Author

I kept searching for the previous discussions but couldn't find them. Thank you for providing the link.

Would an update frequency of 100ms work? I think it would be very effective for subtitles and not frequent enough to cause a high load on devices.

if you agree, i can open a new pr with that change and add a test to make sure captions are included in the timer consideration

@abdelaziz-mahdy
Copy link
Contributor Author

note: this is how i override it from the app side, i didnt like the approach this is why i opened this pr

  void startPositionUpdaterForcer() {
    _positionUpdaterForcerTimer?.cancel();
    _positionUpdaterForcerTimer =
        Timer.periodic(Duration(milliseconds: 100), (timer) {
      _positionUpdaterForcer();
    });
  }

  void stopPositionUpdaterForcer() {
    _positionUpdaterForcerTimer?.cancel();
  }
  /// These parts exists in the video_player package but slightly edited to be called from outside of the class
  Future<void> _positionUpdaterForcer() async {
    Duration? position = await controller.position;
    if (position == null) {
      return;
    }
    VideoPlayerValue value = controller.value;
    if (position > value.duration) {
      position = value.duration;
    }
    value = value.copyWith(
      position: position,
      caption: await _getCaptionAt(position),
      isCompleted: position == value.duration,
    );
  }

  Future<Caption> _getCaptionAt(Duration position) async {
    /// the internal [ClosedCaptionFile]
    ClosedCaptionFile? closedCaptionFile;
    closedCaptionFile = await controller.closedCaptionFile;
    if (closedCaptionFile == null) {
      return Caption.none;
    }

    VideoPlayerValue value = controller.value;
    final Duration delayedPosition = position + value.captionOffset;
    // TODO(johnsonmh): This would be more efficient as a binary search.
    for (final Caption caption in (closedCaptionFile).captions) {
      if (caption.start <= delayedPosition && caption.end >= delayedPosition) {
        return caption;
      }
    }

    return Caption.none;
  }

@abdelaziz-mahdy
Copy link
Contributor Author

i updated the pr to be an update to value from 500 to 100 ms, and the test case. hope thats okay

@abdelaziz-mahdy abdelaziz-mahdy changed the title [video_player]: add setVideoPositionUpdatesInterval method to allow setting position updates periodic intervals [video_player]: reduce video player position update interval from 500ms to 100ms Dec 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants