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

WIP/PoC: On-demand thumbnail downloads #8490

Closed
wants to merge 6 commits into from
Closed

WIP/PoC: On-demand thumbnail downloads #8490

wants to merge 6 commits into from

Conversation

andres-asm
Copy link
Contributor

What it says, this makes RA download thumbnails on-demand instead of the current slow, monolithic method that doesn't even work in many platforms due to memory constraints.

I don't really understand the thumbnail code (LEFT/RIGHT seem meaningless in OZONE) and it's all a weird mess in my opinion (no insult intended)

Anyway what this does is download the currently selected thumbnail type if available.

It forms the URLs and saves the files correctly.
All that needs to be done is untangle it from ozone, and fix-up the trigger action so it actually triggers there.

@natinusala @twinaphex

@natinusala
Copy link
Contributor

I will have a look at that tomorrow, seems nice!

@jdgleaver
Copy link
Contributor

Nice idea. Some feedback:

  • There needs to be a way to disable this behaviour, and it should be off by default

  • If a thumbnail does not exist on the server, it's still going to try to download it every time the user accesses the relevant playlist entry (and this can happen multiple times for the affected entry, when launching content, toggling the quick menu, etc., not just when the user selects it in the playlist). There should be a way of recording that a download request has failed (create dummy file with image name?)

  • Scrolling through a list of images in the file browser will generate download requests for each image. This is not desirable behaviour...

  • Browsing databases will download thumbnails for every entry (regardless of whether the user possesses the corresponding content). Not sure if this is desired...

@natinusala
Copy link
Contributor

This is neat, the code can be moved to menu_thumbnail to make it work on all menu drivers.

About commit ad8010c, I don't mind putting sublabels back but it needs to be under a setting (unique to ozone I suppose). This new setting would be disabled by default - if enabled, it would hide the content metadata panel and put sublabels back.

@andres-asm
Copy link
Contributor Author

andres-asm commented Mar 18, 2019 via email

@jdgleaver
Copy link
Contributor

@fr500 Yes, sorry if that sounded overly critical - this is fine for a testing branch 🙂

Download on button press is awkward. It is very difficult to override action_switch_thumbnail() to give Y any useful alternative purpose (not without making it a generic 'thumbnail action' callback, and handling what that means in each menu driver itself). Or maybe assign 'download' to a new button? (although there are only the left/right thumbstick buttons going spare at the moment... 🙁 )

@andres-asm
Copy link
Contributor Author

No worries.

menu/cbs/menu_cbs_ok.c Outdated Show resolved Hide resolved
@@ -73,6 +73,8 @@ typedef struct ozone_handle ozone_handle_t;
#define OZONE_TICKER_SPACER "\xE2\x80\x83\xE2\x80\xA2\xE2\x80\x83"
#endif

bool missing_thumbnail;
Copy link

Choose a reason for hiding this comment

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

no need for this to be global to the whole program

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually it will have to be moved to be "even more global" if we want to reuse the code in other drivers.
so I guess I should go the fake C++ route, adding a variable to retroarch.c and "getters and setters"

@andres-asm
Copy link
Contributor Author

andres-asm commented Mar 19, 2019

I'll repost what I said in #programming on discord:

I just tested the PoC on demand downloader thing
on my shield ATV
it worked well
one thing I'd like is for it to refresh once it downloads though
and maybe we can add a field on the playlist to indicate there is no thumb
so it doesn't try to dl again
every single time
this is how it looks to the user right now

https://streamable.com/ion9g

if either you or @jdgleaver want to give me a hand and we can decide on functionality
I'd be willing to finish it up
I was thinking maybe a spinner animation the first time
and then make it auto-refresh
when it downloads
ofc in most cases it would take just a sec to download so the spinner may be pointless
I do think that saving that the download wasn't successful is important
also another thing I was thinking is that we could de-hardcode the icon from both the console and the entries now that the format is flexible
so by default it would use the playlist name
but then the user could change it, even on a per-game basis
I think @kcin2001 has something like that setup
the default behavior would be as-is
but it would let users customize a lot more

The resulting playlist would be something like this:

{
  "version": "1.0",
  "config": [
    {
      "icon": "default",
      "default_core_path": "D:\\GameData\\Emulators\\RetroArch\\libretro\\snes9x_libretro.dll",
      "default_core_name": "Nintendo - SNES / Famicom (Snes9x)"
    },
  ],
  "items": [
    {
      "path": "W:\\roms\\Emulated\\Console\\Nintendo Super Nintendo Entertainment System\\Super Mario All-Stars (USA).zip#Super Mario All-Stars (USA).sfc",
      "label": "Super Mario All-Stars (USA)",
      "core_path": "D:\\GameData\\Emulators\\RetroArch\\libretro\\snes9x_libretro.dll",
      "core_name": "Nintendo - SNES / Famicom (Snes9x)",
      "crc32": "925637C7|crc",
      "db_name": "Nintendo - Super Nintendo Entertainment System.lpl",
      "icon": "default",
      "thumbnail_found": "true"
    },
  ]
}

Regarding icon flags on entries:
That way we could have an entry in each entry to change the icon for instance (obviously that would require a lot of work to render such icon).

Regarding icon flags for playlists:
Not sure, I have always thought there should be a sort of contextual menu for playlist to do general playlist maintenance stuff (rescan entries, remove all entries, etc)

Regarding the default core stuff:
Same reasoning as above, also removing this would allow us to remove this from the main config:

playlist_cores = "D:\GameData\Emulators\RetroArch\libretro\snes9x_libretro.dll|D:\GameData\Emulators\RetroArch\libretro\nestopia_libretro.dll"
playlist_names = "Nintendo - Super Nintendo Entertainment System.lpl|Nintendo - Nintendo Entertainment System.lpl"

While it looks harmless, it's terrible in my opinion because:

  1. it's several values in a key=value entry
  2. it can't be modified by hand easily because both have to be aligned
  3. picking the cores via left right is annoying af
  4. it could overflow with just a few playlists
  5. it should be part of playlist maintenance itself, not the main config

@andres-asm
Copy link
Contributor Author

@jdgleaver regarding history, images and favorites, those should be blacklisted from downloading I guess, no point downloading if it's always gonna fail.

On history and favorite entries it depends, it would work if the entries were added from a proper playlist.
But if it's a history from good'ol regular load content (or a favorite from such history) it would never work because the resulting entry doesn't have a db_name field filled.

@andres-asm
Copy link
Contributor Author

And this is me over reaching and making this little thing too big to even be considered 🤣

@andres-asm
Copy link
Contributor Author

@natinusala moved the code to menu_thumbnail_paths.c

@natinusala
Copy link
Contributor

I'm all in for a loading spinner and auto refresh. I can do the fancy display part if you want, we have a hourglass icon that we can use (I already use it in the tasks widget). However I think it would require a field indicating no thumb, UX will be down the drain without.

Having some sort of contextual menu is also something I had in mind but I don't know how well it could be integrated into the current menu code. We could reduce the complexity by having only "actions" in this menu, as in never push another menu from this one. Open the contextual menu, do your action, and it closes immediately.

@jdgleaver
Copy link
Contributor

@fr500 Oh boy - that got complicated real fast... 🙂

There are lots of good ideas here, but also lots of work involved (and heaps of edits to my new thumbnail code). I have a vested interest in anything that changes my stuff, but I'm busy with other things at the moment... I'll try to have a proper think about this later...

(On a side note, I personally detest downloading thumbnails inside RetroArch, in the same way that I detest the content scanner. I have never used either feature, and I never will. On the systems I actually game with, my ROM names and thumbnail file names/directories have nothing to do with 'official' releases/identifiers, and I manage everything with my own scripts. So this kind of on-demand thumbnail download would fail on every single one of my playlist entries, and do nothing for me other than waste CPU cycles - hence my main concern being that the whole thing can be easily and efficiently disabled for people who don't want it, and that it should be ifdef'd out completely for platforms without HAVE_NETWORKING. But I understand why others would find this stuff useful)

@inactive123
Copy link
Contributor

inactive123 commented Mar 19, 2019

Yeah, I agree with @jdgleaver that this should be something that should be easily possible to be disabled, and possibly opt-in so that people who don't want it to waste CPU cycles or networking resources are not emburdened by it.

But it is definitely a cool feature for the casuals. I can see why a lot who are into stuff like Launchbox or EmulationStation would want something like this to be enabled by default. We might at some point want to really consider a startup wizard or a first-time startup where we ask the user what kind of features they want to enable at the beginning. Stuff like this is something they could opt-in and opt-out of then. That is also how Windows itself handles these things when the installer is done and it asks you whether to enable certain key features.

@andres-asm
Copy link
Contributor Author

I don't think the current 3GB packs are useful for anyone really.
You have to consider the whole model is broken for all consoles and all 32 bit systems.

@FinixFighter
Copy link

Very nice idea!

@andres-asm
Copy link
Contributor Author

Testing builds for android here:
http://builds.retromods.org/retroarch/

@inactive123
Copy link
Contributor

@fr500 Yeah there's something to be said for that.

I guess I need to try to introduce a first-run wizard as soon as possible so that we can ask the user if they want this to be enabled or not.

@andres-asm andres-asm closed this Mar 20, 2019
@inactive123 inactive123 reopened this Mar 20, 2019
@@ -840,7 +840,7 @@ static int action_bind_sublabel_playlist_entry(
unsigned last_played_minute = 0;
unsigned last_played_second = 0;

if (!settings->bools.playlist_show_sublabels || string_is_equal(settings->arrays.menu_driver, "ozone"))
Copy link
Member

Choose a reason for hiding this comment

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

Strange that ozone was set to always show sublabels. Is this option omitted in Ozone's settings?

Copy link
Contributor

Choose a reason for hiding this comment

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

It was a debug leftover IIRC

@RobLoach
Copy link
Member

Happy you've started this up! The packs are pretty large, and extracting them doesn't really work on smaller hardware. Thanks so much for getting the ball rolling on it.

@andres-asm
Copy link
Contributor Author

andres-asm commented Mar 30, 2019 via email

@inactive123
Copy link
Contributor

inactive123 commented May 24, 2019

OK so we finally implemented this -

#8842

@fr500 Let me know if you're satisfied with this implementation. If there's any further criticism you have on the implementation, maybe @jdgleaver could address it.

If you're satisfied, you can close the PR I guess.

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.

None yet

6 participants