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

feat(MediaSource): client code can get the tag of a MediaSource #5187

Merged
merged 3 commits into from
Dec 5, 2018
Merged

feat(MediaSource): client code can get the tag of a MediaSource #5187

merged 3 commits into from
Dec 5, 2018

Conversation

BrainCrumbz
Copy link
Contributor

Closes #5177

@googlebot
Copy link

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 all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@GiuseppePiscopo
Copy link
Contributor

I'm ok with my commits being contributed to this project.

@BrainCrumbz
Copy link
Contributor Author

For completeness: when running tests (on a Windows dev machine) we experienced 28 failed tests both before and after the change.

@tonihei
Copy link
Collaborator

tonihei commented Dec 3, 2018

For completeness: when running tests (on a Windows dev machine) we experienced 28 failed tests both before and after the change.

That's interesting :) Would you mind giving us a list? No one in our team is using Windows, so probably difficult to fix, but still seems worth knowing why they fail.

@tonihei tonihei added cla: yes and removed cla: no labels Dec 3, 2018
@tonihei tonihei self-assigned this Dec 3, 2018
Copy link
Collaborator

@tonihei tonihei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good in general.

@@ -275,6 +275,11 @@ void prepareSource(
*/
void releasePeriod(MediaPeriod mediaPeriod);

/**
* Returns the tag set on media source, or null when none was set.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on the media source

/**
* Returns the tag set on media source, or null when none was set.
*/
Object getTag();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move the method somewhere else to keep the release methods bundled together? For example, above addEventListener or below removeEventListener.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, can you make this a default method which returns null for now? That prevents breaking other existing implementations.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nullable is missing here too.

@BrainCrumbz
Copy link
Contributor Author

Re. test failing on Windows 10: on branch dev-v2, commit

ffbb0da Prevent Cea608Decoder from generating Subtitles with null Cues list.

here are attached the test results (exported as zipped HTML):

Test Results - exoplayer2_in_library-core - ffbb0da.zip

@googlebot googlebot added cla: no and removed cla: yes labels Dec 3, 2018
@BrainCrumbz
Copy link
Contributor Author

Requested changes have been added. Not sure whether those return null should stay in ConcatenatingMediaSource.java. After all, they could be totally replaced by default implementation.

@tonihei
Copy link
Collaborator

tonihei commented Dec 4, 2018

Requested changes have been added.

Thanks! Looks good on a code level. I'd also keep the return null where it is because it's intentional (compared to just not implemented yet).
Let's use the issue to clarify the need to do this.

attached the test results

That's good to see. I suspect they all fail because of some line separator or encoding difference.

@tonihei tonihei added cla: yes and removed cla: no labels Dec 4, 2018
@googlebot
Copy link

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.)

@BrainCrumbz
Copy link
Contributor Author

I'd also keep the return null where it is because it's intentional (compared to just not implemented yet).

True, totally agreeing on making it explicit.

Let's use the issue to clarify the need to do this.

Not sure what you mean. If there's some action we should do on this side, just tell us please

@tonihei
Copy link
Collaborator

tonihei commented Dec 4, 2018

Not sure what you mean.

Never mind, I was referring to the ongoing discussion in #5177.

@ojw28 ojw28 merged commit a11a871 into google:dev-v2 Dec 5, 2018
ojw28 added a commit that referenced this pull request Dec 5, 2018
@BrainCrumbz BrainCrumbz deleted the feat/get-tag branch December 6, 2018 08:47
ojw28 added a commit that referenced this pull request Dec 19, 2018
@google google locked and limited conversation to collaborators May 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants