Opened 6 years ago

Closed 6 years ago

#9813 closed bug (fixed)

[cddb_daemon] slash clash

Reported by: ttcoder Owned by: bga
Priority: normal Milestone: R1
Component: Servers/cddb_daemon Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

The CDDB daemon parsing code has a feature where a "/" slash in a Track title is interpreted as a delimiter for a track specific artist (i.e. an artist that overrides the disc-wide artist for that particular track): http://cgit.haiku-os.org/haiku/tree/src/servers/cddb_daemon/cddb_server.cpp#n259

This causes trouble for track titles that include... a slash! Like track TTITLE0 on this disk: http://www.freedb.org/freedb/misc/5d0a5908

The www.freedb.org FAQ does mention that feature, but it says it only applies if the disc-wide artist is set to "Various" : http://www.freedb.org/en/faq.3.html#22

For highest compatibility, cddb_server.cpp:259 should test for "Various" before applying the overriden artist feature. If the artist is not "Various" but e.g. "Doug Smith" then the slash in title should be interpreted as-is... with the required defensive programming when using that value for creating files of course. Indeed there are provisions for that elsewhere in the code (look for ReplaceSet("/", " ") calls in several places, including when processing the title and artist fields) so the fact that "Various" is not taken into account seems to be a simple overlook.

Attachments (2)

0001-Check-for-compilation-artist-in-cddb-daemon.patch (1.7 KB) - added by jscipione 6 years ago.
Patch to check for Various before looking for track specific artist info
0001-Check-if-disk-has-compilation-artist.patch (1.3 KB) - added by jscipione 6 years ago.
Let's try again. Look for disk title set to Various case-insensitively before parsing track specific artist info using ICompare()

Download all attachments as: .zip

Change History (18)

comment:1 Changed 6 years ago by axeld

Makes perfect sense, patches welcome :-)

Changed 6 years ago by jscipione

Patch to check for Various before looking for track specific artist info

comment:2 Changed 6 years ago by jscipione

Has a Patch: set

comment:3 Changed 6 years ago by axeld

Has a Patch: unset
Owner: changed from nobody to bga
Status: newassigned

comment:4 in reply to:  2 Changed 6 years ago by umccullough

I don't think that's what he meant in the bug description - I believe the problem here is that if the CD-artist is "Various", then the track name is supposed to be used to parse the artist name out when it contains a "/"

Your patch seems to just look for "Various /" in the track name instead.

comment:5 Changed 6 years ago by axeld

Wow, that was fast. Unfortunately, it doesn't look correct. I assume you did not test this, John? You check for "Various / " in the track title. You won't find it there, though -- you'll only have to consider " / " when the disk title is set to "Various".

comment:6 Changed 6 years ago by anevilyak

Component: ServersServers/cddb_daemon

comment:7 in reply to:  5 Changed 6 years ago by jscipione

Replying to axeld:

Wow, that was fast. Unfortunately, it doesn't look correct. I assume you did not test this, John? You check for "Various / " in the track title. You won't find it there, though -- you'll only have to consider " / " when the disk title is set to "Various".

No, I didn't test since I don't have the data to test this with. I guess I misinterpreted the bug. I'll try again.

comment:8 Changed 6 years ago by ttcoder

Right, if I'm not mistaken it could be a one-line change, from

if (pos != B_ERROR) {

to

if (pos != B_ERROR && diskData->artist == "Various") {

(or even using ICompare() for case-insensitivity) Sorry I was not more explicit in my description...

comment:9 Changed 6 years ago by jscipione

Should "Various" be case sensitive or not? Should I match on "VARIOUS"?

comment:10 Changed 6 years ago by axeld

I would assume that a band that calls itself VARIOUS and uses " / " in their titles can be safely ignored. Therefore I would suggest to compare case insensitively.

comment:11 in reply to:  9 Changed 6 years ago by umccullough

Replying to jscipione:

Should "Various" be case sensitive or not? Should I match on "VARIOUS"?

Having done my share of queries against cddb/freedb over the years - I would highly suggest making it a case-insensitive comparison... the data coming out of that database is pretty bad sometimes.

comment:12 Changed 6 years ago by jscipione

Has a Patch: set

Changed 6 years ago by jscipione

Let's try again. Look for disk title set to Various case-insensitively before parsing track specific artist info using ICompare()

comment:13 Changed 6 years ago by axeld

Looks good to me now. Can you test that, ttcoder?

comment:14 Changed 6 years ago by ttcoder

Thanks much guys. I'm making a build of the modified daemon for dsuden (he's the one with the oddball Doug Smith CD :-) and will be sure to report back in a few days.

comment:15 Changed 6 years ago by ttcoder

We finally had time to test the patched daemon on a few CDs. No regression on 'normal' albums, and the oddball album now works great; jscipione you may please apply the patch..!

comment:16 Changed 6 years ago by jscipione

Resolution: fixed
Status: assignedclosed

Fixed in hrev45754

Note: See TracTickets for help on using tickets.