Skip to content
This repository has been archived by the owner on Feb 11, 2022. It is now read-only.

Release 4.5.0 #189

Merged
merged 29 commits into from
Dec 13, 2018
Merged

Release 4.5.0 #189

merged 29 commits into from
Dec 13, 2018

Conversation

Dorvaryn
Copy link
Member

This release is containing the update to exoplayer 2.9.2
Pr inlcuded: #186

This release will require users to include the following in their build.gradle as per updated README

compileOptions {
        targetCompatibility JavaVersion.VERSION_1_8
}

Ryan Feline and others added 29 commits August 30, 2018 10:13
Allow ExoPlayer implementation to be created with a different DataSource.Factory
Wrap to optional inside new instance method
Add experimental to the build script
* Update to the latest version of exo-player.

* Use media selector with fallback.

* Remove extra space

* Fix lint issues.

* Add maven central to try and unblock CI

* Move google over jcenter in dependency order

* Update mockito for lint
@pablisco pablisco self-assigned this Dec 13, 2018
Copy link
Contributor

@pablisco pablisco left a comment

Choose a reason for hiding this comment

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

Just a couple of comments/questions

google()
jcenter()
Copy link
Contributor

Choose a reason for hiding this comment

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

👌 Safer this way 😄

throw new SubtitleDecoderException(e);
}
while (!TextUtils.isEmpty(parsableWebvttData.readLine())) {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be doing something here? 🤔 I was gonna suggest a while(...); but maybe a comment instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a copy from the exo-player version, we try not to change too much to avoid issues later 👅

return new DefaultBandwidthMeter.Builder(context)
.setInitialBitrateEstimate(maxInitialBitrate)
.build();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a potential HOF 😃

}

@Override
public MediaCodecInfo getPassthroughDecoderInfo() {
public MediaCodecInfo getPassthroughDecoderInfo() throws MediaCodecUtil.DecoderQueryException {
Copy link
Contributor

Choose a reason for hiding this comment

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

How come this was fine before? Is it a RuntimeException? Should it be checked?

@@ -508,10 +513,12 @@ public void setUp() {
ExoPlayerCreator exoPlayerCreator = mock(ExoPlayerCreator.class);
given(exoPlayerForwarder.drmSessionEventListener()).willReturn(drmSessionEventListener);
given(exoPlayerForwarder.mediaSourceEventListener()).willReturn(mediaSourceEventListener);
given(trackSelectorCreator.create(eq(OPTIONS), any(DefaultBandwidthMeter.class))).willReturn(trackSelector);
given(bandwidthMeterCreator.create(anyLong())).willReturn(defaultBandwidthMeter);
given(trackSelectorCreator.create(OPTIONS, defaultBandwidthMeter)).willReturn(trackSelector);
Copy link
Contributor

Choose a reason for hiding this comment

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

nice! :) more deterministic


ArgumentCaptor<Boolean> argumentCaptor = ArgumentCaptor.forClass(Boolean.class);
verify(internalMediaCodecUtil).getDecoderInfo(eq(ANY_MIME_TYPE), argumentCaptor.capture());
verify(internalMediaCodecUtil).getDecoderInfos(eq(ANY_MIME_TYPE), argumentCaptor.capture());
Copy link
Contributor

Choose a reason for hiding this comment

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

While we are here. Do we need a captor here and in the previous test? 🤔

@Mecharyry
Copy link
Contributor

@pablisco this is just a sanity check to make sure we are not introducing bugs, see merging from develop to master. All other issues should really have been brought up before this point 😄 if you think these are high priority then we can tackle them 😄

@pablisco pablisco merged commit 5fbb008 into master Dec 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants