Created attachment 1829 [details]
slimserver log

Jim, I am not able to reproduce the behavior that you are seeing here. Are you using something other then a standard install perhaps? As far as the "RATING" tag goes, I know that dBpoweramp uses it. The odd thing is Slim Server assumes a 100 scale when dBpoweramp use a 5 scale, could be another bug.

Did some more testing.  The column in the tracks table is an unsigned tiny int, so it can accept value from 0 to 255.  It looks like SlimServer is doing no data validation on what it reads from the RATINGS tag.  Values from 0 to 255 work, but decimal values, negative numbers, values greater than 255, or alpha characters all cause the failure.

Dan, can you take a look at this one? It sounds similar to Bug 4823 however I have seen decimal values for rating such as 2.5 for 2.5 out of 5. It looks like SlimServer is expecting a whole number out of 100. Ideas?

This is not going to make it in to 6.5.2.  If we don't want to change the data type for this value, we could at least validate the input and discard it if it's unsuited to the tiny int type.

bug 4823 required a check for validating DISC and DISCC, so it should be as simple as adding RATING to this check like so:

Schema.pm line 1594:
        # Bug 4823, 4803 - check boundaries set by our tinyint schema.
  	         for my $tag (qw(DISC DISCC RATING)) {

and add a last line in that section for safety:

$tag = int $tag;

of course, it could also be processed on its own to do some smart scaling for something like 2.5 (ie assume scale of 5 and  assign 50/100)
  	 

This doesn't seem to be an issue with the current nightly build, Jim (or anyone) are you still seeing this? 

Haven't tested it in quite a while.  Was the patch checked in?  I'll check it out tonight or tomorrow.

no patches have been added (hence no change number) in regard to this issue.  Earlier testers reported being unable to reproduce, so Ross may also be unable to reproduce.  

Here's what I'm finding, after more testing:

Running with the bundled MySQL server for Windows, 5.0.22-community-nt, the scanner doesn't crash.  Decimal numbers are rounded or truncated, alpha characters are ignored, out of range values are ignored.  But running with the latest non-bundled MySQL, 5.0.41-community-nt, it can still crash the scanner (though I find that decimal values are now rounded).  So, it's dependant on the behavior of MySQL, which may explain why it isn't always reproduced.  I can't find anything obvious in the MySQL config files of my installed instance and that of the bundled MySQL, so I suspect it may be in the way SlimServer's MySQL binary is compiled for Windows.

Perhaps the severity or the priority of the bug should be set very low, although I can't think of a good reason why you wouldn't want to validate all data before trying to store it in the database.  The only question seems to be whether you'd limit the range to 0-100, or 0-255.  I don't believe ratings are actually used for anything within SlimServer, but they're displayed on the track detail page as 'n/100'.

I'll lower the priority and severity now that this is more clear. Thank you for clarifying Jim. 

Happy to look at this after 7.0 and/or welcome patches.

*** Bug 9500 has been marked as a duplicate of this bug. ***

Jim: Is this still a valid issue with SC 7.4

(In reply to comment #14)
> Jim: Is this still a valid issue with SC 7.4

Which branch?  Since the behavior changed from one version of MySQL to another, it may be different between MySQL and SQLite.

Well...since SQLite will be the next release, I'd say that one should be the testing target.

With SQLite, no crash.  There isn't really a datatype of tinyint in SQLite - there's only one type of integer.  In fact, you can even store strings and real numbers in columns declared as tinyint.

So if you were never to do any data validation, you won't cause a crash with SQLite.  I would contend, however, that it's a big mistake to be so lax and that doing so will inevitably cause SqueezeCenter itself to choke on the data at some point.  Doing the validation of metadata at scan time makes a lot more sense.  Furthermore, if you really do want to pretend that the system is independent of the DBMS, then data validation would be a very good idea.

Until someone actually figures out _what_ is expected in the ratings column (is it 0 to 5, 0 to 10, 0 to 100 or something else?) then for the time-being you should at least make sure the value fits into the standard definition of an unsigned tinyint - 0 to 255.

From the SQLite FAQ:

(3) SQLite lets me insert a string into a database column of type integer!

This is a feature, not a bug. SQLite uses dynamic typing. It does not enforce data type constraints. Any data can be inserted into any column. You can put arbitrary length strings into integer columns, floating point numbers in boolean columns, or dates in character columns. (...)

Moving all SQLite-related bugs to 8.0.

SQLite bugs need to go back to 7.6 target.

 == Auto-comment from SVN commit #33476 to the slim repo by agrundman ==
 == http://svn.slimdevices.com/slim?view=revision&revision=33476 ==

Fixed bug 4803, warn and reject on RATING tags that aren't an integer 0-255

POPM, which is id3, uses 0-255, thats in the id3 spec.

vorbis has no such spec, however, the scale winamp uses, and media monkey, and others is 0-100

in either case, regardless of the raw numbers, apps reduce / interpret the raw numbers to 0-5 stars.

dos this fix mean SBS ignores ratings in vorbis now?

go to the ed of the thread:

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

(In reply to comment #20)
> Fixed bug 4803, warn and reject on RATING tags that aren't an integer 0-255

Andy, the warnings being logged for rejected RATING values seem to be causing some confusion, especially for people who experience scanner crashes. I'd suggest that these not be logged.

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

Probably a topic for a new bug, but I'd also suggest that ratings value be stored in the DB as floating or fixed point numbers, so that numbers like 2.5 or 3.5 would be be acceptable. If some day you needed to fit those to some scale or to display the ratings using stars, you might add a library pref for 'Ratings Scale'.