web-settings-and-get_iplayer-defaults-harmonisation
Pete Beardmore
pete.beardmore at msn.com
Mon Apr 15 15:20:00 EDT 2013
hi
i finally got this done, and hopefully i'll fare a little better than
last time. thank you very much for such a thorough response months
back..
On 21/02/2013 15:39, dinkypumpkin wrote:
> On 30/01/2013 22:39, Pete Beardmore wrote:
>> #patch attached (hopefully!)
>
> I reviewed this patch when it was submitted, but found some minor
> problems and put it aside in my "good intentions to follow up"
> folder. Jon's post prompted me to dig it out again.
>
> First, some responses to the original post:
>
>> having tried combinations of 'flashhd,flashhd1,flashhd2' in option files
>> and in the webserver to no avail, i settled on 'best' (having finally
>> resorted to reading instructions!). this was set in my options files
>> under /srv/www/iplayer/.get_iplayer but still, via the web server (be it
>> my apache or the included stand-alone), i could not get the underlying
>> flvstreamer/rtmpdump processes to behave and show me that attempts at
>> flashhd1 etc. quality streams were being made.
>
> Any option set in the web pvr (and modes is set by default) will
> override the setting in the options file, just as command line
> options do. In the absence of any information about what you were
> trying to do, I suspect that could have been the problem. If you're
> having troubles like this, post your output here and we'll try to
> help.
>
>> so mistake #1. although the instruction state that one should ensure
>> HOME is set correctly, don't assume that that is for reading any options
>> files which you might have created there
>> (/srv/www/iplayer/get_iplayer/options) whilst trying out the
>> 'get_iplayer' script. the web server doesn't read them!
>
> That is simply wrong. Don't extrapolate from a single data point to
> assert it doesn't work in general. Besides, your patch would be
> useless for CGI deployments if the options file isn't read. Without
> knowing how you configured your server I can only guess, but it
> appears you were missing a ".". Normally, the path should be
> ".get_iplayer/options" inside the web server's HOME.
the 'get_iplayer' vs '.get_iplayer' was a mail typo, sorry about that,
but good spot. as for 'useless', i don't follow. given my patch
explicitly read the options file (passing options as overrides to the
underlying get_iplayer script), why would this be useless for CGI
deployment if (as i had originally thought), the options file wasn't
read by get_iplayer?
anyway, given the above response i went back to do testing from scratch:
-installed v2.82 under new dir getiplayer for apache, ensured HOME
variable was set
<Directory /srv/www/getiplayer>
SetEnv HOME /srv/www/getiplayer/
SetHandler cgi-script
Options ExecCGI FollowSymLinks
AllowOverride None
Order deny,allow
Allow from all
</Directory>
test1 : record hd program 1
result1 : records flashhigh, as per the visible web-pvr setting
test2: set web pvr 'modes' to "" and apply settings. record hd program 2
result2: records flashhigh, as per ..the (now invisible) default. some
default is needed, but it would be good to show it still. my patch
didn't deal with that either though
test3. run cmd 'HOME=/srv/www/getiplayer ./get_iplayer --prefs-add
--modes "flashhd,flashvhigh,flashaachigh"'. record hd program 3
result3. records flashhd. #STOP PRESS# here i expected it to record in
flashhigh as i'd seen previously but no, so my original tests were
certainly flawed in some way. i was absolutely wrong in stating that
the options file wasn't read, apologies and thank you for the correction
>> mistake #2. ALWAYS use the --prefs-add functionality to add options to
>> the options file to ensure you get the right format! for example, the
>> entry..
>>
>> modes 'best,flashhd,flashhd1,flashhd2,flashvhigh'
>>
>> ..f@&Sed me over as i'd added quotes!
>
> Again, we're here to help with these issues. In most cases there is
> no reason web pvr users should be expected to use --prefs-add from
> the command line. It kind of defeats the purpose of using the web
> pvr.
i was meaning anybody who uses the command line yes. by then i'd made
up my mind that setting the mode in the webpvr didn't work, and i
moved on to the options file hoping for better luck. looking back,
it's possible that i was just ignorant to the role cache played here.
regardless of what webpvr mode settings i'd set, if get_iplayer was
actually just resuming an initial test stream (started in a non-hd
format) each time then it would have looked like changing the modes
settings wasn't working. the above tests took that element out of the
equation by using different streams each time
but for command line use i'd stand by that suggestion. if i'd used
--prefs-add in the first instance instead of piecing together the
options file manually, it appears from the above tests that that would
have all worked swimmingly too and i would not have felt it necessary
to dig into any code. i'm left thinking that my initial testing was
performed with combinations of over-quoted comma-delimited lists of
modes in the option file, which meant that for every test i did, i was
left with the impression that the file was not being read. in fact it
WAS being read, but get_iplayer failed to parse/use my erroneous
manual additions
>> 1. 20+ empty default is bad, particularly when i have to scroll through
>> them all to find non get_iplayer related cookies on my (apache) server
>
> Bad? Its a trivial amount of data that very few users will ever
> look at. There's no harm in whittling it down though.
i really should use virtual domains but i currently have all my web
applications setup under subdirs of the same domain and therefore all
cookies are pooled together. there are a lot of them! in testing i
needed to clear get_iplayer cookies in order to switch between
settings mechanisms (cookies vs option files) and as there is no
prefix used for the get_iplayer cookie names, they were very
inconveniently mixed between all the other cookies. picking through
the list to remove them all one by one was very bad for my health.
very specific to my setup though
>> 3. i've made the effort to add 'excludecategory' / 'excludechannel'
>> options in my options file, i don't want to change those in multiple
>> places. and if i've set something as important as 'modes' for use with
>> 'get_iplayer' then i really want that same setting used in the web front end
>
> For any web pvr users reading this: This patch doesn't make any
> functional change in how the options file is used with the web pvr.
> The settings in the options file are always used by get_iplayer,
> even when it is invoked through the web pvr, subject to any
> overridden settings specified in the web pvr.
an example of where this may confuse, exposing the problem of 'no
visibility on the ultimate settings in use' is:
starting a fresh web-pvr session, it lists 1000 programmes. i ADD a
filter term to the empty exclude category and apply settings. i now
have 1500 programmes listed. why? a dozen filter terms in an options
file is why - the list was initially heavily filtered by 'invisible'
defaults. similar confusion can occur going the other way. sadly,
'it's broken' was my first impression ..because i couldn't possibly be
using it incorrectly right ;)
> Now to specifics:
>
> 1. This bit:
>
> @@ -3362,10 +3364,10 @@ sub get_display_cols {
> my %cols_status;
>
> # Add some default headings for history mode
> - push @headings_default, 'mode' if $opt->{HISTORY}->{current};
> + push split(/,/, $opt_default{cols}), 'mode' if $opt->{HISTORY}->{current};
>
> does not compile in perl < 5.14, and produces a run-time error in
> all versions. I think the bug unfortunately slips through
> compilation with perl >= 5.14 because of the implicit array
> dereferencing added in perl 5.14. get_iplayer should be able to
> maintain perl 5.8 compatibility, though I suspect there aren't a
> whole lot of users with perl < 5.10.
>
> Of more concern is this means you didn't test your patch with the
> Windows distribution of get_iplayer, based on perl 5.12 at present.
> That's a no-no for changes to the web pvr. If list traffic is any
> gauge, Windows is where the most users favour it. Not every Windows
> user employs the provided installer, but I expect most do. If you
> can't or won't test on Windows, let us know up front.
fixed. i very painfully built and installed perl-5.8.0 but with no
confidence that my hacking of module makefile.pl files to override
minimum requirements didn't compromise testing in some way, i less
painfully built and installed 5.8.1. with this i tested the revised
patches with no issue ..of my own doing
..perl versions pre 5.8.5 contain a CGI::hidden() which drops all non
'standard' hidden element tags, including 'id'. the javascript of the
cgi references hidden navigation tab options by id, fails and leaves
tabs in a permanently collapsed state. i've attached another patch
which fixes this, thus restoring web-pvr compatibility back to at
least 5.8.1
on this topic, is 10 years for backwards compatibility not a little
excessive? i ask sincerely as i have no idea on what's common in any
language
> 2. You shouldn't be hard-coding default values for metadata,
> subtitles, and thumbnail. Any non-default setting (and by that I
> mean built-in defaults) should be a positive choice by the user.
reverted. there is no concept of 'nosubtitles' as an option so this
was a bad mistake
> I recognise that the "modes" option requires a hard-coded default
> because of how the web pvr works. However, making "best" the
> default value is a bad idea in my book. Nobody - especially a new
> user - is going to thank you for eating their time and bandwidth
> with HD downloads they didn't ask for.
reverted. i disagree with the opinion here, but it's not my place to
change it without discussion(s). discussion that i don't want to have
> 3. You may not want to expose the configured value for your output
> path in the web pvr. I'm thinking of the (admittedly rare) case
> where you have a CGI deployment of the web pvr with any sort of
> public exposure.
i've added a patch for this through use of an additional 'hideoptions'
option, set via command line or an options file (details further down
as this work came last)
> 4. It would be a nice-to-have feature if the reset function actually
> invoked get_iplayer to re-read the options file. That way you don't
> have to restart the web pvr server to make changes to it.
> Admittedly, it's not something that would be used often, but it
> seems like a logical extension of the functionality.
'fixed'. that was of course the way i'd envisaged it working, however,
i hadn't given enough thought to the differences in the way the
different deployment mechanisms worked (see notes further down)
> My overall take is that this patch is a solution in search of a problem.
i understand where you're coming from here as a seasoned user,
however, i imagine my ignorance to the inner workings of this software
would be common to all new users. as alluded to above, visibility in
the webpvr on the ultimate settings used by the get_iplayer script, be
them explicitly viewable in the webpvr, hidden
'get_iplayer.cgi/get_iplayer' file defaults or option files settings,
is a must in my opinion! although it may have been pitched in the
wrong way (making incorrect assertions), the original patch at least
fixed this lack of visibility (failure in standalone deployment
aside), therefore i believe it did have purpose. and i certainly
didn't go looking for problems, they found me :)
the attached patch set contains the following modifications:
patch1
1. fixed disabled checkboxes
patch2
1. fixed javascript for perl < 5.8.5
patch3
1. 'fix' erroneous error message introduced by
4ebcbed25a88b395b12a088b2830a64eb8ef225f
patch4
1. ensure 'default' (hard-coded < option files < cmd line < webpvr
state/cookie) options are always visible
2. only persist webpvr settings as cookies when non-default
3. add functionality to return webpvr settings to defaults (hard-coded
< option files < cmd line)
4. ensure 'modes' is never displayed as empty "". reset to default
5. re-read option files for both deployment methods
6. fix selected display tab on reset
7. harmonise options use through single opt_default array
8. allow pvr 'holdoff' to option be set by options file
patch5
1. re-factor initialisation code to improve readability, and merge
remaining command line options into single defaults hash
patch6
1. add 'hideoptions' functionality
misc
X. reverted default changes
X. fixed display cols code
X. perl >=5.8.0 'compatible'
X. windows tested
X. extraneous white-space removed ..my double spaces replaced by tabs
too. thanks for the git diff --check tip!
#########
notes on the changes:
#patch2
my fix above only restored the gui tabs to a working state for
5.8.0/1. ensuring that the deployment was accessible was all that i
initially tested for having fixed the run-time error you mention
above. 11th hour testing revealed that there is something causing 404s
for basic searching / page navigation for those old versions, and the
official v2.8.2 release fails in this regard too ..so it's not my
doing! i don't have the time to look into this though sorry
#patch3
4ebcbed25a88b395b12a088b2830a64eb8ef225f 'Web PVR: fixed auto-refresh
for Run PVR and Refresh Cache' introduced the following output
messages regularly enough to make me think that i'd broken something,
and to go looking for a fix:
ERROR: Command failed to execute: No child processes
INFO: Command exit code 16777215
this occurs in 'get_cmd_output' when the waitpid call is given a
process id that doesn't exist. even if the process has completed prior
to the waitpid call, this shouldn't be an issue, but it appears that
whatever zombie process should remain on my system, is possibly being
'cleaned-up' prematurely. the attached patch is a hack that ignores
the status code in this situation, when it appears the process did
indeed succeed. i left the debug statements there for somebody else to
take a look too
#patch4
for external web server deployment (apache here) the options file was
actually re-read far more frequently than i'd initially thought. i
hadn't really grasped that the entire script was re-read on each form
submission. this re-read functionality didn't work at all for the
built-in server as you rightly pointed out, the options file was only
read once when the server initialised
so revisiting the work, i wanted the implementation to be consistent
for both stand-alone and built-in server deployment methods. i
considered the following two modes of operation:
mode1: rebuild %opt_default on every request
+changes to important options can be reflected immediately
+less reliance on QUERY_STRING
-more file access overhead
mode2: rebuild %opt_default only on RESET request
+less file access overhead
-changes to important options can only be reflected by user
-stand-alone mode must rely on QUERY_STRING to rebuild the
%opt_default each time
-no possibility of truly hiding options
maintaining state with forked processes and not wanting to use the
insecure query string would require bespoke IPC work, or conversion to
use of threads, which i'm probably not cut out to do. so given the
above +/- i opted to make the built-in deployment mode do the same as
the external stand-alone mode, and as such option file changes are
reflected in the %opt_default hash on every form submission / refresh.
updated defaults lurk in the background until the 'reset to defaults'
button is clicked. this pushes them into the current state which is
then reflected in the front-end
an exception to this behaviour is for any variable setup with the
'empty => no' opt key value (only 'modes' currently). if an attempt is
made to set that variable to an empty string, its value is reset to
the default. this could however result in the 'modes' value being set
to an updated default value BEFORE 'reset to defaults' has been
clicked which is not ideal. it should just reset back to its previous
value, but by then that state value has been discarded. a corner case
i didn't want to spend any longer on though
i've fixed the side-effect whereby my reset also reset the selected
navigation tab, which undesirably gave the impression that the whole
page had been reloaded. in looking at this issue i also noted another
quirk (not of my doing). selecting a search tab (columns, recordings
etc.) and hitting 'apply changes' effectively stores that tab as a
default. navigating to an alternative tab and refreshing the page
results in a switch back to whatever that saved tab was
#patch5
i ended up merging all the command-line options into the opt_default
hash, primarily due to the nature of the ffmpeg option which could be
either statically set via the command line, or dynamically updated via
the option files (with the former always taking precedence). i wanted
to ensure a consistent approach to using the options, and feel the
complete merge was warranted. it has made options slightly easier to
use throughout the script too as there is now one less place to get
them from. i split this patch out of the previous patch in an attempt
to make the real functionality changes (patch4) easier to scrutinise
#patch6
i haven't found a way to leak the information i'm trying to hide with
this implementation, but others may succeed! so for example, setting
new option 'hideoptions' as 'output,proxy' (don't include the
quotes!!! :) ) ensures the following for 'output' and 'proxy' options:
1. form element is disabled and always contain a dummy string
2. QUERY_STRING doesn't contains the hidden value
3. values are not persisted as cookies
to ensure state is maintained, the query string now contains a
'hideoptions' value. change to the (now 'dynamic') options file
'hideoptions' value will force any ADDITIONAL options in this value to
be pushed immediately into the current state, and thus reflected on
any further user interaction. having 'hideoptions' in the query string
would be potentially exploitable (successfully injecting an empty
hideoptions value would expose the data i believe), thus the content
of the current hideoptions is, as a minimum, the content of the
default' value
i allow the cgi state's 'hideoptions' value to diverge from this
default hideoptions value (in the %opt_default hash) when options have
been removed from the default (they are not removed from the state
value) as i want (where possible) for the user to explicitly invoke
the 'reset' to see changes
i originally implemented this with the possibility of allowing the
user to set custom values for these hideoptions, however, this opens
the vulnerability whereby the hidden values can be guessed at, with
correctness asserted by the value changing from the custom user value
to the dummy string ('-----') if the guess is correct. so i ditched
that idea
hideoptions is only implemented for the string options too (think i
got them all)
######
finally. i really don't know how much of get_iplayer's functionality
works, so how complete can my testing ever be? with your prompts
though, i can say that everything i deliberately changed appears to
work as i describe above, for both deployment types, both 'nix and
doze and perls 5.8.1/16
cheers,
Pete.
ps. '$icons_base_url' was removed from the cgi initialisation code as
it wasn't being used anywhere
pps. i'll issue a pull request on github too, might make it easier
ppps. i'll be lurking on freenode #get_iplayer for a while in case if
anyone (dinky) needs me to answer lots of questions
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-fix-checkboxes.-deselect-selected-instead-of-disabling.patch
Type: text/x-diff
Size: 2384 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/get_iplayer/attachments/20130415/4a58b47c/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-fix-javascript-for-hidden-elements-created-by-perl-_5.8.5.patch
Type: text/x-diff
Size: 1034 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/get_iplayer/attachments/20130415/4a58b47c/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-web-pvr-dont-process-return-status-from-non-existant-zombies.patch
Type: text/x-diff
Size: 1632 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/get_iplayer/attachments/20130415/4a58b47c/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-web-settings-and-get_iplayer-defaults-harmonisation.patch
Type: text/x-diff
Size: 28542 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/get_iplayer/attachments/20130415/4a58b47c/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-web-pvr-initialisation-and-options-code-refactoring.patch
Type: text/x-diff
Size: 24964 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/get_iplayer/attachments/20130415/4a58b47c/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-web-pvr-add-hideoptions-parameter.patch
Type: text/x-diff
Size: 9095 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/get_iplayer/attachments/20130415/4a58b47c/attachment-0011.bin>
More information about the get_iplayer
mailing list