Pete Beardmore pete.beardmore at msn.com
Mon May 13 02:23:56 EDT 2013


this never made past the list moderation stage due to its size. the  
patch set will follow


----- Forwarded message from Pete Beardmore <pete.beardmore at msn.com> -----
    Date: Mon, 15 Apr 2013 20:19:48 +0100
    From: Pete Beardmore <pete.beardmore at msn.com>
Subject: Re: web-settings-and-get_iplayer-defaults-harmonisation
      To: get_iplayer at lists.infradead.org


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  

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

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  

> 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:
1. fixed disabled checkboxes
1. fixed javascript for perl < 5.8.5
1. 'fix' erroneous error message introduced by  
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
1. re-factor initialisation code to improve readability, and merge  
remaining command line options into single defaults hash
1. add 'hideoptions' functionality

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:

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

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

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

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

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


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

----- End forwarded message -----

More information about the get_iplayer mailing list