785291 - Add support to load fonts from an APK

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

We'd like to ship fonts with Fennec, in the Android APK, and load those so Gecko can use them. Brad says we support that for Windows and it should be simple to add support for Android too.

Comment 1

[Jet Villegas (inactive)]

+cc: jkew

The work already done for bug 619521 may be useful here.

Comment 2

[Jonathan Kew [:jfkthame]]

The easiest solution would be to unpack the font files from the APK into a directory in the filesystem during installation - then we could just add that extra directory to the locations we scan for fonts at startup, and treat them exactly like the device's standard fonts. (Currently, we just load fonts from the Android system fonts directory; the patch in bug 619521 refactors that code somewhat and scans the profile directory as well, to load any downloaded fonts stored there.)

Alternatively, I expect we could avoid unpacking the font files into the filesystem at all, and just load them directly from the APK, though we'd probably have to unpack them (into RAM) at runtime in order for freetype to use the files. (It's possible to use compressed streams directly with freetype, but I'd be concerned about performance, as FT will probably want to have random access to the font data, and accessing random offsets within a compressed stream is typically inefficient.)

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: untested patch

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

This will look in /data/data/org.mozilla.firefox/res/fonts. It doesn't have any logic to extract fonts from the apk to that directory though.

Comment 5

[Jonathan Kew [:jfkthame]]

Comment on attachment 660017 [details] [diff] [review]
patch

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

This looks like it would work (modulo comments below), but I think it'll be tidier to factor out a "FindFontsInDir()" helper. And then there's no need to construct the searchPaths[] array and loop over it; just call FindFontsInDir(root), then look up the additional directory path and call FindFontsInDir(...) for that as well.

See the "part 2" patch (attachment 631153 [details] [diff] [review]) in bug 619521, which does the same thing for fonts saved into the profile - not sure if that patch still applies cleanly on trunk, but it should be trivial to fix up if needed. It's even r+'d already, so we could just land it and then all you'd need to do here is add the extra directory.

::: gfx/thebes/gfxFT2FontList.cpp
@@ +969,5 @@
> +        DIR *d = opendir(searchPaths[i].get());
> +        if (!d) {
> +            if (i == 0)
> +                // if we can't find/read the font directory, we are doomed!
> +                NS_RUNTIMEABORT("Could not read the system fonts directory");

Style rules require braces even for a single-statement if() body.

@@ +970,5 @@
> +        if (!d) {
> +            if (i == 0)
> +                // if we can't find/read the font directory, we are doomed!
> +                NS_RUNTIMEABORT("Could not read the system fonts directory");
> +            else

No need for the "else" here.

@@ +993,5 @@
> +                {
> +                    isStdFont = strcmp(sStandardFonts[i], ent->d_name) == 0;
> +                }
> +
> +                nsCString s(root);

Surely this needs to be searchPaths[i], not root.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch on top of bug 619521 part 2

Comment 7

[Jonathan Kew [:jfkthame]]

Comment on attachment 660747 [details] [diff] [review]
patch on top of bug 619521 part 2

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

Looks good. If you want to land bug 619521 part 2 and this patch together, even though the other patches in 619521 are awaiting updates/review, I think that'd be fine. Just be sure to note that 619521 should not be closed yet.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch to load fonts directly from the APK

In order for the previous patch to be useful, we'd need to extract the fonts to disk. And in order to do that without regressing start up time we'd need to use the notifications from bug 619521 part 3, which is still waiting for review.

This patch allows us to load the fonts directly from the APK without extracting to disk, which I think is preferable overall.

Comment 9

[Jonathan Kew [:jfkthame]]

Comment on attachment 664907 [details] [diff] [review]
patch to load fonts directly from the APK

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

This looks like it will *find* faces in the APK when initially building the font list, but does it enable us to *use* them? ISTM that when those entries get cached in the font list and then restored from there, there's no code that will actually go and find them in the jar file again and make them usable for rendering.

Also, I'm concerned about performance if we use this approach of wrapping an FT_stream around a zip-file reader. It seems likely to me that FreeType assumes there is efficient random access to the font, but a zip reader does not support this. To get to the requested offset, you're having to decode the entire file up to that position. This probably makes it significantly slower to build the font list from a file in the jar than from the equivalent unpacked file; it might also make impact the performance of ongoing operations using the font. (I don't know offhand, for example, whether FreeType will necessarily load the entire 'glyf' table into memory at once, or if it will read individual glyph data packets on demand. The 'sfnt' structure is designed to efficiently support this, but on the assumption there is efficient random access to the data.)

So my guess - but measurements to confirm or deny this would be great - is that overall it would be better to unpack font files from the APK at install time.

Comment 10

[Jonathan Kew [:jfkthame]]

(In reply to Brad Lassey [:blassey] from comment #8)
> Created attachment 664907 [details] [diff] [review]
> patch to load fonts directly from the APK
> 
> In order for the previous patch to be useful, we'd need to extract the fonts
> to disk. 

Yes.

> And in order to do that without regressing start up time we'd need
> to use the notifications from bug 619521 part 3, which is still waiting for
> review.

I assumed we'd unpack them at *install* (not startup) time. I thought one of the use-cases here would be for fonts like Open Sans (or whatever) that we want to use in preference to the device's standard fonts (e.g. in Reader mode, maybe also for web content?). We wouldn't get notifications related to this anyway, as it's not a question of providing additional fonts for missing characters/unsupported scripts, but bundling fonts that we prefer for design reasons. Bug 619521 isn't relevant to that; it's about gaps in the character repertoire.

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Jonathan Kew (:jfkthame) from comment #10)

> > And in order to do that without regressing start up time we'd need
> > to use the notifications from bug 619521 part 3, which is still waiting for
> > review.
> 
> I assumed we'd unpack them at *install* (not startup) time.
Unfortunately, that's not possible on android.

> I thought one of
> the use-cases here would be for fonts like Open Sans (or whatever) that we
> want to use in preference to the device's standard fonts (e.g. in Reader
> mode, maybe also for web content?). We wouldn't get notifications related to
> this anyway, as it's not a question of providing additional fonts for
> missing characters/unsupported scripts, but bundling fonts that we prefer
> for design reasons. Bug 619521 isn't relevant to that; it's about gaps in
> the character repertoire.

So, to work around the fact that we can't extract at install time and we'd like to avoid taking a big start-up time regression, my thought was to do the extraction on a background thread after the rest of start up had finished and then use the notifications to alert the font system to the newly available fonts once they are extracted.

Comment 12

[Jonathan Kew [:jfkthame]]

(In reply to Brad Lassey [:blassey] from comment #11)
> (In reply to Jonathan Kew (:jfkthame) from comment #10)
> 
> > > And in order to do that without regressing start up time we'd need
> > > to use the notifications from bug 619521 part 3, which is still waiting for
> > > review.
> > 
> > I assumed we'd unpack them at *install* (not startup) time.
> Unfortunately, that's not possible on android.

OK, then at first-startup (so they'd be available directly at subsequent startups).

> 
> > I thought one of
> > the use-cases here would be for fonts like Open Sans (or whatever) that we
> > want to use in preference to the device's standard fonts (e.g. in Reader
> > mode, maybe also for web content?). We wouldn't get notifications related to
> > this anyway, as it's not a question of providing additional fonts for
> > missing characters/unsupported scripts, but bundling fonts that we prefer
> > for design reasons. Bug 619521 isn't relevant to that; it's about gaps in
> > the character repertoire.
> 
> So, to work around the fact that we can't extract at install time and we'd
> like to avoid taking a big start-up time regression,

Do we have any measurement of the time it'd take to extract a few font files on first startup? ISTM that if it only impacts *first* startup by some modest number of milliseconds, that's far less significant than something that regresses *every* startup.

> my thought was to do
> the extraction on a background thread after the rest of start up had
> finished and then use the notifications to alert the font system to the
> newly available fonts once they are extracted.

OK, I see. Yes, that'd be a possibility, though it introduces the risk of a disconcerting visual effect if we initially render the UI or the first page using the device's own fonts, then finish extracting our bundled ones and re-render using those instead.

Personally, I'd think that adding a few ms to the first startup would be a better user experience than rendering that shifts/changes just after it's initially been displayed. After all, surely the amount of time it'd take to do the one-time extraction will be minuscule compared to the time the user has just spent downloading the package...

How long does "first launch" typically take, and how long does it take to extract a file from APK to filesystem? Is startup heavily i/o-bound at present?

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch to extract fonts on first run

Comment 14

[Jonathan Kew [:jfkthame]]

Comment on attachment 665343 [details] [diff] [review]
patch to extract fonts on first run

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

This seems reasonable, modulo some style nits, but I'd prefer to see it refactored a bit; how about an ExtractFontsFromJarFile helper function? And I think we can reduce the impact on subsequent startups by avoiding the scan of the JAR file altogether once we've extracted the fonts.

::: gfx/thebes/gfxFT2FontList.cpp
@@ +855,5 @@
>  }
>  
> +#ifdef ANDROID
> +nsresult
> +CopyFromUriToFile(nsCString spec, nsIFile* localFile) {

newline before the open-brace, please.

@@ +865,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    nsCOMPtr<nsIOutputStream> outputStream;
> +    rv = NS_NewLocalFileOutputStream(getter_AddRefs(outputStream), localFile);
> +
> +    NS_ENSURE_SUCCESS(rv, rv);

put the blank line after the NS_ENSURE_SUCCESS.

@@ +866,5 @@
> +    nsCOMPtr<nsIOutputStream> outputStream;
> +    rv = NS_NewLocalFileOutputStream(getter_AddRefs(outputStream), localFile);
> +
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    char buf[1024];

#define a constant for the buffer size here (and then use it below as well).

Do we know whether 1K is a roughly-optimal size, or should we investigate smaller/larger buffers to see if there's a perf impact?

@@ +875,5 @@
> +        PRUint32 written;
> +        rv = outputStream->Write(buf, read, &written);
> +        NS_ENSURE_SUCCESS(rv, rv);
> +        if (written != read)
> +            return NS_ERROR_FAILURE;

braces around the if-statement { ... } body, even when it's a single line.

@@ +877,5 @@
> +        NS_ENSURE_SUCCESS(rv, rv);
> +        if (written != read)
> +            return NS_ERROR_FAILURE;
> +        if (read != 1024)
> +            break;

ditto (and there are several more instances of this below)

@@ +982,4 @@
>      nsresult rv = NS_GetSpecialDirectory(NS_APP_RES_DIR,
>                                           getter_AddRefs(localDir));
>      if (NS_SUCCEEDED(rv)) {
> +        if (NS_SUCCEEDED(localDir->Append(NS_LITERAL_STRING("fonts")))) {

If you combine the two if-expressions with && here, you can reduce the indent of everything that follows, which would be nice.

@@ +985,5 @@
> +        if (NS_SUCCEEDED(localDir->Append(NS_LITERAL_STRING("fonts")))) {
> +            bool exists;
> +            localDir->Exists(&exists);
> +            if (!exists)
> +                localDir->Create(nsIFile::DIRECTORY_TYPE, 0770);

Is mode 0770 needed, or would 0700 be sufficient? Doesn't seem like anyone else should need to touch the directory.

@@ +991,5 @@
> +            nsCString jarPath;
> +            Omnijar::GetURIString(Omnijar::Type::GRE, jarPath);
> +            int64_t jarModifiedTime;
> +            nsCOMPtr<nsIFile> jarFile = Omnijar::GetPath(Omnijar::Type::GRE);
> +            jarFile->GetLastModifiedTime(&jarModifiedTime);

To minimize the impact on normal startup, would it be worth recording the modification time of the last successfully-extracted jar file in the profile (or startupcache?), and only iterating over the fonts if it has changed?

And for readability, I think it'd be better to have an additional helper function that does the iteration over the jar file rather than coding that directly within FindFonts().

That helper could return a success code indicating whether it encountered any errors while extracting the files; if it reports failure, don't update the last-extracted time, and we'll re-try on the next startup. (Perhaps there wasn't room, but then the user deletes something...)

@@ +1009,5 @@
> +                        break;
> +
> +                    nsCString path(tmpPath, len);
> +                    nsCOMPtr<nsIFile> localFile (do_CreateInstance(NS_LOCAL_FILE_CONTRACTID));
> +                    rv = localFile->InitWithFile(localDir);

Shouldn't we check rv for success here? (Though I suppose failure is vanishingly unlikely...)

@@ +1017,5 @@
> +                    else
> +                        if (NS_FAILED(localFile->AppendNative(Substring(path, lastSlash + 1)))) {
> +                            ALOG("failed to append file name");
> +                            continue;
> +                        }

Why does only one of the branches check for AppendNative failure here?

@@ +1025,5 @@
> +                    if (!exists || lastModifiedTime < jarModifiedTime) {
> +                        nsCString spec;
> +                        spec.Append(jarPath);
> +                        spec.Append(path);
> +                        CopyFromUriToFile(spec, localFile);

We should probably check the return value of CopyFromUriToFile for success, and delete the target localFile if it returns an error.

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch to extract fonts on first run

Comment 16

[Jonathan Kew [:jfkthame]]

Comment on attachment 666779 [details] [diff] [review]
patch to extract fonts on first run

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

Looks good - a few minor tweaks/nits below, but let's do it.

I suppose we can't really test this until we decide on a font or fonts to actually include in the package, but once we do that, we should make sure to write testcases that verify they're becoming available as expected.

::: gfx/thebes/gfxFT2FontList.cpp
@@ +855,5 @@
>  }
>  
> +#ifdef ANDROID
> +
> +#define JAR_READ_BUFFFER_SIZE 1024

s/BUFFFER/BUFFER/ :)

@@ +858,5 @@
> +
> +#define JAR_READ_BUFFFER_SIZE 1024
> +
> +nsresult
> +CopyFromUriToFile(nsCString spec, nsIFile* localFile)

Although we sometimes overlook it, style guide calls for "a" prefix on arguments. So aSpec, aLocalFile please.

@@ +877,5 @@
> +    while (true) {
> +        PRUint32 read;
> +        PRUint32 written;
> +
> +        rv = inputStream->Read(buf, 1024, &read);

s/1024/JAR_READ_BUFFER_SIZE/

@@ +896,5 @@
> +}
> +
> +#define JAR_LAST_MODIFED_TIME "jar-last-modified-time"
> +
> +void ExtractFontsFromJar(nsIFile* localDir) {

aLocalDir. Newline before the open-brace, please.

@@ +914,5 @@
> +    uint32_t longSize;
> +    char* cachedModifiedTimeBuf;
> +    if (NS_SUCCEEDED(cache->GetBuffer(JAR_LAST_MODIFED_TIME, &cachedModifiedTimeBuf, &longSize))
> +        && longSize == sizeof(int64_t)) {
> +        if (jarModifiedTime < *((int64_t*) cachedModifiedTimeBuf)) {

I'd be inclined to use == rather than < here, so that we re-extract the fonts in the case of downgrading to an older APK.

@@ +917,5 @@
> +        && longSize == sizeof(int64_t)) {
> +        if (jarModifiedTime < *((int64_t*) cachedModifiedTimeBuf)) {
> +            return;
> +        }
> +    }

A minor tweak, but we should move the check of jarModifiedTime to the start of the function; no point in checking existence of localDir, or getting the zip-reader or the jarPath string, if we're just going to do an early return anyway.

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

> @@ +914,5 @@
> > +    uint32_t longSize;
> > +    char* cachedModifiedTimeBuf;
> > +    if (NS_SUCCEEDED(cache->GetBuffer(JAR_LAST_MODIFED_TIME, &cachedModifiedTimeBuf, &longSize))
> > +        && longSize == sizeof(int64_t)) {
> > +        if (jarModifiedTime < *((int64_t*) cachedModifiedTimeBuf)) {
> 
> I'd be inclined to use == rather than < here, so that we re-extract the
> fonts in the case of downgrading to an older APK.
This is file system modified time, so even down grading will result in a newer modified time

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

Two notes:
1) I've been testing this by adding chrome/chrome/skin/fonts to the jar search path to pick up the open sans fonts that are there and they are being extracted and their font data read
2) As this is constructed now, we'll prefer system fonts to APK fonts because they come first in the search. If we want to change that, it is just a matter of moving the APK extraction and search up in that file.

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7ff5aecff0c
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a788605acfe

If we want to move these fonts up in the search list, file a follow up bug for it

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5a788605acfe
https://hg.mozilla.org/mozilla-central/rev/d7ff5aecff0c

Comment 21

[Mathieu Pellerin]

This development is really exciting. As it stands, Android 2.x doesn't support Khmer rendering while Android >= 3.1 has rendering support but to this day, there is no default Khmer font at the system level.

Would Firefox for Android consider adding a Khmer font into its package to enable proper rendering on pages that do not embed a web font? Should a bug be filed on this?

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Mathieu Pellerin from comment #21)
> This development is really exciting. As it stands, Android 2.x doesn't
> support Khmer rendering while Android >= 3.1 has rendering support but to
> this day, there is no default Khmer font at the system level.
> 
> Would Firefox for Android consider adding a Khmer font into its package to
> enable proper rendering on pages that do not embed a web font? Should a bug
> be filed on this?

See bug 619521 for the solution for that problem. We wouldn't want to ship all fonts for all languages by default, but instead offer a mechanism where they can be downloaded on demand if needed.

Comment 23

[Mathieu Pellerin]

Filed bug 797662. It'd be nice if simply downloading a .ttf/.otf would offer a UI to copy the font into the user profile. 

Based on work done in 619521, could one simply put a font in an add-on .xpi and have it recognized by firefox?

Comment 24

[Jonathan Kew [:jfkthame]]

(In reply to Mathieu Pellerin from comment #23)

> Filed bug 797662. It'd be nice if simply downloading a .ttf/.otf would offer
> a UI to copy the font into the user profile. 

Could you file a bug for this specific suggestion? It sounds like an idea well worth considering.

> Based on work done in 619521, could one simply put a font in an add-on .xpi
> and have it recognized by firefox?

I think so. Provided the .xpi is set up such that installing it will place the font into the profile directory, it should become available after quitting/restarting the browser. (Once all of bug 619521 is done, restart should no longer be needed, as the add-on could notify the browser that it needs to refresh its font list.)

Comment 25

[Mathieu Pellerin]

Jonathan, filed bug 798199 re mechanism to automatically place font URL into user profile directory.

One more question, does the fonts have to be placed in the root of the user profile directory or can it be placed in a sub-folder?

Comment 26

[Jonathan Kew [:jfkthame]]

(In reply to Mathieu Pellerin from comment #25)
> One more question, does the fonts have to be placed in the root of the user
> profile directory or can it be placed in a sub-folder?

Currently, the code looks (only) in the root dir of the profile. However, we might want to create a "fonts" subdir there and designate that as the place we'll search, I guess. (Given that we're not yet making any use of the mechanism, I think you should consider it experimental and subject to change.)