833403 - [webvtt] Integrate parser library into Mozilla build system.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Caitlin Potter (:caitp)]

See: Bug 629350

Integrate WebVTT parser (http://www.github.com/mozilla/webvtt) into build system, so that it can be utilized by code for rendering track element cues.

Comment 1

[Ralph Giles (:rillian)]

We should get the ball rolling on this, since it will be a blocker for a lot of the other text track implementation bugs. Can you post a patch for feedback something in the next week? We won't be able to land until the new dev branch is reviewed, landed and tagged in github, but we can at least get the import ready to go.

The way we've handled importing other external libraries is to write a 'update.sh' script which copies the relevant files from out upstream checkout (or release) into the mozilla tree, applies any local patches, and updates README_MOZILLA with the relevant version information.

Too good examples to start from are https://mxr.mozilla.org/mozilla-central/source/media/libopus/update.sh and https://mxr.mozilla.org/mozilla-central/source/media/libsoundtouch/update.sh

Comment 2

[Ralph Giles (:rillian)]

Once the code is imported it needs to be added to the build system. Mozilla uses a monolithic autoconf-based build system with recursive Makefile.in files for each subtree. Standard includes in this file provide the build rules, so variable declarations are usually all that's needed.

Resolving this bug involves writing a media/webvtt/Makefile.in to go alongside the imported source.

The format for Makefile.in files is documented in https://developer.mozilla.org/en-US/docs/How_Mozilla%27s_build_system_works The other libraries under media/ can serve as examples for a few issues you might encounter. I don't know the best why to handle the generated webvtt-config.h, for example.

Also be aware than mozilla is transitioning to a new pythonic format for declarative build information. If you're curious you could try to write one of those instead.

Comment 3

[Caitlin Potter (:caitp)]

I had been asking in #media about integrating our library into M-C, and have found that: 

A) There don't seem to be very many 3rd party libraries using GNU autotools build systems in M-C, but instead implement custom Makefile.ins that fit into the Mozilla build system nicely. It seems to be some difficulty getting Mozilla's global configure to apply to other libraries, so I think it might be simpler to do things the alternative way and create a slimmed down Makefile.in specifically for Mozilla.

B) Mr. Jesup has informed me that Gerv will be making the call on 3rd party libraries integrated into Mozilla (I am sure that we won't be worried about that for a some time yet) -- But that an explicit license statement for our code (which must be compatible with the MPL2, I think?). So, these are just some things to keep a note of, I'm leaving a public note of it here. I don't think there will be much trouble with the 2-clause BSD license, but I suppose we will need it stated.

I hope to have a patch against humphd's track-element branch to get libwebvtt building by Monday, and will be posting it here.

Comment 4

[Caitlin Potter (:caitp)]

https://github.com/caitp/mozilla-central/tree/track-element is the state of my build integration so far. It requires the application of patch https://bug830879.bugzilla.mozilla.org/attachment.cgi?id=702428. Currently, it runs into trouble on Mac OSX, with these linker errors http://pastebin.mozilla.org/2092533

I think the problem may have revolved around using LIBXUL_LIBRARY=1, however I do not have time to verify this right this moment. I will create a patch against master when I'm positive that this will link correctly.

Comment 5

[Caitlin Potter (:caitp)]

Attachment: Patch against commit 1acf9935... (git)

Described by https://bugzilla.mozilla.org/show_bug.cgi?id=833403#c4

Comment 6

[David Humphrey (:humph)]

Comment on attachment 707300 [details] [diff] [review]
Patch against commit 1acf9935... (git)

I'll help caitp get a better patch.  This one has unrelated stuff.

Comment 7

[Caitlin Potter (:caitp)]

Attachment: Resubmitted patch (without unnecessary changes)

DIFF against humphd/rillian patch, focused on Mozilla build integration
(See https://bugzilla.mozilla.org/show_bug.cgi?id=629350)

Comment 8

[David Humphrey (:humph)]

Comment on attachment 707312 [details] [diff] [review]
Resubmitted patch (without unnecessary changes)

This likely isn't ready for review yet, since other aspects it depends on aren't landed; but feedback from the build-pros would be helpful.

Comment 9

[Ralph Giles (:rillian)]

Comment on attachment 707312 [details] [diff] [review]
Resubmitted patch (without unnecessary changes)

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

::: media/webvtt/patch1.patch
@@ +5,5 @@
> + extern "C" {
> + #endif
> +
> ++# ifndef WEBVTT_STATIC
> ++# define WEBVTT_STATIC (1)

Can you pass this through DEFINES in the Mozilla tree's Makefile.in instead?

Comment 10

[Caitlin Potter (:caitp)]

Ralph: The concern with that is that the same CFLAGS need to be set for each client of the library (however many that may be, I'm not certain). Because WEBVTT_STATIC has an effect on attributes of exported functions, and the default is to expect a shared library export (which will probably only affect windows builds). It's simpler to make the change in this central location, I think. But if it's important to use CFLAGS instead, then that's okay too.

Comment 11

[Ralph Giles (:rillian)]

I see, thanks for describing the motivation.

This seems like a brittle design in general; surely any other clients would hit similar problems trying to use the library? But given you're depending on a generated header file for public api symbols this might be a reasonable approach. See also https://github.com/mozilla/webvtt/issues/12

Comment 12

[Caitlin Potter (:caitp)]

Attachment: Add media/webvtt files, update layout/media makefile.

Add update shell script, Makefile and patch to media/webvtt to integrate libwebvtt into Mozilla's build.

Update layout/media/Makefile.in to link libwebvtt into gkmedia_s

Comment 13

[Ralph Giles (:rillian)]

Comment on attachment 709256 [details] [diff] [review]
Add media/webvtt files, update layout/media makefile.

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

Thanks for the patch. This looks like a good start. A couple of comments, here and inline.

There are several other places where the new makefiles need to be added. In particular, media/webvtt needs to be added to toolkit/toolkit-tiers.mk and the new makefiles need to be added to toolkit/toolkit-makefiles.sh, which generates a list passed back to configure.in's AC_OUTPUT. MOZ_WEBVTT needs to be defined to 1 in configure.in and substituted as well. I suspect you have these changes in your tree and just forgot to include them in your patch.

I think you should either include media/webvtt/src/Makefile.in in the patch, and have it build the actual library (like what the media/libsoundtouch does) or remove it altogether and use VPATH to build everything unprefixed from media/webvtt/Makefile.in. Either of those would be less confusing than having the dummy makefile makes a subdirectory so its parent can compile the files it contains.

Please also include a README_MOZILLA with a description of where the code came from, what version is being used, etc.

Caitp, on irc you mentioned link errors on MacOS. After adding the makefile hooks described above I was able to build under XCode 4.5.2 without error, with the appropriate public symbols appearing in obj-x86_64-apple-darwin11.4.2/dist/lib/XUL.

::: media/webvtt/Makefile.in
@@ +1,4 @@
> +# 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/.
> +
> +DEPTH           = ../..

You can use @DEPTH@ here.

::: media/webvtt/patch1.diff
@@ +19,5 @@
> +-#   include <webvtt/webvtt-config.h>
> + # endif
> +
> + # if defined(_MSC_VER)
> +diff -u -rN ../webvtt2/src/Makefile.in ./src/Makefile.in

Each issues should be a separate patch file, rather than combining them like this.

@@ +26,5 @@
> +@@ -0,0 +1,16 @@
> ++# 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/.
> ++
> ++DEPTH           = ../..

This is the wrong depth value for media/webvtt/src. Better to use @DEPTH@ and let autoconf fill in the correct value for you.

Comment 14

[Caitlin Potter (:caitp)]

Attachment: Add media/webvtt files, update configure.in, toolkit/toolkit-makefiles.sh, toolkit/toolkit-tiers.mk, and layout/media/Makefile.in

Patch addressing https://bugzilla.mozilla.org/show_bug.cgi?id=833403#c13 (against git commit c154a99222b34e870a06f5feb4107b8b41447614)

- Removed media/webvtt/src subdirectory
- Use @DEPTH@ rather than hardcoding depth
- Patches configure.in
- Patches layout/media/Makefile.in
- Patches toolkit/toolkit-tiers.mk
- Patches toolkit/toolkit-makefiles.sh

Comment 15

[Ralph Giles (:rillian)]

Pushed to try as https://tbpl.mozilla.org/?tree=Try&rev=6fc65274ebef

Comment 16

[Ralph Giles (:rillian)]

Comment on attachment 710927 [details] [diff] [review]
Add media/webvtt files, update configure.in, toolkit/toolkit-makefiles.sh, toolkit/toolkit-tiers.mk, and layout/media/Makefile.in

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

::: media/webvtt/update.sh
@@ +16,5 @@
> +  git clone ${URL} ${REPO} -b ${BRANCH}
> +fi
> +
> +#Create directories
> +mkdir include include/webvtt

'mkdir -p include/webvtt' ?

@@ +23,5 @@
> +find ${REPO}/include/webvtt -type f -name '[^w]*.h' -exec cp '{}' ${SRCDIR}/include/webvtt/ \;
> +#Copy C sources
> +find ${REPO}/src/libwebvtt -type f -name '*.[ch]' -exec cp '{}' ${SRCDIR}/ \;
> +
> +rm -rf ${REPO}

This will destroy the pre-existing repo you carefully check for above. Either make this line conditional too, or remove the conditional alltogether.

Comment 17

[Ralph Giles (:rillian)]

More functional try push https://tbpl.mozilla.org/?tree=Try&rev=0466a369b387

Comment 18

[Caitlin Potter (:caitp)]

Attachment: No longer deletes webvtt repository at end of update.sh

Identical to 710927 -- Addresses rillian's comment regarding deletion of $(REPO)

Comment 19

[Caitlin Potter (:caitp)]

Attachment: Small adjustment

files from media/webvtt/ were not included in that last attachment.

Comment 20

[Caitlin Potter (:caitp)]

Attachment: Fixed patch -- see Previous 2 for details

Sorry about all the mistakes in the above patches, I'm not trying to spam the thread.

Comment 21

[Ralph Giles (:rillian)]

Comment on attachment 710982 [details] [diff] [review]
Fixed patch -- see Previous 2 for details

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

Thanks. Looking good. Before it's ready for review, could you please:

- Run update.sh and include the imported files in the patch. As it stands the patch will break the build if committed.

- Include the webvtt git commit id from the import in README_MOZILLA for reference. (Bonus points if you make update.sh do this :)

- Write a commit message and include it in the patch file. See https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Commit_message_restrictions for style guidelines.

::: media/webvtt/update.sh
@@ +19,5 @@
> +#Create directories
> +mkdir include include/webvtt
> +
> +#Copy C headers
> +find ${REPO}/include/webvtt -type f -name '[^w]*.h' -exec cp '{}' ${SRCDIR}/include/webvtt/ \;

Is the [^w] here to avoid copying webvtt-config*.h? If so that probably deserves a comment.

Comment 22

[Ralph Giles (:rillian)]

Previous try push is green: debug builds work on all six primary platforms.

Comment 23

[Caitlin Potter (:caitp)]

Attachment: Integrate WebVTT library

Comment 24

[Caitlin Potter (:caitp)]

Patch comments were not submitted by bugzilla:
Obsoletes #710982

patched against mozilla-central/master git revision 9b2b226aa01823dfc826956dc3fa5a55e43f2d8b

update.sh now updates README_MOZILLA by adding or updating the revision number that is being used.

Source files from the library are included in the patch, so running update.sh is not required prior to building.

Have built successfully on OSX 10.8.2 with Xcode 4.6

Comment 25

[Ralph Giles (:rillian)]

Comment on attachment 712491 [details] [diff] [review]
Integrate WebVTT library

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

Thanks for the update. Commit message?

::: media/webvtt/update.sh
@@ +21,5 @@
> +cd ..
> +
> +sed -e "s/^[a-z0-9]\{40,40\}/$REV/" README_MOZILLA > README_MOZILLA.sed
> +mv README_MOZILLA.sed README_MOZILLA
> +rm -f README_MOZILLA.sed

The 'mv' makes the 'rm' redundant. Better to leave README_MOZILLA.sed lying around if the mv fails, I think.

Comment 26

[Timothy B. Terriberry (:derf)]

(In reply to Ralph Giles (:rillian) from comment #25)
> The 'mv' makes the 'rm' redundant. Better to leave README_MOZILLA.sed lying
> around if the mv fails, I think.

Unless you've carefully vetted the error handling, it's probably better to put a "set -e" command at the start of the script so that it exits on the first failed command anyway.

Comment 27

[Caitlin Potter (:caitp)]

`set -e` breaks on at the very least non-GNU versions of the C shell / improved C shell.

I'm not sure if that should matter since we're asking for a Bourne shell. I think I prefer `mv README_MOZILLA.sed README_MOZILLA || exit -1`, which works on sh/bash/csh/tcsh/ksh/zsh (as tested on this machine), and likely others too.

Comment 28

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

:cdiehl - would peach be good for this? If so then :rforbes can work on it

Comment 29

[Ralph Giles (:rillian)]

Note this code isn't ready for sec-review yet. Lots of obvious things to fix first. 

We'd like to land it with the content api pref'd off so it's not accessible while we work on testing and clean up, at which point we'd love to have some fuzzing happening.

Comment 30

[Caitlin Potter (:caitp)]

Attachment: Integrate WebVTT library

Now reports error and aborts if `mv README_MOZILLA.sed README_MOZILLA` fails.

Comment 31

[Ralph Giles (:rillian)]

Comment on attachment 715563 [details] [diff] [review]
Integrate WebVTT library

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

I'm fine with the update.sh change, but feedback- for the Makefile typo and missing commit message.

::: media/webvtt/Makefile.in
@@ +7,5 @@
> +VPATH           = @srcdir@
> +
> +include $(DEPTH)/config/autoconf.mk
> +
> +MODULE            =:q webvtt

I'm pleased to see you're a vim user! :-)

::: media/webvtt/update.sh
@@ +22,5 @@
> +fi
> +
> +cd ${REPO}
> +REV=`git rev-parse ${BRANCH}`
> +cd ..

This breaks if ${REPO} isn't a relative subdirectory. Same with the line updating an existing checkout.

I'm not much fussed, but you could REV=$(cd ${REPO} && git rev-parse ${BRANCH}). Or save $PWD and cd back to that, or even use $PWD for SRCDIR.

Comment 32

[Caitlin Potter (:caitp)]

Attachment: Integrate WebVTT library

Removed typo in media/webvtt/Makefile.in

now using absolute paths rather than relative paths in media/webvtt/update.sh

Comment 33

[Caitlin Potter (:caitp)]

Attachment: Integrate WebVTT library (With commit message)

I've attached a version of the previous patch (#716743) with the commit message included

Comment 34

[(not currently active) Ted Mielczarek]

Comment on attachment 716798 [details] [diff] [review]
Integrate WebVTT library (With commit message)

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

I have a few comments but overall this looks great! Sorry for taking a while to get you feedback.

::: layout/media/Makefile.in
@@ +126,5 @@
>  
> +ifdef MOZ_WEBVTT
> +SHARED_LIBRARY_LIBS += \
> +	$(DEPTH)/media/webvtt/$(LIB_PREFIX)webvtt.$(LIB_SUFFIX) \
> +	$(NULL)

nit: two-space indentation after a continuation.

If you don't think you're going to add more lines here, you don't need to do the multi-line list.

Do you think there's any chance that WebVTT will be shipped as a standalone library in the future, such that we'd want to support linking to a system libwebvtt on Linux?

::: media/webvtt/Makefile.in
@@ +11,5 @@
> +MODULE            = webvtt
> +LIBRARY_NAME      = webvtt
> +
> +#Doesn't seem to matter
> +#FORCE_STATIC_LIB = 1

Yeah, we default to static libs these days. Just remove this instead of leaving it commented out.

::: media/webvtt/README_MOZILLA
@@ +3,5 @@
> +https://github.com/mozilla/webvtt.
> +
> +The whole library is not used, only the C library (libwebvtt) is imported in the
> +tree, using the script `update.sh`. Some changes have been made to
> +include/webvtt/util.h in order to remove dependence on generated header files.

These changes are the changes in patch1.diff? You might want to mention that. Do you think we could fix util.h so that we could simply add a new preprocessor define that does the right thing instead of having to patch the source?

::: media/webvtt/alloc.c
@@ +1,1 @@
> +#include <webvtt/util.h>

I'm obviously not going to review the libwebvtt source, but I think you should add license headers to all your source files. It makes it a lot easier to track licensing since we have so much third-party code in the tree.

::: media/webvtt/update.sh
@@ +10,5 @@
> +BRANCH=dev
> +SRC=$PWD
> +REPO=$SRC/remote
> +
> +rm -rf include *.c *.h

You might want to explicitly cd to `dirname $0` here to make sure you're working in the right directory.

@@ +17,5 @@
> +  cd $REPO && git fetch && git checkout ${BRANCH} -f && git clean -x -f -d && cd $SRC
> +else
> +  echo "Downloading source from ${URL}"
> +  git clone ${URL} ${REPO} -b ${BRANCH}
> +fi

I think this might be easier to use if you just forced the user to point the update script at an existing git repo, and used `git archive` to export the sources to the local tree, so that you don't have a git repo mixed into your hg repo:
http://stackoverflow.com/a/163769/69326

This is what my Breakpad update script does:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/update-breakpad.sh

As a bonus, you could then have this script do a `hg addremove` at the end and not wind up with extraneous files in the repo.

@@ +34,5 @@
> +    $SRC/include/webvtt/ \; #Copy C sources
> +find $REPO/src/libwebvtt -type f -name '*.[ch]' -exec cp '{}' \
> +    $SRC/ \;
> +
> +patch -p 0 -r . < patch1.diff

You should probably have this fail if the patch fails to apply.

Comment 35

[Caitlin Potter (:caitp)]

Attachment: Integrate WebVTT library

I've addressed (most) of the issues raised by :ted in https://bugzilla.mozilla.org/show_bug.cgi?id=833403#c34

We still do not have a license at the head of each source file or header, an issue has been raised on the github issue tracker by :rillian and will be addressed shortly

I've asked for suggestions on where to place preprocessor definitions, and teammates present agreed that it shouldn't be much of a hassle to #define the appropriate tokens prior to inclusion of WebVTT API headers, so these definitions are performed in media/webvtt/Makefile.in's DEFINES variable rather than requiring us to patch a header file.

Additionally, the script no longer sticks a git repository in the tree, and relies on git archive instead.

Finally, hg addremove is called against media/webvtt/ if mercurial is determined to be present on the system (however no check is performed to verify whether or not it is a mercurial repository, as I get the impression that it isn't necessary from :ted's example)

Comment 36

[Caitlin Potter (:caitp)]

Attachment: Integrate libwebvtt into Mozilla

Hopefully correctly updated patch to accommodate new build system.

Imported updated version with LICENSE file and LICENSE-headed source files. Additionally added some more proper MPL2 license information to media/webvtt/Makefile.in and media/webvtt/update.sh

Comment 37

[(not currently active) Ted Mielczarek]

Comment on attachment 719700 [details] [diff] [review]
Integrate libwebvtt into Mozilla

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

I'm going to go even farther than feedback+ and just grant r+. This looks fine to land.

::: media/webvtt/update.sh
@@ +46,5 @@
> +echo "Updating media/webvtt to revision ${webvtt_branch} (${webvtt_revision})"
> +
> +#Ensure that ${repo_dir} is not present to prevent mkdir from failing
> +rm -rf ${start_dir}/${repo_dir} 2>/dev/null
> +mkdir ${start_dir}/${repo_dir} 2>/dev/null

Nitpicky, but I'd probably leave stderr alone in everything in this script just so you get a clear sense of what happened if something goes wrong.

Comment 38

[Ralph Giles (:rillian)]

https://tbpl.mozilla.org/?tree=Try&rev=8331f53833b7

Comment 39

[Caitlin Potter (:caitp)]

There's a Windows (probably MSVC) build error -- syntax errors in the code. I'm kind of astounded that things like that stay in the tree for so long, but I guess we're mostly working on things that C99 by default or something.

I'll be addressing this and Ted's nit shortly.

Comment 40

[(not currently active) Ted Mielczarek]

MSVC doesn't actually have very good C support, they don't support much C99, FYI, so you'll have to tread lightly there.

Comment 41

[Caitlin Potter (:caitp)]

Attachment: Integrate libwebvtt into Mozilla

- Updated for new build system (2/28/2013)
- declaration-after-statement no longer breaking MSVC build
- Source files and scripts now contain appropriate license info
- media/webvtt/update.sh no longer hiding unexpected/significant errors.

Comment 42

[Ralph Giles (:rillian)]

https://tbpl.mozilla.org/?tree=Try&rev=478babd6c6f4

Comment 43

[(not currently active) Ted Mielczarek]

Comment on attachment 720180 [details] [diff] [review]
Integrate libwebvtt into Mozilla

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

Unless you make any substantial changes to the build side of things here, you can carry forward my r+ on any new revisions of this patch.

Comment 44

[Caitlin Potter (:caitp)]

Right, sorry =)

Comment 45

[Ralph Giles (:rillian)]

So we're green on try except for the B2G ics_armv7a_gecko build which fails with a missing ssh key for the 'make uploadsymbols' step. I don't see how that's the fault of this patch, and it's occurring on other contemporary try pushes, so I think we're good to land.

Comment 46

[Ralph Giles (:rillian)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bdcde3b122c

Comment 47

[Justin Wood (:Callek)]

(In reply to caitlin.potter from comment #3)
> B) Mr. Jesup has informed me that Gerv will be making the call on 3rd party
> libraries integrated into Mozilla (I am sure that we won't be worried about
> that for a some time yet) -- But that an explicit license statement for our
> code (which must be compatible with the MPL2, I think?). So, these are just
> some things to keep a note of, I'm leaving a public note of it here. I don't
> think there will be much trouble with the 2-clause BSD license, but I
> suppose we will need it stated.

Gerv has previously expressed concern that we don't land things until AFTER we get signoff that its legally ok. I don't get why we would have code that seems licensed by Mozilla not MPL in our tree though.

Either way, it concerns me that gerv was never even CC'ed on this bug with that statement

Comment 48

[Ralph Giles (:rillian)]

As with other media libraries Mozilla helps maintain, we chose 2-clause BSD as the most liberal possible license to encourage use elsewhere and thus adoption of the format.

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3bdcde3b122c

Comment 50

[Gervase Markham [:gerv]]

The time to have had the licensing discussion would really have been at the start of the project.

Mozilla is currently trying to figure out its future licensing strategy with respect to MPL vs. Apache vs. other permissive licences. See the discussion in mozilla.legal. 

Given that Mozilla controls (as far as I can see) the upstream for this (Ralph: is that right? Is this a Mozilla-originated codebase?), deciding what to do in this case can be done independent of an m-c landing.

Gerv