Thumb loader refactor #2890

Merged
merged 6 commits into from Jul 5, 2013

Conversation

Projects
None yet
5 participants
Member

arnova commented Jun 18, 2013

This is an attempt to improve the (video) thumb-loader's performance, which currently is considerably worse than it was in Eden (11.0).

Member

arnova commented Jun 18, 2013

@jmarshallnz : Let me know what you think. Perhaps there are better function names for the *Stage1/Stage2 I used?

Member

jmarshallnz commented Jun 19, 2013

One thing to look at is that CThumbLoader (from which the others are derived) should open the texture database in OnLoaderStart and close again in OnLoaderFinish (as well as opening/closing in the functions that use it, in case there are uses outside of the normal list processing). i.e. just like m_database is done for CVideoThumbLoader et. al.

This will speed up files mode quite a bit, which in my understanding is the only bit that's actually slow.

Member

arnova commented Jun 19, 2013

@jmarshallnz: Are you sure about? CTextureCache() already uses a global m_database which is initialized by CApp right from the start, see CTextureCache::Initialize(). But perhaps I misunderstand what you meant?

Member

jmarshallnz commented Jun 19, 2013

@arnova: see CThumbLoader::GetCachedImage. Notice that it doesn't use CTextureCache.

Member

arnova commented Jun 20, 2013

@jmarshallnz: Oh crap, you meant that one; I was looking at CTextureCache::GetCachedImage(). My bad :/

Member

arnova commented Jun 20, 2013

@jmarshallnz : I've updated the PR. Seems to work quite well now :-) Let me know what you think about it...

Member

jmarshallnz commented Jun 20, 2013

Could be some squashing done, but as I look at it more, I suspect a general "do everything slow" job per item might in the end be more suited. The idea being that it's easier to extend that later.

The disadvantage is that anything that requires the db is slower at that point (as you can't share db instances between jobs in case they're done in parallel) but the idea is that these are the slower parts of the process anyway, so perhaps it doesn't matter?

LoadItem() then would be LoadItemStage() as it is now, perhaps with the cache lookup portions of art discovery (if not already in there).

Member

arnova commented Jun 21, 2013

I'm fine with the slow stuff being jobs, that's why I already mentioned that before as an option :-) Slow stuff normally only runs once and after that never again, which is fine IMO. I'm not sure what you meant by your last line, could you elaborate that?

EDIT: You mean having LoadItem being LoadItemStage1() and from there setup a single job (if required) right? I think it would also make sense to get rid of the BackgroundCacheImage-job, and probably even the ThumbExtraction-job then as calling jobs from jobs, doesn't make any sense to me, right?

Member

arnova commented Jun 23, 2013

@jmarshallnz : I started working on making the stuff in Stage2 a job but this causes problems for eg. the Music Thumbloader where we have fallbacks depending on the outcome of the VideoThumbLoader. Using jobs also seems to make things overly complicated (vs. the stage1/stage2 solution).

And about my other comment it really seems like the VideoThumbLoader benefits from using a TextureDatabase member, do you still want this in the BaseClass then (which is not that straightforward since CThumbLoader::Get/SetCachedImage are static)?!

Let me know what you think about the above...

Member

jmarshallnz commented Jun 23, 2013

Regarding Get/SetCachedImage, you could just replace the 4 uses of the functions outside of the thumbloaders with non-static equivalents (just create a temporary CThumbLoader where you need it).

Regarding the splitting, that's OK - just make the first function something like LoadCached() or similar so that it's purpose is clear, rebase things up a bit nicer and it'll be ready to go for the next merge window.

Member

arnova commented Jun 30, 2013

@jmarshallnz: Updated the PR. Mind giving it another final review before we merge it in the July-window? Just want to make sure the move to the baseclass was done properly. Note that the open/closing of the texture db was intentionally left out the music thumbloader as it doesn't benefit from it...

Member

jmarshallnz commented Jul 1, 2013

The video stuff is fine, but it seems to me that you'll get benefits for program, music et. al. if FillThumb() is also moved non-static. Then your initialisation of the texture db can be done in the base class.

There's some other music stuff that can be done based on a quick look: It appears we check tags before checking cached art?

I don't have a problem with this being merged in as-is, though would prefer FillThumb() move non-static to get the benefits to the other loaders first.

Member

arnova commented Jul 2, 2013

@jmarshallnz : I have a few questions:

  1. If we move the initialisation, did you have the constructor/destructor of CThumbLoader mind? Since the new/delete is now in Onloaderinit etc.

  2. I needed to update the picture/music infoloaders anyway since this PR broke em previously (just did). They also have a proper LoadItemCached/Lookup now (please check). I had to choose where to put the ProgressCallback-update, and it ended up in Lookup, I assume this is fine? I do wonder whether we shouldn't just get rid of the progress-dialog all together as I've always found it rather annoying?

Member

jmarshallnz commented Jul 2, 2013

You'd new the CTextureDatabase in the constructor of CThumbLoader, yes. You'd open it in OnLoaderStart.

As for the progress callback, it's actually not used anywhere in the thumbloaders. It is used in the music tag and picture tag loaders where it makes more sense (they're synchronous as sorting can potentially rely on it).

Member

arnova commented Jul 2, 2013

@jmarshallnz : But we already do the initialisation of the texture-db in the base-class's constructor, so it's already ok, right (except for the additional Open/Close that should go into some of the derived classes (Picture/Program etc.)?

Also note arnova/xbmc@0171821 concerning your comment about the tag loading stuff.

Member

jmarshallnz commented Jul 2, 2013

That's fine. Note that the open/close of the texturedatabase can then be done in the baseclass OnLoaderStart (and then make sure that's called in the derived classes if they're also doing stuff - video/music only most likely)

Member

arnova commented Jul 2, 2013

Since I'm no derived-class-jedi-master (still a C-coder with "some" C++ experience ;-)), if we put stuff in the baseclass's OnLoaderStart, shouldn't the derived classed OnloaderStart be renamed in order to be called from the baseclass's OnLoaderStart? Not sure what the proper way is here...

Member

jmarshallnz commented Jul 2, 2013

class base
{
  virtual void a()
  {
    // do stuff
  }
};

class derived: public base
{
  virtual void a()
  {
    // do stuff
    base::a()
    // do stuff
  }
};

i.e. you call the base class from the derived class in the case you want to do stuff in the derived class as well as the base. If you don't have anything extra to do in a derived class (pictures, programs for example) you need not have the function defined in the derived class.

Member

arnova commented Jul 2, 2013

@jmarshallnz : Thanks for elaborating that. Really appreciate it!

Will take care of the rest this week (probably today).

arnova changed: Split background info loading into a 2 stages
This allows us to first perform loading of cached (fast) stuff and after that load the lookup (slow) stuff (partially fixes #14071)
8f3a951
Member

arnova commented Jul 2, 2013

@jmarshallnz : Updated PR. Also squashed/cleaned it and added an additional commit fixing poor performance with music thumbs in files mode.

Member

arnova commented Jul 3, 2013

@jmarshallnz : Updated PR again. Also added an additional bugfix commit. It fixes usage of the video thumbloader outside the bg info loader where it keeps extracting thumbs/flags but never stores it for such items. In my case the recentlyaddedjob triggered several of those every time I started XBMC on my machine. Not 100% sure this is the correct way we need to handle this (?)

Member

jmarshallnz commented Jul 3, 2013

The problem I presume is that the thumbloader in GUIWindowHome goes out of scope, thus cancelling it's jobs, yet the jobs are still being run? If this is the case you'd expect no art to be set on the recently added items.

There's no problem with moving the setting within the database into the job, assuming you've added it in the right spot.

Other than that, things are looking good.

Member

arnova commented Jul 3, 2013

@jmarshallnz : I'll move it then. But do we still want to keep CGUIWindowVideoBase::OnStreamDetails then, since it's only used for details db storing? Probably a remnant of our pre-jobmanager era?

Member

jmarshallnz commented Jul 3, 2013

It may be best to get @Montellese involved on that one.

Member

arnova commented Jul 3, 2013

@jmarshallnz / @Montellese : Please check arnova/xbmc@b071674 concerning the StreamDetails observer stuff etc.

Owner

Montellese commented Jul 4, 2013

Not sure what the use of the IStreamDetailsObserver is/was.

Member

jmarshallnz commented Jul 4, 2013

Push it in in that case.

arnova added some commits Jun 20, 2013

arnova changed: Renamed m_database to m_video/m_musicDatabase for clarity 7ea5521
arnova changed: Use a member variable for the texture database
This speeds up thumb loading considerably. This also gets rid of the Initialize/Deinitialize ThumbLoader functions
59c6386
arnova fixed: Music thumb loading for files view was slow
This was caused by the fact that we never checked for cached images for music items
428b580
arnova fixed: Thumb/flag extractions were never stored when used outside the…
… bg infoloader

Also factored out some (obsolote?) code as a bonus
487cd9f

@arnova arnova added a commit that referenced this pull request Jul 5, 2013

@arnova arnova Merge pull request #2890 from arnova/thumb_loader_refactor
Thumb loader refactor
2b4209e

@arnova arnova merged commit 2b4209e into xbmc:master Jul 5, 2013

Member

wsoltys commented on 9752e49 Jul 6, 2013

@arnova : this broke tag reading on audio files for me. Whenever I enter a directory with audio files (mixed formats) the window "scanning media info 100%" pops up and nothing happened anymore. The mouse still reacts until I press cancel. Then XBMC hangs totally.
I noticed that we never leave the loop "while (m_musicInfoLoader.IsLoading())" in GUIWindowMusicBase.cpp l1072. Isloading is still true even though the BackgroundLoader thread already ended.
Another message I noticed but it might be irrelevant is "Control 50 in window 10501 has been asked to focus, but it can't".
Please turn on music tag reading and try your changes again.

Member

arnova replied Jul 7, 2013

@wsoltys : I think I understand why that happens. Will fix it later today. Thanks for the heads up.

Member

arnova replied Jul 7, 2013

@wsoltys : Fixed in 94817e6

Member

wsoltys replied Jul 7, 2013

@arnova : yup works now. Thanks.

stefansaraev referenced this pull request in OpenELEC/OpenELEC.tv Jul 13, 2013

Closed

Pictures - thumbnail generation f.u. since 3.1.2 #2461

Contributor

ulion commented Jul 23, 2013

this introduce the multi-thread conflict here: http://trac.xbmc.org/ticket/14500

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment