Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#7342 - Improve use of ALBUMARTIST tag

Attached to Project: Rockbox
Opened by Dieter (dip) - Friday, 22 June 2007, 15:33 GMT+2
Last edited by Dan Everton (safetydan) - Monday, 25 June 2007, 13:47 GMT+2
Task Type Patches
Category ID3 / meta data
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

This patch enables that in the Rockbox database the ALBUMARTIST tag is set to the value of the ARTIST tag of a track if the ALBUMARTIST tag is not set in the music file.

With this amendement it is no longer necessary to tag ALL your files with an ALBUMARTIST tag in order to have a browse structure like "albumartist->album->title" for ALL your albums. You need only to tag the tracks of various artists albums with ALBUMARTIST=various or of albums for which ALBUMARTIST differs from ARTIST for another reason (e.g. albums of a certain artist on which e.g. one song is a duet with another artist (all songs except the duet have ARTIST=artist1 and the duet has ARTIST=artist1 & artist2).

In addition, ALBUMARTIST support for MP4 tags and APE tags has been added (but without testing since I don't have MP4 and APE files).
   improve_albumartist_patch (2.3 KiB)
This task depends upon

Closed by  Dan Everton (safetydan)
Monday, 25 June 2007, 13:47 GMT+2
Reason for closing:  Out of Date
Additional comments about closing:  I went ahead and committed my version of this patch. Should work the same way for database users without messing up the tags the ID3 screen shows.
Comment by Dan Everton (safetydan) - Saturday, 23 June 2007, 08:56 GMT+2
Rather than copying the metadata, would it not be better to just change the way the database is built? So in the database code, just make it write out the artist name instead of the album artist if that's empty. This way the change would only affect the database stuff.
Comment by Dan Everton (safetydan) - Saturday, 23 June 2007, 10:02 GMT+2
Maybe some code will make my idea clearer. Only lightly tested, but seems to do the same thing with less code.
   tagcache_albumartist.patch (1.1 KiB) 
 apps/tagcache.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comment by Tan Yu Sheng (Farpenoodle) - Sunday, 24 June 2007, 13:43 GMT+2
Nice. This was for me the last remaining reason to not use the database.
Comment by Dieter (dip) - Sunday, 24 June 2007, 16:40 GMT+2
Don't the two patches do finally the same thing, only at different places in the code or is there a priciple difference I don't see? Of course, since the second patch only changes tagcache.c while my version must patch each "codec-file" separately I would prefer the second verson if it does the same thing.

However, the first part of my patch must be included in the second version since this part enables support for mp4 files (independently from the kind the albumartist value is finally handled).
Comment by Dieter (dip) - Sunday, 24 June 2007, 21:11 GMT+2
@Dan
I tried your version of the patch but and found one difference to my initial version. While browsing and searching for albumartist works for files which have no albumartist tag set the value for albumartist ist not displayed in the id3 info page. Do you have an idea why?
Comment by Dan Everton (safetydan) - Sunday, 24 June 2007, 22:59 GMT+2
The patch I did up was only to demonstrate the concept of copying artist to albumartist in tagcache.c rather than changing code in all the metadata parsers. Doing it that way is cleaner and should result in smaller increase in binary size used which is always good. It also means that music files won't have "fake" albumartist data when they aren't tagged with albumartist, which you saw in the id3 info screen. I don't think the id3 info should ever tell you anything that isn't actually in the file.

I understand that the mp4 metadata stuff should also be included, but I think that's a separate patch and should be split from this one.

Loading...