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.

Sign up for GitHub

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] Optimize caption retrieval with binary search in VideoPlayerController #8347

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
+141 −26
Open

[video_player] Optimize caption retrieval with binary search in VideoPlayerController #8347

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
a67e5ef
Optimize caption retrieval with binary search in VideoPlayerController
abdelaziz-mahdy Dec 26, 2024
106f838
Update Change Log
abdelaziz-mahdy Dec 26, 2024
44aefb8
Refactor: Optimize caption lookup using binary search with `package:c…
abdelaziz-mahdy Dec 26, 2024
08f21d9
Refactor: Enhance closed caption retrieval using binary search from `…
abdelaziz-mahdy Dec 26, 2024
be6ba80
Merge branch 'main' into binary-search-captions
abdelaziz-mahdy Dec 26, 2024
246ee3f
Bump version to 2.9.3
abdelaziz-mahdy Dec 27, 2024
1fd8e70
Merge branch 'binary-search-captions' of https://github.com/zezo357/p…
abdelaziz-mahdy Dec 27, 2024
2142659
test: Enhance closed caption tests for sorting and seeking behavior
abdelaziz-mahdy Dec 27, 2024
5e9ace9
feat: Add sortedCaptions getter to VideoPlayerController
abdelaziz-mahdy Dec 27, 2024
b74b352
fix: Correct binary search logic for caption retrieval and update tes…
abdelaziz-mahdy Dec 27, 2024
cedd1a4
adding more captions checks
abdelaziz-mahdy Dec 27, 2024
9a23e47
refactor: Remove sortedCaptions getter and related test.
abdelaziz-mahdy Dec 27, 2024
57260cb
test: Add test to verify input captions are unsorted
abdelaziz-mahdy Dec 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/video_player/video_player/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
## NEXT
## 2.9.3

* Updates minimum supported SDK version to Flutter 3.22/Dart 3.4.
* Use Binary search for finding the correct caption

## 2.9.2

Expand Down
53 changes: 46 additions & 7 deletions packages/video_player/video_player/lib/video_player.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import 'dart:async';
import 'dart:io';
import 'dart:math' as math;

import 'package:collection/collection.dart' as collection;
import 'package:flutter/foundation.dart';
import 'package:flutter/material.dart';
import 'package:flutter/services.dart';
Expand Down Expand Up @@ -378,6 +379,10 @@ class VideoPlayerController extends ValueNotifier<VideoPlayerValue> {

Future<ClosedCaptionFile>? _closedCaptionFileFuture;
ClosedCaptionFile? _closedCaptionFile;
List<Caption>? _sortedCaptions;

/// The sorted list of captions from the closed caption file.
List<Caption>? get sortedCaptions => _sortedCaptions;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is new public API needed? I don't see any implementation code using this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be able to access that list in testing, as I mentioned in the last comment I couldn't make it private and available for testing,

If the test is not needed I can remove it with the test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i removed it and the test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If something needs to be visible only for testing, it should be annotated @visibleForTesting.

It's not clear to me why even that would be necessary though; API to get the current caption is already public, and I would expect that it should be straightforward to construct a case where getting the caption for some time would be incorrect unless the captions are sorted internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

abstract class ClosedCaptionFile {
  /// The full list of captions from a given file.
  ///
  /// The [captions] will be in the order that they appear in the given file.
  List<Caption> get captions;
}

its only a getter so calling sort on it doesnt sort the original list, this is why i copied the list and sorted it and the public method was for the testing part only, if the test is not needed the public method is not needed

but _sortedCaptions is needed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is why i copied the list and sorted it
[...]
but _sortedCaptions is needed

I'm not sure what part of my comments led you to believe I don't understand _sortedCaptions. My comment is about the new public API.

the public method was for the testing part only, if the test is not needed the public method is not needed

I still don't understand why you are presenting a binary choice between adding new public API and having testing. Whether or not the correct caption is fetched for a given time is already an observable property with existing public API. You should be able to construct a case where, with binary search, the wrong caption is reported without the internal sorting step, and thus validate that it is being handled correctly without directly inspecting internal state via new public accessors.

Testing internal implementation details is an anti-pattern, and should only be done when the important properties cannot be tested via the actual client APIs. I have not seen any argument for why that would not be the case here. What scenario are you unable to test without adding a new public API?

Copy link
Contributor Author

@abdelaziz-mahdy abdelaziz-mahdy Dec 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i didnt mean all the testing of the testing is ignored, just this test captions are sorted correctly on initialization since it requires the expose of the public interface so i can check if they are sorted correctly

List<Caption>? get sortedCaptions without it i cant get the sorted list and without the sorted list i cant make that test

but i did add test cases for not sorted captions and tested retrieving them which works correctly and if the sorting is removed they wont pass

so i think those are enough, and no need to introduce the public interface since all of the cases are covered as you mentioned

and i did remove the public method.

edit: i am sorry if my english is not clear enough since its not my first language,

but in the end the public method is not needed since the other tests already covered the sorting checks

Timer? _timer;
bool _isDisposed = false;
Completer<void>? _creatingCompleter;
Expand Down Expand Up @@ -728,20 +733,46 @@ class VideoPlayerController extends ValueNotifier<VideoPlayerValue> {
///
/// If no [closedCaptionFile] was specified, this will always return an empty
/// [Caption].

Caption _getCaptionAt(Duration position) {
if (_closedCaptionFile == null) {
if (_closedCaptionFile == null || _sortedCaptions == null) {
return Caption.none;
}

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;
}

final int captionIndex = collection.binarySearch<Caption>(
_sortedCaptions!,
Caption(
number: -1,
start: delayedPosition,
end: delayedPosition,
text: '',
),
compare: (Caption candidate, Caption search) {
if (search.start < candidate.start) {
return 1;
} else if (search.start > candidate.end) {
return -1;
} else {
// delayedPosition is within [candidate.start, candidate.end]
return 0;
}
},
);

// -1 means not found by the binary search.
if (captionIndex == -1) {
return Caption.none;
}

final Caption caption = _sortedCaptions![captionIndex];
// check if it really fits within that caption's [start, end].
if (caption.start <= delayedPosition && caption.end >= delayedPosition) {
return caption;
}

return Caption.none;
return Caption.none; // No matching caption found
}

/// Returns the file containing closed captions for the video, if any.
Expand All @@ -763,6 +794,14 @@ class VideoPlayerController extends ValueNotifier<VideoPlayerValue> {
Future<ClosedCaptionFile>? closedCaptionFile,
) async {
_closedCaptionFile = await closedCaptionFile;

_sortedCaptions = _closedCaptionFile?.captions;

/// Sort the captions by start time so that we can do a binary search.
_sortedCaptions?.sort((Caption a, Caption b) {
return a.start.compareTo(b.start);
});

value = value.copyWith(caption: _getCaptionAt(value.position));
}

Expand Down
4 changes: 3 additions & 1 deletion packages/video_player/video_player/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ description: Flutter plugin for displaying inline video with other Flutter
widgets on Android, iOS, and web.
repository: https://github.com/flutter/packages/tree/main/packages/video_player/video_player
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+video_player%22
version: 2.9.2
version: 2.9.3

environment:
sdk: ^3.4.0
Expand All @@ -25,11 +25,13 @@ dependencies:
flutter:
sdk: flutter
html: ^0.15.0
collection: ^1.19.0
video_player_android: ^2.3.5
video_player_avfoundation: ^2.5.6
video_player_platform_interface: ^6.2.0
video_player_web: ^2.1.0


dev_dependencies:
flutter_test:
sdk: flutter
Expand Down
101 changes: 85 additions & 16 deletions packages/video_player/video_player/test/video_player_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,9 @@ class FakeController extends ValueNotifier<VideoPlayerValue>
Future<void> setClosedCaptionFile(
Future<ClosedCaptionFile>? closedCaptionFile,
) async {}

@override
List<Caption>? get sortedCaptions => null;
}

Future<ClosedCaptionFile> _loadClosedCaption() async =>
Expand All @@ -98,12 +101,34 @@ class _FakeClosedCaptionFile extends ClosedCaptionFile {
start: Duration(milliseconds: 100),
end: Duration(milliseconds: 200),
),

const Caption(
text: 'two',
number: 1,
start: Duration(milliseconds: 300),
end: Duration(milliseconds: 400),
),

/// out of order subs to test sorting
const Caption(
text: 'three',
number: 1,
start: Duration(milliseconds: 500),
end: Duration(milliseconds: 600),
),

const Caption(
text: 'five',
number: 0,
start: Duration(milliseconds: 700),
end: Duration(milliseconds: 800),
),
const Caption(
text: 'four',
number: 0,
start: Duration(milliseconds: 600),
end: Duration(milliseconds: 700),
),
];
}
}
Expand Down Expand Up @@ -727,7 +752,7 @@ void main() {
});

group('caption', () {
test('works when seeking', () async {
test('works when seeking, includes all captions', () async {
final VideoPlayerController controller =
VideoPlayerController.networkUrl(
_localhostUri,
Expand All @@ -750,17 +775,50 @@ void main() {
await controller.seekTo(const Duration(milliseconds: 301));
expect(controller.value.caption.text, 'two');

await controller.seekTo(const Duration(milliseconds: 400));
expect(controller.value.caption.text, 'two');

await controller.seekTo(const Duration(milliseconds: 401));
expect(controller.value.caption.text, '');

await controller.seekTo(const Duration(milliseconds: 500));
expect(controller.value.caption.text, 'three');

await controller.seekTo(const Duration(milliseconds: 601));
expect(controller.value.caption.text, 'four');

await controller.seekTo(const Duration(milliseconds: 701));
expect(controller.value.caption.text, 'five');

await controller.seekTo(const Duration(milliseconds: 800));
expect(controller.value.caption.text, 'five');
await controller.seekTo(const Duration(milliseconds: 801));
expect(controller.value.caption.text, '');

// Test going back
await controller.seekTo(const Duration(milliseconds: 300));
expect(controller.value.caption.text, 'two');
});

await controller.seekTo(const Duration(milliseconds: 301));
expect(controller.value.caption.text, 'two');
test('captions are sorted correctly on initialization', () async {
final VideoPlayerController controller =
VideoPlayerController.networkUrl(
_localhostUri,
closedCaptionFile: _loadClosedCaption(),
);

await controller.initialize();

final List<Caption> sortedCaptions = controller.sortedCaptions!;
for (int i = 1; i < sortedCaptions.length; i++) {
expect(
sortedCaptions[i - 1].start <= sortedCaptions[i].start, isTrue);
}
});

test('works when seeking with captionOffset positive', () async {
test(
'works when seeking with captionOffset positive, includes all captions',
() async {
final VideoPlayerController controller =
VideoPlayerController.networkUrl(
_localhostUri,
Expand All @@ -772,32 +830,43 @@ void main() {
expect(controller.value.position, Duration.zero);
expect(controller.value.caption.text, '');

await controller.seekTo(const Duration(milliseconds: 99));
expect(controller.value.caption.text, 'one');

await controller.seekTo(const Duration(milliseconds: 100));
expect(controller.value.caption.text, 'one');

await controller.seekTo(const Duration(milliseconds: 101));
expect(controller.value.caption.text, '');

await controller.seekTo(const Duration(milliseconds: 250));
await controller.seekTo(const Duration(milliseconds: 150));
expect(controller.value.caption.text, '');

await controller.seekTo(const Duration(milliseconds: 200));
expect(controller.value.caption.text, 'two');

await controller.seekTo(const Duration(milliseconds: 300));
await controller.seekTo(const Duration(milliseconds: 201));
expect(controller.value.caption.text, 'two');

await controller.seekTo(const Duration(milliseconds: 301));
expect(controller.value.caption.text, '');
await controller.seekTo(const Duration(milliseconds: 400));
expect(controller.value.caption.text, 'three');

await controller.seekTo(const Duration(milliseconds: 500));
expect(controller.value.caption.text, '');
expect(controller.value.caption.text, 'three');

await controller.seekTo(const Duration(milliseconds: 300));
expect(controller.value.caption.text, 'two');
await controller.seekTo(const Duration(milliseconds: 600));
expect(controller.value.caption.text, 'five');

await controller.seekTo(const Duration(milliseconds: 301));
await controller.seekTo(const Duration(milliseconds: 700));
expect(controller.value.caption.text, 'five');

await controller.seekTo(const Duration(milliseconds: 800));
expect(controller.value.caption.text, '');
});

test('works when seeking with captionOffset negative', () async {
test(
'works when seeking with captionOffset negative, includes all captions',
() async {
final VideoPlayerController controller =
VideoPlayerController.networkUrl(
_localhostUri,
Expand Down Expand Up @@ -831,10 +900,10 @@ void main() {
expect(controller.value.caption.text, 'two');

await controller.seekTo(const Duration(milliseconds: 600));
expect(controller.value.caption.text, '');
expect(controller.value.caption.text, 'three');

await controller.seekTo(const Duration(milliseconds: 300));
expect(controller.value.caption.text, 'one');
await controller.seekTo(const Duration(milliseconds: 700));
expect(controller.value.caption.text, 'three');
});

test('setClosedCaptionFile loads caption file', () async {
Expand Down
Loading