Closed Bug 1324739 Opened 3 years ago Closed 3 years ago

Skia PDF generates a large size file in Windows

Categories

(Core :: Printing: Output, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: fatseng, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

No description provided.
Blocks: 1321496
Looks like Chrome use subsetting to reduce font size.

https://skia.org/dev/design/pdftheory#Fonts

Font subsetting

Many fonts, especially fonts with CJK support are fairly large, so it is desirable to subset them. Chrome uses the SFNTLY package to provide subsetting support to Skia for TrueType fonts.
Looks like SkPDF is already prepared to use this, all we need to do is add it to our build and enable it.
Attachment #8824945 - Flags: review?(lsalzman)
Note that sfntly depends on ICU, hence the dependency on ENABLE_INTL_API here; this means we can't (currently) enable this on Android. There are probably better ways to handle the build-config side of this, but I don't know much about the whole build system stuff.
Attachment #8824946 - Flags: review?(lsalzman)
Nice! Previously you said "I notice that currently SFNTLY doesn't seem to support subsetting for CFF-based fonts, for example, so integrating it would be only a partial solution (though it should help in many common cases)." Do you know if cairo also has the same limitations as SFNTLY? Or would someone need to bring SFNTLY up to parity with cairo's PDF backend with regards to subsetting?
I don't know much about the cairo pdf code (Jeff may be more familiar with this stuff), but from a cursory look into cairo-pdf-surface.c it appears to include code to generate subsets of CFF fonts (as well as TrueType).

So I strongly suspect that using SkiaPDF, even with SFNTLY, to generate PDFs would be less-optimal compared to cairo in the case of documents that use large CFF fonts (prime examples would be Adobe CJK fonts).

I don't know if there's any current interest/work upstream re adding CFF subsetting to SFNTLY, but it certainly sounds like something that should be done.
Attachment #8824945 - Flags: review?(lsalzman) → review+
Comment on attachment 8824946 [details] [diff] [review]
patch 2 - Enable use of sfntly in Skia-PDF to subset fonts

r+ but two issues:

1) You need to add SFNTLY_INCLUDES to LOCAL_INCLUDES in the generate_mozbuild, not just moz.build

2) I don't think this will work on Linux or Android at all for the reason that we haven't implemented accessing streams in SkFontHost_Cairo yet, and possibly also for issues with advanced typeface metrics which we haven't implemented either... Though it will conceivably work on the font hosts for other platforms which do implement those in Skia. I guess this is solvable eventually, but it does need to be solved somehow.
Attachment #8824946 - Flags: review?(lsalzman) → review+
Updated to fix (1) from comment 7; carrying over r=lsalzman. Tagging :glandium for additional r? here, as this is build-config stuff.
Attachment #8825321 - Flags: review?(mh+mozilla)
Attachment #8824946 - Attachment is obsolete: true
Comment on attachment 8825321 [details] [diff] [review]
patch 2 - Enable use of sfntly in Skia-PDF to subset fonts

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

::: gfx/sfntly/cpp/src/moz.build
@@ +12,5 @@
> +    'sample/chromium/font_subsetter.cc',
> +    'sample/chromium/subsetter_impl.cc',
> +]
> +
> +UNIFIED_SOURCES += [

Separating the lists doesn't make any kind of difference, so you can just put everything in the same list.

::: toolkit/moz.configure
@@ +830,5 @@
> +def skia_pdf_sfntly(value, skia_pdf, milestone):
> +    if value.origin == 'default':
> +        if not skia_pdf:
> +            return None
> +        if milestone.is_nightly:

You can just return skia_pdf here. Or do you want to keep sfntly default to nightly only when skia pdf starts to ride the train?
Attachment #8825321 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] from comment #9)
> Comment on attachment 8825321 [details] [diff] [review]
> patch 2 - Enable use of sfntly in Skia-PDF to subset fonts
> 
> Review of attachment 8825321 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/sfntly/cpp/src/moz.build
> @@ +12,5 @@
> > +    'sample/chromium/font_subsetter.cc',
> > +    'sample/chromium/subsetter_impl.cc',
> > +]
> > +
> > +UNIFIED_SOURCES += [
> 
> Separating the lists doesn't make any kind of difference, so you can just
> put everything in the same list.

I realize it doesn't change the result, but it seemed more convenient to generate and maintain because of the requirement to be sorted by filename (not, apparently, by path), which means we'd have to interleave files from the various subdirectories. Doing it this way allows the build system to deal with that as it merges each subdir's contents into the result.

So I'd prefer to leave things this way, unless you specifically object to that.

> You can just return skia_pdf here. Or do you want to keep sfntly default
> to nightly only when skia pdf starts to ride the train?

I was initially thinking we might want to keep separate control of this, but I doubt that's really useful, so I'll simplify this. Thanks!
Blocks: 1321689
> because of the requirement to be sorted by filename (not, apparently, by path)

Huh? The requirement is to be sorted by path, not filename.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b28f77d3a69a51af7e7f196f488a3d75421bf9ea
Bug 1324739 - patch 1 - Import sfntly (C++ port) into the mozilla tree. r=lsalzman

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8257327be700ff391848d9568ba106d45cbf4a3
Bug 1324739 - patch 2 - Enable use of sfntly in Skia-PDF to subset fonts. r=lsalzman,glandium
(In reply to Mike Hommey [:glandium] from comment #11)
> > because of the requirement to be sorted by filename (not, apparently, by path)
> 
> Huh? The requirement is to be sorted by path, not filename.

Oh? I thought I ran into issues that suggested the opposite, but perhaps I was mistaken; and now I just pushed it before seeing your comment. I'll double-check, and we can potentially combine them in a followup.
OK, this does work, as you said; I think I was thrown off-track by my editor "sorting" things slightly differently.
Attachment #8825957 - Flags: review?(mh+mozilla)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Comment on attachment 8825957 [details] [diff] [review]
followup, put all source files into a single UNIFIED_SOURCES list

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

Hopefully, this still builds despite the reordering.
Attachment #8825957 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/af583a04d3f99af606fa27229b0f991fbcc3a00a
Bug 1324739 followup, put all source files into a single UNIFIED_SOURCES list. r=glandium
Depends on: 1330495
https://hg.mozilla.org/mozilla-central/rev/b28f77d3a69a
https://hg.mozilla.org/mozilla-central/rev/b8257327be70
https://hg.mozilla.org/mozilla-central/rev/af583a04d3f9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1384902
You need to log in before you can comment on or make changes to this bug.