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

Local icons - Cache channel icons #174

Closed
wants to merge 17 commits into from

Conversation

andyb2000
Copy link
Contributor

This PR adds the ability to cache channel icons locally. Add icons in as normal and enable it through general config tab (Also requires TVH ip to be added in).
TVH will then pull all icons down and store them in the ~/.hts/tvheadend/icons/ folder.
When an htsp client requests a channel icon it will query TVH via the virtual url /logo/logoname.jpg, the iconserve.c routine will then check if the icon is cached, if it is it will be served via a redirect to the virtual url /iconserve/logoname.jpg
If the icon doesn't exist it will try to download it to the cache, if that fails it will simply redirect the user to the real logo url (non-caching).

added in loader function, needs work
existance of icons, ready to grab icons if required
Added in code to the htsp icon retrieval to refer to the TVH
webserver.
tidy of code and admin on/off master switch for icon
cache
Conflicts:
	src/htsp_server.c
	src/webui/extjs.c
	src/webui/webui.c
redirect to /iconserve/ so I've removed this and serve
the icon binary directly from the /logo/ url
@john-tornblom
Copy link
Contributor

Giving anonymous access to files located on the disk can be dangerous. There are a few tricks that can be used in order to gain access to sensitive information.

For example, "../../accesscontrol/1" , or if you got limited access to the filesystem, symlinking to /etc/shadow. They might not be exploitable but we should be cautious.

There is support for issuing tickets to resources that can be used without password access. The tickets are only valid for 5 minutes but perhaps that is enough?

@adamsutton
Copy link
Contributor

@john-tornblom We should actually fix that in the TVH webserver ;)

However this is a good point and having not read the PR code I think I might need to suggest an alternative. But I'll leave my comments until AFTER I've read the code :)

Adam

if(ch->ch_icon != NULL)
htsmsg_add_str(out, "channelIcon", ch->ch_icon);

if(ch->ch_icon != NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think maybe this can be simplified, I'm tempted to say that we make this HTSP version dependant. So for HTSPv7+ we simply will return
channelIcon: /channellogo/CHANID
or something like that.

Alternatively if we need to return a fully qualified URL, then assume that the IP being used for the HTSP connection is the same as the HTTP?

@adamsutton
Copy link
Contributor

@andyb2000 Ok, I've started reading the code and I think it needs simplifying.

Here's what I would do:

  1. All channel logo returns (and references in the the UI) should be re-written to be something like /channellogo/CHANID.
  2. You then need a handler within the web server code that will handle ALL /channellogo/* pages and extract the CHANID
  3. Lookup the channel. if not found return server error.
  4. No lookup in the cache whether the channel logo has been cached, need to define how this is done. But it should explicitly list the orig URL (to ensure its not been changed) and the local file (this might be implicit, such as by using a hash of the original URL?).
  5. If logo is in the cache return it, if its not, return the original URL.

OK that deals with the serving of logos whether cached or not, next...

  1. Create a thread that will continually process a list of remote icons that should be fetched and cached.
  2. When user changes the configuration (adds icon) it should insert in the above list (Note: possibly need to remove old cached icon? - beware if you're sharing icons need to handle this - ideally don't want to have multiple local copies but it wouldn't be the end of the world).
  3. When the thread fetches a logo it should:
    • store the file
    • update any info to say the cached file exists (if required)
    • inform htsp of a channel logo change (so it now uses the cached version).

OK, think that deals with caching, might be a bit more to that.

Some things that need to be considered:

  • what happens if icons are added (uncached) and then user enables caching. Possibly in this case you need to scan all channels to check if icons currently not cached and add to list.
  • Caching enable/disable only needs to affect the adding of new icons to the fetch thread list (you could stop/start the thread, but not really necessary since if nothing is added to the list the thread should be dormant anyway).

Adam

/channellogo/CHANID implemented and returns the logo from cache
or from original URL
@adamsutton
Copy link
Contributor

For reference: http://pastebin.com/eRQUxEuw

@andyb2000
Copy link
Contributor Author

@adamsutton , when you get a sec can you take a look, have incorporated all the discussions, threading, etc, and running on my local stable.

@@ -32,7 +32,7 @@ CFLAGS += -Wmissing-prototypes -fms-extensions
CFLAGS += -g -funsigned-char -O2
CFLAGS += -D_FILE_OFFSET_BITS=64
CFLAGS += -I${BUILDDIR} -I${CURDIR}/src -I${CURDIR}
LDFLAGS += -lrt -ldl -lpthread -lm
LDFLAGS += -lrt -ldl -lpthread -lm -lcurl
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll probably need to make this optional somehow. I imagine not all users will want to link against curl (thinking embedded stuff here). But we'll see.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, probably via configure option, default to on perhaps?

@@ -853,7 +854,7 @@
htsmsg_add_str(m, "channel", ch->ch_name);
htsmsg_add_u32(m, "channelid", ch->ch_id);
if(ch->ch_icon != NULL)
htsmsg_add_str(m, "chicon", ch->ch_icon);
htsmsg_add_str(m, "chicon", logo_query(ch->ch_id, ch->ch_icon));
Copy link
Contributor

Choose a reason for hiding this comment

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

See comment about channel_icon()

@adamsutton
Copy link
Contributor

@andyb2000 I've added some fairly general comments, I need to review the guts of the code but I've timed out and didn't want you to think I'd forgotten it. I'll try and take a look later this week.

But on first glance I think some mods will possibly be necessary to the main processing and storage, but I need to read it through properly before commenting.

@andyb2000
Copy link
Contributor Author

@adamsutton no worries, thanks for the feedback so far, its working a dream on my local setup, but yes I agree the url part in the htsp message would be nice as it'll save that annoying ip request box. Will take your lead on this so catch up with you at some point

@adamsutton
Copy link
Contributor

@andyb2000 I've started working on integrating this. Although I've decided to rewrite most of it as I had some ideas to better generalise its usage and to cover some other use cases that have been requested in the past.

It's mostly working (apart from HTSP and config) but I will try and tidy that up tonight.

@andyb2000
Copy link
Contributor Author

No worries, thanks for the update, anything I can help with let me know.

:-) I take it you do as little on new year as I do!

Adam Sutton notifications@github.com wrote:

@andyb2000 I've started working on integrating this. Although I've
decided to rewrite most of it as I had some ideas to better generalise
its usage and to cover some other use cases that have been requested in
the past.

It's mostly working (apart from HTSP and config) but I will try and
tidy that up tonight.


Reply to this email directly or view it on GitHub:
#174 (comment)

Sent from my Android phone with K-9 Mail. Please excuse my brevity.

@adamsutton
Copy link
Contributor

@andyb2000 this has now been merged. I merged your original code (squashed) and then overlayed my own.

Adam

@adamsutton adamsutton closed this Jan 1, 2013
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

3 participants