Bug 279493 - [Patch] Request for optional maximum dimensions for embedded cover art
Summary: [Patch] Request for optional maximum dimensions for embedded cover art
Status: RESOLVED FIXED
Alias: None
Product: amarok
Classification: Applications
Component: Collections/Local (show other bugs)
Version: 2.4.3
Platform: Arch Linux Linux
: NOR normal
Target Milestone: 2.5
Assignee: Amarok Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-06 00:11 UTC by Kevin
Modified: 2012-04-29 10:05 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In: 2.6
Sentry Crash Report:


Attachments
Adds two small options for cover embedding into $HOME/.kde4/share/config/amarokrc (2.49 KB, patch)
2011-08-06 02:41 UTC, Kevin
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin 2011-08-06 00:11:44 UTC
Version:           2.4.3 (using KDE 4.7.0) 
OS:                Linux

Hello. I've noticed that whenever I try to set a custom cover, the version that is embedded into the file is automatically resized if it is over 200x200 pixels (and also that it is not embedded if the file is under 1 MB, but this isn't as big of a deal to me). 

I'd like to propose an option to set a custom maximum size for album art. It's agreeable that there should be an optional maximum, but I personally like to have large art for use with different players and I'm sure others have different preferences (obviously there's at least two - my own [I think 500x500 is large enough] and the developer who put in the 200x200 limit :-))

Reproducible: Always

Steps to Reproduce:
Try to set a custom cover with dimensions above 200x200, then open the audio file with a separate program which can edit tags (I used Guayadeque). The embedded cover art will be 200x200.

Actual Results:  
Embedded cover art is 200x200 pixels max.

Expected Results:  
Allowed a user-configurable maximum size.
Comment 1 Kevin 2011-08-06 02:41:18 UTC
Created attachment 62597 [details]
Adds two small options for cover embedding into $HOME/.kde4/share/config/amarokrc
Comment 2 Kevin 2011-08-06 02:43:17 UTC
Comment on attachment 62597 [details]
Adds two small options for cover embedding into $HOME/.kde4/share/config/amarokrc

I'm by no means a developer, but I tried to patch this myself. The attached patch adds the following two user-configurable options to ~/.kde/share/config/amarokrc:

1. "Write Back Cover Dimensions" which allows a maximum height/width to be specified
2. "Write Back Cover MinSize", which allows a user to specify whether or not to obey the 1 MB file minimum for embedding

As I said, I'm not a developer so I haven't been able to figure out how to allow modifying these in the Amarok settings (I had something that worked but I couldn't figure out how to set the width of a QSpinBox so it just took up the whole dialog width and looked terrible). Perhaps somebody who's adept at GUI development can do this, or it can just remain a behind-the-scenes option.
Comment 3 Ralf Engels 2011-08-08 09:49:32 UTC
Hi,
the reason for this conditions was that the size of an audio file should not grow too much by adding covers.

That would happen with small audio files or big pictures.

Your patch looks OK at the first glance.
Comment 4 Myriam Schweingruber 2011-08-09 22:48:15 UTC
This is actually a regression and therefore a bug as this was working before.
Comment 5 Ralf Engels 2011-08-10 09:50:35 UTC
I have to humbly disagree.
I wrote the code to write back image covers end of last year and it's unchanged since.

Note: we are talking about images that are written to the audio file.
Downloading images, setting images or reading images is not affected.
Comment 6 Kevin 2011-08-10 10:29:56 UTC
I guess I should clarify. As Ralf said, the artwork saved in the Amarok database is not resized. If I click on the artwork in the context pane, a window pops up with the full-sized artwork as read from the database. It's the artwork that is actually saved to the music files that is resized to no more than 200x200 and is what this request was for. Phones or tablets read the embedded artwork, and if it's too low-res then it doesn't look very pleasing to the eye on these devices.

The patch I've written (should) keep the default behavior unless the key is changed in $HOME/.kde/share/config/amarokrc. 

Hopefully that clears up any possible confusion. Thanks!
Comment 7 Daniel Faust 2012-04-08 18:03:07 UTC
Hi,
I created a review request at https://git.reviewboard.kde.org/r/104513/ which includes the user defined cover dimensions part.
Even though it doesn't has much impact on me, I don't understand the 1MB file size limit either. A cover is only a few kB big (especially if you use the default setting of 200px) so if somebody wants to write covers to a small file why shouldn't he. He might actually be upset that the cover didn't got written without any feedback.
Comment 8 Daniel Faust 2012-04-29 10:05:24 UTC
Git commit 69c568773360ba70f11334a1c70deae0cacb5eff by Daniel Faust.
Committed on 29/04/2012 at 11:51.
Pushed by dfaust into branch 'master'.

Some changes to the handling of cover art reading and writing

The maximum dimensions for embedded covers are now configurable.
When writing covers to files, all existing covers will be replaced.
FIXED-IN: 2.6
REVIEW: 104513
GUI: New configuration option for the maximum cover size in the 'local collection' tab

M  +2    -0    ChangeLog
M  +55   -213  shared/tag_helpers/ASFTagHelper.cpp
M  +34   -45   shared/tag_helpers/ID3v2TagHelper.cpp
M  +17   -17   shared/tag_helpers/MP4TagHelper.cpp
M  +2    -0    shared/tag_helpers/TagHelper.h
M  +4    -31   shared/tag_helpers/VorbisCommentTagHelper.cpp
M  +4    -0    src/amarokconfig.kcfg
M  +2    -2    src/core-impl/collections/db/sql/SqlMeta.cpp
M  +29   -12   src/dialogs/CollectionSetup.cpp
M  +7    -3    src/dialogs/CollectionSetup.h

http://commits.kde.org/amarok/69c568773360ba70f11334a1c70deae0cacb5eff