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 MediaSource instances to be reused #3498

Closed
stoleruedgar opened this issue Nov 24, 2017 · 11 comments
Closed

Allow MediaSource instances to be reused #3498

stoleruedgar opened this issue Nov 24, 2017 · 11 comments
Assignees

Comments

@stoleruedgar
Copy link

Issue description

Passing the same DynamicConcatenatingMediaSource to a new SimpleExoPlayer instance doubles the internal mediaSourceHolders which in turn breaks the Player behaviour

Reproduction steps

public class TestActivity extends Activity {

	private DynamicConcatenatingMediaSource playlist = null
	private SimpleExoPlayer player = null

	protected onCreate(Bundle savedInstance) {
		super.onCreate(savedInstance)
		playlist = ....init with mediasources       
	}	

	protected onStart() {
		super.onStart()      
		if (player == null) {
			player = ...init player
			if (playlist != null) {
				player.prepare(playlist, false, true)
			}
		} 
	}

	protected onStop() {
		super.onStop()
		player = ...release player
	}	
}

Put the activity in background and then bring the app in the foreground.

Version of ExoPlayer being used

r2.5.4

Device(s) and version(s) of Android being used

Samsung Galaxy S7, Android 7.0

@tonihei
Copy link
Collaborator

tonihei commented Nov 24, 2017

Media sources shouldn't be reused (documented here). This includes the dynamic concatenating media source as well as the single playlist item media sources.

@tonihei tonihei closed this as completed Nov 24, 2017
@ojw28
Copy link
Contributor

ojw28 commented Nov 27, 2017

Could/should we add some assertions that check this and give a developer friendly error message? Seems like an easy mistake to make.

@tonihei
Copy link
Collaborator

tonihei commented Nov 27, 2017

I think we can also make DynamicConcatenatingMediaSource reusable (despite the general guideline). This would deliberately allow reusing the wrapping dynamic media source. Marking this as enhancement to do this.

@tonihei tonihei reopened this Nov 27, 2017
@tonihei
Copy link
Collaborator

tonihei commented Nov 27, 2017

To clarify: this enhancement tracks reusing media sources in general. Reusing just the DynamicConcatenatingMediaSource wouldn't make sense if the contained media sources can't be reused.

@ojw28 ojw28 changed the title DynamicConcatenatingMediaSource keeps mediaSourceHolders after releaseSource Allow MediaSource instances to be reused Nov 27, 2017
ojw28 pushed a commit that referenced this issue Feb 1, 2018
GitHub:#3498

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=183253017
ojw28 pushed a commit that referenced this issue Feb 1, 2018
GitHub:#3498

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=183369041
ojw28 pushed a commit that referenced this issue Feb 1, 2018
GitHub:#3498

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=183373647
ojw28 pushed a commit that referenced this issue Feb 1, 2018
GitHub:#3498

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=183378776
ojw28 pushed a commit that referenced this issue Feb 1, 2018
…ources.

This removes some boiler-plate code for compostite sources and will also
simplify resuing media source in the future (because this class can keep track
of child listeners).

Issue:#3498

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=183387123
ojw28 pushed a commit that referenced this issue Feb 1, 2018
GitHub:#3498

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=183391117
ojw28 pushed a commit that referenced this issue Feb 1, 2018
GitHub:#3498

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=183392095
ojw28 pushed a commit that referenced this issue Feb 1, 2018
Issue:#3498

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=183648419
ojw28 pushed a commit that referenced this issue Feb 1, 2018
Also fixes a bug where deferred media periods were kept in the list for
an unprepared media source although the media period was already released.

Issue:#3498

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=183800889
ojw28 pushed a commit that referenced this issue Feb 1, 2018
This make sure all media sources can be reprepared after being released.

Issue:#3498

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=183990416
@eneim
Copy link
Contributor

eneim commented Feb 9, 2018

@tonihei @ojw28 it is great to see that MediaSource can be reused. I still not see javadoc update for it in dev-v2. Do you plan it for 2.7.0 or it must be in the release after?

I have a couple question about this:

  • What I need to do to reset the MediaSource so that it can be reuse right after. (with the latest update in dev-v2)
  • Is ExoPlayer#prepare(MediaSource) an expensive operation? Right now I'm trying to reuse MediaSource (2.6.1) by cloning current MediaSource and prepare it again. It sounds weird, but have no other way to do ...

Thanks.

@tonihei
Copy link
Collaborator

tonihei commented Feb 9, 2018

The current updates mentioned above are only one part of making MediaSource reusable. You should now be able to re-prepare with the same media source. However, we are still working on the possibility to reuse media source in parallel, e.g. adding them twice to a ConcatenatingMediaSource. That's also why we haven't updated the docs yet because we only do this after the feature is fully implemented and ready to use. It will most likely be part of 2.7.0.

For the second question: prepare is not in itself an expensive operation but it might trigger other operations. For example, manifest files for DASH, SmoothStreaming or HLS are downloaded after calling prepare.

andrewlewis pushed a commit that referenced this issue Mar 2, 2018
Up to now we use a boolean "preventListenerNotification" to suppress updates
while other operations are still in progress. This ensures, for example, that
only one initial timeline is issued even for multiple child sources.

As soon as we allow to reuse the same instance, it becomes increasingly difficult
to manage this manual listener notification suppression. Therefore, this change
schedules an update as a new message on the playback thread immediately after the
current message. This way, we also ensure that all simultaneous updates within one
looper message iteration are reported together.

Issue:#3498

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=187180342
andrewlewis pushed a commit that referenced this issue Mar 2, 2018
This is achieved by adding a BaseMediaSource which keeps a reference count of the
number of times the source has been prepared and forwards to the actual implementations
only once, such that only minimal changes are needed for each media source.

Issue:#3498

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=187186691
ojw28 pushed a commit that referenced this issue Mar 7, 2018
Especially the cast demo app benefits from this feature as it can keep its
ConcatenatingMediaSource all the time without having to repopulate it
when switching players.

Issue:#3498

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=187866048
@tonihei
Copy link
Collaborator

tonihei commented Mar 12, 2018

Closing this issue as the feature is now available in the dev-branch.

@tonihei tonihei closed this as completed Mar 12, 2018
@prat-z
Copy link

prat-z commented Apr 11, 2018

@tonihei @ojw28 Will the reuse of MediaSource enable a MediaSource object to be shared between more than one players, like one player in a list and one full screen, playing the same media ?
And, when will this change be available on the release branch?

@tonihei
Copy link
Collaborator

tonihei commented Apr 11, 2018

@prat-z Sorry, this will be the only restriction. Using the same media source simultaneously from multiple players would require us to synchronize the access to the source which we currently avoid by using it from one player at a time only. However, creating a media source isn't that cost-intensive and this is really mostly a convenience feature. So you wouldn't save much by avoiding to create a new media source for your second player anyway.

This feature is already in the dev-branch if you want to try it out. And it's going to be part of release 2.8.

@prat-z
Copy link

prat-z commented Apr 12, 2018

@tonihei The problem with creating a new media source is that I am using HLSMediaSource, and creating a new data source means loosing out on the downloaded bytes of the same video, and a buffering state while switching to and from full screen mode.

@tonihei
Copy link
Collaborator

tonihei commented Apr 12, 2018

@prat-z Reusing media source wouldn't solve that I'm afraid. To keep the buffered video data when switching to and from full screen mode, you need to make sure to keep the same player instance. Please have a look around our issue tracker for fullscreen related issues. There are also various suggestions on the web how to implement fullscreen switching with ExoPlayer. Because of the different behavior on different Android versions, there is unfortunately no single, simple way to do this.

If you have more question regarding full-screen switching, please open a new issue.

@google google locked and limited conversation to collaborators Aug 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants