Skip to content

Conversation

Redirion
Copy link
Member

@Redirion Redirion commented Sep 24, 2021

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

Issue 1 is when the option "Show thumbnail" is turned off. It caused setMetadata to never be called.
// setMetadata only updates the metadata when any of the metadata keys are null mediaSessionManager.setMetadata(getVideoTitle(), getUploaderName(), showThumbnail ? getThumbnail() : null, duration);

basically the first line in setMetadata checks, if the provided thumbnail/albumArt is not null.

Issue 2: if no thumbnail could be loaded, the dummy thumbnail resource would be used to be created through a Bitmap factory. Firstly it will be created each time (expensive) and secondly hashCode() will always yield different results. This means that setMetadata was basically executed in its whole every single time (each second since last updates).

Fixes the following issue(s)

Don't know if there is an issue that would be closed by this. I found it while looking at issue #7087

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.

Due diligence

@triallax triallax added bug Issue is related to a bug player Issues related to any player (main, popup and background) labels Sep 24, 2021
@litetex
Copy link
Member

litetex commented Sep 24, 2021

@Redirion
Thank you for the PullRequest 😄

I quickly read the code (not in depth) and large parts of the code seem to be duplicated.
Is it possible to de-duplicate those?

PS: Will test it out later with the Android profiler 😄

@Redirion
Copy link
Member Author

apart from the debug logs nothing is really duplicated. It just looks similar ;)

the albumArt is completely dropped in the new method and the check for updating does not focus around the hashCode of the bitmap but uses equals on the title instead

However this is not a fix for the general performance of setMetadata. It fixes two specific issues I found when checking the method. I did not find a way to increase the performance. hashCode should already be the most cost effective way to compare bitmaps.
Other than that we could only drop the comparison of bitmaps altogether and just use equals for title, artist and compare the duration.

@litetex
Copy link
Member

litetex commented Sep 24, 2021

@Redirion
I create a update version of this pull request with additional fixes: #7166

You might close this PR (Update: It got closed 5 minutes ago 😆)
Also note that your commit-mail seems to be wrong (no profile picture showing up).
Anyway thank you for the PR 😄

@Redirion Redirion deleted the fixsetmetadata branch September 27, 2021 11:55
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 this pull request may close these issues.

3 participants