Opened 13 years ago

Closed 12 years ago

#2942 closed task (fixed)

Use id3v2 APIC tag for reading albumart

Reported by: myth@… Owned by: stuartm
Priority: minor Milestone: 0.21
Component: mythmusic Version: 0.20
Severity: low Keywords:
Cc: Ticket locked: no

Description

The attached patch moves the albumart loading into MetaIO, and makes MetaIOID3v2 look for a APIC field to grab the album art from, and fallback to picking a random image from the directory.

It also adds the album art to the banner shown when visualisers are in fullscreen mode.

Attachments (4)

albumart.patch (13.6 KB) - added by myth@… 13 years ago.
the patch
mythmusic_apic2.diff (17.4 KB) - added by myth@… 12 years ago.
updated version that works on files with "Other" type images.
mythmusic_apic.2.diff (20.4 KB) - added by stuartm 12 years ago.
Updated with several fixes and improvments
mythmusic_apic.diff (20.8 KB) - added by stuartm 12 years ago.
Added image size sanity check

Download all attachments as: .zip

Change History (17)

Changed 13 years ago by myth@…

Attachment: albumart.patch added

the patch

comment:1 Changed 13 years ago by stuartm

Owner: changed from Isaac Richards to stuartm

comment:2 Changed 13 years ago by myth@…

There's a memoryleak in the change for playbackbox.cpp, it needs to delete the tagger.

comment:3 Changed 12 years ago by stuartm

Milestone: unknown0.21
Status: newassigned
Summary: Use id3v2 APIC tag for reading albumart and show in fullscreen bannerUse id3v2 APIC tag for reading albumart
Type: patchtask

The 'show in fullscreen banner' portion of this patch has been implemented by Paulh in [13235]

comment:4 Changed 12 years ago by stuartm

I've attached a patch using taglib and updated for the new visualiser/albumart code. I would appreciate some testing from those people who have albumart embedded in ID3 tags (I don't). You will need to run a scan for albumart to be found and delete existing tracks from the database first if they contain the albumart.

The patch itself is functional, but needs a little work before it's committed.

comment:5 Changed 12 years ago by myth@…

Nope, just a lot of ;

2007-04-21 01:35:35.157 Music Scanner - APIC tag found, but missing description.
2007-04-21 01:35:35.177 Music Scanner - APIC tag found, but missing description.
2007-04-21 01:35:35.192 Music Scanner - APIC tag found, but missing description.
2007-04-21 01:35:35.212 Music Scanner - APIC tag found, but missing description.
2007-04-21 01:35:35.256 Music Scanner - APIC tag found, but missing description.
2007-04-21 01:35:44.578 DB Error (music delete artwork):
Query was:
DELETE FROM music_albumart WHERE filename= 'cover.gif' AND directory_id= 0;
No error type from QSqlError?  Strange...
2007-04-21 01:35:44.583 DB Error (music delete artwork):
Query was:
DELETE FROM music_albumart WHERE filename= 'cover.jpg' AND directory_id= 0;
No error type from QSqlError?  Strange...
2007-04-21 01:35:44.586 DB Error (music delete artwork):
Query was:
DELETE FROM music_albumart WHERE filename= 'cover2.jpg' AND directory_id= 0;
No error type from QSqlError?  Strange...

during scanning.

I'd look into it, but not tonight.

comment:6 Changed 12 years ago by stuartm

Ok, I can fix the description problem. The ID3v2 specs say that each APIC frame must have a unique description, I assumed that should mean that the field contained a value for a valid tag.

I've updated the patch with the description check disabled. It's not a permanent fix but should at least allow it to find the art.

The other check on which I suspect it may fail is the artwork type. At the moment we only accept 5 types as valid.

comment:7 Changed 12 years ago by myth@…

Ah, many rippers/taggers will just add an APIC frame with the cover art, but with no description

Here's an addition to MetaIOTagLib::readAlbumArt (insert around line 327),

                default:
                    VERBOSE(VB_GENERAL, QString ("Music Scanner - APIC tag found "
                                                 "with unsupported type %1").arg(frame->type()));
                    continue;
            }

            artlist.append(art);
        }

        // We didn't get any entries in artlist. If there are any APIC
        // frames, many taggers/rippers just add the coverart but with no
        // description, so let's just grab the first (we could go for biggest?)
        if (artlist.isEmpty () && apicframes.size () >= 1) {
            VERBOSE(VB_GENERAL, "Music Scanner - no useable APIC tags found, "
                    "using first...");
            AlbumArtImage art;
            AttachedPictureFrame *frame = static_cast<AttachedPictureFrame*> (*apicframes.begin ());
            art.imageType = (ImageType)frame->type();
            art.typeName = "Front Cover";
            art.description = TStringToQString(frame->description());
            art.embedded = true;
            artlist.append(art);
        }

Pretty nasty, ie. casting the frame->type() directly to a ImageType? is going to be trouble, but it got me a bit closer. In most cases the 1 apic will have type 0 (other), so instead of horrorcasting, we could just pick the first type 0 frame and default to that if the artlist is empty. But now it crashes because AlbumArtImages::getImageAt(unsigned int) is missing...

'night...

Changed 12 years ago by myth@…

Attachment: mythmusic_apic2.diff added

updated version that works on files with "Other" type images.

comment:8 Changed 12 years ago by myth@…

Stuart, I've modified your patch so it works with files that have just a "Other" type image. Basically, anything that's not a known type just gets added as a IT_UNKNOWN, then I added the getImageAt, plus a isImageEmbedded (instead of requiring filename to be "embedded") and voila, all is well.

comment:9 Changed 12 years ago by paulh

Just to point out editmetadata.cpp will also have to be changed. At the moment it uses the filename to get the images and to update the image type in the DB. Probably should use the albumart_id to change the type in AlbumArtImage::saveImageType()

Also line 374 in mainvisual.cpp should be

if (visMode != 2 && fullScreen && albumArt.isNull()) 

There's no point in 3 of us working on this so I will withdraw gracefully from this one :-).

comment:10 Changed 12 years ago by myth@…

The more the merrier! I'll fix editmetadata (unless someone beats me to it).

comment:11 Changed 12 years ago by stuartm

eskil - I'm working on it right now.

Changed 12 years ago by stuartm

Attachment: mythmusic_apic.2.diff added

Updated with several fixes and improvments

comment:12 Changed 12 years ago by stuartm

I've attached the latest version of the patch, with a few fixes and changes. I'm not sure I've got all the required editmetadata changes so that might need a look. For the purposes of editmetadata I opted to ignore any embedded images, at least for now.

Before I commit it, there is one known issue to be addressed. I've got a few files which contain empty APIC frames and so I'll add something to ignore them, maybe even delete them from the tag.

Changed 12 years ago by stuartm

Attachment: mythmusic_apic.diff added

Added image size sanity check

comment:13 Changed 12 years ago by stuartm

Resolution: fixed
Status: assignedclosed

(In [13318]) Adds support for reading album art from ID3v2 tags.

Closes #2942

Note: See TracTickets for help on using tickets.