Embedded cover art support

[Dan Meltzer]

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
>>
>> 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.
>>
>> 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.)
>>
>> 4) This should probably also be implemented for Meta::File.  Not
>> necessarily in this patch, but it shouldn't be too miserable to do.
>>
>> 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.
>>
>> 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?

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.  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.

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

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.

Dan,
>
> --
> Mark Kretschmann
> Amarok Developer, Software Engineer at Collabora Ltd
> Fellow of the Free Software Foundation Europe
> http://amarok.kde.org - http://www.fsfe.org - http://collabora.co.uk
> _______________________________________________
> Amarok-devel mailing list
> Amarok-devel at kde.org
> https://mail.kde.org/mailman/listinfo/amarok-devel
>