ALL-3936/Assert support DRM #54

Mecharyry · 2017-06-26T15:55:21Z

Problem

We were crashing when a device that does not support Media DRM apis was attempting to play content. The error message was not useful to clients as it did not provide information as to why the crash occurred.

Solution

Check the device version when trying to create a Player with a DrmHandler, if it does not meet the required api level then we crash with a meaningful message.

Changed the scope of some associated classes, removing static factory methods where possible.

Test(s) added

Add tests to the DrmSessionCreatorFactory to assert the exceptions and messages.

Screenshots

No UI changes.

Paired with

@ouchadam

…rCreator.

…layerCreator.

zegnus · 2017-06-27T09:27:40Z

core/src/main/java/com/novoda/noplayer/drm/DrmSessionCreatorFactory.java

+        this.androidDeviceVersion = androidDeviceVersion;
+        this.handler = handler;
+    }
+
    public DrmSessionCreator createFor(DrmType drmType, DrmHandler drmHandler) {


It seems that we can throw unchecked exceptions, we can help the user of this method to manage expectations if we document them ;)

do you think we should add a checked exception to the PlayerBuilder.build()?

if so we can propagate those errors up into PlayerCreationExceptions

Can we recover from those exceptions or is a fatal crash?

nope, they'll be thrown if the device or available players can't handle the drm types (content types should eventually be taken into account here as well)

if we cannot recover this is a fatal exception, don't check them then

zegnus · 2017-06-27T09:32:43Z

core/src/main/java/com/novoda/noplayer/drm/StreamingDrmSessionCreator.java

@@ -19,12 +18,7 @@
    private final FrameworkMediaDrmCreator frameworkMediaDrmCreator;
    private final Handler handler;

-    static StreamingDrmSessionCreator newInstance(MediaDrmCallback mediaDrmCallback, FrameworkMediaDrmCreator frameworkMediaDrmCreator) {


zegnus · 2017-06-27T09:32:52Z

core/src/main/java/com/novoda/noplayer/drm/provision/ProvisionExecutor.java

@@ -12,13 +12,7 @@
    private final HttpPoster httpPoster;
    private final ProvisioningCapabilities capabilities;

-    public static ProvisionExecutor newInstance() {


zegnus · 2017-06-27T09:36:44Z

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

+
+public class NoPlayerExoPlayerCreator {
+
+    private final InternalCreator internalCreator;


I'm not sure we gain much doing this (apart from creating a whole class). You can just hold a reference to the handler and then use a static method just use a local create method. We then reduce indirection.

the internal classes are to enable testing

Also if you do this, you can remove the static newInstance and simply use the normal constructor

Checked the code, ok.

ExoPlayerTwoImpl and AndroidMediaPlayerImpl class can be package protected ;)

We did this in the #55 PR 😄

zegnus · 2017-06-27T09:37:07Z

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

+
+public class NoPlayerMediaPlayerCreator {
+
+    private final InternalCreator internalCreator;


Same as before

zegnus · 2017-06-27T09:38:56Z

core/src/main/java/com/novoda/noplayer/player/PlayerFactory.java

+        return new PlayerFactory(
+                context,
+                prioritizedPlayerTypes,
+                NoPlayerExoPlayerCreator.newInstance(handler),


If you remove the internal indirection for the creation then you can just use new NoPlayerE....(handler)

zegnus · 2017-06-27T09:42:57Z

Please review public facing methods and document all possible exceptions that those methods can throw, also document those exceptions in internal implementation so that developers can keep track of them :)

ouchadam · 2017-06-27T12:48:30Z

@zegnus We'll add javadoc to the PlayerBuilder in the lastest PR #40

juankysoriano · 2017-06-27T12:52:02Z

core/src/main/java/com/novoda/noplayer/drm/provision/HttpPoster.java

@@ -6,7 +6,7 @@
 import java.net.HttpURLConnection;
 import java.net.URL;

-class HttpPoster {
+public class HttpPoster {


Thy is this class now public?

I would rather keep this package and in case make a newInstance on the ProvisioningExecutor. I think that's one of the little benefits, on my opinion, of the newInstance and therefore I don't see justification from removing it from ProvisioningExecutor

~~these classes were being used in the client project, we'll replace this class with an interface and make this impl package again~~

We inlined the logic into the client and instead hide all of this logic

juankysoriano · 2017-06-27T13:01:02Z

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

+
+    public Player createExoPlayer(Context context, DrmSessionCreator drmSessionCreator, boolean downgradeSecureDecoder) {
+        ExoPlayerTwoImpl player = internalCreator.create(context, drmSessionCreator, downgradeSecureDecoder);
+        player.initialise();


juankysoriano · 2017-06-27T13:02:52Z

core/src/main/java/com/novoda/noplayer/player/PlayerFactory.java

@@ -58,10 +67,10 @@ public Player create(DrmType drmType, DrmHandler drmHandler, boolean downgradeSe
    private Player createPlayerForType(PlayerType playerType, DrmType drmType, DrmHandler drmHandler, boolean downgradeSecureDecoder) {
        switch (playerType) {
            case MEDIA_PLAYER:
-                return mediaPlayerCreator.createMediaPlayer(context);
+                return noPlayerMediaPlayerCreator.createMediaPlayer(context);


An alternative could have been not having the InternalCreator in the creators (therefore you don't need testing) and doing the test for the player.initialise() here (which I believe is the bit you wanted to test)

this would mean making all the impls public as they live inside their own player specific packages

Well, we can still have the InternalCreator it that benefits us for reducing the scope of those classes, but I still believe we can benefit from doing player.initialise() here instead of the two creators.

Not a blocker, just a thought

I mentioned this the no-player room but I can say again 👻

It comes down to which layer should provide a fully instantiated player, should it be the concrete player packages (media/exo) which allows all the classes to remain package local or the noplayer root and then expose implementation details about the separate players

ahhhhhhhhhhhhhhhhh

I undertand now, thanks. Yes let's keep this like it is

juankysoriano · 2017-06-27T13:04:26Z

core/src/test/java/com/novoda/noplayer/mediaplayer/NoPlayerMediaPlayerCreatorTest.java

+import static org.mockito.BDDMockito.given;
+import static org.mockito.Mockito.verify;
+
+public class NoPlayerMediaPlayerCreatorTest {


read my comment on NoPlayerFactory, we could take rid of two test classes.

scratch this

Ryan Feline added 7 commits June 26, 2017 10:34

Add specialised exception for non-target device.

6e9623c

Collapse static new instance for testing.

5f2192f

Add tests for DrmSessionCreatorFactory.

2873c73

Inlined ExoPlayerImplFactory to be an inner class inside the ExoPlaye…

e88977d

…rCreator.

Inlined MediaPlayerImplFactory to be an inner class inside the MediaP…

64acc6b

…layerCreator.

Create static factory for PlayerFactory to pull out Handler.

4e68a31

Merge branch 'exo-player-2' into ALL-3936/ensure_target_api

398cae1

Mecharyry added the enhancement label Jun 26, 2017

zegnus self-assigned this Jun 27, 2017

zegnus reviewed Jun 27, 2017

View reviewed changes

juankysoriano self-assigned this Jun 27, 2017

zegnus approved these changes Jun 27, 2017

View reviewed changes

juankysoriano reviewed Jun 27, 2017

View reviewed changes

juankysoriano approved these changes Jun 27, 2017

View reviewed changes

juankysoriano merged commit 5a50fe1 into exo-player-2 Jun 27, 2017

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-3936/Assert support DRM #54

ALL-3936/Assert support DRM #54

Mecharyry commented Jun 26, 2017

zegnus Jun 27, 2017

ouchadam Jun 27, 2017

zegnus Jun 27, 2017

ouchadam Jun 27, 2017

zegnus Jun 27, 2017

zegnus Jun 27, 2017

zegnus Jun 27, 2017

zegnus Jun 27, 2017

ouchadam Jun 27, 2017 •

edited

Loading

zegnus Jun 27, 2017

zegnus Jun 27, 2017

Mecharyry Jun 27, 2017

zegnus Jun 27, 2017

zegnus Jun 27, 2017

zegnus Jun 27, 2017

zegnus commented Jun 27, 2017

ouchadam commented Jun 27, 2017

juankysoriano Jun 27, 2017

juankysoriano Jun 27, 2017

ouchadam Jun 27, 2017 •

edited

Loading

juankysoriano Jun 27, 2017

juankysoriano Jun 27, 2017

ouchadam Jun 27, 2017

juankysoriano Jun 27, 2017

ouchadam Jun 27, 2017

juankysoriano Jun 27, 2017

juankysoriano Jun 27, 2017

juankysoriano Jun 27, 2017


		public class NoPlayerExoPlayerCreator {

		private final InternalCreator internalCreator;


		public class NoPlayerMediaPlayerCreator {

		private final InternalCreator internalCreator;

ALL-3936/Assert support DRM #54

ALL-3936/Assert support DRM #54

Conversation

Mecharyry commented Jun 26, 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

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

ouchadam Jun 27, 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

zegnus commented Jun 27, 2017

ouchadam commented Jun 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouchadam Jun 27, 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

ouchadam Jun 27, 2017 •

edited

Loading

ouchadam Jun 27, 2017 •

edited

Loading