feat(playlist): remove MediaSource range #4564

BrainCrumbz · 2018-07-24T10:47:13Z

See #4542 for related discussion

googlebot · 2018-07-24T10:47:22Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

BrainCrumbz · 2018-07-24T10:58:04Z

CLA added for all commit authors

googlebot · 2018-07-24T10:58:08Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

GiuseppePiscopo · 2018-07-24T11:02:27Z

I confirm I'm ok with that.

tonihei

Looks good in general. Just a couple of formatting issues and minor changes.

tonihei · 2018-07-25T13:19:22Z

library/core/src/main/java/com/google/android/exoplayer2/source/ConcatenatingMediaSource.java

@@ -50,6 +50,7 @@
 private static final int MSG_ADD = 0;
 private static final int MSG_ADD_MULTIPLE = 1;
 private static final int MSG_REMOVE = 2;
+ private static final int MSG_REMOVE_RANGE = 200;


Can you renumber this to be 3 and change the numbers below accordingly?

tonihei · 2018-07-25T13:28:38Z

library/core/src/main/java/com/google/android/exoplayer2/source/ConcatenatingMediaSource.java

+ * @param fromIndex The initial range index, pointing to the first media source that will be
+ * removed. This index must be in the range of 0 &lt;= index &lt; {@link #getSize()}.
+ * @param toIndex The final range index, pointing to the first media source that will be left
+ * untouched. The last media source to be removed is at index {@code toIndex} - 1.


"The last media source to be removed is at index {@code toIndex} - 1." - That seems redundant and can be removed.

tonihei · 2018-07-25T13:32:23Z

library/core/src/main/java/com/google/android/exoplayer2/source/ConcatenatingMediaSource.java

+ * is thrown.
+ *
+ * @param fromIndex The initial range index, pointing to the first media source that will be
+ * removed. This index must be in the range of 0 &lt;= index &lt; {@link #getSize()}.


Both ranges should be "in the range of 0 <= index <= {@link #getSize()}." That is, including the getSize index.
Otherwise you wouldn't be able to do removeMediaSourceRange(getSize(), getSize()) as an edge case.

At a given time, "occupied" track indices are 0, 1, ... getSize() - 1. fromIndex should point to an occupied index, as the initial range index is included in the range. How can one remove the track at index getSize(), if there's no track at that index? Just trying to understand

As the end index is exclusive, the method must allow getSize() as end index.

And to support the example you gave in the issue, to keep an element at index N only, it would be nice to be able to call
removeMediaSourceRange(0, N) followed by removeMediaSourceRange(1,getSize()). If this is supposed to remove the last element, the second call will result in removeMediaSourceRange(1, 1) which should be a no-op.

We've abandoned the "simplicity" implied by initial issue example when we "gave up" on API tolerance (so to say). Since then, the idea seemed: have a clear API with errors when arguments are not correct. And for fromIndex to be equal to getSize() seems not correct. Of course we noticed that you cannot call removeMediaSourceRange(getSize(), getSize()) anymore. And also you cannot call removeMediaSourceRange(1, getSize()) in issue second call, that would throw too.

Staying consistent with allowed param values in signature seems quite a thing. By allowing passing getIndex() in fromIndex, it seems to go back to the "tolerant" approach somehow, which is fine anyway. Don't know, your call.

I'd say it's best to follow the existing behaviour of List.subList and the like. That is, throwing if startIndex<0, endIndex>size or startIndex>endIndex. This ensures the operation is valid, but is also tolerant enough to support common use cases like "remove everything behind element N" with removeRange(N, size) which will work even if N is the last element.

Cool. Just to tease a little bit, in case like "remove everything behind element N" when N is the last element, then N = size - 1, and removeMediaSourceRange(size - 1, size) already works as is. What instead currently throws (and probably looks not tolerant) is removeMediaSourceRange(size, size). Right?

Anyway, changes are coming

removeMediaSourceRange(size - 1, size) would remove the element at N = size - 1 though, right?

And if I'm not mistaken removeMediaSourceRange(size, size) won't throw too because List.subList doesn't throw for this case.

Q1: correct, current implementation would remove element at size - 1, i.e. "the last element"

Q2: removeMediaSourceRange(size, size) throws in current (<=> initial version) of this PR. It will not throw after applying your proposed changes, as List.subList(size, size) does not throw. I was referring to current implementation.

Cool

tonihei · 2018-07-25T13:36:54Z

library/core/src/main/java/com/google/android/exoplayer2/source/ConcatenatingMediaSource.java

+ * @throws IndexOutOfBoundsException when either index is out of playlist bounds, i.e. {@code
+ * fromIndex} &lt; 0 or &gt;= {@link #getSize()}, {@code toIndex} &lt; 0 or &gt; {@link
+ * #getSize()}
+ * @throws IndexOutOfBoundsException when range is malformed, i.e. {@code fromIndex} &gt;


Can you combine them in one @throws doc? Maybe just as "When the range is malformed, i.e. {@code fromIndex} < 0, {@code toIndex} > {@link #getSize()}, or {@code fromIndex} > {@code toIndex}."

Totally agree with grouping. Are you also suggesting to slim down the number of (broken) examples provided in JavaDoc?

Yes. As we already require fromIndex>lastIndex, the other two examples are not needed.

tonihei · 2018-07-25T13:38:04Z

library/core/src/main/java/com/google/android/exoplayer2/source/ConcatenatingMediaSource.java

+
+ /**
+ * Removes a range of {@link MediaSource}s from the playlist, by specifying an initial index
+ * (included) and a final index (excluded), and executes a custom action on completion..


Remove second full stop.

tonihei · 2018-07-25T13:38:33Z

library/core/src/main/java/com/google/android/exoplayer2/source/ConcatenatingMediaSource.java

+ * #getSize()}
+ * @throws IndexOutOfBoundsException when range is malformed, i.e. {@code fromIndex} &gt;
+ * {@code toIndex}
+ * @param actionOnCompletion A {@link Runnable} which is executed immediately after the media


This param should be together with the other params above.

tonihei · 2018-07-25T13:38:58Z

library/core/src/main/java/com/google/android/exoplayer2/source/ConcatenatingMediaSource.java

+ * @param toIndex The final range index, pointing to the first media source that will be left
+ * untouched. The last media source to be removed is at index {@code toIndex} - 1.
+ * This index must be in the range of 0 &lt;= index &lt; {@link #getSize()}.
+ *


We usually leave no empty line between @param and @throws.

tonihei · 2018-07-25T13:41:10Z

library/core/src/main/java/com/google/android/exoplayer2/source/ConcatenatingMediaSource.java

+ }
+ return;
+ }
+ mediaSourcesPublic.subList(fromIndex, toIndex).clear();


Can you replace that with Util.removeRange(mediaSourcesPublic, fromIndex, toIndex)? That calls the same method but is slightly more readable.

Also move it above the if(fromIndex == toIndex) check to ensure the exceptions are thrown first.

tonihei · 2018-07-25T13:42:42Z

library/core/src/main/java/com/google/android/exoplayer2/source/ConcatenatingMediaSource.java

+ */
+ public final synchronized void removeMediaSourceRange(
+ int fromIndex, int toIndex, @Nullable Runnable actionOnCompletion) {
+ if (fromIndex < 0 || fromIndex >= mediaSourcesPublic.size()) {


No need to do the explicit bound checks here. As soon as you call the subList method, the IndexOutOfBoundsException will be thrown anyway.

tonihei · 2018-07-25T13:44:38Z

library/core/src/main/java/com/google/android/exoplayer2/source/ConcatenatingMediaSource.java

+ mediaSourcesPublic.subList(fromIndex, toIndex).clear();
+ if (player != null) {
+ player
+ .createMessage(this)


These should be indented by 4 spaces (relative to "player")

ojw28 · 2018-07-25T14:03:18Z

No particular need to fix it for this contribution, but for future contributions it's preferable to configure git to use the same email as the one you specified when signing the CLA (setting user.email is described here).

googlebot · 2018-07-25T14:03:47Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ojw28 · 2018-07-25T14:07:01Z

CLA status was manually verified for the committing username (GiuseppePiscopo), as the commits use a @users.noreply.github.com email address.

googlebot · 2018-07-25T16:31:31Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2018-07-25T16:32:59Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

tonihei · 2018-07-25T16:35:08Z

Thanks! Looks good now :)

BrainCrumbz · 2018-07-26T12:22:59Z

Not sure what's happening re. CLA. The other commit author account (GiuseppePiscopo) has registered google CLA with the same email address also associated to Github profile.

Can that be related to the "keep my address private" option in github profile?

ojw28 · 2018-07-26T15:11:51Z

I think it's because the email address the CLA is signed under doesn't match the email address on the commits (which is a @users.noreply.github.com address). See my comment above (#4564 (comment)) for how to fix that for future commits. It could well be that the "keep my address private" option overrides whatever address you configure, so yes, that could be related!

For this commit we manually verified the username against our CLA records, so it's not a blocking issue.

BrainCrumbz · 2018-07-27T07:45:18Z

Thanks for feedback, yes we read that. Do you know if CLA can be signed under a @users.noreply.github.com address?

ojw28 · 2018-07-27T09:52:17Z

Unsure. It seems a bit strange to do that :). We should probably just make the googlebot clever enough to look at the username as well. I'll try and get someone to do that internally!

ojw28 · 2018-07-31T11:11:26Z

Turns out the issue with the CLA is that the account sending the pull request is different to the account whose commits are included within it. Hence we need to manually confirm that the commit author is OK with their changes being part of the pull request, under their signed CLA. If we didn't do this, it would be possible for someone to try and contribute commits by an author who has signed a CLA, but does not want the commits in question to be contributed.

Please could you confirm with account @GiuseppePiscopo that you're OK with all commits that are part of this pull request being submitted (you'll need to do this again whenever a new commit is added).

In future, using the same account for commits and pull requests, which is an account with a signed CLA, would help streamline this process.

BrainCrumbz · 2018-07-31T13:30:34Z

Hi there. Yes, we were aware of this distinction between pull request opener and commit authors, thanks for clarifying. Could not find more info on how to get a user confirmation, so we just added this comment here above. Does that count as a confirmation?

Re. CLA, @GiuseppePiscopo also has a signed CLA in google, using the same email address associated with Github. Problem is (we think), email preference on Github is set to private, hence commits appear authored by a [email protected], and it's not possible to sign a CLA under that "dummy" email address.

If one wants to keep distinct commit authors, it seems there's no way to also keep Github email private. Anyway, for the next time we'll try to switch user so that all commits show up under same account as PR creator.

ojw28 · 2018-07-31T14:06:45Z

Re. CLA, @GiuseppePiscopo also has a signed CLA in google, using the same email address associated with Github. Problem is (we think), email preference on Github is set to private, hence commits appear authored by a [email protected], and it's not possible to sign a CLA under that "dummy" email address.

It turned out this was not the problem. The CLA bot is clever enough to look at the username in this case. The problem is solely that the pull request opener is different to the commit author. In this case there is no automated way for the CLA bot to validate that the commit author is OK with their commit being included in the pull request, which is why manual validation is necessary. Them having signed the CLA is not sufficient (since they might have signed it to contribute some completely unrelated change to another Google project).

We need the commit author to manually confirm they are OK with their commits being included in this case. Since additional commits were added after the comment you reference, we need the commit author to comment again on this thread, to cover the additional commits since that comment. Thanks!

GiuseppePiscopo · 2018-07-31T14:31:55Z

I confirm I'm ok with including those additional commits as well

BrainCrumbz · 2018-08-17T12:04:59Z

How can we track when this PR will be included in a public release? Thanks

ojw28 · 2018-08-17T12:21:01Z

It will be in 2.9.0 (for which we don't yet have a solid eta).

GiuseppePiscopo added 2 commits July 24, 2018 10:08

feat(playlist): add API to remove range of indices

c9b6a73

docs(playlist): fix javadoc comments

2c801ca

googlebot added the cla: no label Jul 24, 2018

ojw28 requested a review from tonihei July 24, 2018 12:22

ojw28 assigned tonihei Jul 24, 2018

tonihei reviewed Jul 25, 2018

View reviewed changes

ojw28 added cla: yes and removed cla: no labels Jul 25, 2018

GiuseppePiscopo added 2 commits July 25, 2018 18:30

feat(playlist): stick to list.subList API

cc7f011

chore(playlist): reorder message IDs, fix tabbing

7878bf6

googlebot added cla: no and removed cla: yes labels Jul 25, 2018

tonihei added cla: yes and removed cla: no labels Jul 25, 2018

tonihei added the should merge label Jul 25, 2018

ojw28 merged commit 52b6b3b into google:dev-v2 Aug 16, 2018

BrainCrumbz deleted the feat/playlist-remove-range branch August 16, 2018 16:35

BrainCrumbz mentioned this pull request Sep 26, 2018

SinglePeriodTimeline.getPeriod assertion fails when manipulating playlist #4871

Closed

google locked and limited conversation to collaborators Dec 20, 2018

feat(playlist): remove MediaSource range #4564

feat(playlist): remove MediaSource range #4564

Conversation

BrainCrumbz commented Jul 24, 2018

googlebot commented Jul 24, 2018

BrainCrumbz commented Jul 24, 2018

googlebot commented Jul 24, 2018

GiuseppePiscopo commented Jul 24, 2018

tonihei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrainCrumbz Jul 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojw28 commented Jul 25, 2018

googlebot commented Jul 25, 2018

ojw28 commented Jul 25, 2018 • edited Loading

googlebot commented Jul 25, 2018

googlebot commented Jul 25, 2018

tonihei commented Jul 25, 2018

BrainCrumbz commented Jul 26, 2018

ojw28 commented Jul 26, 2018 • edited Loading

BrainCrumbz commented Jul 27, 2018

ojw28 commented Jul 27, 2018

ojw28 commented Jul 31, 2018

BrainCrumbz commented Jul 31, 2018 • edited Loading

ojw28 commented Jul 31, 2018 • edited Loading

GiuseppePiscopo commented Jul 31, 2018

BrainCrumbz commented Aug 17, 2018

ojw28 commented Aug 17, 2018

BrainCrumbz Jul 25, 2018 •

edited

Loading

ojw28 commented Jul 25, 2018 •

edited

Loading

ojw28 commented Jul 26, 2018 •

edited

Loading

BrainCrumbz commented Jul 31, 2018 •

edited

Loading

ojw28 commented Jul 31, 2018 •

edited

Loading