Bug 298332 - Writing covers to files should be done in a separate thread to avoid UI freezing.
Summary: Writing covers to files should be done in a separate thread to avoid UI freez...
Status: RESOLVED FIXED
Alias: None
Product: amarok
Classification: Applications
Component: general (show other bugs)
Version: 2.5.0
Platform: Debian testing Linux
: NOR minor
Target Milestone: ---
Assignee: Amarok Developers
URL:
Keywords: regression
Depends on:
Blocks:
 
Reported: 2012-04-18 04:35 UTC by Mathieu Roy
Modified: 2012-05-08 13:32 UTC (History)
2 users (show)

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


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Mathieu Roy 2012-04-18 04:35:28 UTC
Almost each time I download a cover, amarok freezes for a sec or even more after informing me the cover was successfully downloading.

It's not blocker at all, it's just weird, as if the interface was waiting for the cover to be properly saved to be refreshed and available, as if it was on the same thread.

Reproducible: Always

Steps to Reproduce:
1. Select in the current playlist a song without cover
2. Download a new cover (selected from google)
3. Wait for it
Actual Results:  
Freezes for a sec or more.

Expected Results:  
No freeze at all. The software interface should not be affected, slowed or froze by the download and cover saving activity.
Comment 1 Myriam Schweingruber 2012-04-18 12:22:08 UTC
Please upgrade your version, 2.4.1 is already quite old, current is Amarok 2.5, release last December.
Comment 2 Myriam Schweingruber 2012-04-18 12:41:07 UTC
Looks like our posts crossed each other. Do you still have this with Amarok 2.5.0? I can't reproduce this here with current Amarok 2.5-git
Comment 3 Mathieu Roy 2012-04-18 12:44:05 UTC
I do, I had 2.5.0 from the start, I just forgot to set the relevant field in my initial submission.
Comment 4 Myriam Schweingruber 2012-04-18 16:11:39 UTC
I can't reproduce this at all with Amarok 2.5-git. Do you have a slow connection?
Comment 5 Mathieu Roy 2012-04-18 16:18:51 UTC
No. And I'm not sure connectivity is relevant here.
The problem is not that cover download is slow. The problem is that, whenever a cover is succesfully downloaded, the interface is not responsive until it is definitely stored wherever it goes.
It should not. The interface should stay responsive all along. It should not wait for the cover at any moment.
Comment 6 Myriam Schweingruber 2012-04-18 17:03:03 UTC
Well, it doesn't wait for me, the interface is responsive while it downloads covers, so I expect that is already solved in the developer version.

Do you by any chance write the cover to the file as an id3 tag?
Comment 7 Mathieu Roy 2012-04-18 17:18:57 UTC
>Well, it doesn't wait for me, the interface is responsive while it downloads covers, so I expect
>that is already solved in the developer version.

Hum, I'm not sure you can safely assume a bug is fixed this way :-)
We have no clue that we have or have not similar setup, the bug apparently never even occured on your instance. Bug may be fixed by inadvertance but I think we'd best to try to check first.

>Do you by any chance write the cover to the file as an id3 tag?

Good pick.
No bug if I untick the checkbox  "Write covers to file".
Bug back if I tick the checkbox.

In the terminal, I get some noise like

amarok(21700)/KSharedDataCache: Unable to free up memory for "file:///home/klink/.kde/share/apps/amarok/albumcovers/cache/90@eb481cbdb6361793b96654524d828fd9:99x100b5" 
amarok(21700)/KSharedDataCache: Unable to free up memory for "file:///home/klink/.kde/share/apps/amarok/albumcovers/cache/90@d400077a02d1690047c9af770064226a:100x100b5" 
QWidget::setMinimumSize: (Media Sources dock/BrowserDock) Negative sizes (141,-1) are not possible

I'm don't think normal that Amarok is unable to free up memory, considering the following:

  $ vmstat 
procs -----------memory---------- ---swap-- -----io---- -system-- ----cpu----
 r  b   swpd   free   buff  cache   si   so    bi    bo   in   cs us sy id wa
 0  0   1816 538220 138092 5815404    0    0    10    20  153   61 12  6 81  1
8303 19:17 klink@bender: ~
  $ free
             total       used       free     shared    buffers     cached
Mem:       8199632    7661140     538492          0     138108    5815396
-/+ buffers/cache:    1707636    6491996
Swap:       975868       1816     974052

So is this issue really fixed in 2.6?
(note that, in general, interface seems unresponsive whenever amarok write to files, which is, in my humble opinion, a bug)
Comment 8 Myriam Schweingruber 2012-04-18 17:33:36 UTC
Thank you for the feedback.If I had just assumed it to be fixed without verification I would have closed it, no? :-)

While I still can't reproduce this here, the output you sent looks very much like a bug indeed, I marked this as a regression.

P.S. Please do not Cc me for bugs, I am subscribed already.
Comment 9 Mathieu Roy 2012-04-18 17:45:58 UTC
>Thank you for the feedback.If I had just assumed it to be fixed without verification I would have closed it, no? :-)

Correct :-)
Tell me if I can provide anything else useful.

>While I still can't reproduce this here, the output you sent looks very much like a bug indeed, I marked this as a regression.

Ok. This is anyway not at all a blocker bug. 
Thanks anyway for all the good work, I use Amarok since several years and I'm very happy with it, that's a nice piece of software.

(>P.S. Please do not Cc me for bugs, I am subscribed already.

Hum, I just used the web interface, without doing anything special)
Comment 10 Daniel Faust 2012-04-20 21:25:36 UTC
Just to make this clear. When a cover gets written to an audio file it is usually written to the beginning, making it necessary to rewrite the complete file. This can take a moment especially for big files. Amarok 2.6 will support writing covers to flac files so this issue might increase. Also if you write covers to multiple files at once the writing time will increase.
Unfortunately Amarok doesn't use an extra thread for this so the user interface will be blocked for a moment.
The question is, is the blocking really that long that it must be a bug or could it be due to the mechanisms I described above?
Comment 11 Mathieu Roy 2012-04-21 03:28:34 UTC
Hello,

IMHO, it's a bug to have a task like altering data done on the same thread as refreshing the interface. 
In general, any activity related to the database should be on a distinct thread of the UI. There's nothing more puzzling to the end user than getting the UI unresponsive, whatever the cause. If the UI should not allow user input at a specific moment while working on the music database, it's probably best practice to have a responsive UI that says so than just an UI frozen until the work on the database is done.

For the record, I'm hitting this issue with ogg files around 7 Mb while downloading covers of 700x700 pixels and such on a AMD Athlon(tm) II X4 640 CPU. I would dare to imagine what would be the result on a way older workstation with bigger files.
Comment 12 Mathieu Roy 2012-04-21 03:30:11 UTC
(s/would dare to/wouldn't dare to/)
Comment 13 Daniel Faust 2012-04-21 07:45:07 UTC
I'm not trying to argue whether this issue should be fixed or not. You are right, those operations should be done in a separate thread. All I'm trying to do is understand what the issue is exactly and prioritize this bug report accordingly. You have to understand that our resources are limited.
Comment 14 Matěj Laitl 2012-05-08 13:32:36 UTC
Git commit 8dcfb77d2c9995a6e172679161a816a29bb855d4 by Matěj Laitl.
Committed on 08/05/2012 at 15:29.
Pushed by laitl into branch 'master'.

Move WriteTagsJob out of IpodCollection and use it everywhere

CHANGES:
 * Album cover images are written in background to prevent freezes
FIXED-IN: 2.6
DIGEST: Amarok now writes back covers in background to prevent freezes

M  +1    -0    ChangeLog
M  +18   -4    shared/MetaTagLib.h
M  +1    -0    shared/MetaValues.h
M  +1    -0    src/CMakeLists.txt
M  +9    -1    src/core-impl/collections/db/sql/SqlMeta.cpp
M  +0    -1    src/core-impl/collections/ipodcollection/CMakeLists.txt
M  +2    -2    src/core-impl/collections/ipodcollection/IpodMeta.cpp
M  +0    -2    src/core-impl/collections/ipodcollection/IpodMeta.h
R  +2    -3    src/core-impl/collections/support/jobs/WriteTagsJob.cpp [from: src/core-impl/collections/ipodcollection/jobs/WriteTagsJob.cpp - 088% similarity]
R  +5    -4    src/core-impl/collections/support/jobs/WriteTagsJob.h [from: src/core-impl/collections/ipodcollection/jobs/WriteTagsJob.h - 087% similarity]
M  +12   -3    src/core-impl/meta/file/File_p.h

http://commits.kde.org/amarok/8dcfb77d2c9995a6e172679161a816a29bb855d4