Bug 426558

Summary: abcde aac/m3a tagging post encoding (With Patch)
Product: [Fedora] Fedora Reporter: Bill Adams <gofish>
Component: abcdeAssignee: Ville Skyttä <ville.skytta>
Status: CLOSED WONTFIX QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: low Docs Contact:
Priority: low    
Version: 7   
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2008-01-23 16:33:46 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Attachments:
Description Flags
Patch to use AtomicParsley to Post-Tag Tracks
none
Patch to the abcde Spec File
none
New Patch for Post-Tagging M4A Files none

Description Bill Adams 2007-12-22 02:38:04 UTC
Description of problem:

abcde is not capable of post-tagging aac/m3a tracks. Attached is a patch to use
AtomicParsley to post-tag m4a tracks. I suppose an AtomicParsley RPM will need
to be generated to -- although the patch checks for AtomicParsley and falls back
to the previous method when it is not available.


Version-Release number of selected component (if applicable):

abcde-2.3.99.6-3.fc7


How reproducible:

always


Steps to Reproduce:
1. abcde -o m3a
  
Actual results:
No tags.

Expected results:
Tags.


Additional info:

Please apply to FC7 and FC8.

Comment 1 Bill Adams 2007-12-22 02:38:04 UTC
Created attachment 290282 [details]
Patch to use AtomicParsley to Post-Tag Tracks

Comment 2 Bill Adams 2007-12-22 02:39:24 UTC
Created attachment 290283 [details]
Patch to the abcde Spec File

Bump the version, apply the patch, added a changelog entry.

Comment 3 Ville Skyttä 2007-12-22 19:33:44 UTC
Have you tried sending this patch upstream (jesus.climent)?

It seems that abcde already contains aac/m4a tagging code using faac, it's just
disabled currently.  I'd feel much more comfortable with enabling that than
adding a patch that upstream really should look into first.  More info:
https://bugs.launchpad.net/ubuntu/+source/abcde/+bug/138367
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=449045

Could you test if that patch works for you?

A couple of quick comments about your patch:

+			#  case core dumps with AtomicParsely 0.9.0

s/Parsely/Parsley/

+		echo "WARING: $METAAAC Not Found Not Post-Tagging!"

s/WARING/WARNING
s/Not Found Not Post-Tagging/not found, not post-tagging/

The METAAAC variable name sounds odd to me, I think a better choice would be
ATOMICPARSLEY.


Comment 4 Bill Adams 2007-12-23 23:50:53 UTC
Created attachment 290313 [details]
New Patch for Post-Tagging M4A Files

Good day.

o Fixed the spelling errors.

o Changed METAAAC (I was going off off METAFLAC) to ATOMICPARSLEY.

I added post-encoding tagging because of the text in the code "Quick hack
to...." I figured that post-tagging was preferred. If AtomicParsley is not
present, it falls back to inline tagging.

I'll send the patch to the supplied email address and see if it gets accepted.

Thanks for your feedback.

Comment 5 Bill Adams 2008-01-12 22:17:55 UTC
FYI: I sent an email on Dec. 23 to jesus.climent at hispalinux.es and have still
heard no response. I just sent another email trying to get a response to if he
will apply the patch to the main branch or not.



Comment 6 Ville Skyttä 2008-01-20 21:24:14 UTC
More feedback:

- If AtomicParsley is not available, an ugly error message is printed; I think
that's undesirable: which: no AtomicParsley in ($PATH goes here...)

- If it's not found, another "WARNING: AtomicParsley Not Found Not
Post-Tagging!" message is printed.  Why is this worth a warning in the first
place?  AFAICT faac does the job just fine.  If you want to keep it, I think
some rephrasing would be in order, eg. "WARNING: AtomicParsley not found,
falling back to inline tagging."

- After applying your patch to my work in progress package by hand, if
AtomicParsley was not available, abcde did not fall back to inline faac tagging
(it did not tag at all), and failed to create the real target "Author-Albumname"
directory, leaving just the abcde.xxxxxxxx dir and all the cruft in it behind.

- The core dump commentary in the patch does not give me a warm fuzzy feeling
about AtomicParsley.

Having played around with m4a tagging for a while now and seeing that faac is
the only supported m4a encoder in abcde, I must say that I fail to see the
reason to introduce AtomicParsley support.  faac appears to do the job just
fine, whereas AtomicParsley is another dependency, adds more code, and the
implementation seems slightly broken at the moment.

If you wish to continue improving the patch, please rebase it with abcde
2.3.99.6-5 which will soon hit F8 testing updates and the devel repository.  In
the meantime, you can grab it from CVS or
http://koji.fedoraproject.org/koji/taskinfo?taskID=360697

Comment 7 Bill Adams 2008-01-22 20:18:54 UTC
Thanks for the feedback. All good points.

Let's drop this patch/bug in favor of enabling/using faac tagging.

Thanks!


Comment 8 Ville Skyttä 2008-01-23 16:33:46 UTC
Ok, let's do that, thanks for the quick reply.