Last Comment Bug 743720 - Import Speex's audio resampler in the tree
: Import Speex's audio resampler in the tree
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Paul Adenot (:padenot)
:
Mentors:
Depends on: 753603
Blocks: 495040
  Show dependency treegraph
 
Reported: 2012-04-09 10:20 PDT by Paul Adenot (:padenot)
Modified: 2012-05-15 06:34 PDT (History)
5 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Import the resampler bits of the Speex codec in the tree (86.57 KB, patch)
2012-04-13 14:23 PDT, Paul Adenot (:padenot)
kinetik: review+
Details | Diff | Splinter Review
Updated with AUTHORS and about:license bits (90.56 KB, patch)
2012-04-23 11:42 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
v2 - Addressed khuey concerns (87.96 KB, patch)
2012-04-24 18:20 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
v3 - Addressed cpearce comments. (85.02 KB, patch)
2012-04-25 16:00 PDT, Paul Adenot (:padenot)
khuey: review+
Details | Diff | Splinter Review
rebased patch. (84.97 KB, patch)
2012-05-09 16:49 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Ready to land. (84.97 KB, patch)
2012-05-14 15:27 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review

Description Paul Adenot (:padenot) 2012-04-09 10:20:59 PDT
We need to import the Speex audio resampler bits in the tree, for the |playbackRate| member of the audio and video elements, and the Media Graph API.
Comment 1 Paul Adenot (:padenot) 2012-04-13 14:23:55 PDT
Created attachment 614912 [details] [diff] [review]
Import the resampler bits of the Speex codec in the tree

Maybe I should find a build peer to review that as well ?
Comment 2 Matthew Gregan [:kinetik] 2012-04-13 14:41:53 PDT
Comment on attachment 614912 [details] [diff] [review]
Import the resampler bits of the Speex codec in the tree

Looks good to me, but this does need a build peer to review it as well.

Please add a README_MOZILLA to media/libspeex_resampler that describes the provenance of the code and the version number (or changeset ID) the current import is at.

This code is under a BSD style license, so it'll be fine to import, but I think all imports of third party code need to be run past licensing@mozilla.org (http://www.mozilla.org/MPL/license-policy.html#Licensing_of_Third_Party_Code) and add an attribution blurb to about:license.
Comment 3 Paul Adenot (:padenot) 2012-04-13 16:34:55 PDT
Comment on attachment 614912 [details] [diff] [review]
Import the resampler bits of the Speex codec in the tree

Ted, I need a review from the Build module owner (or a peer) for this patch to land. I've also sent an email to licensing@mozilla.org, and I am waiting for their answer.
Comment 4 Paul Adenot (:padenot) 2012-04-23 11:42:30 PDT
Created attachment 617567 [details] [diff] [review]
Updated with AUTHORS and about:license bits

I've updated the code and added the original AUTHORS and COPYING files. Also added a README_MOZILLA file containing the revision and added a line in about:license.
Comment 5 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-04-24 06:41:52 PDT
Comment on attachment 617567 [details] [diff] [review]
Updated with AUTHORS and about:license bits

Review of attachment 617567 [details] [diff] [review]:
-----------------------------------------------------------------

Why do we need separate include and src directories here?

::: media/libspeex_resampler/Makefile.in
@@ +45,5 @@
> +
> +DIRS		= \
> +		include \
> +		src \
> +		$(NULL)

Please don't use tabs here.  Tabs are only necessary in rules.

::: media/libspeex_resampler/REAME_MOZILLA
@@ +2,5 @@
> +commit a6d05eb5.
> +
> +It consists in the audio resampling code (resampler.c) and its header files
> +dependancies. No changes have been made to those files, imported in the tree
> +using the update.sh script.

Please spell README correctly.

::: media/libspeex_resampler/include/Makefile.in
@@ +58,5 @@
> +endif
> +
> +EXPORTS_speex_resampler = \
> +		speex_resampler.h \
> +		$(NULL)

Again, no tabs please.

::: media/libspeex_resampler/src/Makefile.in
@@ +61,5 @@
> +
> +
> +CSRCS		= \
> +		resample.c \
> +		$(NULL)

And again.
Comment 6 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-04-24 11:49:59 PDT
Comment on attachment 617567 [details] [diff] [review]
Updated with AUTHORS and about:license bits

Per IRC discussion Paul is going to remove the include/src split.
Comment 7 Paul Adenot (:padenot) 2012-04-24 18:20:27 PDT
Created attachment 618119 [details] [diff] [review]
v2 - Addressed khuey concerns

This updated patch hopefully addresses your concerns : include/ has been merged into src/, tabs has been replaced by space in new files (I've kept tabs in files with tabs, for consistency). And I've fixed the typo in the file name.
Comment 8 Chris Pearce (:cpearce) 2012-04-25 15:21:55 PDT
Comment on attachment 618119 [details] [diff] [review]
v2 - Addressed khuey concerns

Review of attachment 618119 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libspeex_resampler/Makefile.in
@@ +1,1 @@
> +# ***** BEGIN LICENSE BLOCK *****

You should be using the new MPL2 boiler plate rather than MPL1.1 on all new source files added to the tree:

http://www.mozilla.org/MPL/headers/
Comment 9 Paul Adenot (:padenot) 2012-04-25 16:00:46 PDT
Created attachment 618472 [details] [diff] [review]
v3 - Addressed cpearce comments.
Comment 10 Paul Adenot (:padenot) 2012-05-09 16:49:04 PDT
Created attachment 622579 [details] [diff] [review]
rebased patch.
Comment 11 Paul Adenot (:padenot) 2012-05-14 15:27:06 PDT
Created attachment 623847 [details] [diff] [review]
Ready to land.

(just adding proper commit message and my name).
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-05-14 16:01:40 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b94dc0ad1c2

Also, to make life easier for those checking in patches on your behalf, please follow the directions below to make sure your patches have all the right metadata in them. Thanks!
https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in
Comment 13 Ed Morley [:emorley] 2012-05-15 06:34:51 PDT
https://hg.mozilla.org/mozilla-central/rev/1b94dc0ad1c2

Note You need to log in before you can comment on or make changes to this bug.