Bug 11330 - SqueezeSlave playertype bandwidth limiting bug fix and new features support
: SqueezeSlave playertype bandwidth limiting bug fix and new features support
Status: RESOLVED FIXED
Product: Logitech Media Server
Classification: Unclassified
Component: Misc
: 7.4.0
: All All
: -- normal (vote)
: Future
Assigned To: Unassigned bug - please assign me!
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-12 04:45 UTC by Ralph Irving
Modified: 2009-06-11 04:43 UTC (History)
4 users (show)

See Also:
Category: ---


Attachments
Updates SqueezeSlave.pm, Slimproto.pm Creates Text16VFD.pm, Text16.pm (64.46 KB, patch)
2009-03-12 04:45 UTC, Ralph Irving
Details | Diff
Updates SqueezeSlave.pm, Slimproto.pm, TextVFD.pm and Text.pm (21.18 KB, patch)
2009-03-13 09:15 UTC, Ralph Irving
Details | Diff
Fix dBToFixed calls and Can't call method hasPowerControl SC log error (749 bytes, patch)
2009-05-14 15:34 UTC, Ralph Irving
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ralph Irving 2009-03-12 04:45:58 UTC
Created attachment 4918 [details]
Updates SqueezeSlave.pm, Slimproto.pm Creates Text16VFD.pm, Text16.pm

Set maxBitrate preference to 0, instead of undefined to fix the issue of band width limiting being enabled by default for squeezeslave.  This is a problem for users who do not have lame installed as the band width limiting option is not available to change in the WebUI player settings until lame is installed and SC has been restarted.

Updated hasIR function to reflect Squeezeslave new IR feature.

Added canDoReplayGain gain function to SqueezeSlave.pm to support replay gain.

Added support for variable sized displays for use with console and lcd displays.
Comment 1 Michael Herger 2009-03-12 05:19:57 UTC
Please correct me if I'm wrong, I only had a brief look at the patch. But couldn't the Text16 classes inherit from Text instead of copying the full files with only a few changes applied?
Comment 2 Ralph Irving 2009-03-12 06:28:16 UTC
You are correct of course.  I didn't write those modules they were
provided by another squeezeslave user.  He suggested that the new modules
could be used as drop in replacements for the standard Text modules.

What's your suggestion on how to proceed?
Comment 3 Michael Herger 2009-03-12 06:41:51 UTC
If these should be _replacements_, then a patch to the existing file would do.

If they're to be _added_, then they should be refactored to inherit from the base Text classes and only overwrite what's needed to be changed.
Comment 4 Ralph Irving 2009-03-13 09:15:57 UTC
Created attachment 4924 [details]
Updates SqueezeSlave.pm, Slimproto.pm, TextVFD.pm and Text.pm

I've respun the patch to support variable width displays using Text.pm and TextVFD.pm removing the hard coded limit of 40 characters.  This patch applies the updates to SqueezeSlave.pm and Slimproto.pm as well.
Comment 5 Ralph Irving 2009-04-02 06:11:25 UTC
Michael,

Any chance you'll get some time to review my patch and include in 7.4?
Comment 6 Michael Herger 2009-04-02 06:24:24 UTC
Adrian/Andy - you know that part of the code much better than I do. Any opinion on this patch?
Comment 7 Andy Grundman 2009-04-02 06:28:56 UTC
I don't really know much about the text display modules.  Has this been tested with slimp3 and SB1?
Comment 8 Ralph Irving 2009-04-02 09:39:49 UTC
No, I don't have either unit to test with.

The patch uses the default 40 character display width if no size is specified, so hopefully, it's transparent.
Comment 9 Adrian Smith 2009-04-02 10:59:25 UTC
I'm happy to look at the text bit, but could I ask for details of the application?

If you are looking to get a display for squeezeslave, would a better method be to use the displaystatus updates which already exist in the server.  You can register over cli or json to receive display updates and will get the a bitmap from the graphics display which could then be rendered by squeezeslave, it allows setting the width too for graphical displays.
Comment 10 Ralph Irving 2009-04-02 12:12:50 UTC
Actually the display code is already in squeezeslave.  It supports both an on screen  text display using the curses library and usb lcd displays.  The text display change in SC was to accomodate displays with different sizes.   The squeezeslave wiki page has more details in the linux help section.  If we could confirm that the current changes work fine with slimp3 and SB1 then the patch could just be applied.
Comment 11 Adrian Smith 2009-04-02 12:25:36 UTC
OK understand.  Can the display stuff be tested easily on linux without an external display?  

I'll have a look at it and integrate - I think there may be a couple of bits I would like to change to make them as consitent as possible with the graphics display code, but nothing major from a quick review.
Comment 12 Ralph Irving 2009-04-02 12:32:22 UTC
Yes, you can specify the display width using -w60 for example with the on screen curses display.
Comment 13 Ralph Irving 2009-04-02 12:33:03 UTC
Sorry, -D -w60
Comment 14 Adrian Smith 2009-04-02 12:44:16 UTC
Ralph - is there a prebuilt linux r33 version to try this with (not on sourceforge?)
Comment 15 Ralph Irving 2009-04-02 13:30:58 UTC
The display code hasn't changed in a while.  You could use anything from r25 or r26.  Alternately, I can build a current one for you.  2.6 kernel and alsa?
Comment 16 Ralph Irving 2009-04-02 13:32:44 UTC
Actually squeezeslave-0.8-25-lnx26-alsa-display-i686.tar.gz is the only one that has the display code option enabled.
Comment 17 Ralph Irving 2009-04-02 16:22:32 UTC
I've uploaded squeezeslave-0.8-33-lnx26-alsa-display-i686.tar.gz to sf.net as well.
Comment 18 Adrian Smith 2009-04-03 13:28:28 UTC
Ralph - applied with a few minor changes in change 25809.  

Please check it works for you - I've tested on SB1 and squeezeslave and think it does what you where expecting..
Comment 19 Jim McAtee 2009-04-05 03:04:31 UTC
I just saw the following warnings in my server log.  Could they be related to this new code?

[09-04-05 03:55:33.7152] Slim::Utils::Misc::msg (1113) Warning: [03:55:33.7147] Use of uninitialized value in multiplication (*) at C:/Program Files/SqueezeCenter 7.4 Trunk/server/Slim/Player/SqueezeSlave.pm line 136.
[09-04-05 03:55:33.7159] Slim::Utils::Misc::msg (1113) Warning: [03:55:33.7155] Use of uninitialized value in multiplication (*) at C:/Program Files/SqueezeCenter 7.4 Trunk/server/Slim/Player/SqueezeSlave.pm line 136.
Comment 20 Adrian Smith 2009-04-05 03:30:19 UTC
Try change 25816
Comment 21 Ralph Irving 2009-04-05 12:59:02 UTC
Adrian,

Thank you for taking the time to make the modifications to the patch and
including them in SC.

Initial testing looks good.

I'm going to fix a squeezeslave problem with the time display resetting to the total track time if you pause/unpause.  I'll then create a post asking the squeezeslave users to test it further.  I don't have an external display either, but I know there are users who do.
Comment 22 Ralph Irving 2009-05-14 15:34:53 UTC
Created attachment 5222 [details]
Fix dBToFixed calls and Can't call method hasPowerControl SC log error

The display updates have been working very well with squeezeslave.

I did find a couple other issues with SqueezeSlave.pm and have attached a patch to fix them.

Define sub hasPowerControl to fix this SC log error.

[09-05-14 07:21:26.7579] Slim::Networking::IO::Select::select (271) Error: Select task failed: Can't call method "hasPowerControl" on an undefined value at /usr/lib/perl5/5.8.8/Slim/Web/Settings/Player/Audio.pm line 36.

Calls to dBToFixed were not returning the correct values due to incorrect calling reference.
Comment 23 Adrian Smith 2009-05-24 10:26:51 UTC
patch added in change 26720
Comment 24 Ralph Irving 2009-06-11 04:43:22 UTC
Changes have been working great.