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

implement stream state management #553

Merged
merged 14 commits into from
Dec 1, 2023

Conversation

TheBeastLT
Copy link
Contributor

Allow storing stream state based on user actions (user changes default audio/subs track, changes sync or playback speed) and restoring it when playing the same stream or reusing parts of the state when playing next videos in the meta.

@TheBeastLT TheBeastLT force-pushed the implement-stream-state-management branch from 1dae6e8 to 6c09576 Compare November 7, 2023 07:24
@elpiel elpiel mentioned this pull request Nov 7, 2023
@elpiel
Copy link
Member

elpiel commented Nov 7, 2023

Closes #55

@tymmesyde
Copy link
Member

What are subtitle_language, audio_language and player_type for ?

@tymmesyde
Copy link
Member

Should probably be named to reflect the stremio-video properties names:
https://github.com/Stremio/stremio-video/blob/master/src/HTMLVideo/HTMLVideo.js#L673

subtitle_id -> selected_subtitles_track_id
audio_id -> selected_audio_track_id
subtitle_delay -> subtitles_offset
etc...

@tymmesyde
Copy link
Member

tymmesyde commented Nov 10, 2023

Also do you think handling the extra subtitles tracks would be nice ?
Like selected_extra_subtitles_track_id to also set selected subtitles from addon requests

@TheBeastLT TheBeastLT force-pushed the implement-stream-state-management branch from 01f418e to 42bc988 Compare November 10, 2023 12:57
Copy link
Member

@elpiel elpiel left a comment

Choose a reason for hiding this comment

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

2 small nitpicks. I need to test the PR more thoroughly.

src/models/ctx/update_streams.rs Outdated Show resolved Hide resolved
src/types/streams/streams_item.rs Outdated Show resolved Hide resolved
@TheBeastLT
Copy link
Contributor Author

TheBeastLT commented Nov 10, 2023

Also do you think handling the extra subtitles tracks would be nice ? Like selected_extra_subtitles_track_id to also set selected subtitles from addon requests

This is to store any state of any action the user does manually. It should not store any info that is selected by default based on settings.
subtitle_track_id can be either the id of the embedded subs or identifier of addon returned and manually selected subtitle.
language param for audio and subtitles is required if you want to maintain the same embedded track selected when moving to the next video. Their id is usually the index of the track in video file, but since track order might change then it's best to also have the language so that you would be sure that in the next video you're selecting the correct track (id and lang must match).
player_type is required since in android tv you can change the player on demand from the player view, so you'd want to maintain this info and use the same player throughtout the series. (Ie. on TV some users prefer using libVLC on anime videos due to better ass subs support)

@tymmesyde
Copy link
Member

Also do you think handling the extra subtitles tracks would be nice ? Like selected_extra_subtitles_track_id to also set selected subtitles from addon requests

This is to store any state of any action the user does manually. It should not store any info that is selected by default based on settings. subtitle_track_id can be either the id of the embedded subs or identifier of addon returned and manually selected subtitle. language param for audio and subtitles is required if you want to maintain the same embedded track selected when moving to the next video. Their id is usually the index of the track in video file, but since track order might change then it's best to also have the language so that you would be sure that in the next video you're selecting the correct track (id and lang must match). player_type is required since in android tv you can change the player on demand from the player view, so you'd want to maintain this info and use the same player throughtout the series. (Ie. on TV some users prefer using libVLC on anime videos due to better ass subs support)

In web / theater since we use stremio-video, it makes the distinction between embedded subtitles tracks and the ones provided by addons
selectedSubtitlesTrackId is for embedded tracks
selectedExtraSubtitlesTrackId is for addon tracks

For embedded tracks it's their index but for extra tracks it's the id returned by the addon:
https://github.com/Stremio/stremio-addon-sdk/blob/master/docs/api/responses/subtitles.md#subtitles-object

So maybe we can remove *_language and / or add selected_extra_subtitles_track_id,
This way we don't have to match for id (index) + language, we can just match the id

@TheBeastLT
Copy link
Contributor Author

TheBeastLT commented Nov 10, 2023

In web / theater since we use stremio-video, it makes the distinction between embedded subtitles tracks and the ones provided by addons selectedSubtitlesTrackId is for embedded tracks selectedExtraSubtitlesTrackId is for addon tracks

You can only have one selected subtitle at the time, so there shouldn't be any distinction. What you have is web specific, but still you have to decide which subs you're displaying so in the end there should be a single active id.

For embedded tracks it's their index but for extra tracks it's the id returned by the addon: https://github.com/Stremio/stremio-addon-sdk/blob/master/docs/api/responses/subtitles.md#subtitles-object

So maybe we can remove *_language and / or add selected_extra_subtitles_track_id, This way we don't have to match for id (index) + language, we can just match the id

Like I mentioned, since it's an index for embedded tracks, you need some extra protection to check that if you want to select that track for the next stream that the order didn't change, so we need the language.

@tymmesyde
Copy link
Member

tymmesyde commented Nov 10, 2023

In web / theater since we use stremio-video, it makes the distinction between embedded subtitles tracks and the ones provided by addons selectedSubtitlesTrackId is for embedded tracks selectedExtraSubtitlesTrackId is for addon tracks

You can only have one selected subtitle at the time, so there shouldn't be any distinction. What you have is web specific, but still you have to decide which subs you're displaying so in the end there should be a single id.

For embedded tracks it's their index but for extra tracks it's the id returned by the addon: https://github.com/Stremio/stremio-addon-sdk/blob/master/docs/api/responses/subtitles.md#subtitles-object
So maybe we can remove *_language and / or add selected_extra_subtitles_track_id, This way we don't have to match for id (index) + language, we can just match the id

Like I mentioned, since it's an index for embedded tracks, you need some extra protection to check that if you want to select that track for the next stream that the order didn't change, so we need the language.

I mean we need selected_extra_subtitles_track_id for every apps that uses stremio-video
Otherwise we don't know if subtitle_track_id is for embedded or extra / addon tracks

@TheBeastLT
Copy link
Contributor Author

In web / theater since we use stremio-video, it makes the distinction between embedded subtitles tracks and the ones provided by addons selectedSubtitlesTrackId is for embedded tracks selectedExtraSubtitlesTrackId is for addon tracks

You can only have one selected subtitle at the time, so there shouldn't be any distinction. What you have is web specific, but still you have to decide which subs you're displaying so in the end there should be a single id.

For embedded tracks it's their index but for extra tracks it's the id returned by the addon: https://github.com/Stremio/stremio-addon-sdk/blob/master/docs/api/responses/subtitles.md#subtitles-object
So maybe we can remove *_language and / or add selected_extra_subtitles_track_id, This way we don't have to match for id (index) + language, we can just match the id

Like I mentioned, since it's an index for embedded tracks, you need some extra protection to check that if you want to select that track for the next stream that the order didn't change, so we need the language.

I mean we need selected_extra_subtitles_track_id for every apps that uses stremio-video Otherwise we don't know if subtitle_id is for embedded or extra / addon tracks

Atm in core subtitle id should be the url to the subtitle itself as that's unique, so it will be very different from embedded subs id, so you will be able to just iterate through combined array of embedded subs and addons subs and determine which ones need to be selected.

@tymmesyde
Copy link
Member

Atm in core subtitle id should be the url to the subtitle itself as that's unique, so it will be very different from embedded subs id, so you will be able to just iterate through combined array of embedded subs and addons subs and determine which ones need to be selected.

I'm talking about subtitle_track_id from this PR, we need a way to know if this is an id for embedded track or extra subtitles track
So i suggest adding another field extra_subtitles_track_id

@TheBeastLT
Copy link
Contributor Author

I'm talking about subtitle_track_id from this PR, we need a way to know if this is an id for embedded track or extra subtitles track So i suggest adding another field extra_subtitles_track_id

Don't think we're on the same page here, so to clarify:
subtitle_track_id in StreamState is an identifier of currently shown subtitle the user manually selected (subtitle chosen by default based on preference settings shouldn't be stored here). There can only be one subtitles shown at a time regardless whether it's embedded or addon subtitle.

Embedded subtitles usually come in format (index,name,lang) and for them index would be the unique identifier which is a number.

Addon subtitle response in core has structure (lang,url) where unique identifier for them would be url.

So at any point you can always determine the subtitle subtitle_track_id based on these unique identifiers and you'll either find the sub in embedded track when and index was stored (together checking the lang as well to be safe) or iterating subtitle responses and checking based on url.

Having subtitle_track_id and then extra_subtitle_track_id will lead to having corrupt state, since when you populate both of them you won't know which the user actually choose, the embedded one or the addon one, so there needs to be a single id stored.
Same is done currently in desktop/mobile, just that it's stored per language preference https://github.com/Stremio/stremio-models/blob/baa9ac813c2fcb2830a6af7110110a56edb24135/subtitles.js#L482

src/types/streams/streams_bucket.rs Outdated Show resolved Hide resolved
src/types/streams/streams_bucket.rs Outdated Show resolved Hide resolved
src/types/streams/streams_bucket.rs Show resolved Hide resolved
src/types/streams/streams_item.rs Show resolved Hide resolved
src/types/streams/streams_item.rs Show resolved Hide resolved
Copy link
Member

@elpiel elpiel left a comment

Choose a reason for hiding this comment

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

LGMT, Great job!

@elpiel elpiel merged commit 109d2cb into development Dec 1, 2023
1 check passed
@elpiel elpiel deleted the implement-stream-state-management branch December 1, 2023 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants