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

Allow selecting image quality among multiple images #10062

Merged
merged 13 commits into from
Sep 22, 2023

Conversation

Stypox
Copy link
Member

@Stypox Stypox commented May 1, 2023

What is it?

  • Feature (user facing)

Description of the changes in your PR

  • This PR adapts to Multiple images support NewPipeExtractor#889, which introduces breaking changes regarding images. Instead of providing just one URL per image (e.g. getThumbnailUrl()), the extractor now provides a List<Image> (e.g. getThumbnails()), and each image has width, height and resolutionLevel attributes that allow determining an image's quality.
  • This PR introduces a preference that allows users to either disable images completely, or to choose a level amongst "low", "medium" and "high". This setting replaces the "Load thumbnails" switch.
  • The strategy that determines which image is chosen based on the user preference is here, along with documentation.
  • In the database only one image URL is still stored, instead of the whole List<Image>. The saved URL is the preferred one at the time of saving. This keeps it simple, avoids needing to do database migrations (not really a big deal, but still) and saves storage. Otherwise we would probably double the database size just to store a whole lot of images that will never be all used anyway. The only downside is that upon changing preferred resolution level in settings, only newly fetched List<Image>s will have the correct level, while videos in history/feed will not. I don't think it's such a big deal, also because when opening a (remote) video (or playlist), the thumbnail(s) would get updated to the newly set preferred resolution level.
  • The description tab in the video detail fragment now shows all available thumbnails, highlighting the one being chosen and allowing to tap on them to open in browser.

Before/After Screenshots/Screen Record

Before preference Before list
Preference Low (list) Medium (list) High (video details) Description

TODO

  • Add description to PR
  • Allow user to choose thumbnail quality
  • Refine preferred image choosing method
  • Decide whether it's fine to save only one thumbnail in the database - Yes, it's fine.
  • How should PlaylistRemoteEntity.isIdenticalTo behave? - it's fine if the thumbnail is updated in the database if the current preference has changed (since that implies choosePreferredImage returns a different url than the one saved in the database, and hence isIdenticalTo returns false)
  • The description fragment should show all thumbnail URLs (and maybe highlight the preferred one being chosen)
  • Add comments to the code, and possibly rename some variables
  • Add tests for ImageStrategy
  • Update extractor commit after channel tabs PR is merged

Fixes the following issue(s)

Relies on the following changes

TeamNewPipe/NewPipeExtractor#889

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.

At the moment the highest quality thumbnails will load everywhere, and that's the only noticeable difference with normal NewPipe, until a quality setting is added.

Due diligence

@Stypox Stypox changed the title Multiple images Allow selecting image quality among multiple images May 2, 2023
@Stypox Stypox force-pushed the multiple-images branch 3 times, most recently from bd3d6ef to 7351ff7 Compare May 2, 2023 18:21
@Stypox Stypox marked this pull request as ready for review May 2, 2023 19:55
@Jean-BaptisteC

This comment was marked as resolved.

@Stypox
Copy link
Member Author

Stypox commented May 3, 2023

That's unrelated to this PR and is fixed in the channel tabs PR. I don't know why suddenly that issue started arising, as the code there didn't change... Probably a kotlin update

Copy link
Member

@Isira-Seneviratne Isira-Seneviratne left a comment

Choose a reason for hiding this comment

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

This looks good to me overall, I checked on an Android 5.0 emulator and the image loading seems to be working well. I've suggested a few small changes.

@ghost
Copy link

ghost commented May 7, 2023

Hi, I'm here to just make some quick note on your work, it's almost perfect, the image quality has increased a lot compared to before, however it's not sharp enough when you compare it to the highest image quality you get when you click on the thumbnail url in the description.
I hope you'll take a look into it, and thank you for your time.

Screenshot_20230507-230731 Screenshot_20230507-230809 Screenshot_20230507-231237

@at1orianda

This comment was marked as duplicate.

@SameenAhnaf SameenAhnaf added feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface labels May 13, 2023
@AudricV
Copy link
Member

AudricV commented May 18, 2023

however it's sharp enough when you compare it to the highest image quality you get when you click on the thumbnail url in the description.

There is nothing we can really do I am afraid.

The extractor returns the thumbnails that YouTube returns on its desktop website for search results. It is not planned to build custom thumbnails ourselves on YouTube outside of RSS feeds and mixes, on which only one thumbnail or no thumbnail is available, as YouTube provides thumbnails without black borders and these thumbnails require signatures and parameters we are not able to generate.

Also, not every thumbnail resolution is available on all videos (look at the extractor PR for more details).

Using different thumbnails would create a different behavior compared to YouTube official clients in more places than RSS feeds and mixes, and I think we like to avoid that as much as we can.

@ghost
Copy link

ghost commented May 18, 2023

Alright, I can't complain anymore, you already did your best to provide us with such a great app.

Thank you again.

@Stypox Stypox force-pushed the multiple-images branch 3 times, most recently from 69ee195 to 79b6ede Compare August 13, 2023 21:56
@Stypox
Copy link
Member Author

Stypox commented Aug 13, 2023

  • Rebased
  • Even though the images extractor PR has been merged, the current NewPipe is incompatible with it because of the channel tab changes, so the NPE commit used at the moment is this one
  • Added a test for ImageStrategy and fixed a boolean condition that should have been the opposite (image.getHeight() == HEIGHT_UNKNOWN && image.getWidth() == WIDTH_UNKNOWN instead of the previous != || !=)
  • Applied Isira-Seneviratne's suggestions

Copy link
Contributor

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

LGTM, just some small things

@TeamNewPipe TeamNewPipe deleted a comment from sonarqubecloud bot Sep 19, 2023
@TeamNewPipe TeamNewPipe deleted a comment from sonarqubecloud bot Sep 19, 2023
@AudricV AudricV added the multiservice Issues related to multiple services label Sep 19, 2023
@Stypox Stypox merged commit b508dd6 into TeamNewPipe:dev Sep 22, 2023
2 of 4 checks passed
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 1 Security Hotspot
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface multiservice Issues related to multiple services
Projects
Status: Done
7 participants