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

Reduced CPU usage when playing a video by 7-10% #4080

Merged
merged 2 commits into from
Aug 17, 2020

Conversation

avently
Copy link
Contributor

@avently avently commented Aug 5, 2020

What is it?

  • Bug fix (user facing)
  • Feature (user facing)
  • Code base improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

After reading this comment #3948 (comment)

this new unified player ... drains the bettery twice as much

I checked how much CPU the player uses. And found out that @MD77MD said incorrect information but even if it's not true (about double difference) the new player definitely uses more CPU than 0.19.8. Difference is about 15%. So I started seaching for the cause and found that this line https://github.com/TeamNewPipe/NewPipe/blob/dev/app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java#L1774
adds 7-10% of CPU load. Actually there is no need to animate the progress bar when it's hidden under the video but it should be updated anyway (without animation) to reflect playing position changes.

Next. After this change the difference is still here but small: around 5%. Please, note, that I tested it on 5-year old phone while it had 1% of battery and worked on a charger with screen recorder impact so absolute values in your case would be less as well as difference.

I couldn't find a cause until I realized that MainVideoPlayer (old video player in 0.19.8) doesn't use notification. So it doesn't need to update it twice every second. This was it! After commenting out everything after this line: https://github.com/TeamNewPipe/NewPipe/blob/dev/app/src/main/java/org/schabi/newpipe/player/VideoPlayerImpl.java#L640 the CPU usage was the same compared to 0.19.8 (0-1% difference).

Here is a screenrecord of comparison (with commented code related to notification) of new and old apps:
https://www.dropbox.com/s/l2sdo5rsjfmjusc/2020-08-05-12-01-09.mp4?dl=0

Of course this shouldn't be commented out in the current codebase but it gives us information about why this little difference exists: it's needed to update notification.

Testing apk

test-apk.zip

Agreement

@MD77MD
Copy link

MD77MD commented Aug 8, 2020

@avently
I have been actively comparing between newPipe 19.8 and new player... and found it's more like 30% (not double). however this was on my Samsung s6, if you think it might be different on S4 I'll do a test.. let me know if you need me to.

another thing, I wanted to ask your thought on an idea and weather I sould open a request for it.. now the player starts loading comments then starts to play video (thus introducing some waiting depending on internet speed)...I was wandering if we can make the player start playing the video first while the comments and related videos sections can catch-up... please let me know if I should open a request to discuss this.

@avently
Copy link
Contributor Author

avently commented Aug 8, 2020

I have been actively comparing between newPipe 19.8 and new player... and found it's more like 30% (not double

Did you compare test apk from this PR? Is there any improvements? I would like to see real numbers instead of "I think it's around 30% in difference". I used Simple System Monitor for showing popup with graph of cpu usage on the same video with the same quality. Compare only CPU, it the only thing matters because your difference in battery life can be result of a difference in brightness, quality, video format, etc. The only thing I could do bad is to make computation harder but as showed in the screenrecord, the only difference makes worse CPU usage is updating notification which can not be removed. I can send a test apk with disabled notification updating just to ensure that you will get near equals result with 0.19.8 (it didn't have notification so this process didn't add a cpu pressure)

now the player starts loading comments then starts to play video

I'm not sure you will get any improvements in terms of start time of video loading because amount of comments is so small. And skipping comments from initial loading will probably require additional call to the website. But it's only my expectations I don't know what happens in extractor under the hood. So you can open an issue and to discuss with guys who made the extractor.

@wb9688 wb9688 merged commit b7f50c3 into TeamNewPipe:dev Aug 17, 2020
This was referenced Sep 27, 2020
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