Bug 151609 - OSD album cover/embedded image bug
Summary: OSD album cover/embedded image bug
Status: RESOLVED UNMAINTAINED
Alias: None
Product: amarok
Classification: Applications
Component: general (show other bugs)
Version: 1.4.7
Platform: Ubuntu Linux
: NOR normal
Target Milestone: ---
Assignee: Amarok Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-10-31 14:20 UTC by Jeff Kerr
Modified: 2009-08-03 04:07 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Patch for the fix (821 bytes, patch)
2007-10-31 22:23 UTC, Jeff Kerr
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeff Kerr 2007-10-31 14:20:05 UTC
Version:           1.4.7 (using KDE KDE 3.5.8)
Installed from:    Ubuntu Packages
OS:                Linux

Certain MP3s with embedded images for cover art show no image in the OSD (not even the stock amarok icon).  In my case the songs in question are some of those whose album art was set in iTunes (not sure if an old version caused the problem) but it's possible that other players/tag editors could reproduce the problem.

A few other notes:
- My ~/.kde/share/apps/amarok/albumcovers/tagcover folder shows numerous files that linux (ubuntu 7.10) doesn't recognize as images in file browser
- The 'bad' embedded image looks fine in iTunes (i have dual-boot to win xp)
- The 'bad' embedded image is set for entire album
- When the tagcover folder is cleared then a 'bad' song is played, the OSD shows the song info with no image.
- After going to iTunes and clearing cover art for a song on the album, coming back, clearing tagcover and playing the song, no new file is created in tagcover and the proper image is shown (from cover manager I assume).  Then if a 'bad' song from that album is played, it shows no cover and the 'bad' image file is created in tagcover. If I then play the 'good' song again, no image is shown. (looks like it finds the album by hash and chooses the 'bad' image file from tagcover)

After getting the SVN version of amarok, compiling, inserting debug statements, re-compiling, etc, I've found a fix for the issue:

In the code where the embedded image is found and saved to the tagcover folder, I added a test to create an image using the file.  Then I check if that image variable is null and if so, I delete the newly-saved file and return false.  

This seems to fix the problem and should also help for anything amarok seems as an embedded image that it can't understand.  I can post the actual code I added (4 lines) when I'm back at that computer.
Comment 1 Jeff Kerr 2007-10-31 21:29:08 UTC
Proposed fix (tested, works fine):
On save, test to make sure the extracted image is useable by amarok - if it's not, delete it.

metabundle.cpp
MetaBundle::EmbeddedImage::save
...
   const Q_LONG s = file.writeBlock( m_data.data(), m_data.size() );
   if( s >= 0 && Q_ULONG( s ) == m_data.size() ) {
      debug() << "EmbeddedImage::save " << file.name() << endl;
//NEW LINES START HERE...
      //Verify that the saved embedded image will work in Amarok            
      QImage image( file.name() );
      if(image.isNull()){
         debug() << "EmbeddedImage::save - embedded image saved but unusable - deleting..." << endl;
         file.remove();
         return false;
      }
//NEW LINES END HERE...
      return true;
   }
...
Comment 2 Seb Ruiz 2007-10-31 21:38:26 UTC
Please attach the patch as a diff. You can do this by issuing `svn diff > embedded_artwork.patch`.

Thanks
Comment 3 Jeff Kerr 2007-10-31 22:23:19 UTC
Created attachment 21964 [details]
Patch for the fix

Created with command:
svn diff > embedded_artwork.patch
Comment 4 Jeff Mitchell 2007-10-31 22:30:59 UTC
I don't think this patch should go in.

The problem there might be that Qt wasn't compiled with whatever format the album art is in.  For instance, it could be a gif but Qt wasn't compiled with gif support.  In this case the "bug" would be rather distribution dependent, and you may "solve" it simply by recompiling Qt into a new package.  But deleting the album art is permanent, and we shouldn't be assuming that the user wants that -- what if they want to play that image in iTunes later, from whence it came?

A better solution is the following:  when <wherever covers are used> is determining the image to use for the cover, check there to see if there is image data there but the image is null according to QImage.  If so, force it to use the "no cover" cover.

Comment 5 Jeff Kerr 2007-10-31 22:37:51 UTC
Actually, the fix isn't deleting the image out of the Mp3 file, just from
the folder where it extracted it, so it wouldn't affect iTunes.

Also, it was my understanding that album art is stored in another folder as
well - if you clear all files from your tagfolder folder, it still exists
when you run amarok and run the cover manager.

On 31 Oct 2007 21:31:00 -0000, Jeff Mitchell <kde-dev@emailgoeshere.com>
wrote:
[bugs.kde.org quoted mail]
Actually, the fix isn&#39;t deleting the image out of the Mp3 file, just from the folder where it extracted it, so it wouldn&#39;t affect iTunes.<br><br>Also, it was my understanding that album art is stored in another folder as well - if you clear all files from your tagfolder folder, it still exists when you run amarok and run the cover manager.
<br><br><div><span class="gmail_quote">On 31 Oct 2007 21:31:00 -0000, <b class="gmail_sendername">Jeff Mitchell</b> &lt;<a href="mailto:kde-dev@emailgoeshere.com">kde-dev@emailgoeshere.com</a>&gt; wrote:</span><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
------- You are receiving this mail because: -------<br>You reported the bug, or are watching the reporter.<br><br><a href="http://bugs.kde.org/show_bug.cgi?id=151609">http://bugs.kde.org/show_bug.cgi?id=151609</a><br><br>
<br><br><br>------- Additional Comments From kde-dev emailgoeshere com&nbsp;&nbsp;2007-10-31 22:30 -------<br>I don&#39;t think this patch should go in.<br><br>The problem there might be that Qt wasn&#39;t compiled with whatever format the album art is in.&nbsp;&nbsp;For instance, it could be a gif but Qt wasn&#39;t compiled with gif support.&nbsp;&nbsp;In this case the &quot;bug&quot; would be rather distribution dependent, and you may &quot;solve&quot; it simply by recompiling Qt into a new package.&nbsp;&nbsp;But deleting the album art is permanent, and we shouldn&#39;t be assuming that the user wants that -- what if they want to play that image in iTunes later, from whence it came?
<br><br>A better solution is the following:&nbsp;&nbsp;when &lt;wherever covers are used&gt; is determining the image to use for the cover, check there to see if there is image data there but the image is null according to QImage.&nbsp;&nbsp;If so, force it to use the &quot;no cover&quot; cover.
<br></blockquote></div><br>
Comment 6 Seb Ruiz 2007-10-31 22:44:26 UTC
afaict, no the patch only stops the writing of embedded artwork to our cache if it is invalid.
Comment 7 Seb Ruiz 2007-10-31 23:05:06 UTC
*** Bug 151657 has been marked as a duplicate of this bug. ***
Comment 8 Matt Rogers 2009-08-03 04:07:44 UTC
Thank you for taking the time to report this bug for Amarok. Amarok 1.4 is now unmaintained and will no longer see any improvements. Because of this, and the massive amount of changes Amarok has undergone throughout the 2.x series of releases, we are closing bugs that no longer apply to the 2.x series due to changes in functionality, the underlying architecture, or a conflict for the vision of Amarok 2.

We appreciate the time you took to provide feedback about Amarok 1.4 and will look forward to any feedback you may provided about Amarok 2. Thanks.