Bug 8237 - AutoDisplay plugin broken by new SqueezeCenter v7.0.1
: AutoDisplay plugin broken by new SqueezeCenter v7.0.1
Status: NEW
Product: Logitech Media Server
Classification: Unclassified
Component: Plugins
: 7.0.1
: All All
: -- enhancement (vote)
: Future
Assigned To: Unassigned bug - please assign me!
:
Depends on: 7922
Blocks:
  Show dependency treegraph
 
Reported: 2008-05-26 23:15 UTC by Daryle Tilroe
Modified: 2009-09-08 09:21 UTC (History)
3 users (show)

See Also:
Category: ---


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Daryle Tilroe 2008-05-26 23:15:29 UTC
The AutoDisplay plugin has been broken by the new SqueezeCenter v7.0.1.  The revised version worked fine with v7.0.  As reported in the forum:

http://forums.slimdevices.com/showthread.php?t=47857

It seems to have been working as late as 7.01-19352 but the release version "fights" with the plugin and turns the display back up to its off level.  Basically there now seems to be some sort of brightness check hidden away in the new core that checks everytime the brightness is changed to make sure it is at the "correct" off level.

The autodimming functionality should probably just be rolled into the core anyhow as noted way back in:

https://bugs-archive.lyrion.org/show_bug.cgi?id=1850
Comment 1 KDF 2008-05-26 23:29:50 UTC
the plugin should be storing its own pluginParam and setting the pref accordingly.
SC is doing what it should be doing, and actively updating based on pref (should anyone be changing the pref on the fly, etc)
Comment 2 KDF 2008-05-27 13:49:42 UTC
sorry, make that $client->pluginData. This is designed for holding plugin state information

see also bug 7922 (unfortunately, this bug fix was missed in the changelog)

Given those details, this is probably best merged with bug 1850 as this is either a fix for the plugin author to make, or the existing enhancement request.
Comment 3 Eric Koldinger 2008-05-27 16:04:43 UTC
It appears that what is happening is roughly thus:
The AutoDisplay plugin comes in, and correctly executes a Client->brightness() call.  This causes the brightness to be set for a short period of time (a couple seconds max), then the brightness seems to reset to the value stored in the main settings file for that.

This is only being executed when the player is "off", so it's some sort of an inactive screensaver issues.

The plugin does not try to change the value set in the client's main preferences value, only the dynamic call to Client->brightness().
Comment 4 KDF 2008-05-27 22:34:59 UTC
Eric, the change is due to the fix in bug 7922, which applies whenever brightness is set to automatically adjust. The server was previously not taking into account pref changes, which is why it updates based on the pref.

The plugin can set a pluginData value to store the old pref, then change the pref and reset later. This would have the added benefit of no longer requiring the player to be off (which I would guess was required because in other modes, the server would correctly update the brightness to the pref)

Current workaround would be to set to manual brightness only, but this is set to be deprecated around the 7.2 timeline.

Comment 5 Eric Koldinger 2008-05-28 11:03:03 UTC
Unfortunately, this solution, at least as I understand it, makes preference setting very confusing.  If the plugin is manipulating the value for brightness, that will show up in the interface, and it would be dynamic.  You'd set a value into the preferences, and a while later it would change magically when the plugin went around mangling things.  Making sure that all the values made sense would be a bit of a programming nightmare (potential bug minefield with all the corner cases).

This also pretty much makes the Client->brightness(value) call worthless, as it no longer does anything.  It sets a value, but that value's going to get changed in a second.  Definitely not what the API implies.

Rather than attempting to set the brightness every second (I hate polling), I'd think it would make more sense to change the brightness when it needs to be changed, such as power on/off, going idle, and when the value is changed in the preferences.  Requires either tracking the previous values to detect changes, or having a listener/callback interface to watch for preference changes, but in the long run, I think it's cleaner code.
Comment 6 Daryle Tilroe 2008-05-28 11:28:12 UTC
I'm not sure that the fix for https://bugs-archive.lyrion.org/show_bug.cgi?id=7922 was really the correct thing to do; sort of echoing Eric's comment of the now apparent uselessness of Client->brightness(value) call and the expense of constant polling.  My admittedly brief skim of 7922 suggests that it was more of a firmware bug that has now been fixed with what seems to be a bit of a kludge in SqueezeCenter.

Also without actually going though any of the scenarios some of the suggested workarounds might potentially lead to the display blanking during the "dim" time even if the player is on (*not* what you want happening).
Comment 7 James Richardson 2008-06-02 10:02:18 UTC
Andy: Can you please look this over and comment accordingly?
Comment 8 Chris Owens 2008-06-23 09:15:15 UTC
KDF, Andy asks your opinion about the right thing to do.
Comment 9 Daryle Tilroe 2008-06-23 09:32:08 UTC
I'd just like to empahsize what I said in my initial comment:

The autodimming functionality should probably just be rolled into the core anyhow as noted way back in:

https://bugs-archive.lyrion.org/show_bug.cgi?id=1850

I don't know how anyone who has a squeezebox in their bedroom can live without this functionality.
Comment 10 KDF 2008-06-23 12:03:41 UTC
I've already given my personal opinion.  It's your software so it's up to you to either revert and re-open the other bug in order to fix this.  Then you'll need to mark the other as wont fix and redefine the auto-brightness as something that WONT change when the pref is changed, only when the modes change.

If you leave the autodim working as it should, then the way to manipulate brightness is through the pref, or you end up doing yet another special case to handle a plugin that may not be in use by every user who downloads and installs SC

I've said my peace so I will leave the debate to everyone else
Comment 11 Chris Owens 2008-06-30 09:24:16 UTC
After talking to Felix and the other developers, it seems like bug 1850 is the "right" way to fix this issue if we want to do that.
Comment 12 Blackketter Dean 2008-07-18 10:37:16 UTC
*** Bug 1850 has been marked as a duplicate of this bug. ***
Comment 13 Daryle Tilroe 2008-07-26 06:00:24 UTC
So is this the correct summary of the current situation?:

-The "fix" for bug 7922 has rendered the Client->brightness() call basically
 useless unless you are in 'Manual' brightness mode.  This "fix" has broken
 the AutoDisplay, showBriefly, and possibly other plugins.  It also makes the
 control of brightness by future plugins difficult.

-Bug 1850, suggesting rolling AutoDisplay (time based dimming) into the core,
 has been closed.  This was contrary to the consensus above that it was the
 "right" way to prevent contention over the control of brightness.  Of course
 this would not help showBriefly or any other present of future plugins that
 might want to control brightness.

-The kludge of 'Manual' brightness mode is going away very shortly and those
 of us with squeezeboxes in our bedrooms will have to unplug them, or throw
 dirty underwear over top to sleep... :(

Isn't there someway to rethink what 7922 does?  For example only adjust the
brightness when the preference changes rather than constantly poll or check for
changes.  This is arguably wasteful of cycles as well.  As an aside I have found
7.x more sluggish than older slimserver versions.  Now I don't really blame 7922,
but "fixes" like this could add up.

I'll cross post some of the above to 7922.
Comment 14 Chris Owens 2008-09-10 15:49:31 UTC
Eric, Andy suggests that the plugin be edited to change all three of the SC preference in addition to the current brightness.
Comment 15 Daryle Tilroe 2008-09-10 17:03:36 UTC
So basically it's going to have to store the active off preference as another plugin preference and set the SC off preference as appropriate during it's active and inactive periods?  I guess that would work, with the user realizing that the "real" SC preference will always be ignored and over ridden by the plugin.  Could be a bit confusing for those who don't think about it.