| Summary: | AFT tagger always recalculating a new tag in m4a/mp4 files | ||
|---|---|---|---|
| Product: | [Applications] amarok | Reporter: | Stefano Pettini <stefano> |
| Component: | general | Assignee: | Amarok Developers <amarok-bugs-dist> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | matej, ralf-engels, stefano |
| Priority: | NOR | ||
| Version: | 2.8-git | ||
| Target Milestone: | 2.9 | ||
| Platform: | Compiled Sources | ||
| OS: | Linux | ||
| Latest Commit: | http://commits.kde.org/amarok/dca658b98adaa843699dfbcc267dac0c2f00b350 | Version Fixed In: | |
| Sentry Crash Report: | |||
| Attachments: | Patch that solves the bug, 3 lines changed | ||
Created attachment 86116 [details]
Patch that solves the bug, 3 lines changed
Please put the bug as confirmed, review and apply the attached patch. I replicated the bug again and verified that the patch is working on a bunch of .m4a files. I verified both adding, regenerating and deleting the AFT. If no command is given, the existing AFT is now detected properly and the file is not overwritten.
This shows that the problem is fixed. Thank you, Stefano INFO: Processing file /home/xxx/Music/Test/Code 64/2003 - Storm/12 - Reaktor (Original mix).m4a INFO: Temporary file is at /home/xxx/Music/Test/Code 64/2003 - Storm/12 - Reaktor (Original mix).m4a.amarok-afttaggertemp.pid-20234.random-xrczmmvk.m4a INFO: File is a MP4 file, opening... INFO: Found an existing AFT identifier: ----:com.apple.iTunes:Amarok 2 AFTv1 - amarok.kde.org INFO: AFT identifier is version 1 INFO: ID is current INFO: Cleaning up... INFO: Processing file /home/xxx/Music/Test/Code 64/2003 - Storm/13 - Without You (Zprochek mix).m4a INFO: Temporary file is at /home/xxx/Music/Test/Code 64/2003 - Storm/13 - Without You (Zprochek mix).m4a.amarok-afttaggertemp.pid-20234.random-fbNJp3UM.m4a INFO: File is a MP4 file, opening... INFO: Found an existing AFT identifier: ----:com.apple.iTunes:Amarok 2 AFTv1 - amarok.kde.org INFO: AFT identifier is version 1 INFO: ID is current INFO: Cleaning up... INFO: Processing file /home/xxx/Music/Test/Code 64/2003 - Storm/14 - Without You (Spektron mix).m4a INFO: Temporary file is at /home/xxx/Music/Test/Code 64/2003 - Storm/14 - Without You (Spektron mix).m4a.amarok-afttaggertemp.pid-20234.random-OLe7wC54.m4a INFO: File is a MP4 file, opening... INFO: Found an existing AFT identifier: ----:com.apple.iTunes:Amarok 2 AFTv1 - amarok.kde.org INFO: AFT identifier is version 1 INFO: ID is current INFO: Cleaning up... INFO: All done, exiting... Thank you for the fix. Could you please submit your patch to http://reviewboard.kde.org ? We do not handle patches in bugzilla. Sorry for the involuntary changes, I really have no idea what is wrong with my browser, since yesterday it keeps changing all bug titles :( Some weird auto-complete I have no idea how to get rid of... Patch posted in review 117580. Git commit dca658b98adaa843699dfbcc267dac0c2f00b350 by Daniel Meltzer. Committed on 08/05/2014 at 20:31. Pushed by dmeltzer into branch 'master'. Properly Calculate and store AFT tags in mp4 files Amarok previously was looking in the wrong spot for the AFT version. Now it calculates the offset properly. REVIEW: 117580 M +2 -0 ChangeLog M +7 -5 utilities/afttagger/AFTTagger.cpp http://commits.kde.org/amarok/dca658b98adaa843699dfbcc267dac0c2f00b350 The patch also fixes your %2 issue, amarok was passing two ints to QString::arg, but the overloads for multiple values take strings. We were matching a strange overload instead. |
Basically for m4a/mp4 files the tag is inserted but it's not recognized if the tool runs again. So the file is modified and a possibly-new tag is rewritten. Please see: INFO: File is a MP4 file, opening... INFO: Found an existing AFT identifier: ----:com.apple.iTunes:Amarok 2 AFTv1 - amarok.kde.org INFO: AFT identifier is version -1 INFO: Upgrading AFT identifier from version -1 to version %2 INFO: Removing current AFT frame INFO: Adding new field and saving file with UID: 2f348a8a6cc4fa16747521edfed28a2babcdabcdabcdabcdabcd INFO: Safe-saving file INFO: Cleaning up... INFO: Processing file /home/xxx/Music/Test/Code 64/2003 - Storm/14 - Without You (Spektron mix).m4a INFO: Temporary file is at /home/xxx/Music/Test/Code 64/2003 - Storm/14 - Without You (Spektron mix).m4a.amarok-afttaggertemp.pid-10467.random-Z2TE3L1d.m4a I had a look at the code, this is because we prefix the tag with "----:com.apple.iTunes:". I don't understand the reason for this but I assume there is a valid one. However when we try to detect the version number, we blindly use the char at position 13. This is valid only in case the tag name begins with "AMAROK 2 AFT", but this is not the case for MP4 as it has the iTunes prefix. We should basically fix it by saving the output of: else if( ukey.find( "AMAROK 2 AFT" ) != -1 ) and adding it to that 13. I haven't verified Amarok code, only the code of the aft tagger. Minor thing: I also don't understand why the "%2" placeholder is not replaced in the info message ... Reproducible: Always Steps to Reproduce: 1. File a m4a file. 2. Run the aft tagger on it, in verbose mode. 3. Run the aft tagger on it, on and on again. Actual Results: The AFT tag is added/replaced all the times. Expected Results: The AFT tag should be added only during the first run, and the file should not be modified anymore.