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

Add url, progress and duration for iOS parity #36

Closed
wants to merge 3 commits into from

Conversation

tychota
Copy link

@tychota tychota commented Nov 10, 2016

Fix #35

@tychota
Copy link
Author

tychota commented Nov 10, 2016

I will test using my project that all works with this commit hash then let you merge.
You can start review it.

Copy link
Owner

@tlenclos tlenclos left a comment

Choose a reason for hiding this comment

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

Except the stacktrace part it seems ok. I will test it also on my side before merging.

PrintWriter pw = new PrintWriter(sw);
t.printStackTrace(pw);
Intent BroadcastIntent = new Intent(Mode.ERROR);
BroadcastIntent.putExtra("stacktrace", sw.toString());
Copy link
Owner

Choose a reason for hiding this comment

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

BroadcastIntent is not used here and I don't think we should pass this much data over the bridge. We should create custom error code maybe, but this is an other issue. Could you remove the stacktrace part ?

Copy link
Author

Choose a reason for hiding this comment

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

Forgot to remove dead code.

Nowadays, when there is an error that is hard to see why (some of my file were private on AWS and I took me some time). But I agree, it is another feature.

I'm gonna update this PR.

@tychota
Copy link
Author

tychota commented Nov 10, 2016

It is working perfectly in my project. I did not use getStatus thought, so if you can test that.

Thank for the help.

@brettpappas
Copy link
Collaborator

This will be a big help for getting Android closer to feature parity with iOS (ability to track progress).

Looks like the 1 remaining thing is the ability to play the audio at a specific starting point. If we could do that with aacdecoder then we would be able to add the seekToTime, goForward, and goBack methods like we have for iOS. I use these methods to give the player a button to advance 30 seconds, rewind 30 seconds like below:

screen shot 2016-11-10 at 9 58 15 am

@tychota
Copy link
Author

tychota commented Nov 14, 2016

Any ETA before merge of this PR ?

I've sadly worked on the master branch of the fork and I think I will need it the to push some unrelated commits on the fork to be able to debug an issue with some stream on android.

If you need more time to review, I can rewrite the history to move this PR's commits into another branch and reopen a new PR with those commits.

@tlenclos
Copy link
Owner

tlenclos commented Nov 14, 2016

@tychota you can just use an other branch based on master for your work if you want. I will not merge this before thursday sadly.

@tychota
Copy link
Author

tychota commented Nov 14, 2016

Oki ! I will do that ;)

Thank for that pragamatic advice : I had not thought this solution :)

@tlenclos
Copy link
Owner

tlenclos commented Nov 17, 2016

Hello there, I'm sorry but it does not seem to work.
I'm always getting a duration of 2.5 and a progress between 0.5 and 1.5.

What stream did you use to test ? Maybe something is wrong on my side.

@tychota
Copy link
Author

tychota commented Nov 17, 2016

Mmh. The progress stuff look weird on my side too.

I'm getting progress in percent (progress/duration) between 0 and 100% but they seems to oscillate.

I guess it does not take the full file duration as duration but rather the already streamed duration. One coworker also did also catch this today. Since we are playing 1 minute file but over great network, I guess I didn't catch this earlier.

Now I'm lost. I don't know what are exactly the information I'm getting but I guess it is only what's buffered. It is clearly not the same as in iOS. And i don't know how to obtain feature parity.

Is it ever possible with aacPlayer ?

@tlenclos
Copy link
Owner

It appears not, I'm trying to migrate to ExoPlayer anyway that will be much much better to handle this.
The problem is the decoding of shoutcast stream metadata (mandatory for webradios) that is not supported natively.

I'll keep you in touch.

@tychota
Copy link
Author

tychota commented Nov 17, 2016

Shall I remove the progress part, open another PR with only url and try to get something smaller merged ?

Or do you think this will be lost of time ?

@tlenclos
Copy link
Owner

tlenclos commented Nov 17, 2016

Don't waste your time on this. The url is something that comes from user side anyway so this is not really a problem. I will add it later after the migration.

@tychota
Copy link
Author

tychota commented Nov 17, 2016

So let' close :)

Sorry that it did not end up being merged :/

@tychota tychota closed this Nov 17, 2016
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants