We're trying to retrieve some metadata associated to MediaSources found in a ConcatenatingMediaSource playlist.
Searching through past issues (#5131, #4707) and reference doc, we got that it is indeed possible to associate some Object as a tag when MediaSource is built, then it is possible to get the tag (from player) only for the currently playing item.
What we're looking for is to get that tag for any source item already added to a ConcatenatingMediaSource, so that we can have code similar to this:
private static void logTracklist(ConcatenatingMediaSource trackList) {
int nrOfTracks = trackList.getSize();
for (int i = 0; i < nrOfTracks; i++) {
MediaSource source = trackList.getMediaSource(i);
// previously, name has been set with Factory.setTag()
String name = (String) source.getTag(); // this API does not exist
Log.d(TAG, "#" + i + ": " + name);
}
}
This comment correctly suggested to open a new issue for this request, so here we are.
Is that something already doable? If not, would you consider a PR to add this feature? Thanks
Assigning to Toni for ConcatenatingMediaSource.
Thanks for the clarification!
I think the API you are looking for is
player.getCurrentTimeline().getWindow(itemIndex, new Window()).tag.
Note that this is directly coupled to the media known to the player and it may not immediately reflect changes made in ConcatenatingMediaSource. I recently filed #5155 for this problem (see bullet point 4 in there).
Does that help? Or does the mentioned problem prevent you from using the tag in the way you intended it to use?
Uhm, not really helping. We're looking for the tag on MediaSources that are not necessarily playing at that moment, and might have already played, or have to be played yet.
This is what we were wondering in issue #4707 :
Isn't tag Object stored somewhere within the MediaSource, right when source is created?
And thus shouldn't it be already available somehow before being added to a playlist, and before being prepare-d as a single track on a player, and before becoming the currently playing track/window/timeline/...?
We'd like to extract that tag from MediaSources in general. I mean, ideally, as it seems from the outside, when at some point we create a source with bits like:
// [...create DataSource.Factory...]
ExtractorMediaSource.Factory mediaSourceFactory = new ExtractorMediaSource.Factory(dataSourceFactory).setTag(name);
// [...create RawResourceDataSource...]
MediaSource source = mediaSourceFactory.createMediaSource(rawResourceDataSource.getUri());
one would expect that right after I can call something like source.getTag(). I just passed it, why should I wait for the timeline to be created, etc.? It seems (from the outside) that providing a getter is all that is needed. I guess there is more than that?
We're looking for the tag on MediaSources that are not necessarily playing at that moment, and might have already played, or have to be played yet.
The timeline is independent of the currently playing item. You can query any item at any time after it has been created.
And thus shouldn't it be already available somehow before being added to a playlist, and before being prepare-d as a single track on a player, and before becoming the currently playing track/window/timeline/...?
I agree that is a valid use case. The problem is, however, that the tag is undefined for some cases where a media source consists of multiple windows (e.g. the ConcatenatingMediaSource).
We could add a method which returns the tag if it's available on the source. Given that the tag is already marked as @Nullable, this may not be a problem after all. Marking as an enhancement again.
Given the mentioned issue you filed on Timeline being temporarily empty, and especially being that the tag seems a lot like something that is associated with the mediasource you attach it to, we'd say to go for your last option: adding the getter method.
By the way, client code should know if/when it is adding tags to sources it creates, and reasonably it should be able to handle cases where tag is present or not.
Actually, you might want to consider adding a tag to a ConcatenatingMediaSource as well, if you agree that the tag is firstly something related to source, before than being related to timeline.
Would you consider a PR to address this enhancement?
Would you consider a PR to address this enhancement?
Yes, gladly! If you want to give it a go.
you might want to consider adding a tag to a ConcatenatingMediaSource as well,
Maybe not. This tag wouldn't end up in the Timeline (as all the other tags do) and thus it would only be confusing to have it there.
Regarding the PR, so are we agreeing to add an Object getTag(); method to MediaSource interface? Implementation should be in BaseMediaSource. CompositeMediaSource and similar will always return null to such getter.
add an Object getTag();
@Nullable Object getTag(); in fact.
Implementation should be in BaseMediaSource.
Implementations can't be in BaseMediaSource I think because the respective tag is only available in the actual MediaSource subclass implementation.
CompositeMediaSource and similar will always return null.
That's not strictly true either, because composite media source which just forward to a single internal media source (e.g. ClippingMediaSource) can and should return the tag of the inner source. But yes, for composite source which have multiple internal sources (e.g. ConcatenatingMediaSource) we can just return null.
@Nullable Object getTag(); in fact.
Correct in code. Our bad in copy-pasting here.
Implementations can't be in BaseMediaSource I think [...]
ExtractorMediaSource, DashMediaSource, HlsMediaSource, SsMediaSource already have a private field. Here is just a matter of moving private field as protected to BaseMediaSource
SingleSampleMediaSource hasn't got that field, but has the input parameter. The protected field in BaseMediaSource seems unharmful to this class
DummyMediaSource, CompositeMediaSource don't even possess the input parameter. The protected field in BaseMediaSource seems unharmful to this class. The tag should always be null for them, at least as input parameter passed. Then the getTag implementation could differ for CompositeMediaSource [see later on]
ImaAdsMediaSource hasn't got the input parameter, and currently has two constructors. Without prior knowledge, we'd say to add an alternative signature to each of those (reaching 4 c'tors in total) in order to provide a non-null tag when needed
That's not strictly true either, because [...]
Please could you expand more on the different classes inherithing from CompositeMediaSource<>? Like when you say composite media source which just forward to a single internal media source, how many of those derived classes behaves like that? And how do other derived classes behave? At the moment we see there are:
AdsMediaSourceClippingMediaSourceConcatenatingMediaSourceLoopingMediaSourceMergingMediaSourceThanks
Yes, it's possible to have the tag in BaseMediaSource. However, if we do go down this route, the usage has to be unambiguous and impossible to get wrong. That is, the tag should be set in the constructor of BaseMediaSource and the getter must be the only access point (that is, no protected access). The tag must also be forwarded through CompositeMediaSource in this case (see below for the cases where this is needed).
More specifically:
ExtractorMediaSource, DashMediaSource, HlsMediaSource, SsMediaSource and SingleSampleMediaSource are easy as the value is already available.FakeMediaSource in the test utils needs to forward the tag of the provided timeline (if not empty).DummyMediaSource and ConcatenatingMediaSource can use null.AdsMediaSource, ImaAdsMediaSource, LoopingMediaSource and ClippingMediaSource should forward the respective innerSource.getTag() value.MergingMediaSource uses the timeline of the first source in the list as the "master" timeline. Thus, it would make sense to forward the tag of the first media source in the list as well for consistency.In the end I think we're agreeing with your first proposal, i.e. do not move field & implementation to BaseMediaSource. It seems cleaner instead to have a specific implementation (and field when needed) for each derived class. The price is some duplication of a very simple getter in some classes, but that seems a good compromise to us.
Re. FakeMediaSource, ok, but we're missing how to get the tag from its timeline field, did not actively worked with that. Could you share some directions?
Also: how can do you build and run all involved modules, not only library one? (E.g. we would have completely missed the build error due to FakeMediaSource not being updated)
How do you run all tests, too?
Doesn't "Build->Make project" in Android Studio build everything except tests?
For the tests, I know no other way except right-click on the test project and "Run tests...". Should be sufficient to check if the tests in the core module are fine.
Re. FakeMediaSource, ok, but we're missing how to get the tag from its timeline field, did not actively worked with that. Could you share some directions?
Sorry, forgot that :)
I has something like timeline.isEmpty ? null : timeline.getWindow(/* windowIndex= */ 0, newWindow()).tag in mind.
PR has been created, I guess any further feedback should go there.
Reading this thread, I'm a little confused about what use case the proposal is actually trying to solve. What's wrong with the existing API (i.e. player.getCurrentTimeline().getWindow(itemIndex, new Window()).tag)? I understand the tag is not immediately available via this API, but I can't find anything in this thread explaining a use case for which this is a problem. The logging example isn't one IMO; since you could just log a little bit later in onTimelineChanged.
Please clarify.
Very briefly: to get a "meaningful" list of data/metadata/info for each item currently found in the playlist, whether is currently playing or not, having only a reference to the ConcatenatingMediaSource around.
The description above more or less just describes what the pull request implements. What I'm after is a description of what you'd actually use the functionality for, and an explicit description of why it cannot be built on top of the existing API (not having a reference to the player probably doesn't count, since you could presumably get hold of one).
Without understanding the actual purpose, it's very hard to figure out if this is really necessary and/or whether this is the best way of doing it. Thanks!
I understand you owners are looking for more info, that's fine. What I find harder to understand is why you're asking this after having said "yes please go on with PR". We'd have much preferred if you posed all needed questions ahead of time, not after. We're all working on something I'm sure, and it's disappointing when you see time wasted. Please next time make sure an external proposal passes all your "checks" before agreeing to start work on it.
Back to the point: we'd use the functionality in order to:
ConcatenatingMediaSources), to report about playlist contents,Runnable of an addSources, and not wait for some other event later to make those available.To our understanding (from the outside, of course), all those needs seem to point to an easy "culprit": make the tag available on the MediaSource, as that's the object that is receiving/holding/being attached the tag in the first place. Actually, it seems more "natural" to ask a tag to the mediasource than to the timeline window (again, from the outside).
Thanks
What I find harder to understand is why you're asking this after having said "yes please go on with PR"
Even though we are working on the same project, we may have different opinions on matters :) It's not always possible to discuss all proposals with all team members in detail before giving advice.
And back to the topic:
I think this change goes in the right direction, even if it not fully addresses the problems in #5155 (point 4 only).
To improve the situation described in #5155, it would be necessary to return a timeline from a media source before, during, and after preparation. This timeline would include the tag (and would make this change obsolete). However, we can't do this currently because our timelines do not have a flag (yet) to indicate that the information is incomplete (in case the media is not prepared yet). It would also change the way we currently prepare media quite a bit and may cause other problems if people rely on the current behaviour. This means that it's unlikely we can fully solve all issues in #5155 right now, at least not without a more detailed design and more considerations.
Having said that, the problems caused by the delayed publishing of the media source tag can be solved right now with the PR in an fairly easy way. Especially reason 4 above is important to even allow using the tag for UI purposes in dynamic playlists. I think there are workarounds for all the issues above (like keeping a map from media source to metadata in parallel) but that's against the idea of having the tag in the first place. So all in all, I'm in favour of accepting the PR because it solves an outstanding issue in an easy way without requiring an in-depth redesign.
Complaint ;)
+1 to what Toni said. It's pretty much impossible to always get this right. This is true even when a single developer works in isolation (I think we've all started work on a change that we think is a great idea, only to stop later, think about what we're actually doing, and abandon the idea :)). Obviously it only gets worse as the project gets larger. It's our job to manage that and minimize wasted effort, and we should always try and do better. But it's not possible to eliminate it without adopting a policy of never revisiting a decision, which is a pretty bad idea.
Use case
So, to clarify, the use case for MediaSource.getTag is basically to query the playlist. Fine so far.
... so to have them aligned with our custom data structure which has the same order ...
This is the bit I don't fully understand. I've always envisaged that most applications would have their own data structure that they essentially mirror into ConcatenatingMediaSource. It sounds a like this is the case here. So what I don't understand is why an app wouldn't just query their own data structure. From a design point of view it seems preferable, because the app data structure is probably a flat playlist, meaning getTag(index) will always means something. Conversely, MediaSource composition can be used to build trees, so MediaSource.getTag doesn't always mean something sensible, which is why ConcatenatingMediaSource.getTag is having to slightly awkwardly return null in the pull request. So:
ConcatenatingMediaSource children is better or required, then I think it's fine to proceed with the change. I'm not sure pure convenience / getting a reference to the right place counts, but there may well be a more compelling reason that I just don't understand yet. If there is, I'm happy to proceed as soon as I understand!This is the bit I don't fully understand [...]
Say we have a dictionary/map with all my track metadata, accessed by track ID/name: Map<String, MyMetaData>. And in our case all means all tracks known and managed at any time by our multimedia app, they're stored locally.
Then, at some point during application lifetime, we have a player prepared with a playlist, and such playlist holds a subset of all tracks, just those currently needed in that part of application, in that scenario.
As is now, we need a custom data structure, e.g. a List<>, to hold metadata (or IDs) of those currently used tracks in the right order. And upon every playlist add/remove/move operation on this subset (we happen to do that) we have to keep our custom data structure in sync.
With our proposed change, we don't need a custom data structure to hold this "temporary" state at all, and we don't need to keep that in sync. Once we decide which tracks need to be initially in playlist for a scenario, we add them to playlist, each track with its own metadata (or ID) as tag. Then, whenever we need to retrieve metadata for one of those tracks currently in playlist, in whatever moment we need that (especially right after completing an add/move/remove), we just query the playlist. The playlist becomes the "single source of truth" about what is currently playing, been played, going to play.
Does that make a little more sense?
Final argument, I know it sounds weak as any single added API should be weighed, but really it sound strange that I can pass a tag to a media source, but then I cannot retrieve that from it. I'm not really sure there's a need for a TaggableMediaSource intermediate class in hierarchy, so to group all those sources for which it make sense to have a tag. So e.g. ConcatenatingMediaSource would not be part of that family.
We reckon that we don't really mess with source trees, but mostly with flat playlists, so there are more use cases to consider.
Does that make a little more sense?
Yes; thanks! Approximately speaking, it sounds like my assumption about the app already having a data structure that mirrors the ConcatenatingMediaSource is just not correct. Thanks for answering my annoying questions. Proceed ;).
Thanks for answering my annoying questions
Keep 'em going (at the right time 馃槤 )