ALL-3936/Player Builder #57

Mecharyry · 2017-06-27T10:12:40Z

Problem

As per issue #53 and the conversation that took place in https://github.com/novoda/no-player/pull/52/files/4748c1dd02defdb565cd7174a38deb90ee214d9d#diff-d137d202ec4e30de3f26dfb2c0131896 we want to replace the PlayerFactory with a PlayerBuilder as the player has a number of customizations that can be exposed to our clients.

Solution

Create a PlayerBuilder and a PlayerCreator. We have exposed a withDowngradedSecureDecoder to hide the boolean we had before. This boolean is passed down to the ExoPlayer level at which point it is transformed into the necessary MediaCodecSelector. The MediaCodecSelector is not directly exposed in the Builder because it is only necessary for ExoPlayer.

Test(s) added

Updating tests that we broke as a result of these changes.

Screenshots

No UI changes.

Paired with

@ouchadam

Mecharyry · 2017-06-27T10:13:33Z

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

@@ -51,6 +52,8 @@ ExoPlayerTwoImpl create(Context context, DrmSessionCreator drmSessionCreator, bo

            DefaultTrackSelector trackSelector = new DefaultTrackSelector();

+            MediaCodecSelector mediaCodecSelector = downgradeSecureDecoder ? SecurityDowngradingCodecSelector.newInstance() : MediaCodecSelector.DEFAULT;


boolean downgradeSecureDecoder is transformed into the associated MediaCodecSelector.

@joetimmins the boolean now lives on PlayerBuilder -> NoPlayerCreator and finally here where it gets converted to the codec selector

Mecharyry · 2017-06-27T10:14:12Z

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

+import com.novoda.noplayer.exoplayer.drm.DrmSessionCreatorFactory;
+import com.novoda.noplayer.mediaplayer.NoPlayerMediaPlayerCreator;
+
+class NoPlayerCreator {


This is the old PlayerFactory minus a few wrapper methods, we only expose the one method create and it is used only in the PlayerBuilder.

Mecharyry · 2017-06-27T10:14:26Z

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

+import java.util.Arrays;
+import java.util.List;
+
+public class PlayerBuilder {


Builder party 🎉

juankysoriano · 2017-06-27T15:26:38Z

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

+        return this;
+    }
+
+    public Player build(Context context) {


as a user of the library I can still use NoPlayerExoPlayerCreator directly and create it without the builder.

Is this intentional?

I don't see how to avoid this without flattening all the packages 😢

😢

I have taken a look and I don't see either

Can NoPlayerCreator be properly used by the client? if it doesn't make sense for the client to do so, we might have to flatten the packages... java, libraries and packages are not friends, it really sucks :S

NoPlayerCreator is not public, it's the individual player creators which are, like NoPlayerExoPlayerCreator

I don't think it's problem or enough of a reason to flatten the entire library.

There's no issue or side effects of a client creating a single player directly via the public player creators, if anything it's an architectural feature. The PlayerBuilder is syntactic sugar around selecting a player for given types, which isn't needed if a client decides to manually use a specific creator

It's the NoPlayerMediaPlayerCreator and NoPlayerExoPlayerCreator that are exposed. They are the sole entry points into the exo-player and media-player packages. Flattening would solve this problem but make maintaining no-player extremely painful.

Agreed offline, we'll expose these creators as part of the public API

juankysoriano · 2017-06-27T15:31:42Z

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

        this.internalMediaCodecUtil = internalMediaCodecUtil;
-        this.downgradeSecureDecoder = downgradeSecureDecoder;
    }

    @Override
    public MediaCodecInfo getDecoderInfo(String mimeType, boolean contentRequiresSecureDecoder)


is contentRequiresSecureDecoder not used anymore? if so... kill it? (if we can, which I don't know)

it's from ExoPlayer MediaCodecSelector, we can't remove it, unless you mean rename it?

I didn't notice it was from the exoplayer one, ok then 👍

juankysoriano · 2017-06-27T15:32:13Z

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

+        return this;
+    }
+
+    public PlayerBuilder withDowngradedSecureDecoder() {


juankysoriano · 2017-06-27T15:33:24Z

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

        DataPostingModularDrm drmHandler = new DataPostingModularDrm(EXAMPLE_MODULAR_LICENSE_SERVER_PROXY);

-        player = playerFactory.create(DrmType.WIDEVINE_MODULAR_STREAM, drmHandler);
+        player = new PlayerBuilder()


To be honest I feel the builder is a super cool idea! 💯

you can thank @Mecharyry trolling me and @zegnus 😂

juankysoriano

LGTM, I like the builder

I don't like the NoPlayer creators for media player or exo player being public, but I can't see how to make it better

zegnus · 2017-06-28T09:19:43Z

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

    }

    @Override
    public MediaCodecInfo getDecoderInfo(String mimeType, boolean contentRequiresSecureDecoder)
            throws MediaCodecUtil.DecoderQueryException {
-        return internalMediaCodecUtil.getDecoderInfo(mimeType, contentRequiresSecureDecoder && !downgradeSecureDecoder);
+        return internalMediaCodecUtil.getDecoderInfo(mimeType, false);


can you extract that boolean? I have no idea what's false there :P

zegnus · 2017-06-28T09:24:30Z

💯 niiice!

Ryan Feline added 5 commits June 27, 2017 10:32

Pull out UnableToCreatePlayerException.

604c7b7

Replace PlayerFactory with PlayerBuilder.

6a0f17d

Rename Factory to creator.

678e07d

Remove deprecation from withDrm.

48a6e98

Pass down media codec selector instead of a boolean.

2c87f0d

Mecharyry added ❌ do-not-merge enhancement labels Jun 27, 2017

Mecharyry commented Jun 27, 2017

View reviewed changes

Merge branch 'ALL-3936/cues' into ALL-3936/builder

2b09917

juankysoriano self-requested a review June 27, 2017 15:21

juankysoriano self-assigned this Jun 27, 2017

juankysoriano reviewed Jun 27, 2017

View reviewed changes

juankysoriano approved these changes Jun 27, 2017

View reviewed changes

Mecharyry changed the base branch from ALL-3936/cues to exo-player-2 June 27, 2017 16:02

Mecharyry removed the ❌ do-not-merge label Jun 27, 2017

zegnus self-assigned this Jun 28, 2017

zegnus reviewed Jun 28, 2017

View reviewed changes

Extract constant for secure decoder flag.

5688de5

zegnus approved these changes Jun 28, 2017

View reviewed changes

zegnus merged commit f422d3e into exo-player-2 Jun 28, 2017

zegnus deleted the ALL-3936/builder branch June 28, 2017 10:36

This was referenced Jun 28, 2017

PlayerFactory use a Builder #53

Closed

Provide client-side rules for MediaCodecSelectorFactory #11

Closed

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/Player Builder #57

ALL-3936/Player Builder #57

Mecharyry commented Jun 27, 2017

Mecharyry Jun 27, 2017

ouchadam Jun 27, 2017

Mecharyry Jun 27, 2017

Mecharyry Jun 27, 2017

juankysoriano Jun 27, 2017

ouchadam Jun 27, 2017

juankysoriano Jun 27, 2017

zegnus Jun 28, 2017

ouchadam Jun 28, 2017 •

edited

Mecharyry Jun 28, 2017

zegnus Jun 28, 2017

juankysoriano Jun 27, 2017 •

edited

ouchadam Jun 27, 2017

juankysoriano Jun 27, 2017

juankysoriano Jun 27, 2017

juankysoriano Jun 27, 2017 •

edited

ouchadam Jun 27, 2017

juankysoriano left a comment •

edited

zegnus Jun 28, 2017

zegnus commented Jun 28, 2017

		@@ -51,6 +52,8 @@ ExoPlayerTwoImpl create(Context context, DrmSessionCreator drmSessionCreator, bo

		DefaultTrackSelector trackSelector = new DefaultTrackSelector();

		MediaCodecSelector mediaCodecSelector = downgradeSecureDecoder ? SecurityDowngradingCodecSelector.newInstance() : MediaCodecSelector.DEFAULT;

ALL-3936/Player Builder #57

ALL-3936/Player Builder #57

Conversation

Mecharyry commented Jun 27, 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 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juankysoriano Jun 27, 2017 • edited

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

juankysoriano Jun 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juankysoriano left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zegnus commented Jun 28, 2017

ouchadam Jun 28, 2017 •

edited

juankysoriano Jun 27, 2017 •

edited

juankysoriano Jun 27, 2017 •

edited

juankysoriano left a comment •

edited