This repository has been archived by the owner on Feb 11, 2022. It is now read-only.
Provide both Surface and SurfaceHolder in MediaPlayer #162
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ouchadam
reviewed
Jun 12, 2018
| mediaPlayer.setDisplay(value); | ||
| } | ||
| }; | ||
| surface.apply(setSurface, setDisplay); |
ouchadam
reviewed
Jun 12, 2018
| @@ -0,0 +1,50 @@ | |||
| package com.novoda.noplayer.model; | |||
There was a problem hiding this comment.
Are we exposing this to clients? If not the project is using the convention of an internal package to describe public classes which do not provide a guaranteed api.
There was a problem hiding this comment.
Not directly, but it's used in SurfaceRequester which is kinda exposed in the public API through PlayerSurfaceHolder(the clients need to provide an instance of this when they implement a custom implementation of PlayerView).
Mecharyry
reviewed
Jun 12, 2018
| @@ -67,7 +68,7 @@ static AndroidMediaPlayerFacade newInstance(Context context, MediaPlayerForwarde | |||
| this.mediaPlayerCreator = mediaPlayerCreator; | |||
| } | |||
|
|
|||
| void prepareVideo(Uri videoUri, Surface surface) { | |||
| public void prepareVideo(Uri videoUri, Either<Surface, SurfaceHolder> surface) { | |||
There was a problem hiding this comment.
Think this public was done in error, the class is still package.
Mecharyry
suggested changes
Jun 12, 2018
Mecharyry
approved these changes
Jun 12, 2018
ouchadam
approved these changes
Jun 13, 2018
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
While adding a support for
TextureViewI (possibly) changed the behavior ofMediaPlayerwith theSurfaceViewbecause instead of providingSurfaceHolderto MediaPlayer I started providingSurface. I don't know if that changes any behavior, so it's probably for the best to keep the changes to minimum.Solution
Add support to provide both
SurfaceandSurfaceHolderto MediaPlayer by introducing a concept ofEither. That way, theSurfaceis only provided if no-player is used withTextureViewwhich is completely separate from the previousSurfaceViewbehavior that usesSurfaceHolder.Test(s) added
updated and added where necessary
Screenshots
no UI changes
Paired with
nobody