Closed Bug 779997 Opened 8 years ago Closed 7 years ago

Import SoundTouch audio processing library in mozilla-central

Categories

(Core :: Audio/Video, defect)

16 Branch
Other
Other
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox18 --- unaffected
firefox19 --- disabled
firefox20 --- fixed

People

(Reporter: padenot, Assigned: padenot)

References

Details

Attachments

(2 files, 6 obsolete files)

For playbackRate, we need to use SoundTouch. This bug is about importing the files in the tree.
Attached patch v0 (obsolete) — Splinter Review
about:license things may have to be changed.
Attached patch v1 (obsolete) — Splinter Review
Attachment #648496 - Attachment is obsolete: true
Attachment #648763 - Flags: review?(khuey)
I need some background here before reviewing.  Why does this code need to be in a separate shared library?
Kyle, it is LGPL-licensed. Gerv and the legal folks have determined that it needs to be in a shared library to comply with the license. The discussion happened in bug 764495.
So, to be clear, the shared library this code becomes will be shipped under the LGPL, as opposed to the rest of Mozilla, which is tri-licensed?

If that's the case, we need to do a bunch more work here, because we do things put the tri-license in the copyright field on Windows DLLs.
I think you're right. We cannot re-license code that we take from somewhere on the internet, right?

Are you referring to an .rc files such as this one: http://mxr.mozilla.org/mozilla-central/source/gfx/angle/src/libEGL/libEGL.rc, to put infos in the DLL ?
Yes.  I didn't realize you could override them by providing your own.  The default one is generated by http://mxr.mozilla.org/mozilla-central/source/config/version_win.pl#237.

So this code will need a custom rc file.
Comment on attachment 648763 [details] [diff] [review]
v1

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

This needs the custom .rc file that we talked about.

Also we need to add soundtouch.dll to dependentlibs.list.

Other than that it looks good.

::: browser/installer/windows/nsis/shared.nsh
@@ +1030,5 @@
>    Push "nspr4.dll"
>    Push "nssdbm3.dll"
>    Push "mozsqlite3.dll"
>    Push "xpcom.dll"
> +  Push "soundtouch.dll"

I'd like Rob Strong to glance at this to make sure that the order doesn't matter here or something weird.

Also there are several dlls missing ...

::: layout/media/symbols.def.in
@@ +104,5 @@
>  speex_resampler_strerror
>  #endif
> +#ifdef MOZ_SOUNDTOUCH
> +
> +#endif

....
Attachment #648763 - Flags: review?(khuey) → review-
Attached patch v2: .rc file not picked up. (obsolete) — Splinter Review
I added the RCFILE and RESFILE variable assignment in the makefile and the .rc file (adapted from the one present in the original soundtouch sources).

Somehow, they are not picked up by the build system, and that result in a .dll that has the default information. Troubleshooting that issue, I noticed that the same thing happens with the libEGL{v2,}.dll we have in the tree, that _should_ be copyright Google, but are shown as copyright Mozilla.
Attachment #648763 - Attachment is obsolete: true
Attached patch v2.1: .rc file not picked up. (obsolete) — Splinter Review
(Spotted a typo when checking I had attached the right patch.)
Attachment #651239 - Attachment is obsolete: true
Any news on the .rc problem?
I tried to apply the patch and build with it, but the build fails.
I guess the attached patch on the bug was a bit old or something. This compiles
on all platform on try.
Attachment #651447 - Attachment is obsolete: true
That patch works fine for me if I uncomment the relevant lines in the makefile.  It's pretty messed up that we have to point RESFILE at a non-existent file to get RCFILE to get picked up by the build though.
Indeed, this compiles fine locally, but not on try. I'll try to install the same VS version to check what's going on.
Disregard the previous comment, it was just me being stupid.

I've checked that libsoundtouch is picked up by the dependentlibs.py script, and
addressed the other comment.

I've also added the license bits that Gerv told me to put in about:license.
Attachment #661824 - Flags: review?(khuey)
Attachment #655634 - Attachment is obsolete: true
Attachment #661824 - Flags: review?(robert.bugzilla)
Comment on attachment 661824 [details] [diff] [review]
Import SoundTouch Library in the tree. r=

Robert, could you look at the Windows installer bits in this patch?
Attachment #661824 - Flags: review?(robert.bugzilla)
Attachment #661824 - Flags: review?(robert.bugzilla)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) [Away until September 18th (MozCamp EU, then vacation)] from comment #9)
> Comment on attachment 648763 [details] [diff] [review]
> v1
> 
> Review of attachment 648763 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This needs the custom .rc file that we talked about.
> 
> Also we need to add soundtouch.dll to dependentlibs.list.
> 
> Other than that it looks good.
> 
> ::: browser/installer/windows/nsis/shared.nsh
> @@ +1030,5 @@
> >    Push "nspr4.dll"
> >    Push "nssdbm3.dll"
> >    Push "mozsqlite3.dll"
> >    Push "xpcom.dll"
> > +  Push "soundtouch.dll"
> 
> I'd like Rob Strong to glance at this to make sure that the order doesn't
> matter here or something weird.
> 
> Also there are several dlls missing ...
This is done before installing so we can notify the user prior to installing that they will likely need to restart their system if any of these files are in use. It doesn't list them all because it has to make copies of the dll which can take a while for the larger ones. I also don't want to add to the list unless we know it is necessary and I plan on reworking this code anyways.
Comment on attachment 661824 [details] [diff] [review]
Import SoundTouch Library in the tree. r=

Don't bother with adding the dll to the check in shared.nsh per previous comment
Attachment #661824 - Flags: review?(robert.bugzilla) → review-
Addressed rstrong comments.
Attachment #663177 - Flags: review?(robert.bugzilla)
Attachment #663177 - Flags: review?(khuey)
Attachment #661824 - Attachment is obsolete: true
Attachment #661824 - Flags: review?(khuey)
Comment on attachment 663177 [details] [diff] [review]
Import SoundTouch Library in the tree. r=

r+ for the installer/* changes
Attachment #663177 - Flags: review?(khuey) → review+
Comment on attachment 663177 [details] [diff] [review]
Import SoundTouch Library in the tree. r=

I just noticed that I cancelled the wrong review. Kyle, can you make sure this is alright? Changes from last patch are mentioned in comment 17.
Attachment #663177 - Flags: review?(robert.bugzilla) → review?(khuey)
Backed out because reviewers names were missing from the commit message in https://hg.mozilla.org/integration/mozilla-inbound/rev/a100edfd3ca0, relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/2df5960779db.
Blocks: 809435
https://hg.mozilla.org/mozilla-central/rev/2df5960779db
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Depends on: 809766
Depends on: 809853
Blocks: 809833
The library is not used by anything at the moment, and we're going to merge to aurora. Are there plans to use this library in FF19 or is that for later? If the former, we should really disable building and shipping it.
(In reply to Mike Hommey [:glandium] from comment #27)
> If the former

err, latter.
Would disabling it in the file system be sufficient? It would not be compiled nor shipped this way, I guess.
(In reply to Paul ADENOT (:padenot) from comment #29)
> Would disabling it in the file system be sufficient? It would not be
> compiled nor shipped this way, I guess.

You'd need to remove the -lsoundtouch from the libxul linkage command line.
This is green on try. I plan to enable it and land the patch that uses the
library right after the uplift.
Attachment #683095 - Flags: review?(mh+mozilla)
Attachment #683095 - Flags: review?(mh+mozilla) → review+
Comment on attachment 683095 [details] [diff] [review]
Disable build and linking of libsoundtouch as it is not used for now. r=

Paul: I think you forgot to add the soundtouch lib to removed-files.in so that the file gets removed on update via the installer or the in-app update.
I don't think this matters much for a file that was never shipped in a release or a beta.
(In reply to Phil Ringnalda (:philor) from comment #32)
> https://hg.mozilla.org/mozilla-central/rev/e35029dcac2d

Note this didn't make it to aurora, we need an uplift.
Comment on attachment 683095 [details] [diff] [review]
Disable build and linking of libsoundtouch as it is not used for now. r=

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 779997
User impact if declined: libsoundtouch would be shipped with aurora and subsequent releases, but not used. This makes the packages bigger in size.
Testing completed (on m-c, etc.): Green try.
Risk to taking this patch (and alternatives if risky): Not risky, the code was not exercised anyways. It's only a build system change, the removal of a library.
String or UUID changes made by this patch: None.
Attachment #683095 - Flags: approval-mozilla-aurora?
Comment on attachment 683095 [details] [diff] [review]
Disable build and linking of libsoundtouch as it is not used for now. r=

[Triage Comment]
Low risk build system change in support of smaller package sizes. Approving for Aurora 19.
Attachment #683095 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
So this is disabled on m-c now and will soon be disabled on aurora too, right?
Ryan, this is not disabled on central, because I'm about to land a big patch that uses this library. This is disabled on aurora because I lacked time to land the patch before the uplift and we did not want to increase the size of the package.
OK, I guess I was confused by comment 32 then.
And backed out since I landed the feature patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/98a1b656277b
You need to log in before you can comment on or make changes to this bug.