Closed Bug 1176300 Opened 5 years ago Closed 5 years ago

Move libsoundtouch to lgpllibs

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox41 --- affected
firefox42 --- fixed

People

(Reporter: qdot, Assigned: qdot)

References

Details

Attachments

(4 files, 4 obsolete files)

As discussed in bug 1157768, we want to keep libsoundtouch in a separate library for lgpl coverage. Once bug 1157768 lands, move libsoundtouch to the lgpllibs library.
Comment on attachment 8628627 [details] [diff] [review]
Patch 1 (v2) - Add lgpllibs library to build system

This is all stuff that was in Bug 1157768, but moved it over here so we build in soundtouch first since it's built for all platforms.
Attachment #8628627 - Flags: review?(mh+mozilla)
Comment on attachment 8628625 [details] [diff] [review]
Patch 2 (v1) - Move libsoundtouch in lgpllibs

Added libsoundtouch to lgpllibs, and also cleaned up some of the build commands. padenot, let me know if I overstepped at all on my cleanup.

The permissions hack is a reflection of what we came up with for libav. Seems to work, but still not super happy with it.

The one weird thing I had to do here was pull the soundtouch header include up into the AudioStream.h header, otherwise the symbols are compiled in as hidden and linking doesn't work right. There's possibly a better solution?
Attachment #8628625 - Flags: review?(padenot)
Attachment #8628625 - Flags: review?(mh+mozilla)
Comment on attachment 8628626 [details] [diff] [review]
Patch 3 (v1) - Update libsoundtouch to patched r222

Combination of r222 update and application of our patch to those files.
Attachment #8628626 - Flags: review?(padenot)
Blocks: 1157768
No longer depends on: 1157768
Comment on attachment 8628627 [details] [diff] [review]
Patch 1 (v2) - Add lgpllibs library to build system

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

::: config/external/lgpllibs/moz.build
@@ +2,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

While we're here, please add a comment as to why this library is being created (its name kind of gives a clue, but a more verbose explanation would be welcome).

You'll want to file a followup bug to add the library to the relevant package-manifests in comm-central.
Attachment #8628627 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8628625 [details] [diff] [review]
Patch 2 (v1) - Move libsoundtouch in lgpllibs

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

::: media/libsoundtouch/moz-libsoundtouch.patch
@@ -155,5 @@
> -+#endif
> -+
> - using namespace soundtouch;
> -     
> - /// test if two floating point numbers are equal

Why aren't those files touched by the patch changed as well? Presumably, if you change the patch, the files need to be updated as well.

::: media/libsoundtouch/src/moz.build
@@ +32,5 @@
>          SOURCES += ['mmx_optimized.cpp']
>          SOURCES['mmx_optimized.cpp'].flags += CONFIG['MMX_FLAGS']
>  
> +if CONFIG['OS_ARCH'] in ['Darwin', 'Linux']:
> +    CXXFLAGS += ['-include', 'soundtouch_perms.h']

This should be done on non Windows instead of restricting to Darwin+Linux, which is overly restrictive (because of the BSDs and others).

::: media/libsoundtouch/src/soundtouch_perms.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Include file for fixing symbol visibility on OS X, until system headers work

s/headers/headers wrappers/
Attachment #8628625 - Flags: review?(mh+mozilla) → feedback+
Attachment #8628625 - Flags: review?(padenot)
Comment on attachment 8628626 [details] [diff] [review]
Patch 3 (v1) - Update libsoundtouch to patched r222

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

Looks good. Do you need to rebase the patch maybe ?

::: media/libsoundtouch/src/STTypes.h
@@ +165,4 @@
>  };
>  
>  // define ST_NO_EXCEPTION_HANDLING switch to disable throwing std exceptions:
> +// #define ST_NO_EXCEPTION_HANDLING    1

Why is that ?
Attachment #8628626 - Flags: review?(padenot) → review+
> 
> Why aren't those files touched by the patch changed as well? Presumably, if
> you change the patch, the files need to be updated as well.

Because in the next patch they're all overwritten anyways, since we're upgrading the library while doing this.
(In reply to Paul Adenot (:padenot) from comment #10)
> Looks good. Do you need to rebase the patch maybe ?
> 
> ::: media/libsoundtouch/src/STTypes.h
> @@ +165,4 @@
> >  };
> >  
> >  // define ST_NO_EXCEPTION_HANDLING switch to disable throwing std exceptions:
> > +// #define ST_NO_EXCEPTION_HANDLING    1
> 
> Why is that ?

It's defined in moz.build, so it's still defined, just did it outside the patch. Can have the patch also fix this I suppose.
Ok. I jumped the gun on my review requests apparently, because windows and android aren't happy at ALL.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ae52a067c83

I'll try to get these fixed up and a full green try in before round 2.
Comment on attachment 8630168 [details] [diff] [review]
Patch 4 (v1) - Add lgpllibs library handling to android

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

Those library loading things are needed for nss and sqlite because java code is using them directly and wants to do that without having to load libxul first. I don't suppose you want to do that with any of the code in lgpllibs. So you don't need this, except the part adding the file to upload-files.mk, but that should be part of the other patch.
Attachment #8630168 - Flags: review?(mh+mozilla) → review-
Attachment #8630245 - Flags: review?(mh+mozilla) → review?(padenot)
(In reply to Mike Hommey [:glandium] from comment #16)
> Comment on attachment 8630168 [details] [diff] [review]
> Patch 4 (v1) - Add lgpllibs library handling to android
> 
> Review of attachment 8630168 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Those library loading things are needed for nss and sqlite because java code
> is using them directly and wants to do that without having to load libxul
> first. I don't suppose you want to do that with any of the code in lgpllibs.
> So you don't need this, except the part adding the file to upload-files.mk,
> but that should be part of the other patch.

Of course that's the /last/ change I made that got everything working before putting the patch in. :|

Following nss's lead was a bad idea again. Will take those out and fold this into Patch 2 for re-review.
Here's the try for Patch 5 on windows. Seems to fix the crash issues. https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d3e72c22877
Attachment #8630245 - Flags: review?(padenot) → review+
Moving android upload-files.mk back to patch 1.
Attachment #8628627 - Attachment is obsolete: true
Attachment #8630168 - Attachment is obsolete: true
Addressed review comments.
Attachment #8628625 - Attachment is obsolete: true
Attachment #8630552 - Flags: review?(mh+mozilla)
Attachment #8630552 - Flags: review?(mh+mozilla) → review+
You need to log in before you can comment on or make changes to this bug.