ALL-3995/Add subtitles functionality #47

joetimmins · 2017-06-16T14:29:52Z

Problem

We weren't showing subtitles in our NoPlayer demo app.

Solution

We added the concept of a PlayerSubtitlesView as an interface, so we had something the player could call back to in order to show subtitles.

We added methods to our Player interface to get the subtitle tracks from a video, show a specific subtitle track, or show just the first available track. We also added a method to hide subtitles.

We have also provided dynamic indexing for all renderer tracks. Before, we were hardcoding the track at position 0 to always be audio.

Test(s) added

We tested the ExoPlayerImpl class around the new subtitles methods we added.

Screenshots

After

Paired with

@zegnus

… in the demo application

…lass

juankysoriano · 2017-06-16T14:41:04Z

core/src/main/java/com/novoda/noplayer/PlayerSubtitlesView.java

+
+import java.util.List;
+
+public interface PlayerSubtitlesView {


What about renaming to SubtiteableView and making PlayerView extend SubtiteableView?

Otherwise I think this is still ok, but maybe with a renaming, PlayerSubtitlesView sounds weird to me (maybe just me)

juankysoriano · 2017-06-16T14:42:51Z

core/src/main/java/com/novoda/noplayer/Player.java

@@ -33,8 +33,18 @@

    void selectAudioTrack(PlayerAudioTrack audioTrack);

+    boolean hasAvailableSubtitles();


Do we have an equivalent method for audioTrack?

I think that the current impl assumes that there is always an audio track but that might not be the case for the subtitles

Wouldn't using getSubtitleTracks and checking for empty enough ?

yup, will remove

juankysoriano · 2017-06-16T14:43:16Z

core/src/main/java/com/novoda/noplayer/Player.java

+
+    void selectSubtitleTrack(PlayerSubtitleTrack subtitleTrack, PlayerSubtitlesView playerSubtitlesView);
+
+    void selectFirstAvailableSubtitleTrack(PlayerSubtitlesView playerSubtitlesView);


Do we have an equivalent method for audioTrack? If we don't how do we select the first available one for audio? We should push for symmetry

I agree. We should push for symmetry, I'd rather avoid having lots of helper methods that is going to make the Player interface bloat.

juankysoriano · 2017-06-16T14:43:29Z

core/src/main/java/com/novoda/noplayer/Player.java

+
+    void selectFirstAvailableSubtitleTrack(PlayerSubtitlesView playerSubtitlesView);
+
+    void clearSubtitleTrack(PlayerSubtitlesView playerSubtitlesView);


What would be an use case for this?

will rename to hide...

This is invoked when the user wants to turn subtitles off

jszmltr · 2017-06-16T14:45:11Z

core/src/main/java/com/novoda/noplayer/PlayerSubtitlesView.java

+
+import java.util.List;
+
+public interface PlayerSubtitlesView {


What I don't like about this interface that it is supposed to be used on both implementation of Player but it is coupled with exoplayer. Maybe we could find a way to pass something else than com.google.android.exoplayer2.text.Cue

I agree. We should avoid using ExoPlayer or MediaPlayer specific code in the no-player part of this library.

…of delegation

Dorvaryn · 2017-06-19T12:17:44Z

core/src/main/java/com/novoda/noplayer/mediaplayer/AndroidMediaPlayerImpl.java

    public List<PlayerAudioTrack> getAudioTracks() {
        return mediaPlayer.getAudioTracks();
    }

    @Override
+    public List<PlayerSubtitleTrack> getSubtitleTracks() {
+        SubtitlesError subtitlesError = new SubtitlesError("Subtitles not implemented for Android Media Player", new IllegalStateException());
+        listenersHolder.getErrorListeners().onError(this, subtitlesError);


Dorvaryn · 2017-06-19T12:20:48Z

core/src/main/java/com/novoda/noplayer/exoplayer/mediasource/ExoPlayerTrackSelector.java

+                              TrackGroupArray trackGroups,
+                              MappingTrackSelector.SelectionOverride selectionOverride) {
+        Optional<Integer> audioRendererIndex = rendererTrackIndexExtractor.extract(trackType, mappedTrackInfoLength(), rendererTypeRequester);
+        if (audioRendererIndex.isPresent()) {


This being absent is a case that should not happen right? I think w should warn the caller it didn't work so that we can bubble up until we can send an error though the ErrorListener.

We considered to return a boolean in the method indicating if it was ok or not and bubble up to the client, what about that? This should not be possible as this method requires a trackGroups that is retrieved by a get by type.

It might be good to do yep, if this happens we did something wrong and being warned rather than swallowing would help to catch it faster.

done, also added lots of tests 6794007

Dorvaryn · 2017-06-19T12:21:33Z

.../src/main/java/com/novoda/noplayer/exoplayer/mediasource/ExoPlayerSubtitleTrackSelector.java

+    public void selectFirstTextTrack(RendererTypeRequester rendererTypeRequester) {
+        List<PlayerSubtitleTrack> subtitleTracks = getSubtitleTracks(rendererTypeRequester);
+        if (subtitleTracks.isEmpty()) {
+            NoPlayerLog.e("No subtitles tracks available");


Should we bubble back up to warn through the ErrorListener ? Seems like a feedback the user of the lib should have

This method is not used anymore, will remove :)

juankysoriano · 2017-06-19T14:17:42Z

core/src/main/java/com/novoda/noplayer/mediaplayer/AndroidMediaPlayerImpl.java

 import com.novoda.noplayer.PlayerView;
 import com.novoda.noplayer.SurfaceHolderRequester;
 import com.novoda.noplayer.Timeout;
 import com.novoda.noplayer.VideoDuration;
 import com.novoda.noplayer.VideoPosition;
+import com.novoda.noplayer.exoplayer.playererror.SubtitlesError;


⚠️ This cannot be here

zegnus · 2017-06-19T14:41:54Z

test this please

zegnus and others added 17 commits June 15, 2017 16:46

Use dynamic indexes for audio and text tracks and add subtitle toggle…

fb17948

… in the demo application

Make subtitles appear when using correct dialog

04323ae

Add button for dismissing subtitles

a48fee2

Use MAp, not EnumMap

d515ea8

Add incorrectly set up test, will fix now

1fd37db

RendererTrackIndexExtractor depends on track counter, not exoplayer c…

ba44ad5

…lass

Wrap mappedTrackInfo class

4b9ff10

Wrap MappedTrackInfo

3fd2962

Add tests for subtitles and video tracks

89abb71

Add test for map with three track types

37a9a66

Add default subtitles option

282e269

Add hasAvailableSubtitles method

2892e37

Replace if with switch

9c6fbb4

Fix failing tests - they all pass now

5fdd019

Subtitles methods take PlayerSubtitlesView interface

6471da6

Add tests for subtitles functionality

5116c6e

Add tests for subtitles functionality

d7d5080

joetimmins added the feature label Jun 16, 2017

stefanhoth closed this Jun 16, 2017

joetimmins reopened this Jun 16, 2017

Remove unnecessary final parameter

9d568ed

juankysoriano self-requested a review June 16, 2017 14:39

juankysoriano self-assigned this Jun 16, 2017

Dorvaryn self-requested a review June 16, 2017 14:40

Dorvaryn self-assigned this Jun 16, 2017

juankysoriano reviewed Jun 16, 2017

View reviewed changes

jszmltr reviewed Jun 16, 2017

View reviewed changes

zegnus added 13 commits June 19, 2017 11:35

Manage requests to tracks that do not have available renderers

450779f

Rename

36c679f

Unset subtitleRendererOutput onDetach

3f5ecfc

Rename method

36c1a15

Remove subtitleRendererOutput on hideSubtitleTrack

dc84869

Remove NoPlayerLog from AndroidMediaPlayerImpl for subtitles

eb8e3ee

Remove helper method for selecting the first subtitle track

a31d1e5

Handle getSubtitleTracks at the AndroidMediaPlayerImpl level instead …

1b76add

…of delegation

Remove unnecessary method

e30450c

Extract TextRenderer into our own class

9d1cc0e

Rename class

19c58fc

Wrap text native cue in TextCues class

4089f65

Amend tests

b81b9ad

Dorvaryn reviewed Jun 19, 2017

View reviewed changes

zegnus added 3 commits June 19, 2017 13:25

Remove unnecessary methods

479babc

Reduce class visibility

39013f9

Return boolean for functions selecting tracks

6794007

Dorvaryn approved these changes Jun 19, 2017

View reviewed changes

zegnus added the ❌ do-not-merge label Jun 19, 2017

juankysoriano reviewed Jun 19, 2017

View reviewed changes

Move class

2fb1c97

zegnus changed the base branch from exo-player-2 to ALL-3995/feature-branch-select-subtitles June 19, 2017 14:41

zegnus removed the ❌ do-not-merge label Jun 19, 2017

juankysoriano approved these changes Jun 19, 2017

View reviewed changes

juankysoriano merged commit 65a817f into ALL-3995/feature-branch-select-subtitles Jun 19, 2017

juankysoriano deleted the ALL-3995/select-subtitles branch June 19, 2017 14:50

ouchadam mentioned this pull request Jul 5, 2017

ExoPlayer2 / Major library redesign #68

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ALL-3995/Add subtitles functionality #47

ALL-3995/Add subtitles functionality #47

joetimmins commented Jun 16, 2017

juankysoriano Jun 16, 2017

juankysoriano Jun 16, 2017

juankysoriano Jun 16, 2017

zegnus Jun 16, 2017 •

edited

Loading

Dorvaryn Jun 16, 2017

zegnus Jun 16, 2017

juankysoriano Jun 16, 2017

Mecharyry Jun 19, 2017

juankysoriano Jun 16, 2017

zegnus Jun 16, 2017

joetimmins Jun 16, 2017

jszmltr Jun 16, 2017

Mecharyry Jun 19, 2017

Dorvaryn Jun 19, 2017

Dorvaryn Jun 19, 2017

zegnus Jun 19, 2017

Dorvaryn Jun 19, 2017

zegnus Jun 19, 2017

Dorvaryn Jun 19, 2017

zegnus Jun 19, 2017

juankysoriano Jun 19, 2017

Mecharyry Jun 19, 2017

zegnus commented Jun 19, 2017


		import java.util.List;

		public interface PlayerSubtitlesView {

		@@ -33,8 +33,18 @@

		void selectAudioTrack(PlayerAudioTrack audioTrack);

		boolean hasAvailableSubtitles();


		void selectSubtitleTrack(PlayerSubtitleTrack subtitleTrack, PlayerSubtitlesView playerSubtitlesView);

		void selectFirstAvailableSubtitleTrack(PlayerSubtitlesView playerSubtitlesView);


		void selectFirstAvailableSubtitleTrack(PlayerSubtitlesView playerSubtitlesView);

		void clearSubtitleTrack(PlayerSubtitlesView playerSubtitlesView);

ALL-3995/Add subtitles functionality #47

ALL-3995/Add subtitles functionality #47

Conversation

joetimmins commented Jun 16, 2017

Problem

Solution

Test(s) added

Screenshots

Paired with

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zegnus Jun 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zegnus commented Jun 19, 2017

zegnus Jun 16, 2017 •

edited

Loading