-
-
Notifications
You must be signed in to change notification settings - Fork 643
Use user defined seek times, relative to current position, with optional confirmation #4803
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
base: master
Are you sure you want to change the base?
Conversation
…Fragment - Implemented skipping forward and backward using DPAD keys based on user preferences.
- Introduced a new user preference to require confirmation before seeking with D-pad. - Updated CustomPlaybackOverlayFragment to handle seek preview and confirmation logic. - Added UI elements in PlaybackPreferencesScreen for the new preference. - Updated strings.xml with titles and descriptions for the new preference.
- Deleted the user preference for requiring confirmation before seeking with D-pad. - Cleaned up related logic in CustomPlaybackOverlayFragment to eliminate seek preview handling. - Removed UI elements and bindings in PlaybackPreferencesScreen associated with the seek confirmation preference. - Updated strings.xml to remove references to the seek confirmation feature.
- Introduced state management for accelerated seeking in CustomPlaybackOverlayFragment. - Implemented methods to reset seeking state and calculate accelerated seek amounts based on consecutive seeks. - Updated key event handling to reset seeking state when fast forward, rewind, or play/pause actions are performed. - Enhanced skip functionality to utilize accelerated seeking logic for both forward and backward skips.
- Removed accelerated seeking state management and related methods. - Updated key event handling to directly call fast forward and rewind methods for consistent visual feedback. - Eliminated unnecessary comments and code related to seeking state resets. - Streamlined skip functionality by removing direct seek mode logic.
- Updated fast forward and rewind functionality to use VideoPlayerAdapter for consistent behavior across controls. - Added getPlayerAdapter method in LeanbackOverlayFragment to facilitate this change. - Improved code readability by removing outdated comments and ensuring consistent method calls.
- Updated the getSafeSeekPosition method to use a minimum safe position of 100ms to prevent ExoPlayer issues with position 0. - Simplified the logic by using coerceIn for better readability and maintainability.
- Introduced a new user preference for enhanced D-pad seeking in UserPreferences.kt. - Updated CustomPlaybackOverlayFragment to utilize the new preference for improved D-pad seeking logic. - Added UI elements in PlaybackPreferencesScreen for the enhanced D-pad seeking option. - Updated strings.xml with titles and descriptions for the new preference.
- Revert FF/RW key changes to playbackController methods - Restore setInjectedViewsVisibility() when feature flag is disabled - Update setting name to "Natural D-pad controls"
…able - Update preference name and storage key to use "natural" terminology - Extract preference check to variable for better readability - Update comments to match new naming convention
…ayFragment - Revised comments for fast forward and rewind methods to clarify the purpose of using VideoPlayerAdapter. - Removed unnecessary blank lines for improved code readability.
- Reverted changes in debounce time for seeking actions. - Expose isSeekBarFocused method in CustomPlaybackTransportControlGlue to check seek bar focus state directly instead of checking it via reflection.
- Replaced SeekBar with ProgressBar in CustomPlaybackTransportControlGlue for better UI representation. - Introduced constants for minimum safe seek position and end position buffer in Utils.kt to enhance seek position logic.
- Added support for preview seeking in CustomPlaybackOverlayFragment, allowing users to preview seek positions without affecting playback. - Introduced methods to enter and exit preview seeking mode, and to update the preview position in PlaybackController. - Updated UserPreferences to enable natural D-pad seeking by default.
… and set a new preference instead to allow the user to choose if the skip will happen immediately or after confirmation - Renamed natural D-pad seeking preference to preview seeking in UserPreferences. - Updated CustomPlaybackOverlayFragment to support preview seeking, allowing users to preview positions before confirming. - Adjusted UI elements in PlaybackPreferencesScreen and strings.xml to reflect the new preview seeking feature.
…ckOverlayFragment
- MEDIA_PLAY and MEDIA_PLAY_PAUSE now confirm the preview position and resume playback (same behaviour as OK / Enter). - MEDIA_PAUSE is ignored during preview-seek; only Back / B / Escape cancel and return to the original position. - Live-TV, guide, chapter overlays and trick-play should remain unaffected by this feature. - Fixes progress bar and progress-reporting getting stuck when exiting preview with Play / Play-Pause.
…tControlGlue for now
…il specific logic
…s also when the preview mode is turned off when trickplay is enabled
…rs when media aspect ratio differs
…inology and clarity
…layback overlay is closing with cancel button
@nielsvanvelzen I created a new PR with better understanding of the codebase and hopefully a much cleaner solution |
@nielsvanvelzen just clarifying this is ready for review on my part |
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.
Initial review based on a quick look over the code and testing in emulator. I'm currently unsure if this is the correct approach or if we should replace the entire seekbar UI to avoid hacking into leanback code.
* Require confirmation before seeking to allows users to preview seek positions before confirming them. | ||
* When disabled, D-pad left/right will seek immediately. | ||
*/ | ||
var seekConfirmationRequired = booleanPreference("seek_confirmation", true) |
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.
We should choose one behavior for the seek control and stick with that. No preferences to complicate it necessary.
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.
To me, the intuitive behavior would be to pause the video when seeking, especially when thumbnail previews can be utilized, this is what I'm used to from netflix, youtube (well, smart tube), etc.
However, I saw that there were some issues open specifically about that by users that prefer the other behavior (#2399, #3154, ...).
I can leave only one option, but I would consider giving the option to choose, since it seems like it would allow more users to feel at home/enjoy the behavior they're used to (especially given supporting both behaviors in the code was pretty easy) 🙏 .
Anyway, let me know what you decide.
|
||
import org.jellyfin.androidtv.R; | ||
|
||
public class ThumbnailPreviewHandler { |
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.
New classes must be written in Kotlin
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.
Done
@@ -116,6 +116,18 @@ | |||
app:layout_constraintTop_toTopOf="parent" /> | |||
</androidx.constraintlayout.widget.ConstraintLayout> | |||
|
|||
<ImageView |
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.
New UI must be written in compose unless there is a good reason not to.
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.
Done
this.skipForwardLength = skipForwardLength; | ||
} | ||
|
||
public void updateThumbnailPreview(long previewSeekPosition) { |
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.
This doesn't properly cancel already loading images causing race conditions.
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.
@nielsvanvelzen I thought about this issue and figured there are 2 possible race conditions here:
- Multiple requests for thumbnails/tiles from the same sheet (e.g. tiles 0-99 all fetching the same tile sheet simultaneously)
- Callbacks returning in wrong order, showing outdated thumbnails during fast scrubbing
For the first issue - I think the ImageLoader
in CustomSeekProvider
handles caching/deduplication network requests? Although even if not then the tile sheets are relatively small, so this shouldn't be too heavy on the network.
As an optimization I added a prefetch for the sheet that includes the tiles near the starting position of the media.
For the second issue (which I assume you were referring to) - I tested simply canceling all previous requests, but this creates terrible UX during fast scrubbing since very few thumbnails actually get shown while the user is moving.
In the end I chose the following, which improved performance a lot in my tests:
- Ignore callbacks from older requests (meaning, we already showed a thumbnail from a newer request)
- Ignore stale callbacks that are 20+ requests behind (this threshold gave the best feel in my opinion)
Not sure what the implication/complexity of that would be since I'm too new here, but if this is not something that is planned anytime soon then perhaps one does not negate the other (meaning this PR as the short term solution)? I must say I am pretty shocked this isn't the native leanback behavior and every client has to implement this separately though. |
more precide intervals for the seek positions/thumbnails replaced thumbnail element to Compose
- added prefetching of relevant tile sheet in case trickplay is enabled to reduce chance of multiple HTTP requests for tiles in the same sheet around the playback position on play - fixed thumbnail compose look - performance improvements in ThumbnailPreviewHandler: -- ignoring a thumbnail request if it's too far away from the current request (and just returned with a big delay) -- ignoring a thumbnail request if we already displayed a thumbnail that came from a newer request
@nielsvanvelzen how would you like to proceed with this? |
Changes
Issues
#460
#2529
#2592
#3276
#3154
#2399