Skip to content
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

Allow video url redirects #66

Merged
merged 6 commits into from
Mar 15, 2017
Merged

Allow video url redirects #66

merged 6 commits into from
Mar 15, 2017

Conversation

luoser
Copy link
Contributor

@luoser luoser commented Mar 15, 2017

what

Video playback has been broken on our HQ environments with a 302 redirect error since our HQ video urls redirect.

Luckily the fix is nice: ExoPlayer has multiple constructors for DefaultUriDataSource (which we use in our KSRendererBuilder) one of which allows us to set a boolean for allowCrossProtocolRedirects. You go ExoPlayer.

Bonus modernization of our #weird VideoViewModel and refactoring of our ProjectViewModel and tests.

@luoser luoser changed the title Allow hq video url redirects Allow video url redirects Mar 15, 2017
Copy link
Contributor

@mbrandonw mbrandonw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me!

@@ -44,7 +43,7 @@ public void onCreate(final @Nullable Bundle savedInstanceState) {
setContentView(R.layout.video_player_layout);
ButterKnife.bind(this);

viewModel.outputs.video()
viewModel.outputs.preparePlayerWithUrl()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

muuuuch better name

this.startViewPledgeActivity = currentProject.compose(takeWhen(this.viewPledgeButtonClicked));

this.shareButtonClicked
.compose(bindToLifecycle())
.subscribe(__ -> this.koala.trackShowProjectShareSheet());

this.playVideo
this.startVideoActivity
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

muuuuuch better name


interface Outputs {
/** Emits the video for the player. */
Observable<String> preparePlayerWithUrl();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looooovin these new modern vm's

@@ -51,7 +51,7 @@ public KSRendererBuilder(final @NonNull Context context, final @NonNull String v
@Override
public void buildRenderers(final @NonNull KSVideoPlayer player) {
final Allocator allocator = new DefaultAllocator(BUFFER_SEGMENT_SIZE);
final DataSource dataSource = new DefaultUriDataSource(context, videoLink);
final DataSource dataSource = new DefaultUriDataSource(context, null, videoLink, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this is the flag that allows redirects?

def makes you miss swift's named arguments when reading this code......

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep

@luoser luoser merged commit 5b22951 into master Mar 15, 2017
@luoser luoser deleted the internal-video branch March 15, 2017 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants