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

NO-149/SurfaceRequester #150

Merged
merged 14 commits into from
May 9, 2018
Merged

NO-149/SurfaceRequester #150

merged 14 commits into from
May 9, 2018

Conversation

Mecharyry
Copy link
Contributor

@Mecharyry Mecharyry commented May 8, 2018

Problem

The current implementation of NoPlayer relies upon a SurfaceHolderRequester to pass a SurfaceHolder to the underlying player. Clients may want to use SurfaceView, TextureView etc and the underlying player, ExoPlayer specifically only cares about the Surface.

Solution

Switch out the SurfaceHolderRequester with a SurfaceRequester passing a Surface to the underlying player.

Paired with

Nobody.

import java.util.Deque;
import java.util.LinkedList;
import java.util.concurrent.TimeUnit;

Copy link
Contributor

Choose a reason for hiding this comment

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

should this class be in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I would consider to remove it @Mecharyry, just to avoid to ship it in what is effectively a non-related change set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@mr-archano mr-archano left a comment

Choose a reason for hiding this comment

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

minor remarks only 💯


public interface SurfaceRequester {

void requestSurfaceTexture(Callback callback);
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to use texture, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -5,7 +5,7 @@
android:layout_height="wrap_content">

<SurfaceView
android:id="@+id/surface_view"
android:id="@+id/texture_view"
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we keep this as it was?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Override
public void onClick(View v) {
dialogCreator.showAudioSelectionDialog();
if (player == null || !player.isPlaying() || player.getAudioTracks().size() == 0) {
Toast.makeText(MainActivity.this, "no audio tracks available!", Toast.LENGTH_LONG).show();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The buttons to trigger the dialog are always present, if you tap them before the player is playing then it will throw an IllegalStateException.

}
};

private final View.OnClickListener showSubtitleSelectionDialog = new View.OnClickListener() {
@Override
public void onClick(View v) {
if (player.getSubtitleTracks().isEmpty()) {
if (player == null || !player.isPlaying() || player.getSubtitleTracks().isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here (I know that this is a demo, I am just curious)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above 😄

@@ -51,7 +51,7 @@ public View getContainerView() {
}

@Override
public SurfaceHolderRequester getSurfaceHolderRequester() {
public SurfaceRequester getSurfaceTextureRequester() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to leak texture here? SurfaceRequester should be enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I missed one apparently 🔍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ouchadam ouchadam merged commit fdcbbe5 into NO-149/surface_feature May 9, 2018
@ouchadam ouchadam deleted the NO-149/surface branch May 9, 2018 10:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants