Bug 279493

Summary:	[Patch] Request for optional maximum dimensions for embedded cover art
Product:	[Applications] amarok	Reporter:	Kevin <tool.dredg>
Component:	Collections/Local	Assignee:	Amarok Developers <amarok-bugs-dist>
Status:	RESOLVED FIXED
Severity:	normal	CC:	hessijames, ralf-engels, stuffcorpse
Priority:	NOR
Version:	2.4.3
Target Milestone:	2.5
Platform:	Arch Linux
OS:	Linux
Latest Commit:	http://commits.kde.org/amarok/69c568773360ba70f11334a1c70deae0cacb5eff	Version Fixed In:	2.6
Sentry Crash Report:
Attachments:	Adds two small options for cover embedding into $HOME/.kde4/share/config/amarokrc

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