-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Conversation
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.
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) |
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.
shall be moved to Next release ... sorry ...
src/types/events.ts
Outdated
@@ -81,7 +81,7 @@ export type OnTextTracksData = Readonly<{ | |||
export type OnVideoTracksData = Readonly<{ | |||
videoTracks: ReadonlyArray< | |||
Readonly<{ | |||
trackId: number; | |||
trackId: string; |
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.
Is this really necessary ?
It will be a breaking change for applications...
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.
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.
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 |
This request was created in connection with the issue #3312
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).