packages
packages copied to clipboard
flutter
[video_player] Optimize caption retrieval with binary search in VideoPlayerController
Just found the todo in the code and it was from 3 years ago, so i wanted to implement it since it may help people using captions to have more optimized code
Pre-launch Checklist
- [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
- [x] I read the Tree Hygiene page, which explains my responsibilities.
- [x] I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use
dart format.) - [x] I signed the CLA.
- [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g.
[shared_preferences] - [ ] I linked to at least one issue that this PR fixes in the description above.
- [x] I updated
pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes. - [x] I updated
CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes. - [x] I updated/added relevant documentation (doc comments with
///). - [x] I added new tests to check the change I am making, or this PR is test-exempt.
- [x] All existing and new tests are passing.
If you need help, consider asking for advice on the #hackers-new channel on Discord.
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).
If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?
Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.
![flutter-dashboard[bot] avatar](https://avatars.githubusercontent.com/in/64368?v=4 "Published by a pub.dev verified publisher"){kind=small}
@stuartmorgan does this change need a test? It shouldn't be changing any behaviors, I'm not sure how a test would work honestly.
@stuartmorgan does this change need a test? It shouldn't be changing any behaviors, I'm not sure how a test would work honestly.
The fact that it's a more efficient search doesn't inherently need a test; that's essentially just a refactoring. However, this seems like a good opportunity to make sure we have reasonably robust coverage of caption correctness. E.g., is there a unit test that would fail if the initial sort were commented out?
@stuartmorgan does this change need a test? It shouldn't be changing any behaviors, I'm not sure how a test would work honestly.
The fact that it's a more efficient search doesn't inherently need a test; that's essentially just a refactoring. However, this seems like a good opportunity to make sure we have reasonably robust coverage of caption correctness. E.g., is there a unit test that would fail if the initial sort were commented out?
I can add that , so in general more caption edge cases
I just checked the test for sorting, and it shows that the captions are not sorted. After rechecking the code, I realized that the captions are implemented as a getter, which means it’s a read-only operation.
I have two solutions in mind:
1- Modify the public-facing controller by adding a getter to retrieve the sorted captions. However, based on my observations, it’s not recommended to change the interface, so this solution might not be valid.
2- Ensure that all ClosedCaptionFile classes return sorted captions. I will implement this solution for now, and if the public-facing approach turns out to be better, we can always revert to it.
Edit: fix number 2 is not clean since it doesnt enforce the sorting so i am going with the first fix
tests implemented and improved the logic for the binary search since it was incorrect in some cases
for this test test('captions are sorted correctly on initialization'
this test can be ignored and if thats the case we can remove the new public member to the class (my main issue is that i was not able to expose a private member only for testing) if someone can point me to the right direction that will help a lot
From triage: what's the status of this PR? Is it ready for re-review?
From triage: what's the status of this PR? Is it ready for re-review?
Yes it's ready for a re-review
Now I don't know why that test is failing. @stuartmorgan have you seen this type of failure with the analyze legacy tests before?
Now I don't know why that test is failing. @stuartmorgan have you seen this type of failure with the analyze legacy tests before?
Yes. The output explains the problem and what needs to be changed to fix it, so I'm not following what the issue is.
Is this ready for re-review?
Yes, all the checks and changes are fixed.
From triage: @abdelaziz-mahdy Are you still planning to update this PR based on the review comments?
From triage: @abdelaziz-mahdy Are you still planning to update this PR based on the review comments?
yes, I was busy, will update it and let you know
i created a mockup benchmark test https://github.com/abdelaziz-mahdy/packages/commit/880c2d243f6d35f44ecc8eff87a06a650bdf9c8c#diff-2030dff5accbb0e6ccf7df8fbd5fc0374da840a562c9594478f997b164a5921e
the results are great even in the case of big files, if we need to eliminate the not sorted to avoid the duplicate, i think we can do that, what do you think? but that will require us to override it i think?
if we need to eliminate the not sorted to avoid the duplicate, i think we can do that, what do you think?
Unfortunately:
- the API of the caption file is explicitly that the captions are in file order, and
- the video player has public API to get back the caption file that was set.
I don't see a compelling reason for either of those things to be true, but I'd rather not make a breaking change just for this.
The speedup is definitely worthwhile given those benchmarks, as the lookup in a large file could be a non-trivial portion of a frame allowance. I think the best option here is to track fixing the API in the future (https://github.com/flutter/flutter/issues/168823), but for now duplicate the list for fast lookup.
(Per my earlier comment though, we don't need two extra copies of the file, or an expensive compare step to determine when to create a new sorted list. We just need to reset the cached sorted version in the setter.)
I switched the logic to use whenComplete to only call the sort once on the future completion, I hope that matches what you meant, if not I currently don't know how to solve the event listener call to avoid resorting on every event fired,
the plan in my mind currently to add a flag so I know if the current file was sorted or not, but don't know if its good or not.
this will be really cool to have in the subtitle selection @stuartmorgan-g , pls review the latest changes 🙏🏻
@abdelaziz-mahdy Are you still planning on updating this PR to address the review comments above?
@abdelaziz-mahdy Are you still planning on updating this PR to address the review comments above?
Sadly I have been quite busy lately, and not easy to figure out, so I cant fix it yet
Review comments addressed. Ready for re-review.
Changes Made:
- Optimized caption lookup
- Added local variable in _getCaptionAt to avoid repeated force-unwraps
- Removed unnecessary validity check after binary search (compare function guarantees correctness)
- Fixed caching strategy
- Refactored setClosedCaptionFile to always sort when explicitly setting a new file
- Updated _updateClosedCaptionWithFuture to only sort on first initialization (_sortedCaptions == null)
- Prevents expensive re-sorting on every event listener call
- Fixed test structure
- Unnested incorrectly placed tests
- Added edge case tests
Added 5 new tests covering binary search edge cases:
- Exact caption start/end time boundaries
- Gaps between captions
- Single caption scenario
- Overlapping captions
The caching optimization ensures captions are only sorted when the file actually changes (initialization or explicit setClosedCaptionFile call), avoiding performance issues.