SkPDF: refactor font subset: fewer copies

SkPDF: refactor font subset: fewer copies

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2190643002

Committed: https://skia.googlesource.com/skia/+/fe8f0e0d3126d27fe9fdd5bc4804392492f14e51

[hal.canary, 2016-07-27 14:39:53]

Description was changed from

==========
SkPDF: refactor font subset: fewer copies

BUG=skia:
==========

to

==========
SkPDF: refactor font subset: fewer copies

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2190643002
==========

[hal.canary, 2016-07-27 14:40:01]

The CQ bit was checked by halcanary@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 14:40:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hal.canary, 2016-07-27 14:42:03]

The CQ bit was unchecked by halcanary@google.com

[hal.canary, 2016-07-27 14:42:48]

halcanary@google.com changed reviewers:
+ bungeman@google.com

[hal.canary, 2016-07-27 14:42:49]

PTAL

Note:  all gm/skp PDFs are identical.

[hal.canary, 2016-07-27 18:25:21]

rebased. take another look.

[hal.canary, 2016-07-27 18:31:17]

The CQ bit was checked by halcanary@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 18:31:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 19:18:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-27 19:18:22]

Dry run: This issue passed the CQ dry run.

[bungeman-skia, 2016-07-27 19:26:44]

https://codereview.chromium.org/2190643002/diff/40001/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

https://codereview.chromium.org/2190643002/diff/40001/src/pdf/SkPDFFont.cpp#n...
src/pdf/SkPDFFont.cpp:1033:
fontAsset.reset(this->typeface()->openStream(&ttcIndex));
Hopefully it doesn't matter too much, but note that this means 'fontData' and
'fontAsset' are both active here, meaning that there may be two copies / mmaps
of the same data in memory (and the the 'subsetFont' data as well). Indeed, the
lifetime of 'fontData' appears to just be around the call to SubsetFont, it
could go away even before making the SkPDFStream.

I know inlining this made some of this more straight forward, but the five
levels of indentation plus indentation for expression continuation makes this
much more difficult to take in.

[hal.canary, 2016-07-27 19:54:08]

Patchset #4 (id:60001) has been deleted

[hal.canary, 2016-07-27 19:54:12]

The CQ bit was checked by halcanary@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 19:54:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hal.canary, 2016-07-27 19:54:25]

good catches!

https://codereview.chromium.org/2190643002/diff/40001/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

https://codereview.chromium.org/2190643002/diff/40001/src/pdf/SkPDFFont.cpp#n...
src/pdf/SkPDFFont.cpp:1033:
fontAsset.reset(this->typeface()->openStream(&ttcIndex));
On 2016/07/27 19:26:44, bungeman-skia wrote:
> Hopefully it doesn't matter too much, but note that this means 'fontData' and
> 'fontAsset' are both active here, meaning that there may be two copies / mmaps
> of the same data in memory

Fixed.  now fontData is out of scope (in another fn) before this call.

> (and the the 'subsetFont' data as well). 

nope.

> Indeed, the
> lifetime of 'fontData' appears to just be around the call to SubsetFont, it
> could go away even before making the SkPDFStream.

done.

> I know inlining this made some of this more straight forward, but the five
> levels of indentation plus indentation for expression continuation makes this
> much more difficult to take in.

fixed.

[hal.canary, 2016-07-27 19:54:26]

good catches!

https://codereview.chromium.org/2190643002/diff/40001/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

https://codereview.chromium.org/2190643002/diff/40001/src/pdf/SkPDFFont.cpp#n...
src/pdf/SkPDFFont.cpp:1033:
fontAsset.reset(this->typeface()->openStream(&ttcIndex));
On 2016/07/27 19:26:44, bungeman-skia wrote:
> Hopefully it doesn't matter too much, but note that this means 'fontData' and
> 'fontAsset' are both active here, meaning that there may be two copies / mmaps
> of the same data in memory

Fixed.  now fontData is out of scope (in another fn) before this call.

> (and the the 'subsetFont' data as well). 

nope.

> Indeed, the
> lifetime of 'fontData' appears to just be around the call to SubsetFont, it
> could go away even before making the SkPDFStream.

done.

> I know inlining this made some of this more straight forward, but the five
> levels of indentation plus indentation for expression continuation makes this
> much more difficult to take in.

fixed.

[hal.canary, 2016-07-27 20:05:51]

The CQ bit was unchecked by halcanary@google.com

[bungeman-skia, 2016-07-27 20:15:59]

https://codereview.chromium.org/2190643002/diff/80001/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

https://codereview.chromium.org/2190643002/diff/80001/src/pdf/SkPDFFont.cpp#n...
src/pdf/SkPDFFont.cpp:994: fontData = nullptr;  // Release memory if necessary.
Instead of doing this, can this be like

unsigned char* subsetFont = nullptr;
int subsetFontSize = 0;
{
    sk_sp<SkData> fontData(...);
    subsetFontSize = ...
}

or even something roughly like

unsigned char* subsetFont = nullptr;
int subsetFontSize = [&]() {
    sk_sp<SkData> fontData(...);
    return SfntlyWrapper::SubsetFont( ... );
}

so that fontData just naturally goes out of scope.

https://codereview.chromium.org/2190643002/diff/80001/src/pdf/SkPDFFont.cpp#n...
src/pdf/SkPDFFont.cpp:996: if (subsetFontSize > 0) {
Can we reverse this so that less code goes into the block.

if (subsetFontSize < 1) {
    return nullptr;
}

...

Then the code that's currently in the block is less indented, so it can fit more
per line. Also, the 'successful' 'return subset;' really wants to be on the last
line of this function.

[hal.canary, 2016-07-27 20:22:56]

The CQ bit was checked by halcanary@google.com to run a CQ dry run

[hal.canary, 2016-07-27 20:23:04]

https://codereview.chromium.org/2190643002/diff/80001/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

https://codereview.chromium.org/2190643002/diff/80001/src/pdf/SkPDFFont.cpp#n...
src/pdf/SkPDFFont.cpp:994: fontData = nullptr;  // Release memory if necessary.
On 2016/07/27 20:15:59, bungeman-skia wrote:
> Instead of doing this, can this be like
> 
> unsigned char* subsetFont = nullptr;
> int subsetFontSize = 0;
> {
>     sk_sp<SkData> fontData(...);
>     subsetFontSize = ...
> }
> 
> or even something roughly like
> 
> unsigned char* subsetFont = nullptr;
> int subsetFontSize = [&]() {
>     sk_sp<SkData> fontData(...);
>     return SfntlyWrapper::SubsetFont( ... );
> }
> 
> so that fontData just naturally goes out of scope.

Done.

https://codereview.chromium.org/2190643002/diff/80001/src/pdf/SkPDFFont.cpp#n...
src/pdf/SkPDFFont.cpp:996: if (subsetFontSize > 0) {
On 2016/07/27 20:15:59, bungeman-skia wrote:
> Can we reverse this so that less code goes into the block.
> 
> if (subsetFontSize < 1) {
>     return nullptr;
> }
> 
> ...
> 
> Then the code that's currently in the block is less indented, so it can fit
more
> per line. Also, the 'successful' 'return subset;' really wants to be on the
last
> line of this function.

Done.

[commit-bot: I haz the power, 2016-07-27 20:23:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bungeman-skia, 2016-07-27 20:27:29]

So long as tomhudson is ok with your use of auto, lgtm.

[hal.canary, 2016-07-27 20:29:22]

everywhere I use auto is with sk_make_sp

[hal.canary, 2016-07-27 20:29:31]

The CQ bit was unchecked by halcanary@google.com

[hal.canary, 2016-07-27 20:29:39]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2016-07-27 20:29:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hal.canary, 2016-07-27 21:01:14]

The CQ bit was unchecked by halcanary@google.com

[hal.canary, 2016-07-27 21:01:25]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2016-07-27 21:01:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 21:14:14]

Description was changed from

==========
SkPDF: refactor font subset: fewer copies

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2190643002
==========

to

==========
SkPDF: refactor font subset: fewer copies

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2190643002

Committed:
https://skia.googlesource.com/skia/+/fe8f0e0d3126d27fe9fdd5bc4804392492f14e51
==========

[commit-bot: I haz the power, 2016-07-27 21:14:21]

Committed patchset #5 (id:100001) as
https://skia.googlesource.com/skia/+/fe8f0e0d3126d27fe9fdd5bc4804392492f14e51