1227058 - Update OTS to version 5.0.0

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Comment 1

[Frédéric Wang (:fredw)]

This new version removes the bundled woff2 source and adds https://github.com/google/woff2 as a git submodule. How should we handle that? I see that we have brotli in modules/ but I don't know where our WOFF2 support is actually implemented.

Comment 2

[Jonathan Kew [:jfkthame]]

We currently rely on OTS to provide WOFF2 decoding, so we'll need to ensure that we move to https://github.com/google/woff2 as well -- probably by adding woff2 under /modules/, I guess.

Comment 3

[Frédéric Wang (:fredw)]

Attachment: Update Brotli to bed93862608d4d232ebe6d229f04e48399775e8b

Comment 4

[Frédéric Wang (:fredw)]

Attachment: Include the woff2 library in the gecko build

Comment 5

[Frédéric Wang (:fredw)]

Attachment: Update OTS to version 5.0.0

Comment 6

[Frédéric Wang (:fredw)]

I tested the patches on Linux using layout/reftests/font-face/woff2-1.html and http://fred-wang.github.io/MathFonts/

The additional "XXXXXX" in the update scripts are needed for me to avoid the error "mktemp: too few X's in template".

For the woff2 library, I tried to build only the files that are necessary. However, the whole src/ directory is included the tree. Do we want to cherry-pick the files?

Comment 7

[Frédéric Wang (:fredw)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5688dcc05564

Comment 8

[Frédéric Wang (:fredw)]

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

Comment 9

[Frédéric Wang (:fredw)]

Attachment: Update Brotli to bed93862608d4d232ebe6d229f04e48399775e8b

Comment 10

[Frédéric Wang (:fredw)]

Attachment: Include the woff2 library in the gecko build

Comment 11

[Jonathan Kew [:jfkthame]]

(In reply to Frédéric Wang (:fredw) from comment #6)
> I tested the patches on Linux using layout/reftests/font-face/woff2-1.html
> and http://fred-wang.github.io/MathFonts/
> 
> The additional "XXXXXX" in the update scripts are needed for me to avoid the
> error "mktemp: too few X's in template".
> 
> For the woff2 library, I tried to build only the files that are necessary.
> However, the whole src/ directory is included the tree. Do we want to
> cherry-pick the files?

Importing the complete src/ directory seems fine to me; it's not big enough to be worth cherry-picking the parts we really need.

Looks like there are still some build issues to figure out, though. :(

Comment 12

[Frédéric Wang (:fredw)]

(In reply to Jonathan Kew (:jfkthame) from comment #11)
> Importing the complete src/ directory seems fine to me; it's not big enough
> to be worth cherry-picking the parts we really need.

OK, sounds good.

> 
> Looks like there are still some build issues to figure out, though. :(

Yes, apparently the woff2 library uses some C++11 features but specifying -std=c++11 is not enough for OSX/Android/B2G... I need to do more attempts.

Comment 13

[Frédéric Wang (:fredw)]

Attachment: Include the woff2 library in the gecko build

Comment 14

[Frédéric Wang (:fredw)]

Attachment: Update OTS to version 5.0.0.

Comment 15

[Frédéric Wang (:fredw)]

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

Removing the dependency as we do not really need to update brotli.

Comment 16

[Frédéric Wang (:fredw)]

Comment on attachment 8712390 [details] [diff] [review]
Update OTS to version 5.0.0.

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

(In reply to Frédéric Wang (:fredw) from comment #12)
> > Looks like there are still some build issues to figure out, though. :(
> 
> Yes, apparently the woff2 library uses some C++11 features but specifying
> -std=c++11 is not enough for OSX/Android/B2G... I need to do more attempts.

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

So the problem seems to be that -std=... is forced somewhere else in the build system and so the -std=c++11 option concatenated via moz.build is just ignored. It does not seem possible to easily workaround that, at least after a quick search it is not done elsewhere in mozilla-central. Even if we can, it will probably cause more issues when linking.

The alternative is to move back to C++0X features. Actually, two small changes were introduced in the latest commit and the commit author agreed with fixing these changes (see https://github.com/google/woff2/pull/42/). The remaining point is the use of 'std::unique_ptr' to handle an array of points in woff2_dec. I've patched that by going back to what the old code was doing: using a std::vector. To limit the number of lines to patch, I'm accessing the internal buffer via &points.front() to pass the array as an argument but if that's problematic we can certainly fix the functions that handle the array.

Comment 17

[Jonathan Kew [:jfkthame]]

(In reply to Frédéric Wang (:fredw) from comment #16)
> Comment on attachment 8712390 [details] [diff] [review]
> Update OTS to version 5.0.0.
> 
> Review of attachment 8712390 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Frédéric Wang (:fredw) from comment #12)
> > > Looks like there are still some build issues to figure out, though. :(
> > 
> > Yes, apparently the woff2 library uses some C++11 features but specifying
> > -std=c++11 is not enough for OSX/Android/B2G... I need to do more attempts.
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2798d4dc904
> 
> So the problem seems to be that -std=... is forced somewhere else in the
> build system and so the -std=c++11 option concatenated via moz.build is just
> ignored. It does not seem possible to easily workaround that, at least after
> a quick search it is not done elsewhere in mozilla-central. Even if we can,
> it will probably cause more issues when linking.

Sigh...that's too bad. Let's ask a build-system expert like :gps whether there's something we can do about this on the build side, or if we just have to resort to patching the code for now...

> 
> The alternative is to move back to C++0X features. Actually, two small
> changes were introduced in the latest commit and the commit author agreed
> with fixing these changes (see https://github.com/google/woff2/pull/42/).
> The remaining point is the use of 'std::unique_ptr' to handle an array of
> points in woff2_dec. I've patched that by going back to what the old code
> was doing: using a std::vector. To limit the number of lines to patch, I'm
> accessing the internal buffer via &points.front() to pass the array as an
> argument but if that's problematic we can certainly fix the functions that
> handle the array.

If we do end up with this solution, then rather than just appending the patch to the README.mozilla file, let's include it as a standalone file and apply it as part of the update script. (And then anyone running that script will see a clear failure if it no longer applies.)

But let's see if :gps can suggest any better options on the build side of things, before proceeding here.

Comment 18

[Rod]

FYI the changes requested against https://github.com/google/woff2/pull/42/ are now on master.

Comment 19

[Frédéric Wang (:fredw)]

Attachment: Include the woff2 library in the gecko build

(In reply to Rod from comment #18)
> FYI the changes requested against https://github.com/google/woff2/pull/42/
> are now on master.

Thanks for the quick fix! I update the patch to include these changes.

(In reply to Jonathan Kew (:jfkthame) from comment #17)
> Sigh...that's too bad. Let's ask a build-system expert like :gps whether
> there's something we can do about this on the build side, or if we just have
> to resort to patching the code for now...

OK, I also wrote a message on the platform mailing list:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/ph-63xS3zwA

> If we do end up with this solution, then rather than just appending the
> patch to the README.mozilla file, let's include it as a standalone file and
> apply it as part of the update script. (And then anyone running that script
> will see a clear failure if it no longer applies.)

Done in the latest patch.

Comment 20

[Frédéric Wang (:fredw)]

@Jonathan: So here is the workaround proposed to be able to use std::unique_ptr. It seems that we'll have to patch the woff2 source anyway... What do you think?

> ---------- Forwarded message ----------
> From: Lee Salzman <[e-mail]>
> Date: Mon, Feb 1, 2016 at 11:44 AM
> Subject: Re: Use of C++11 std::unique_ptr for the WOFF2 module
> To: Jeff Muizelaar <[e-mail]>
> 
> 
> The solution looks something like this (beware that it is indeed hacky):
> 
> #include <memory> // must be included first so that we don't interfere with pre-existing unique_ptr implementation
> #include "mozilla/UniquePtr.h"
> namespace std
> {
>   using mozilla::DefaultDelete;
>   using mozilla::UniquePtr;
>   #define default_delete DefaultDelete
>   #define unique_ptr UniquePtr
> }
> 
> An important feature of this workaround is that both the actual unique_ptr and UniquePtr will happily coexist in the same std namespace, but it will pollute the global namespace a bit because of the #defines. It works, though.

Comment 21

[Gregory Szorc [:gps]]

The last I checked, CFLAGS values set by moz.build should get appended to whatever the build system defaults are and last write should win. Although, I do recall glandium writing some patches that sorted CFLAGS values. But I think this was only for clang compilation database generation. I could be wrong, however.

What does `make -C <objdir>/gfx/ots/src showbuild` say? Is the -std argument what you expect when you modify the moz.build?

glandium: needinfo you so you can weigh this use case as part of refactoring how compiler flags and moz.build work.

Comment 22

[Mike Hommey [:glandium]]

Your try push doesn't have your addition of a -std argument, so I can't tell anything, but there is no reason it would be ignored. That said, your problem doesn't seem to be related to -std=c++0x vs -std=c++11 (which, in practice, doesn't make much difference). That said(2), we could switch to -std=gnu++11 globally, since the minimum version of gcc we support supports it.

Comment 23

[Frédéric Wang (:fredw)]

Here is one try build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dda5dc4f9eaa&selectedJob=15862667

If you check the log for mac and android, you see that -std=gnu++0x is specified first and -std=c++11 after. android fails at unique_ptr. the mac fails at the unordered_set header, because it does not use libc++, as someone mentioned on the platform mailing list.

Here is another try build that forces -std=gnu++0x -stdlib=libc++:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe0068111b20&selectedJob=15952354

Now woff2 is correctly built, but linking with the rest of the objects fail.

Comment 24

[Mike Hommey [:glandium]]

So this is as I said, your added flag is not ignored by the build system, it just doesn't have the effect you think it would have, and doesn't help with your problem.

(In reply to Frédéric Wang (:fredw) from comment #23)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=fe0068111b20&selectedJob=15952354
> 
> Now woff2 is correctly built, but linking with the rest of the objects fail.

... for some value of correctly built. I'm glad you're getting a linking error because:

std::__1::__vector_base_common<true>::__throw_length_error() const

is a disaster in wait to happen. If the woff code uses exceptions directly or indirectly, that's not going to pan out.

Comment 25

[Frédéric Wang (:fredw)]

(In reply to Mike Hommey [:glandium] from comment #24)
> So this is as I said, your added flag is not ignored by the build system, it
> just doesn't have the effect you think it would have, and doesn't help with
> your problem.

Well you are a bit playing with words here... The original question was whether we can build in C++11 mode or whether we'll have to patch the woff2 library. But I got this reply from Ehsan on the platform mailing list:

"We're working on moving all of our platforms to use a C++11-ish standard library.  For std::unique_ptr, at least, the best tack is to write a small polyfill based on mfbt/UniquePtr.h."

So we now have two options to patch the library: either going back to std::vector or relying on UniquePtr. Jonathan?

Comment 26

[Jonathan Kew [:jfkthame]]

I think I'd prefer the UniquePtr approach, as that seems like a less invasive option than patching the actual code to eliminate its unique_ptr usage.

(And if the WOFF2 lib extends its usage of unique_ptr in future revisions, the same UniquePtr polyfill should continue to work, whereas patching each usage in the code could get increasingly difficult to maintain...)

Comment 27

[Frédéric Wang (:fredw)]

Attachment: Include the woff2 library in the gecko build

(In reply to Jonathan Kew (:jfkthame) from comment #26)
> I think I'd prefer the UniquePtr approach, as that seems like a less
> invasive option than patching the actual code to eliminate its unique_ptr
> usage.

OK, here is the new version with the UniquePtr approach.

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

Comment 28

[Jonathan Kew [:jfkthame]]

Comment on attachment 8712390 [details] [diff] [review]
Update OTS to version 5.0.0.

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

LGTM. (Please make sure the patch to add the WOFF2 lib lands before this one, to avoid a broken revision that people might hit during bisection.)

Comment 29

[Jonathan Kew [:jfkthame]]

Comment on attachment 8714858 [details] [diff] [review]
Include the woff2 library in the gecko build

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

This seems fine to me, but redirecting r? to :gps to check that the build-system aspect of things is correct.

Comment 30

[Gregory Szorc [:gps]]

Comment on attachment 8714858 [details] [diff] [review]
Include the woff2 library in the gecko build

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

Seems OK.

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/43f8fc8c8a85
https://hg.mozilla.org/integration/mozilla-inbound/rev/b97b9546f96a

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/43f8fc8c8a85
https://hg.mozilla.org/mozilla-central/rev/b97b9546f96a