Skip to content

pvr: only update channel IconPath if the comparison channel IconPath is ... #1860

Closed
wants to merge 1 commit into from

3 participants

@nemphys
nemphys commented Nov 29, 2012

...not empty (extra check).

This avoids persisting unchanged channels that have their icons set automatically in XBMC but do not have an icon set in the backend.
Speeds up initial channel loading time, since only actually changed channels are persisted (compared to the current behavior which is all channels with an icon + changed channels)

@nemphys nemphys pvr: only update channel IconPath if the comparison channel IconPath …
…is not empty (extra check).

This avoids persisting unchanged channels that have their icons set automatically in XBMC but do not have an icon set in the backend.
Speeds up initial channel loading time, since only *actually* changed channels are persisted (compared to the current behavior which is all channels with an icon + changed channels)
0ef1b2c
@mikrohard

But this means that the user can not remove the icon through the backend anymore?

@nemphys
nemphys commented Nov 30, 2012

Right, must think of a better way.

@nemphys
nemphys commented Nov 30, 2012

Just took a look at the code; what if we just changed SetChannelIconPath method of CPVRChannelGroup to call channel->SetIconPath(strIconPath, true) (instead of channel->SetIconPath(strIconPath)), so that the m_bIsUserSetIcon bool is set to true?
This would cause the if in this case to return false and accomplish the same result.
After all, if the user selects to scan the icons directory for icons, we can safely assume that the channel has a "user set" icon, right?
Please advise, so that I can update the PR

@mikrohard

When I implemented this I always thought that every icon set by xbmc (and not the backend) should be marked as "user set". But @opdenkamp didn't want the automatically set icons to be marked as such. Maybe the bIsUserSetIcon should be replaced by iIconSourceType (ICON_SOURCE_BACKEND, ICON_SOURCE_USER, ICON_SOURCE_XBMC, ...)?

@nemphys
nemphys commented Nov 30, 2012

That sounds reasonable and should do the trick properly if we exclude eg. ICON_SOURCE_USER, ICON_SOURCE_XBMC from the check.
@opdenkamp please comment so that this is resolved.

@nemphys
nemphys commented Dec 1, 2012

Actually, now that I inspected deeper, maybe @opdenkamp wanted it like this because he has the icon scanning routine running every time a channel group is loaded (which is quite slow and increases loading time a lot; will make a PR for an advanced setting for that).
My opinion is that all this behavior should be changed to work according to @mikrohard 's suggestion.
Please comment so that I can start working on it.

@opdenkamp
Team Kodi member

the channel icon should be refreshed (either from the backend or from the fs) unless the user manually selects an icon. scanning for icons on the filesystem doesn't take that long at all.

@nemphys
nemphys commented Dec 1, 2012

As I understand it, the current behavior is:
1) Load all channels (with previously auto-populated icons) from the DB.
2) Get channels from backend and update the ones loaded from the DB, which considers all channels with icons from the previous step as changed (since backend does not send any icons in my case) and persist them in the DB.
3) Scan once again for channel icons auto-population and, if found, persist them in the DB.
All the above is executed every time we update channels from the backend.
In my opinion, this is far from optimal and causes big unneeded delays on every startup.
Why not just consider auto-populated icons as user set (since the user chose to give the path to scan) and get over with it? Is there some specific case where this could cause a problem?

As you can imagine, I am trying to find ways to speed up the initial loading time (hence PR #1870, too; take a look at it when you can), since it has become really slow lately (used to be much, much faster a few months ago).

As for the icon scan time, don't forget that I have around 2000 channels and the path for the icons is an SMB share.
Also, I just got myself a Raspberry Pi, which I plan to use for a secondary bedroom XBMC host and it is too damn slow at the moment :-)

@nemphys
nemphys commented Dec 1, 2012

PS. in my setup, the icons scan is taking a whooping 10 seconds and since it is executed twice (see PR #1870), it delays pvr startup by 20 seconds. Isn't it too much?
I have temporarily disabled it (after internal group load) when building from source, but I cannot do that in the Pi installation (where it takes much longer than 10x2 seconds).

@nemphys
nemphys commented Dec 2, 2012

Any further comments on this one?

@opdenkamp
Team Kodi member

patience please, i got more to do than reviewing PRs :)

@opdenkamp
Team Kodi member

hmm so smb spams the server with queries. awesome. it would have been nicer if it would get the dir listing and cache that.

the reason for the scan is that users might decide to delete a jpeg icon and replace it by a png (or some other ext).

@nemphys
nemphys commented Dec 7, 2012

My suggestion is that we set the auto-scanned channel icons to be "user set", so that we avoid needless persisting of channels (as I stated above), or at least follow mikrohard's opinion to implement triple icon state to overcome this.
As for the actual icon scan on every internal group load, I think we should at least have a setting for this (and maybe not an advanced one), eg. "Automatically scan for missing channel icons", so that the functionality can be disabled easily on slow setups.
Please advise, I can send PRs for both

@opdenkamp
Team Kodi member

new advancedsetting for this (and enabled by default) will be fine. it doesn't justify a new guisetting imo.

it's too late to change this into a triple state now, with RC1 only days away. in frodo+1, we need to make this code a bit smarter, marking channels as dirty when things change and persist afterwards, and not change the icon twice, or at least detect that we changed it back to what it was and then decide that it doesn't need to be persisted.

@nemphys
nemphys commented Dec 7, 2012

OK, will send a PR for the auto scan on load later today if I find the time.
Don't you think we should at least do the same (some advanced setting maybe? eg. usersetautoscanicons) for the channel mark and implement it later as you say? It will be just a few lines of extra code but will do the trick for now.
It's a shame, because right now the channels take too much time to load during startup (another thing is that the progress controls for pvr and epg are painted one on top of the other and it looks ugly).
I will also take a look at why it takes so much time to fetch the channels from tvheadend (LoadFromClients). I am quite certain that it used to be much faster in the past.

PS. @opdenkamp: how many channels do you have in your setup? It's impossible to see all these delays if you have only eg. 20-50 channels...

@opdenkamp
Team Kodi member

if it's only a couple of lines changed, sure, put up the PR and i'll have a look at it.

for now, my primary concern is that it works for all users with minimal setup efforts, and if it's a bit slow (but works properly), then that's too bad, but that will have to do for frodo.

i got a few hundred channels, not thousands as you have, but that's not what's causing the delay. it's the double change in icons and smb share that you're using for icons that's causing this as far as i can see.

@nemphys
nemphys commented Dec 7, 2012

OK, will send 2 PRs.
As for the delay, I am positive it is the fetching of the channels, as I have already merged the 2 changes I will send PRs for in my local branch and I can still see a delay in my main machine (not the Pi), which has everything locally.
I even put some debug logs for testing and there is a 10 seconds delay just for LoadFromClients.
Will investigate further and report.

@nemphys
nemphys commented Dec 7, 2012

First PR 1901 is up, please check.

@opdenkamp
Team Kodi member

merged #1901

@nemphys
nemphys commented Dec 11, 2012

Thanx, will (hopefully) send the 2nd PR tonight, as my internet connection was not working during the past few days

@nemphys
nemphys commented Dec 12, 2012

PR #1923 is up, please check

@opdenkamp
Team Kodi member

merged. closing this one

@opdenkamp opdenkamp closed this Dec 13, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.