Bugzilla – Bug 17529
In 7.6.2 Now Playing screen is displayed at various "odd" times
Last modified: 2011-09-29 06:04:35 UTC
Hi When running SBController with jive 7.6.2 r9496, the Now Playing screen is displayed right after changing the "Home-Settings-Shuffle" attribute. This can't be right. The Now Playing screen is also displayed right after "other" navigation steps in the SBController menu structure, but alas I don't remember where, and is therefore not able to reproduce the steps.
I can confirm this as a bug, and have traced it pretty thoroughly. My suspicion is that the bug appeared due to a server-side change, but will likely be better fixed on the firmware side. The upshot of the issue is that anything that triggers a notify_playerPlaylistChange will push the UI to NowPlaying this includes changing shuffle or repeat modes, as well as e.g. using the context menu to add something to the current playlist. IMO, this behavior is incorrect. I'm investigating a patch today.
Created attachment 7446 [details] patch to only push to NP when track has changed, not playlist changed The code to push to NP in SlimBrowser was in notify_playerPlaylistChange. I moved it to notify_playlistTrackChange (leaving the code to push to a "nothing" screen when the playlist is empty in playerPlaylistChange). I also made the current playlist window not invoke a screensaver, which I think is appropriate and solved bad behavior with this change. Tested against changes to Shuffle and Repeat, as well as manipulation of the playlist via CM's (e.g. Adding a track) this is a nice improvement and it has my endorsement for check-in. Please jump through the necessary whatever-it-is-that-passes-for-QA-these-days hoops.
Michael, to you for review and decision as to inclusion in the firmware.
== Auto-comment from SVN commit #9500 to the jive repo by mherger == == http://svn.slimdevices.com/jive?view=revision&revision=9500 == Fixed Bug: 17529 Description: Ben's patch to improve "jump to now playing screen" behaviour approved :-). Thanks Ben!
This bug fix has introduced another bug - rapidly switching Now Playing screens make menu use almost impossible on Touch.
Could somebody who knows what they're doing please re-open this bug and assign it to major or suchlike - it's certainly not minor!
Can you be more specific? What does "rapidly switching Now Playing screens" mean...does that mean you can't use the menu at all because it's literally always pushing to Now Playing during menu use, or is it less frequent than that? I am investigating one bug introduced by this change, which is that the Now Playing screen pushes to screen when the track changes, but whatever you are seeing sounds more frequent than that. I'll go ahead and reopen and assign to myself for investigation.
It's almost unusable - the NP screen flashes back almost immediately when selecting a home menu option - get back to option, and whoosh NP is back. At one point the Touch then restarted from cold.
Okay, not seeing anything like that here. So I can try to reproduce, tell me what you are listening to (local music, internet radio, pandora, rhapsody, etc.), and whether you are connected to Squeezebox Server or mysqueezeebox.com for your server. Also, does this terrible behavior exist if no music is playing? If you are using Squeezebox Server, do you use any 3rd party plugins?
(In reply to comment #10) > Okay, not seeing anything like that here. > > So I can try to reproduce, tell me what you are listening to (local music, > internet radio, pandora, rhapsody, etc.), and whether you are connected to > Squeezebox Server or mysqueezeebox.com for your server. > > Also, does this terrible behavior exist if no music is playing? > > If you are using Squeezebox Server, do you use any 3rd party plugins? I am currently tuned to BBC Radio 4 via iPlayer - using local server. It really is almost unusable - trying to change stations is almost a lottery as the NP screen cycles back and forth at almost 1 sec. intervals.
(In reply to comment #11) > (In reply to comment #10) > > Okay, not seeing anything like that here. > > > > So I can try to reproduce, tell me what you are listening to (local music, > > internet radio, pandora, rhapsody, etc.), and whether you are connected to > > Squeezebox Server or mysqueezeebox.com for your server. > > > > Also, does this terrible behavior exist if no music is playing? > > > > If you are using Squeezebox Server, do you use any 3rd party plugins? > > If I stop/pause the radio stream then the menus seem stable. It also occurs if I use BBC Radio.
(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #10) > > > Okay, not seeing anything like that here. > > > > > > So I can try to reproduce, tell me what you are listening to (local music, > > > internet radio, pandora, rhapsody, etc.), and whether you are connected to > > > Squeezebox Server or mysqueezeebox.com for your server. > > > > > > Also, does this terrible behavior exist if no music is playing? > > > > > > If you are using Squeezebox Server, do you use any 3rd party plugins? > > > > > > > If I stop/pause the radio stream then the menus seem stable. > > It also occurs if I use BBC Radio. It seems stable playing My Music ....
okay, so maybe for some reason BBC is generating spurious notify_playerTrackChange. I'll investigate that tomorrow.
(In reply to comment #14) > okay, so maybe for some reason BBC is generating spurious > notify_playerTrackChange. I'll investigate that tomorrow. Thanks
(In reply to comment #14) > okay, so maybe for some reason BBC is generating spurious > notify_playerTrackChange. I'll investigate that tomorrow. I think you're correct about the BBC problem - I tried playing Classic FM and there are no menu problems with that stream.
(In reply to comment #16) > (In reply to comment #14) > > okay, so maybe for some reason BBC is generating spurious > > notify_playerTrackChange. I'll investigate that tomorrow. > > I think you're correct about the BBC problem - I tried playing Classic FM and > there are no menu problems with that stream. I've noticed today that the switching between the menus and NP screen seems variable - sometimes it occurs almost immediately on selecting the home menu, at other times it can be several seconds or more before the NP screen pops back. I've tried switching off the live text - seems to make no difference. It happens whether the stream is WMA or AAC.
Thanks for the additional information. Could you elaborate on what "live text" is? I'm assuming this is a BBC-specific feature?
(In reply to comment #18) > Thanks for the additional information. Could you elaborate on what "live text" > is? I'm assuming this is a BBC-specific feature? It's a feature in the BBC iPlayer plugin - allows scrolling text on the Now Playing screen - also track titles when available (eg Radio 2 often has them).
> It's a feature in the BBC iPlayer plugin - allows scrolling text on the Now > Playing screen - also track titles when available (eg Radio 2 often has them). okay, that sounds like the smoking gun right there...that kind of behavior would probably generate a playerTrackChange notification really frequently. still investigating a fix
I should say, I don't think it's necessarily incorrect for the live text feature to be generating lots of notify_playerTrackChange, it's just unexpected. The desired behavior is for NowPlaying to never automatically push on track change, so I'm also investigating culling that code block completely right now. I want to first confirm what the intention of the block was when it was checked in as part of SlimBrowser's notify_playerPlaylistChange, so I know why it was there in the first place.
BBCiPlayer plugin and the BBCRadio applet will generate 'newmetadata' notifications within the server when the livetext display changes. This will trigger the server to send status updates to all players in the syncgroup. The reason for this is to get the text to change on the now playing screen. I don't think this event should cause the player to switch to the now playing screen, it should however cause the text to update if it is already in the now playing screen.
thanks for the info Triode, that's what I expected. This is no fault of your plugin. Bug is mine to fix on the SlimBrowser side. executive summary: The current bug is because I moved the push-to-now-playing code from the playlistChange notification method to the trackChange notification method. By moving it I fixed the original bug, but now created this one, which is particularly terrible when using BBC iPlayer.
I think changing the display should be linked to PlaylistChange which only occurs if a user consiously does something (or possibly with random play and equivalents which add tracks after the player advances to a new track) TrackChange looking at the logic in Player.lua actually means now playing text or artwork change and hence really should only update if already showing the now playing display. It doesn't actually mean track being played has changed (perhaps it should be renamed!)
sorry for asking, but what it the purpose to "push" the NP screen to the SBController, when I as a user is browsing the menu structure? I find it confusing, to say the least. If I hit "play" to start a new song, it is ok to display NP, but otherwise I am not so sure.
I spent some time digging through svn logs on that block of code, and the part that includes goNowPlaying() in notify_playerPlaylistChange() was added in 7.4, about 2 years ago. This predated the paradigm shift to "touch to play", which IMO now makes this code unnecessary, and in other places just wrong (e.g., the reason this bug was opened in the first place). Recommendation is to pull the block from notify_playerTrackChange, re-insert it into notify_playerPlaylistChange, and remove the _goNowPlaying() method call altogether, as I believe it now serves no purpose. patch coming
Created attachment 7461 [details] remove goNowPlaying() push, move code back to notify_playerPlaylistChange method I've not been able to determine where the exact checkin was made to add _goNowPlaying() to the notify_playerPlaylistChange method in SlimBrowser, but it's definitely between r5341 and r7548, on the 7.4 branch. On viewvc there is a gap between those two checkins, so I don't have further resolution. Given that this was done in 7.4, that predates touch-to-play, and I think this push to NP is incorrect. This patch removes the code block from notify_playerTrackChange, as it was causing bad problems with esp. BBC streams, re-inserts it to notify_playerPlaylistChange, and fully removes any push to NP due to a playlist change notification.
I suspect the main reason for triggering it from playlist changes is if you trigger a play action from an interface other than the native interface of the player - e.g. iPeng or the web interface. In this case it is potentially desirable to push into now playing when playback starts. However I think it should occur when play is initiated, not when the playlist changes (as this could be for other reasons such as just adding tracks...)
(In reply to comment #28) > I suspect the main reason for triggering it from playlist changes is if you > trigger a play action from an interface other than the native interface of the > player - e.g. iPeng or the web interface. In this case it is potentially > desirable to push into now playing when playback starts. However I think it > should occur when play is initiated, not when the playlist changes (as this > could be for other reasons such as just adding tracks...) Any news on the patch? Thanks for all your efforts.
(In reply to comment #28) > However I think it should occur when play is initiated, not when the playlist > changes (as this could be for other reasons such as just adding tracks...) Exactly. I don't understand why bringing up Now Playing would ever be triggered by anything other than a timeout after user interaction while playing, or when play is initiated. Adding items to the current playlist (from any interface, including the native interface) should not push into Now Playing. From the native interface, display the notification briefly, but keep the user in the same place within the menu system.
yes yes, I get it. FWIW, this code up until last week had not changed since *2009*, so that's some indication that it had not caused any big problem between then and now. At any rate, I don't think it serves any purpose now since touchToPlay came to be the paradigm in 7.5. My patch removes the concept of pushing to NP after a playerPlaylistChange notification. AFAICT, this fixes the original bug and gets rid of any of the ugly behavior with the last patch, particularly with BBC streams. Don't expect any official checkin until next week.
(In reply to comment #31) > yes yes, I get it. FWIW, this code up until last week had not changed since > *2009*, so that's some indication that it had not caused any big problem > between then and now. > > At any rate, I don't think it serves any purpose now since touchToPlay came to > be the paradigm in 7.5. My patch removes the concept of pushing to NP after a > playerPlaylistChange notification. > > AFAICT, this fixes the original bug and gets rid of any of the ugly behavior > with the last patch, particularly with BBC streams. > > Don't expect any official checkin until next week. If it's 9504, then I'm afraid it doesn't fix the problem - NP still bouncing back on screen after a couple of seconds.
> Don't expect any official checkin until next week. Does that mean the patch won't be available until next week?
>> Don't expect any official checkin until next week. >>>Does that mean the patch won't be available until next week? yes, that's exactly what that means.
(In reply to comment #34) > >> Don't expect any official checkin until next week. > > >>>Does that mean the patch won't be available until next week? > > yes, that's exactly what that means. Oh, okay ... thanks
A variant of the NP problem has arisen with this bug. The Touch was downloading a firmware update this morning and the NP screen flashed on and off during the download. Never seen this before. The playing stream was via BBC iPlayer. I just hope the fix fixes all these problems!
Created attachment 7465 [details] allow goNowPlaying to happen when moving from an empty playlist to a non-empty playlist Pretty sure I've finally got this licked with this patch. There is one special case where we do want to push to NowPlaying on a playerPlaylistChange notification, and that's when we're moving from a completely empty playlist to a non-empty playlist. This patch handles that special case. I'm going to run through a set of 10 test cases, which I will document in the bug here, including results. If they pass all of those test cases, this patch will get checked in today.
Here's the test cases I ran in csv. I'm satisfied and will check in the patch. "Test Case","Initial State","Expected Behavior","Result" "TouchToPlay on Track","Empty Playlist","Push to NP","PASS" "Press Play on Album","Empty Playlist","Push to NP","PASS" "Press Play via CM","Empty Playlist","Push to NP","PASS" "Play Next via CM","Empty Playlist","Push to NP, do not play","PASS" "Add to End via CM","Empty Playlist","Push to NP, do not play","PASS" "Shuffle mode change","Empty Playlist","No push","PASS" "Repeat mode change","Empty Playlist","No push","PASS" "TouchToPlay on Track","Non-Empty Playlist","Push to NP","PASS" "Press Play on Album","Non-Empty Playlist","Push to NP","PASS" "Press Play via CM","Non-Empty Playlist","Push to NP","PASS" "Play Next via CM","Non-Empty Playlist","No push","PASS" "Add to End via CM","Non-Empty Playlist","No push","PASS" "Shuffle mode change","Non-Empty Playlist","No push","PASS" "Repeat mode change","Non-Empty Playlist","No push","PASS"
== Auto-comment from SVN commit #9511 to the jive repo by bklaas == == http://svn.slimdevices.com/jive?view=revision&revision=9511 == Fixed Bug: 17529 Description: remove recently code that had been added to playerTrackChange notification. change push to NP code in playerPlaylistChange notification to only push when transitioning from empty playlist to non-empty playlist this covers the special case where the empty playlist window (pushed to via TouchToPlay) is removed as the playlist becomes non-empty, and the user should still be pushed to NP
Did this make it into last night's (Sep 13th) nightly. It is on the change log and maybe I'm reading this wrong, but 33376 seems to have 9508 firmware and this seems to be 9511.
(In reply to comment #40) > Did this make it into last night's (Sep 13th) nightly. It is on the change log > and maybe I'm reading this wrong, but 33376 seems to have 9508 firmware and > this seems to be 9511. I've got Sep 14 build - still 9508 - and still problem not resolved. Where is 9511?
> I've got Sep 14 build - still 9508 - and still problem not resolved. Where is > 9511? What device are you using? If it's a SB Radio, then the new firmware might not have been validated yet. This is a manual process we need to do in order not to brick that device.
(In reply to comment #42) > > I've got Sep 14 build - still 9508 - and still problem not resolved. Where is > > 9511? > > What device are you using? If it's a SB Radio, then the new firmware might not > have been validated yet. This is a manual process we need to do in order not to > brick that device. Using the Touch
> Using the Touch Are you running 7.6.2 software? Mine's on 9532 today.
(In reply to comment #44) > > Using the Touch > > Are you running 7.6.2 software? Mine's on 9532 today. Yes - 43433. Stll stuck at 9508
> Yes - 43433. Stll stuck at 9508 I assume this is 33433? Check your server.log file and make sure you didn't disable update-checking
(In reply to comment #46) > > Yes - 43433. Stll stuck at 9508 > > I assume this is 33433? Check your server.log file and make sure you didn't > disable update-checking Okay - had updates off. It's now dloading 9511 .... Should I switch off updates now to avoid the constant nags?
(In reply to comment #47) > (In reply to comment #46) > > > Yes - 43433. Stll stuck at 9508 > > > > I assume this is 33433? Check your server.log file and make sure you didn't > > disable update-checking > > Okay - had updates off. It's now dloading 9511 .... > > Should I switch off updates now to avoid the constant nags? It works! Thanks to all for the fix !!!
(In reply to comment #48) > (In reply to comment #47) > > (In reply to comment #46) > > > > Yes - 43433. Stll stuck at 9508 > > > > > > I assume this is 33433? Check your server.log file and make sure you didn't > > > disable update-checking > > > > Okay - had updates off. It's now dloading 9511 .... > > > > Should I switch off updates now to avoid the constant nags? > > It works! Thanks to all for the fix !!! Any chance of pushing this to the SB Radio? Current 7.6.2 builds seem to still have 7.6.1 firmware for the Radio. This would finish 7.6.2 off for me. :)
I have people complain of something similar to this on duet controllers - did an update get made for them as well?
It should only be Radio firmware that gets delayed releases because it requires manual qualification, but looking at this link: http://update.mysqueezebox.com/update/firmware/7.6.2/ makes me think something's not right with publication of Controller and Touch firmware. r9538 is several revs off of what it should be to be up to date for today...
(In reply to comment #51) > It should only be Radio firmware that gets delayed releases because it requires > manual qualification, but looking at this link: > > http://update.mysqueezebox.com/update/firmware/7.6.2/ > > makes me think something's not right with publication of Controller and Touch > firmware. r9538 is several revs off of what it should be to be up to date for > today... All OK today on Touch and radio :)