Exoplayer: [PR offer] Remove an index range in ConcatenatingMediaSource

[BrainCrumbz]

Issue description

We found ourselves in need of removing a contiguous set of tracks (i.e. playlist items) from a ConcatenatingMediaSource. Basically that would result in extending its public API with two methods like:

public final synchronized void removeMediaSourceRange(int fromIndex, int toIndex) { ... }

public final synchronized void removeMediaSourceRange(
  int fromIndex, int toIndex, @Nullable Runnable actionOnCompletion) { ... }

Involved indices go from fromIndex (included) to toIndex (excluded), following same convention as e.g. List<E>.subList().

We put together a working implementation and started using that: things seems to work fine. If you're interested already in details here, we:

• created a new MSG_REMOVE_RANGE message,
• created a MessageData<Integer> passing 2nd index in customData,
• implemented checks on both indices and sequence length, to avoid empty ranges and overflows,
• handled message by invoking shuffleOrder.cloneAndRemove() and removeMediaSourceInternal() within loops

Would you consider a PR to add this feature (of course with all required tests etc)?

Version of ExoPlayer being used

2.8.2

Please do not hesitate to ask for further details. Regards

[ojw28]

Sounds like something that would be useful. @tonihei - Any thoughts?

[tonihei]

Yes, definitely! PRs are always welcome. The general approach you describe sounds correct and such a method could be useful for other people as well.

[BrainCrumbz]

Great. Could you share some info on the testing side? We had no chance to look at that yet, it was an experimental implementation after all.

We noticed ConcatenatingMediaSourceTest.java is quite a long one. Have you got any suggestion on what/where to add to test such a feature?

Thanks

[tonihei]

Yes, tests would need to be added to this file. The file is already quite long as we added a lot of tests for standard and edge case scenarios. If you find that too confusing we can easily add the test ourselves.

[BrainCrumbz]

Have you got any preference on behaviour w.r.t. indices and overflows? We'd prefer a more tolerant approach, like the following:

1. when range is empty (i.e. beginning === end), no track is removed, no error is thrown, callback is invoked
2. when range is malformed (beginning > end), no track is removed, no error is thrown, callback is invoked
3. when range is partially out of bounds (beginning < 0, or end > size), no track is removed, no error is thrown, callback is invoked
   
   
   
   • one could argue here it could be preferable to remove the range part within bounds
   
   
4. when range is completely out of bounds (end < 0, or beginning > size), no track is removed, no error is thrown, callback is invoked

Here we're aiming at the following advantage:

as a caller, I'd like to ignore the burden of checks on indices on my side, and let the library DoTheRightThing™. After remove operation (if any) is done, just invoke my callback so that I can proceed with my code (e.g. do a seek, or start/stop, add more tracks, ...).

What's your take on that? Thanks

[tonihei]

That's seems slightly too tolerant in my opinion. :)
1 . That's fine.
2./3./4. These should throw an IndexOutOfBoundsException.
In the end, calls to mediaSource.removeMediaSourceRange(i, i+1) should be exactly equivalent to calls to mediaSource.removeMediaSource(i). You'll notice that the latter one throws if the index is out of bounds.

Also, as you said above, the method signature is similar to List<E>.subList(). And this method will also throw in all three cases.

It's unclear to me how you would end up in a situation where you pass in indices which are out of bounds. How do you determine which media source to remove if you don't even know how how many you have?

[BrainCrumbz]

Cool. Ok for 1. -- 4.

About your doubt, well, here's a possible usage:

The caller wants to remove tracks from 0 to N-1, leave N untouched, then remove tracks from N+1 to the last. When implementation is "very" tolerant, caller can just pass those indices, without checking whether N points to the very first or very last item in playlist. So basically without checking N's position w.r.t. to bounds.

With a "less" tolerant implementation, caller has to still perform its own checks to avoid incurring in exceptions.

[tonihei]

Thanks for the explanation. Would still prefer to have the consistency with the other APIs as explained above. The example is also relatively easy to implement:

int size = mediaSource.getSize();
if (N >= size) {
  mediaSource.clear();
} else {
  mediaSource.removeRange(0, N);
  mediaSource.removeRange(N+1, size);
}

[BrainCrumbz]

BTW, actual example from real code was from 1 to N-1, etc. (not from 0). My bad.
Anyway, I digress.

[BrainCrumbz]

Hi there. Here's the initial PR proposed changes. Please have a look at that and feel free to comment or request changes.

Not sure about:

1. message identifier constant value and ordering

2. line length due to exception messages, and your code convention on where to break long lines

3. how to extend tests

[tonihei]

Added some comments to the PR. 1. has been addressed. 2. is probably irrelevant now and 3. can be done by us.

[ojw28]

We will get this merged relatively soon. Given there's a pull request already, I don't think we need to keep this issue open as well. Thanks!