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

Detect loop/repeat but not media source change with ConcatenatingMediaSource #4898

Closed
bigahega opened this issue Oct 2, 2018 · 16 comments
Closed
Assignees
Labels

Comments

@bigahega
Copy link

bigahega commented Oct 2, 2018

Hello,

I am using multiple media sources to feed my ConcatenatingMediaSource and this media source is in constant repeat mode. As far as I have searched, using onPositionDiscontinuity you can detect repeats with it and also you can detect the media source switches of ConcatenatingMediaSource with that as well. However the argument reason in that method is 0 for both cases. So am I missing something or is there another way to get a callback when the ConcatenatingMediaSource repeats?

Thanks!

@tonihei tonihei self-assigned this Oct 2, 2018
@tonihei
Copy link
Collaborator

tonihei commented Oct 2, 2018

The easiest way to distinguish between these two cases is to check if player.getCurrentWindowIndex() changed. If it didn't change, it's a repetition. If it changed, it moved to the next playlist item.

Note that this assumes all your playlist items have just one period. That should be true unless you are using multi-period DASH streams. In this case, you also need to check if the current period is the first in the window to know whether the item is repeating.

@BrainCrumbz
Copy link
Contributor

BrainCrumbz commented Oct 4, 2018

Just to drop our 2¢, we're working with local files being added to a ConcatenatingMediaSource: some of them are non-looping MediaSource, while some are LoopingMediaSource.

In order to detect track transitions within playlist, we register a Player.EventListener and react to onTracksChanged event. At that time we also read player.getCurrentWindowIndex() (as @tonihei mentioned), which by then returns the playlist item index currently playing.

Everytime currently playing track changes to a different one, or starts again in case of a loop, that method gets invoked by ExoPlayer. Reported index will be the same as the last invocation in case of a loop, otherwise will be a different one in case of a new playlist item.

HTH

@tonihei
Copy link
Collaborator

tonihei commented Oct 4, 2018

@BrainCrumbz You really should be using onPositionDiscontinuity for that case. onTracksChanged is called when the selected tracks (i.e. which video or audio tracks are rendered) change. This often happens at period transitions (= position discontinuity), but may also happen if you disable/enable/select tracks in between.

@BrainCrumbz
Copy link
Contributor

BrainCrumbz commented Oct 4, 2018

Thanks for pointing that out. Will have to double check this. At the moment we need to be notified also when playing track changes due to a seek operation, and with this method we can collect both scenarios in the same call. Don't recall whether onPositionDiscontinuity gets invoked in case of a seek as well (I think so), and with which reason.

@bigahega
Copy link
Author

bigahega commented Oct 4, 2018

@tonihei thank you for the advice, now I can track the window indices on discontinuity and detect if we came to the initial window using that.

@tonihei
Copy link
Collaborator

tonihei commented Oct 4, 2018

Glad to be of help :)

@tonihei tonihei closed this as completed Oct 4, 2018
@BrainCrumbz
Copy link
Contributor

BrainCrumbz commented Oct 4, 2018

Yeah, so after repeating some tests we got back some more memories and thoughts.

When playing flows naturally from a playlist item to the next, the two events are basically the same for us. It's enough to detect the DISCONTINUITY_REASON_PERIOD_TRANSITION reason:

12:48:39.003 [...]: onPositionDiscontinuity: index: 0, reason: DISCONTINUITY_REASON_PERIOD_TRANSITION
12:48:39.003 [...]: onTracksChanged: index: 0, trackGroups: 2, trackSelections: 4

Things are different in other scenarios though. When users skips to another playlist item:

12:48:42.702 [...]: onPositionDiscontinuity: index: 1, reason: DISCONTINUITY_REASON_SEEK
12:48:42.785 [...]: onPositionDiscontinuity: index: 1, reason: DISCONTINUITY_REASON_SEEK_ADJUSTMENT
12:48:42.785 [...]: onSeekProcessed
12:48:42.805 [...]: onTracksChanged: index: 1, trackGroups: 1, trackSelections: 4

We could detect onPositionDiscontinuity with DISCONTINUITY_REASON_SEEK, although that arrives some 100ms earlier than the actual track change.
We could detect DISCONTINUITY_REASON_SEEK_ADJUSTMENT, which arrives later, close to the track change. But if I recall correctly that is not guaranteed to always happen after a seek.
Or we could detect another event altogether, onSeekProcessed, which arrives at the same time as the adjustment.

Another scenario with a similar situation is when playing actually starts, after the very first playlist items have been added and we seek to 0 & play to actually start:

12:48:12.899 [...]: onPositionDiscontinuity: index: 0, reason: DISCONTINUITY_REASON_SEEK
12:48:12.966 [...]: onSeekProcessed
12:48:13.220 [...]: onTracksChanged: index: 0, trackGroups: 2, trackSelections: 4

In this case there's even a longer interval between the two events.

@tonihei
Copy link
Collaborator

tonihei commented Oct 4, 2018

That's working as intended. The DISCONTINUITY_REASON_SEEK is sent immediately after calling seek. The seekProcessed callback indicates that the internal player acknowledged the seek and all necessary state changes have been made. That's why you see that a little bit later.

DISCONTINUITY_REASON_SEEK_ADJUSTMENT is only used if the player needed to adjust the requested seek position. I guess that's irrelevant for most people.

In any case, I would argue that onPositionDiscontinuity is enough to distinguish between all the cases:

private int previousWindowIndex;

void onPositionDiscontinuity(int reason) {
  handleDiscontinuity(reason);
}

void onTimelineChanged(...) {
  handleDiscontinuity(DISCONTINUITY_REASON_INTERNAL);
}

private void handleDiscontinuity(int reason) {
  if (reason == DISCONTINUITY_REASON_PERIOD_TRANSITION) {
    if (previousWindowIndex == player.getCurrentWindowIndex() {
      // Repeating the same playlist item automatically.
    } else {
      // Moved to next playlist item automatically.
    }
  } else if (reason == DISCONTINUITY_REASON_SEEK) {
    if (previousWindowIndex == player.getCurrentWindowIndex() {
      // Seek within current playlist item.
    } else {
      // Seek to another playlist item (e.g. skipping to next or previous)
    }
  } else if (reason == DISCONTINUITY_REASON_AD_INSERTION) {
    // Transitioned from content to ad or from ad to content within current playlist item.
  } else {
    // Position jumped discontinuously within current playlist item. For example because
    // seek got adjusted or for other internal reason.
  }
  previousWindowIndex = player.getCurrentWindowIndex();
}

Note that onTimelineChanged may include a discontinuity (e.g. when dynamically updating the playlist), so I moved the handling into a separate method called by both.

@BrainCrumbz
Copy link
Contributor

Thanks for your example.
Just to stress that for us it's more a matter of timing, a question:

ignoring loops for the moment, upon a DISCONTINUITY_REASON_SEEK discontinuity, can we already consider the formerly playing track as "obsolete", i.e. not playing anymore? And hence can we remove it from playlist without any error, visible glitch or other issue?

Looking at reported window index upon the event, that is already up-to-date so maybe that's the case. But on the other hand, the actual seek is completed only upon seekProcessed. So maybe if we start to mangle with playlist before seek has settled, we might mess up player internal state.

@tonihei
Copy link
Collaborator

tonihei commented Oct 4, 2018

Yes, you can consider the previous item as obsolete at this point. Any playlist updates will be queued on the internal playback thread too and thus will be handled after the seek.

@BrainCrumbz
Copy link
Contributor

Sorry to drag this further, but by any chance has onTracksChanged event behaviour changed between 2.8.2 and 2.9.0?
We're trying to upgrade to 2.9.0 and upon a seek from current track (e.g. index 0) to another one in playlist (e.g. index 2) we're seeing that event invoked twice, while in previous version it was once.

@tonihei
Copy link
Collaborator

tonihei commented Oct 15, 2018

@BrainCrumbz
Yes, it did change with this commit. The reason is that for a short period of time no tracks are selected at all because we seeked away from the current item and don't have enough about the new item yet to make an actual track selection. Eventually, this ensures that player.getCurrentTrackGroups and player.getCurrentTrackSelections always tells you something about the current item as returned by player.getCurrentWindowIndex. That was previously not the case for this kind of transition.

@BrainCrumbz
Copy link
Contributor

Thanks for the real quick response. Well, it looks like we'll have to change our track change detection logic, move away from onTracksChanged and rely instead on onPositionDiscontinuity! Let's hope timing will not mess up things 😮 😉

@tonihei
Copy link
Collaborator

tonihei commented Oct 15, 2018

For what it's worth, I think the time you get by listening to onPositionDiscontinuity is a more accurate depiction of the transition (as already pointed out above) because it's the point where you logically switch to the new playlist item and all time spent waiting until the new item is ready should be considered part of the buffering for the new item. If you think that's not true for your use case, please let us know why!

@ojw28
Copy link
Contributor

ojw28 commented Nov 23, 2020

@tonihei - Does this still need additional documentation, now that we have EventListener.onMediaItemTransition. It looks like MediaItemTransitionReason disambiguates between the two cases without the need for index tracking. If no longer required, please remove the documentation candidate label. Thanks!

@tonihei
Copy link
Collaborator

tonihei commented Nov 24, 2020

Yes, MediaItemTransitionReason solves this problem without additional documentation.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants