Bug 7668 - Player status update intolerant of inaccurate duration
: Player status update intolerant of inaccurate duration
Status: CLOSED FIXED
Product: Logitech Media Server
Classification: Unclassified
Component: Web Interface
: 7.0
: PC Other
: P3 normal (vote)
: 7.x
Assigned To: Michael Herger
:
Depends on: 6950
Blocks:
  Show dependency treegraph
 
Reported: 2008-03-30 05:05 UTC by Alan Young
Modified: 2009-07-31 10:18 UTC (History)
0 users

See Also:
Category: ---


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Alan Young 2008-03-30 05:05:06 UTC
If the  reported duration of the track is less than the actual duration then, when playTime > duration, updatePlayTime in Default/html/main.js goes into a loop getting updated status. It should be tolerant of this situation, as in the Nokia skin for example, which puts a lower limit of 1s on status update requests.

This problem was triggered by data retrieved from xmlbrowser which specified time as hh:mm:ss instead of just seconds. See bug 7470.

The relevant code fragment is:

		updatePlayTime : function(time, totalTime){
			// force 0 for current time when stopped
			if (playerStatus.mode == 'stop')
				time = 0;

			if (! isNaN(time))
				playTime = parseInt(time); //force integer type from results

			var shortTime = Utils.formatTime(playTime);
			var remainingTime = shortTime;

			if (!isNaN(playerStatus.duration) && playerStatus.duration > 0) {
				totalTime = playerStatus.duration;

				if (totalTime > 0 && playTime >= totalTime-1) {
					this.getStatus();
					return;
				}

				remainingTime = '-' + Utils.formatTime(totalTime - playTime); 
				shortTime = Utils.formatTime(playTime) + ' / ' + remainingTime;

				this.progressBar('ctrlProgress', playTime, totalTime);
			}
Comment 1 Michael Herger 2008-03-31 00:49:37 UTC
Alan - do you mean this issue _was_ triggered by 7470, but isn't any more? So this bug is mainly about making the web interface more robust for such potential issues?
Comment 2 Alan Young 2008-03-31 00:56:41 UTC
yes, exactly
Comment 3 Michael Herger 2008-03-31 01:12:16 UTC
only updating every second isn't easily done without major rework, as the above code is called twice a second. Thus we could eg. call the update immediately, but give up if after 10 seconds we still don't have any reasonable duration/playtime, falling back to the default updates. What do you think?

Please note that major rework of all JS stuff is planned for 7.1 (js-rework branch). Bigger changes should be targetted to that branch.

Index: /Users/mh/Documents/workspace/trunk/server/HTML/Default/html/main.js
===================================================================
--- /Users/mh/Documents/workspace/trunk/server/HTML/Default/html/main.js	(revision 18219)
+++ /Users/mh/Documents/workspace/trunk/server/HTML/Default/html/main.js	(working copy)
@@ -856,7 +856,9 @@
 			if (!isNaN(playerStatus.duration) && playerStatus.duration > 0) {
 				totalTime = playerStatus.duration;
 
-				if (totalTime > 0 && playTime >= totalTime-1) {
+				// update player status if we're past the current track's duration
+				// but don't try longer than during 10 seconds in case a duration is set wrong (see bug 7470)
+				if (totalTime > 0 && playTime >= totalTime-1 && playTime <= totalTime+10) {
 					this.getStatus();
 					return;
 				}
Comment 4 Alan Young 2008-03-31 01:34:35 UTC
It is the hammering the server that I don't like. Even in the normal case I guess that it will hammer the server continuously during the last second of a track, which is just when the server is busy trying to start the next track, or at least dealing with the track-start event. It really should be rate limited to no more often than (say) 400ms. But I can see that his might be too much of a rework in this case.

I presume with your proposed solution that it is just going to come back after the normal update interval in any case. In which case I would limit the retries to 1-1.5s (beyond the duration), rather than 10s.
Comment 5 Michael Herger 2008-03-31 02:44:33 UTC
change 18221 - adding above fix/workaround to 7.0.1

To be reviewed in js rewrite.
Comment 6 Michael Herger 2008-04-22 07:41:26 UTC
change 19035 - added the same check to SqueezeJS
Comment 7 Chris Owens 2008-07-30 15:30:43 UTC
This bug has now been fixed in the 7.1 release version of SqueezeCenter!  Please download the new version from http://www.slimdevices.com if you haven't already.  

If you are still experiencing this problem, feel free to reopen the bug with your new comments and we'll have another look.
Comment 8 Chris Owens 2009-07-31 10:18:49 UTC
Reduce number of active targets for SC