Exo-player two facade and forwarders #23

Mecharyry · 2017-05-30T08:40:30Z

Problem

ExoPlayerForwarder contained all of the events that we listen to from ExoPlayer and we created Forwarders that adapted the ExoPlayer events to our no-player events. Some of these Forwarders contained state. Facade and Imple were mixing responsibilities with no clear boundary.

Solution

Remove the catch all Forwarder and implement specific forwarders for the events we want to listen to.
Create a generic forwarder that has as collaborators the specific forwarder implementations.
Remove the Facade from exo-player because the code is small enough to be encapsulated in one class with collaborators.

Test(s) added

No tests atm, we hope to branch off and have these covered soon.

Screenshots

No UI changes.

Paired with

@Dorvaryn and @jackSzm

…eners to forwarders.

Mecharyry · 2017-05-30T08:56:02Z

core/src/main/java/com/novoda/noplayer/exoplayer/ExoPlayerEventForwarder.java

-
-import java.io.IOException;
-
-abstract class ExoPlayerEventForwarder implements ExoPlayerForwarder {


🤢 we implemented all events on this one forwarder. We now have dedicated forwarders and a generic forwarder that has the specific forwarders as collaborators.

Mecharyry · 2017-05-30T09:00:03Z

core/src/main/java/com/novoda/noplayer/exoplayer/ExoPlayerTwoImpl.java

@@ -13,74 +22,93 @@
 import com.novoda.noplayer.VideoContainer;
 import com.novoda.noplayer.VideoDuration;
 import com.novoda.noplayer.VideoPosition;
+import com.novoda.noplayer.exoplayer.forwarder.ExoPlayerForwarder;
 import com.novoda.noplayer.player.PlayerInformation;

 import java.util.Collections;
 import java.util.List;

 public class ExoPlayerTwoImpl extends PlayerListenersHolder implements Player {


Facade became a wrapper during our refactorings so quite a few of the calls here are just inlines.

…is currently.

Mecharyry · 2017-05-30T09:03:45Z

core/src/main/java/com/novoda/noplayer/exoplayer/forwarder/BitrateForwarder.java

+
+import java.io.IOException;
+
+class BitrateForwarder implements AdaptiveMediaSourceEventListener {


No longer extending from the abstract ExoPlayerEventForwarder, instead, we map directly from the ExoPlayer events to the no-player events.

Mecharyry · 2017-05-30T09:05:22Z

core/src/main/java/com/novoda/noplayer/exoplayer/forwarder/BitrateForwarder.java

+
+    @Override
+    public void onLoadStarted(DataSpec dataSpec, int dataType, int trackType, Format trackFormat, int trackSelectionReason, Object trackSelectionData, long mediaStartTimeMs, long mediaEndTimeMs, long elapsedRealtimeMs) {
+        //TODO should we send ?


These TODOs were hidden in the abstract ExoPlayerEventForwarder class, not anymore! We should figure out what to do with these!

Mecharyry · 2017-05-30T09:05:54Z

core/src/main/java/com/novoda/noplayer/exoplayer/forwarder/BufferStateForwarder.java

+import com.google.android.exoplayer2.trackselection.TrackSelectionArray;
+import com.novoda.noplayer.listeners.BufferStateListeners;
+
+class BufferStateForwarder implements ExoPlayer.EventListener {


Same here, not extending from the abstract ExoPlayerEventForwarder instead implementing the interface from ExoPlayer that we care about.

Mecharyry · 2017-05-30T09:06:23Z

core/src/main/java/com/novoda/noplayer/exoplayer/forwarder/BufferStateForwarder.java

+
+    @Override
+    public void onTimelineChanged(Timeline timeline, Object manifest) {
+        //TODO should we send ?


Same here, we need to figure out how we should map these from ExoPlayer to no-player

Mecharyry · 2017-05-30T09:08:00Z

core/src/main/java/com/novoda/noplayer/exoplayer/forwarder/EventInfoForwarder.java

+
+import java.util.HashMap;
+
+class EventInfoForwarder implements ExoPlayer.EventListener {


Same as others above. We no longer extend from ExoPlayerEventForwarder but instead implement the interface we care about directly from ExoPlayer.

Mecharyry · 2017-05-30T09:13:17Z

...main/java/com/novoda/noplayer/exoplayer/forwarder/ExoPlayerExtractorMediaSourceListener.java

+import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
+
+class ExoPlayerExtractorMediaSourceListener implements ExtractorMediaSource.EventListener {


Same as others above. We no longer extend from ExoPlayerEventForwarder but instead implement the interface we care about directly from ExoPlayer.

Mecharyry · 2017-05-30T09:14:34Z

core/src/main/java/com/novoda/noplayer/exoplayer/forwarder/ExoPlayerForwarder.java

+import com.novoda.noplayer.listeners.PreparedListeners;
+import com.novoda.noplayer.listeners.VideoSizeChangedListeners;
+
+public class ExoPlayerForwarder {


Our generic forwarder that contains all of the events that we are interested in from ExoPlayer.

Mecharyry · 2017-05-30T09:15:08Z

...c/main/java/com/novoda/noplayer/exoplayer/forwarder/ExoPlayerVideoRendererEventListener.java

+import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
+
+class ExoPlayerVideoRendererEventListener implements VideoRendererEventListener {


Same as others above. We no longer extend from ExoPlayerEventForwarder but instead implement the interface we care about directly from ExoPlayer.

Mecharyry · 2017-05-30T09:15:22Z

core/src/main/java/com/novoda/noplayer/exoplayer/forwarder/ExtractorInfoForwarder.java

+import java.io.IOException;
+import java.util.HashMap;
+
+class ExtractorInfoForwarder implements ExtractorMediaSource.EventListener {


Same as others above. We no longer extend from ExoPlayerEventForwarder but instead implement the interface we care about directly from ExoPlayer.

To avoid huge classes we have an InfoForwarder for each of the ExoPlayer interfaces that we are interested in monitoring.

Mecharyry · 2017-05-30T09:16:59Z

core/src/main/java/com/novoda/noplayer/exoplayer/forwarder/MediaSourceInfoForwarder.java

 import com.google.android.exoplayer2.upstream.DataSpec;
 import com.novoda.noplayer.listeners.InfoListeners;

 import java.io.IOException;
 import java.util.HashMap;

-class InfoForwarder extends ExoPlayerEventForwarder {
+class MediaSourceInfoForwarder implements AdaptiveMediaSourceEventListener {


Same as others above. We no longer extend from ExoPlayerEventForwarder but instead implement the interface we care about directly from ExoPlayer.

Mecharyry · 2017-05-30T09:17:25Z

core/src/main/java/com/novoda/noplayer/exoplayer/forwarder/MediaSourceOnErrorForwarder.java

 import com.novoda.noplayer.Player;
 import com.novoda.noplayer.exoplayer.playererror.MediaSourceError;
 import com.novoda.noplayer.listeners.ErrorListeners;

 import java.io.IOException;

-class OnErrorForwarder extends ExoPlayerEventForwarder {
+class MediaSourceOnErrorForwarder implements ExtractorMediaSource.EventListener {


Same as others above. We no longer extend from ExoPlayerEventForwarder but instead implement the interface we care about directly from ExoPlayer.

Mecharyry · 2017-05-30T09:17:54Z

core/src/main/java/com/novoda/noplayer/exoplayer/forwarder/OnCompletionForwarder.java

+import com.google.android.exoplayer2.trackselection.TrackSelectionArray;
+import com.novoda.noplayer.listeners.CompletionListeners;
+
+class OnCompletionForwarder implements ExoPlayer.EventListener {


Same as others above. We no longer extend from ExoPlayerEventForwarder but instead implement the interface we care about directly from ExoPlayer.

Mecharyry · 2017-05-30T09:18:21Z

core/src/main/java/com/novoda/noplayer/exoplayer/forwarder/OnCompletionForwarder.java

+
+    @Override
+    public void onTimelineChanged(Timeline timeline, Object manifest) {
+        //TODO should we send ?


Again, we need to figure out the mapping from ExoPlayer to no-player for these additional events.

Mecharyry · 2017-05-30T09:18:29Z

core/src/main/java/com/novoda/noplayer/exoplayer/forwarder/OnPrepareForwarder.java

+import com.novoda.noplayer.PlayerState;
+import com.novoda.noplayer.listeners.PreparedListeners;
+
+class OnPrepareForwarder implements ExoPlayer.EventListener {


Same as others above. We no longer extend from ExoPlayerEventForwarder but instead implement the interface we care about directly from ExoPlayer.

Mecharyry · 2017-05-30T09:19:02Z

core/src/main/java/com/novoda/noplayer/exoplayer/forwarder/OnPrepareForwarder.java

+
+    @Override
+    public void onTimelineChanged(Timeline timeline, Object manifest) {
+        //TODO should we send ?


Once again, again. We need to figure out the mapping between ExoPlayer and no-player for these additional events 😄

Mecharyry · 2017-05-30T09:19:10Z

core/src/main/java/com/novoda/noplayer/exoplayer/forwarder/PlayerOnErrorForwarder.java

+import com.novoda.noplayer.Player;
+import com.novoda.noplayer.listeners.ErrorListeners;
+
+class PlayerOnErrorForwarder implements ExoPlayer.EventListener {


Same as others above. We no longer extend from ExoPlayerEventForwarder but instead implement the interface we care about directly from ExoPlayer.

Mecharyry · 2017-05-30T09:19:41Z

core/src/main/java/com/novoda/noplayer/exoplayer/forwarder/PlayerOnErrorForwarder.java

+
+    @Override
+    public void onTimelineChanged(Timeline timeline, Object manifest) {
+        //TODO should we send ?


AND AGAIN! 😄 🎉 We need to figure out the mapping between exoplayer and no-player.

Mecharyry · 2017-05-30T09:19:50Z

core/src/main/java/com/novoda/noplayer/exoplayer/forwarder/VideoRendererInfoForwarder.java

+
+import java.util.HashMap;
+
+class VideoRendererInfoForwarder implements VideoRendererEventListener {


Same as others above. We no longer extend from ExoPlayerEventForwarder but instead implement the interface we care about directly from ExoPlayer.

There are many info forwarders and each implements a single interface from ExoPlayer and are added to the generic forwarder.

Mecharyry · 2017-05-30T09:20:40Z

core/src/main/java/com/novoda/noplayer/exoplayer/forwarder/VideoSizeChangedForwarder.java

+import com.google.android.exoplayer2.video.VideoRendererEventListener;
+import com.novoda.noplayer.listeners.VideoSizeChangedListeners;
+
+class VideoSizeChangedForwarder implements VideoRendererEventListener {


Same as others above. We no longer extend from ExoPlayerEventForwarder but instead implement the interface we care about directly from ExoPlayer.

Mecharyry · 2017-05-30T09:21:05Z

core/src/main/java/com/novoda/noplayer/exoplayer/forwarder/VideoSizeChangedForwarder.java

+
+    @Override
+    public void onVideoEnabled(DecoderCounters counters) {
+        //TODO should we send ?


OOOOoooo more! We need to figure out the mapping between ExoPlayer and no-player.

Mecharyry · 2017-05-30T09:21:42Z

core/src/main/java/com/novoda/noplayer/listeners/PreparedListeners.java

@@ -10,6 +10,8 @@

    private final Set<Player.PreparedListener> listeners = new CopyOnWriteArraySet<>();

+    private boolean hasPrepared = false;


We moved the state up a level because it made more sense here than in each of the individual listeners.

Mecharyry · 2017-05-30T09:22:03Z

demo/src/main/java/com/novoda/demo/MainActivity.java

@@ -30,7 +30,7 @@ protected void onCreate(Bundle savedInstanceState) {
    @Override
    protected void onStart() {
        super.onStart();
-        player = new PlayerFactory(this, PrioritisedPlayers.prioritiseMediaPlayer()).create();
+        player = new PlayerFactory(this, PrioritisedPlayers.prioritiseExoPlayer()).create();


🤷‍♂️ because we are working on ExoPlayer 👅

Leave a TODO maybe?

Yea, we should have this exposed in the UI really.

rock3r · 2017-05-30T10:30:05Z

core/src/main/java/com/novoda/noplayer/exoplayer/ExoPlayerTwoImpl.java

+    private ExoPlayerTwoImpl(SimpleExoPlayer exoPlayer, MediaSourceFactory mediaSourceFactory, ExoPlayerForwarder exoPlayerForwarder) {
+        this.exoPlayer = exoPlayer;
+        this.mediaSourceFactory = mediaSourceFactory;
+        forwarder = exoPlayerForwarder;
        Heart.Heartbeat<Player> onHeartbeat = new Heart.Heartbeat<>(getHeartbeatCallbacks(), this);


Any chance we can move these inits to params so that we only pass in heart instead? I know this is pre-existig but we try avoiding work in ctors usually and this seems a good time to do it

I think we have done this in a follow-up branch that we are waiting to open 😆 but yes we should try and move all of these out of the constructor.

rock3r · 2017-05-30T10:34:55Z

core/src/main/java/com/novoda/noplayer/exoplayer/forwarder/EventInfoForwarder.java

+    @Override
+    public void onTimelineChanged(Timeline timeline, Object manifest) {
+        HashMap<String, String> keyValuePairs = new HashMap<>();
+        keyValuePairs.put("timeline", String.valueOf(timeline));


inb4 "extract constants", I am ok with one-off strings not being extracted here 👍

rock3r · 2017-05-30T10:52:54Z

core/src/main/java/com/novoda/noplayer/exoplayer/forwarder/OnCompletionForwarder.java

+
+    @Override
+    public void onPlayerError(ExoPlaybackException error) {
+        //Sent by ErrorForwarder


How is this different from the others?

Ah. This particular event is handled by the errorForwarder and should not be handled by this Forwarder. This is because it would be a bit strange to notify the no-player complete listeners of an error, when that is handled by the no-player error listeners.

This is a small problem with using the interfaces we are interested in from ExoPlayer it leads to explicit no-ops in the specific implementations.

Figured that was the case, ok :)

rock3r · 2017-05-30T13:12:21Z

core/src/main/java/com/novoda/noplayer/exoplayer/forwarder/OnPrepareForwarder.java

+
+    @Override
+    public void onPlayerError(ExoPlaybackException error) {
+        //Sent by ErrorForwarder


Same as for the onCompletionListener

Same as onCompletionListener response 👅

rock3r · 2017-05-30T13:25:03Z

core/src/main/java/com/novoda/noplayer/exoplayer/forwarder/PlayerOnErrorForwarder.java

+
+    @Override
+    public void onPlayerStateChanged(boolean playWhenReady, int playbackState) {
+        //Handled by OnPrepared and OnCompletion forwarders


Is there a reason why this is here then? Is it because the Exoplayer interface is having both methods and we just split them?

Yea, it is because ExoPlayer.EventListener is too broad a listener 😢 but we decided it was better than having an abstract class. Better to be explicit about these in the implementations then to hide it in the abstraction.

Yep agreed, I'm ok with that

rock3r · 2017-05-30T15:30:47Z

demo/src/main/java/com/novoda/demo/MainActivity.java

@@ -30,7 +30,7 @@ protected void onCreate(Bundle savedInstanceState) {
    @Override
    protected void onStart() {
        super.onStart();
-        player = new PlayerFactory(this, PrioritisedPlayers.prioritiseMediaPlayer()).create();
+        player = new PlayerFactory(this, PrioritisedPlayers.prioritiseExoPlayer()).create();


Leave a TODO maybe?

ouchadam · 2017-05-30T16:39:02Z

core/src/main/java/com/novoda/noplayer/exoplayer/ExoPlayerTwoImpl.java

+        exoPlayer.addListener(forwarder.exoPlayerEventListener());
+        exoPlayer.setVideoDebugListener(forwarder.videoRendererEventListener());
+
+        exoPlayer.setPlayWhenReady(true);


what's the reason for this change? it'll break symmetry with the media player as the we're relying on

loadVideo -> onPrepared -> play

I think we no longer need onPrepared step in exoplayer2.
But if that's the case in mediaPlayer then probably we would need to bring it back.

…that.

Ryan Feline and others added 4 commits May 26, 2017 14:06

Extract generic forwarder, that can handle the binding of player list…

2bc565d

…eners to forwarders.

Remove state in Forwarders

6eb296c

Avoid generic forwarder and prefer dedicated ones

0eca901

Extract forwarder package

fc24d8c

Mecharyry added the help wanted label May 30, 2017

Mecharyry commented May 30, 2017

View reviewed changes

rock3r self-requested a review May 30, 2017 08:56

rock3r self-assigned this May 30, 2017

Extract field for video size changed listener.

ebaec00

Mecharyry commented May 30, 2017

View reviewed changes

Remove bitrate forwarder comment, media player does not know about th…

7392427

…is currently.

Mecharyry commented May 30, 2017

View reviewed changes

Add comment back for loadVideoWithTimeout.

a1abdfc

Mecharyry mentioned this pull request May 30, 2017

Refactoring media player listeners #24

Merged

rock3r reviewed May 30, 2017

View reviewed changes

ouchadam reviewed May 30, 2017

View reviewed changes

Add TODO for prioritising players in UI.

3f7eafe

rock3r approved these changes May 31, 2017

View reviewed changes

Remove redundant setPlayWhenReady, we have the prepared listener for …

feb99c2

…that.

ouchadam approved these changes May 31, 2017

View reviewed changes

ouchadam merged commit 96e964b into exo-player-2 May 31, 2017

Mecharyry deleted the refactor-exo-player-two-facade-some-more branch June 2, 2017 13:52

Mecharyry mentioned this pull request Jun 5, 2017

Break down InfoListener #8

Closed

ouchadam mentioned this pull request Jul 5, 2017

ExoPlayer2 / Major library redesign #68

Merged


		import java.io.IOException;

		abstract class ExoPlayerEventForwarder implements ExoPlayerForwarder {


		import java.io.IOException;

		class BitrateForwarder implements AdaptiveMediaSourceEventListener {


		import java.util.HashMap;

		class EventInfoForwarder implements ExoPlayer.EventListener {


		import java.util.HashMap;

		class VideoRendererInfoForwarder implements VideoRendererEventListener {

		@@ -10,6 +10,8 @@

		private final Set<Player.PreparedListener> listeners = new CopyOnWriteArraySet<>();

		private boolean hasPrepared = false;

Exo-player two facade and forwarders #23

Exo-player two facade and forwarders #23

Conversation

Mecharyry commented May 30, 2017

Problem

Solution

Test(s) added

Screenshots

Paired with

Choose a reason for hiding this comment

Mecharyry May 30, 2017 • edited by rock3r 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mecharyry May 30, 2017 •

edited by rock3r

Loading