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

Potential performance problem with MediaSessionManager#get/setMetadata #7087

Closed
4 tasks done
litetex opened this issue Sep 10, 2021 · 5 comments · Fixed by #7166
Closed
4 tasks done

Potential performance problem with MediaSessionManager#get/setMetadata #7087

litetex opened this issue Sep 10, 2021 · 5 comments · Fixed by #7166
Labels
bug Issue is related to a bug player Issues related to any player (main, popup and background)

Comments

@litetex
Copy link
Member

litetex commented Sep 10, 2021

Checklist

Steps to reproduce the bug

Run the app / play a video (in the background).

What I did:
Opened a video and ran it in the background.

Details

NewPipe performs a progress update when the player is active every 500ms/1s (was increased to 1s in #7071).

However when this progress update is executed the setMetadata-method seems to be very costly (takes 40-80% of the time of the complete progress update) as seen when profiling the app:

Emulator Pixel 3a

cpu-emulator-Pixel3a.zip

Emulator Pixel C

cpu-emulator-PixelC.zip

Note: I'm unable to determine if this is just a android emulator problem as I can't profile my real phone (crashes with SIGSEV in HeapTaskDaemon/libc when running the profiler)

Device info

  • Android version/Custom ROM version: Android 11 - API 30
  • Device model: Android Emulator Pixel 3a/Pixel C
@litetex litetex added the bug Issue is related to a bug label Sep 10, 2021
@litetex litetex changed the title MediaSessionManager: Potential performance problem with get/setMetadata Potential performance problem with MediaSessionManager#get/setMetadata Sep 10, 2021
@Redirion
Copy link
Member

Redirion commented Sep 24, 2021

For the performance of this method I wonder if this is not an issue of inspecting the performance on the debug build. Did you also try to check how the performance is, without all the Logs?

edit: getThumbnail is never null. So don't mind the pre edited part.
edit2: oh no, I confused myself. It is indeed a problem! Original comment:

while looking at setMetaData I noticed another issue:

// setMetadata only updates the metadata when any of the metadata keys are null mediaSessionManager.setMetadata(getVideoTitle(), getUploaderName(), showThumbnail ? getThumbnail() : null, duration);

in other words, if showThumbnail is false, metaData updates do never happen!

I will create a PR for that.

@Redirion
Copy link
Member

@litetex can you check your logcat: does "setMetadata: N_Metadata update" appear every 1 seconds? that would be a bug. Please try my PR in comparison. Do you have "Show thumbnails" set to "off"?

@litetex
Copy link
Member Author

litetex commented Sep 24, 2021

Okay I tested the stuff a bit and found out that the metadata updates - that are done every second - are completely useless.
They should only be done when the metadata changes e.g. when opening a new video.

@Redirion
Copy link
Member

yes, and that's how it works / should work. When I was testing my PR, it was working this way.
What is the log output of setMetadata: N_Metadata update for you?

@litetex
Copy link
Member Author

litetex commented Sep 24, 2021

@Redirion
It seems to be fixed as it only occurs once in the log 😄
With my code on top - where the update only occurs when the metdata is changed - this is definitely fixed.

@AudricV AudricV added the player Issues related to any player (main, popup and background) label Sep 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug player Issues related to any player (main, popup and background)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants