Embedded cover art support

[ecroy at gmx.net]

Hi Dan,
thanks for the highly valued feedback - comments below:

On Friday 26 February 2010 06:54:13 Dan Meltzer wrote:
> On Fri, Feb 26, 2010 at 12:40 AM, Mark Kretschmann <kretschmann at kde.org> 
wrote:
> > On Fri, Feb 26, 2010 at 6:24 AM, Dan Meltzer
> > 
> > <parallelgrapefruit at gmail.com> wrote:
> >> On Thu, Feb 25, 2010 at 8:50 PM,  <ecroy at gmx.net> wrote:
> >>>> merge request will follow after some more testing!
> >>> 
> >>> Well, finally came around to actually do it, so...
> >>> 
> >>>   http://gitorious.org/amarok/amarok/merge_requests/134
> >>> 
> >>> The proposed patch works fine for me and at last makes my music
> >>> collection not look like a greyish dump yard anymore (now it's a
> >>> colorful one :o)
> >> 
> >> Hi:
> >> 
> >> A few things.
> >> 
> >> 1) Please check the style you are using, it's a little different from
> >> what we use throughout Amarok elsewhere.  see
> >> HACKING/intro_and_style.txt

Sure, will do - probably didn't read it thoroughly the first time.

> >> 2) I am a little worried about how many places the code at the
> >> beginning of loadImageFromFile() has been copied to at this point.  I
> >> wonder if we should create a Meta::getFileRef( Meta::TrackPtr ) that
> >> centralizes this code, in case we want to change it at some point in
> >> the future.

Agree ;)

> >> 3) It took me a few read throughs (even with the comment) to
> >> understand what isEmbeddedImage() is doing.  I wonder if it can be
> >> clarified a bit?  It also confuses me because we only deal with id3
> >> tags for embedded images, correct? And some of those formats wouldn't
> >> have id3 tags... (though in that case they probably shouldn't be in
> >> the images table.)

We need some way to distinguish between normal and embedded images, the 
options I see are:
1) explicit database support (boolean 'embedded' column in the images table
   or even a separate 'embedded' table like in Amarok 1.4)
2) implicit database support (some prefix to the path of the image)
3) some function deciding if a given filepath is a normal or an embedded image

I mainly opted for 3) because it has the lowest impact on the rest of the code 
but I think it's a viable way to go.
We probably can rely on the fact that only image files or audio files with 
embedded images have entries in the image table. Filetype recognition is made 
via the file extension on some other places in the code so I went for it.
Of course, more explicit database support might be a bit nicer...

As for the filetypes which won't have id3 tags - true, but they could have 
some other form of embedded cover art (support would of course have to be 
implemented yet). The function currently really only returns whether the given 
path is clearly not an image file. The reason why I did not check for .jpeg, 
.png, ... was, that this function is sometimes called with the path of a 
cached image which does not have any extension.

The only problem I can see with the current approach is if somebody has his 
image files named .mp3 and his songs .jpeg ;o)

> >> 4) This should probably also be implemented for Meta::File.  Not
> >> necessarily in this patch, but it shouldn't be too miserable to do.

Ok, I'll probably look into that a bit later.

> >> 5) The collection scanner portion of this looks like it could be
> >> optimized to me... Gitorious just died so I can't look at the patch
> >> too closely, but you should a) be able to use 0/1 (I think qt will
> >> serialize booleans automatically) and b) default it to false, and
> >> check whether or not it's true, rather than only setting it to true if
> >> it has the frame.

Agreed and implemented - now apic is always present and 0/1.
On a side note: audioproperties is currently true/false - might be worth 
changing as well.

> >> I think this is a very useful feature, and am glad you spent time
> >> working on it :)
> > 
> > Big thanks Andreas, I've merged your patch into mainline :)
> > 
> > But please also consider what Dan wrote. Possibly the patch could
> > still be improved a bit, maybe with another patch?

Wow - I did not expect this to happen that quickly and yes, I'll try to work 
out the issues.

> Okay, so I did some more thinking about this, and while I wasn't fast
> enough to catch machine-gun markey from merging it, I think there are
> a few larger issues that still need to be solved with this patch.
> 
> As far as I can tell, this currently does no caching.  This kind of
> negates the benefit of sqlcollection.  We don't want to have to read
> from the files every time we need data from them.  I think we should
> save embedded images when they are loaded to our cache dir, perhaps
> with an em- prefix.  We could then discard them when the collection
> scanner tells us the file has changed.

Oh but it does use the same caching mechanism as the normal cover arts and 
thus it makes no big difference if we are reading a cover art from a 
front.jpeg or from the apic frame of an audio file (probably in the same 
directory). Once the image was loaded it is cached in the various scaled sizes 
- or am I missing your point?

> I also don't really understand
> the interaction between embedded images for a track and the fact that
> this is in Meta::Album... but that's not exactly your fault.  As it
> stands if an album has thirteen tracks, and they all have embedded
> images, we read the front album cover from up to thirteen files?  That
> doesn't sound remarkably efficient.

Yes this is a problem and maybe a reason why this feature was left 
unimplemented for a pretty long time. There is a conceptual difference from a 
front.jpeg and an embedded image due to the Album vs. Track relationship but 
there are ways to deal with it - we just have to settle for one. In the 
current implementation the thirteen images from the thirteen files would not 
be read because in the absence of a real image file, simply the first embedded 
cover art would be used. More elaborate heuristics could be applied (like 
prefering files which have the APIC type FrontCover, or larger image-file 
size) but I the current way is efficient and probably works good enough for a 
first version of this feature.

On a side note: I realized that all my cached images are deleted upon next 
startup because they are of sizes 28, 90 and 156. App.cpp deletes all cached 
images that are not of size 32, 34 or 50. Is it possible that the image sizes 
got changed and nobody told App.cpp?

> Also, another minor note:  loadImageFromFile() should take a const
> reference, not a const value as it's parameter.

Ok - fixed; there seem to be quite a few other functions where this could be 
changed as well...

> I feel like this is an important fix, and we probably shouldn't have
> merged it without at least discussing it first, but now that it's
> merged let's see if we can find a solution.

Agreed but it's good that we discuss it now :)

- Andreas