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 standalone), 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.
mistake #2. ALWAYS use the --prefs-add fuctionality 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 add 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.
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.
2. i wanted to delete all get_iplayer related cookies and go back to the
default set of columns
It's slightly misleading to refer to the reset values as "defaults".
They're actually whatever is in your options file, plus get_iplayer's
built-in defaults for any unset options.
3. i've made the effort to add 'excludecategory' / 'exlcudechannel'
options in my options file, i don't want to change those is 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 overriden settings
specified in the web pvr. Cookie changes aside, what this patch does do
is make settings from the options file *visible* by default in the web
pvr, which seems like a good idea. Of course, that presupposes that
there are settings to make visible. I suspect most web pvr users don't
bother with the options file. This patch would will be most useful to
people who mainly use the CLI (and options file) but use the web pvr
sometimes as well.
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_defaults{cols}), 'mode' if
$opt->{HISTORY}->{current};
does not compile in perl < 5.14, and produces a runtime 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.
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. Don't make people
download things they may not want by default. Hard-coding those options
also makes it impossible to use those settings from the options file,
which is the ostensible purpose of this patch.
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.
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.
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.
5. If you resubmit, please do a check for any added extraneous
whitespace (git diff --check) and remove it before creating the patch.
My overall take is that this patch is a solution in search of a problem.
However, it would be useful to some people, so if I get a working
version that checks out, I'll commit it.
_______________________________________________
get_iplayer mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/get_iplayer
