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

Updated track selection for exoplayer #3324

Closed
wants to merge 0 commits into from

Conversation

gizomo
Copy link

@gizomo gizomo commented Oct 30, 2023

This request was created in connection with the issue #3312

  • fixed trackId for video tracks
  • fixed video tracks selection by index and resolution
  • fixed audio tracks selection by language and index
  • removed track selection by title (title is not unique track property)
  • audio/text tracks title is based on label now (contains the human-readable text)

Video audio/video tracks selection is possible only supported formats now.
Selecting video by resolution takes into account multiple formats with exact height to support (saving ads)
Selecting video by index is possible only first group (as the most common case)
Selecting a audio/text track by title has been removed, since track titles may not be unique and in the case of several groups of tracks do not allow the correct selection.
Titles of Audio/text tracks are set based on format.label (the human-readable text).

Copy link
Member

@KrzysztofMoch KrzysztofMoch left a comment

Choose a reason for hiding this comment

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

Thanks @gizomo, it looks good 👍
Could you update trackId type in docs and events.ts, please?

@gizomo
Copy link
Author

gizomo commented Oct 30, 2023

Thanks @gizomo, it looks good 👍 Could you update trackId type in docs and events.ts, please?

Done

@stremium
Copy link

Does this track selection already work on iOS?

@gizomo
Copy link
Author

gizomo commented Nov 1, 2023

Does this track selection already work on iOS?

No, it doesn't. I haven't worked on this yet

CHANGELOG.md Outdated
@@ -10,6 +10,7 @@
- Android: ensure audio volume is changed in UI thread [3292](https://github.com/react-native-video/react-native-video/pull/3292)
- Android: multiple internal refactor and switch to kotlin
- Android: refactor log management and add an option to increase log verbosity [#3277](https://github.com/react-native-video/react-native-video/pull/3277)
- Android: updated track selection [#3312](https://github.com/react-native-video/react-native-video/issues/3312)
Copy link
Collaborator

Choose a reason for hiding this comment

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

shall be moved to Next release ... sorry ...

@@ -81,7 +81,7 @@ export type OnTextTracksData = Readonly<{
export type OnVideoTracksData = Readonly<{
videoTracks: ReadonlyArray<
Readonly<{
trackId: number;
trackId: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this really necessary ?
It will be a breaking change for applications...

Copy link
Author

@gizomo gizomo Nov 7, 2023

Choose a reason for hiding this comment

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

I made trackId a string because the 'id' of the Format in Android is a string type. The exoplayer simply assigns the indices of this group as an ids for all formats of the first video/audio group. And despite this they are also strings. Also if there is a second group, then the formats there will receive real ids from the playlist. And real ids may consist of any string characters, not just numbers.

Actually, the trackId has no meaning, because not used anywhere. If someone had previously used the trackId to select video tracks, it looked at least strange. The RNV neither before nor now allows you to select a track by trackId. If the trackId was meant as a video track index, then this is not obvious. However, selecting a video track by index is possible as for other types of tracks.

If there is a real need to assign ids to tracks, then I would rather use a combination of group and format indexes and do this for all track types. Moreover, as far as I understand, iOS uses grouping of tracks too. For example, “0.4” is a track with index 4 from a group with index 0. Using this id, we can accurately determine the track.

@YangJonghun
Copy link
Collaborator

@freeboub @gizomo
I think we can close this PR
(This PR was replaced by PR #3778 and merged)

@freeboub
Copy link
Collaborator

@freeboub @gizomo
I think we can close this PR
(This PR was replaced by PR #3778 and merged)

Yes more or less, in this patch there is also a change in isTrackSupported I didn't backport... I think there are still some issues with unsupported tracks ... But I don't have such unsupported track in the sample playlist manifest

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.

5 participants