Track detailed LibTxt metrics #10127

GaryQian · 2019-07-25T00:41:35Z

This is the LibTxt portion of detailed metrics tracking.

Here, LineMetrics replaces a variety of vectors and objects in layout and tracks the metrics/values for each line in a central location. At the LibTxt level, we provide public access to all the final metrics through GetLineMetrics().

There will be a follow-up PR that will introduce a dart:ui API to expose this data to dart. I plan to slice/provide subsets of the full line metrics in the native side of dart:ui.

Adds features to support flutter/flutter#35994

Hixie · 2019-07-29T17:40:48Z

third_party/txt/src/txt/paragraph_txt.cc

+
+        line_metrics.run_metrics_map.emplace(
+            std::piecewise_construct, std::forward_as_tuple(run.start()),
+            std::forward_as_tuple(run.style(), metrics));


do we pay for this copy every time we paint text, even when we never look at the metrics? (or is it cheaper than a copy?)

Hixie · 2019-07-29T17:41:05Z

third_party/txt/src/txt/paragraph_txt.cc

+    line_metrics.width = line_widths_[line_number];
+    line_metrics.left = line_x_offset;
+    line_metrics.baseline = line_baselines_.back();
+    // line_metrics.run_metrics_map = &run_metrics_map;


Hixie · 2019-07-29T17:42:25Z

Might be interesting to see if there's a way we can avoid doing work in the common case where we're not reading any metrics out of the paragraph, and have a separate mode where we enable metrics collection.

GaryQian · 2019-07-29T18:14:36Z

I should be able to change this to only copy the reference.

The alternative to collecting this in layout is to perform multiple loops to build this when metrics are first requested. This would result in a redundant and parallel set of loops alongside the layout, but prevent additional overhead in layout.

I don't think this operation is particularly heavy though, as compared to layout operations, even a full copy is negligible.

GaryQian · 2019-07-29T20:55:18Z

I was able to make the line_metrics_ the canonical place the SkFontMetrics are tracked, removing the need to copy at all.

GaryQian · 2019-07-29T23:17:13Z

cc @Rusino Please let me know what I can do here to make your job easier! All of the implementation/API as of now is totally in flux.

GaryQian · 2019-07-31T21:27:51Z

I will break this into two parts: Libtxt tracking the metrics, and exposing the dart:ui API to access the metrics. This PR represents the first part.

The new implementation should add negligible overhead as it combines multiple preexisting internal data structures into one, and it could actually slightly increase performance due to less overhead of multiple separate data structures.

chinmaygarde · 2019-07-31T21:46:25Z

third_party/txt/src/txt/line_metrics.h

+
+namespace txt {
+
+class LineMetrics {


Please zero out all members. Not all of them are set in the constructor and will end up containing garbage values.

chinmaygarde · 2019-07-31T21:48:32Z

third_party/txt/src/txt/line_metrics.h

+
+  LineMetrics();
+
+  LineMetrics(size_t s, size_t e, size_t eew, size_t ein, bool h)


I think abbreviating it this way hurts readability. If you want to distinguish between parameters and the struct arguments, you can add the param_ or p_ suffix.

chinmaygarde · 2019-07-31T21:49:51Z

third_party/txt/src/txt/line_metrics.h

+  // Mapping between text index ranges and the FontMetrics associated with
+  // them. The first run will be keyed under start_index. The metrics here
+  // are before layout and are the base values we calculate from.
+  std::map<size_t, RunMetrics> run_metrics_map;


_map is redundant.

chinmaygarde · 2019-07-31T21:51:08Z

third_party/txt/src/txt/run_metrics.h

+  const TextStyle& GetTextStyle() const { return *text_style_; }
+
+ private:
+  const TextStyle* text_style_;


Explicitly disable copy and assign instead of depending on the const-ness of this member (which may change in the future) to achieve the same.

chinmaygarde · 2019-07-31T21:53:29Z

third_party/txt/src/txt/line_metrics.h

+  size_t line_number;
+
+  // Mapping between text index ranges and the FontMetrics associated with
+  // them. The first run will be keyed under start_index. The metrics here


start_index is not a struct member. Is this docstring out of date? Maybe you just meant start. In that case, start_index is a more descriptive name

chinmaygarde · 2019-07-31T21:53:43Z

third_party/txt/src/txt/line_metrics.h

+  // The following fields are used in the layout process itself.
+
+  // The indexes in the text buffer the line begins and ends.
+  size_t start, end;


We usually put one field member per line.

This reverts commit 8682aaf.

flutter/engine@be4c8338a Roll src/third_party/dart 20407e28db..45f892df68 (2 commits) flutter/engine@da352d0f7 Revert "Track detailed LibTxt metrics with LineMetrics(flutter/engine#10127)" (flutter/engine#10982) flutter/engine@23700fc1e Roll src/third_party/dart b9217efc77..20407e28db (6 commits) flutter/engine@07f2cfa0e Roll src/third_party/skia 6e1b7bb2b5df..cd8b6d5c1cb8 (10 commits) (flutter/engine#10980) flutter/engine@7711efbf0 Return an empty mapping for an empty file asset (flutter/engine#10815) flutter/engine@663f9a9e1 Roll src/third_party/skia 6ef31815a694..6e1b7bb2b5df (1 commits) (flutter/engine#10979) flutter/engine@0a0a14202 Roll fuchsia/sdk/core/linux-amd64 from z99ul... to j8BvV... (flutter/engine#10978) flutter/engine@e13df85ce Roll src/third_party/dart 9aea1f3489..b9217efc77 (7 commits) flutter/engine@d0698f275 Roll src/third_party/skia f99631100372..6ef31815a694 (3 commits) (flutter/engine#10976) flutter/engine@7dfdfc6fa Roll src/third_party/skia b875cc709c7f..f99631100372 (2 commits) (flutter/engine#10974) flutter/engine@274659564 Roll src/third_party/dart 06509e333d..9aea1f3489 (8 commits) flutter/engine@e22893966 Expose isolateId for engine (flutter/engine#10823) flutter/engine@a18fa3722 Roll src/third_party/dart baebba06af..06509e333d (7 commits) flutter/engine@5909cf930 Roll fuchsia/sdk/core/linux-amd64 from u8FN9... to z99ul... (flutter/engine#10972) flutter/engine@144007da5 Roll src/third_party/skia ef0406f04761..b875cc709c7f (4 commits) (flutter/engine#10970) flutter/engine@6a4dfa7aa include zx::clock from new location to fix Fuchsia autoroll. (flutter/engine#10968) flutter/engine@ed88d0a44 Change SemanticsNode#children lists to be non-null (flutter/engine#10952) flutter/engine@853464e36 Roll src/third_party/dart b31df28d72..baebba06af (5 commits) flutter/engine@fb317b085 Roll src/third_party/dart 896c053803..b31df28d72 (10 commits) flutter/engine@20b7ea234 Increase the license block scan from 5k to 6k (flutter/engine#10956) flutter/engine@a438469aa Fix format (flutter/engine#10955) flutter/engine@70dd606ba Roll src/third_party/skia 8d1b0bceedd6..ef0406f04761 (8 commits) (flutter/engine#10954) flutter/engine@cff3f3d66 Roll src/third_party/dart 32b70ce2a5..896c053803 (1 commits) flutter/engine@b5c198b69 Fix iOS references to PostPrerollResult (flutter/engine#10949) flutter/engine@971a63915 Allow for dynamic thread merging on IOS for embedded view mutations (flutter/engine#9819) flutter/engine@c92a0d992 Report JUnit test failures (flutter/engine#10941) flutter/engine@e95125aad Fix iOS keyboard crash (flutter/engine#10656) flutter/engine@a50ec07d5 Bump local podspec's iOS deployment target from 7.0 to 8.0 (flutter/engine#10662) flutter/engine@0ce28b100 Roll src/third_party/skia c2da70fbef53..8d1b0bceedd6 (2 commits) (flutter/engine#10937) flutter/engine@07ee128c2 Roll src/third_party/dart 9adf3c119e..32b70ce2a5 (3 commits) flutter/engine@79c50123a iOS JIT support and enhancements for scenarios app (flutter/engine#10820) flutter/engine@d3be62888 Roll src/third_party/dart 261fd6266b..9adf3c119e (2 commits) flutter/engine@68b749624 Roll src/third_party/skia da037b85352e..c2da70fbef53 (1 commits) (flutter/engine#10924) flutter/engine@d3523b9b7 Roll src/third_party/skia a47c48ef6bce..da037b85352e (3 commits) (flutter/engine#10909) flutter/engine@b39019d57 Roll src/third_party/dart e29d6d0ecb..261fd6266b (2 commits) flutter/engine@19f564acf Roll src/third_party/skia 9c23a9e790b2..a47c48ef6bce (1 commits) (flutter/engine#10896) flutter/engine@4099cb146 Roll src/third_party/skia 5e7271a4598e..9c23a9e790b2 (1 commits) (flutter/engine#10838) flutter/engine@a3fa41b3b Roll src/third_party/skia 3e7fa79f2c79..5e7271a4598e (1 commits) (flutter/engine#10827) flutter/engine@99355b086 Roll fuchsia/sdk/core/linux-amd64 from bvUCO... to xqsy5... (flutter/engine#10826) flutter/engine@8a068a1bd Roll src/third_party/skia 89e43889afd9..3e7fa79f2c79 (1 commits) (flutter/engine#10825) flutter/engine@ee6267d04 Roll src/third_party/skia fe19203eb7d7..89e43889afd9 (1 commits) (flutter/engine#10824) flutter/engine@7ad538dcc Roll fuchsia/sdk/core/linux-amd64 from HsuAH... to bvUCO... (flutter/engine#10822) flutter/engine@cbc2fe6e1 Roll src/third_party/dart ffefa124a7..e29d6d0ecb (4 commits) flutter/engine@0afba6b1f Roll src/third_party/skia af89d3972e0b..fe19203eb7d7 (5 commits) (flutter/engine#10819) flutter/engine@6c678bfca Roll src/third_party/dart 15a3bf82cb..ffefa124a7 (11 commits) flutter/engine@03caa67ac Roll Dart SDK 78ce916d82..15a3bf82cb (flutter/engine#10810) flutter/engine@e096f7830 Roll src/third_party/skia b7b0b3ad538c..af89d3972e0b (7 commits) (flutter/engine#10812) flutter/engine@cb1628986 Revert "Remove flutter_kernel_sdk dart script (flutter/engine#10808)" (flutter/engine#10811) flutter/engine@42afb1abb [dart:zircon] Porting Cache re-usable handle wait objects (flutter/engine#10809) flutter/engine@1b287524e Remove flutter_kernel_sdk dart script (flutter/engine#10808) flutter/engine@58f4f27a4 change add part to add child (flutter/engine#10787) flutter/engine@708fc625d rename stub_ui to web_ui (flutter/engine#10776) flutter/engine@70de3ec2c [fuchsia] Migrate from custom FuchsiaFontManager to SkFontMgr_fuchsia (flutter/engine#10700) flutter/engine@e0beaff61 Roll src/third_party/skia 26cedb4c603b..b7b0b3ad538c (11 commits) (flutter/engine#10807) flutter/engine@83b640d4b Rename flutter_java.jar and flutter_engine.jar so they match the Maven comvention (flutter/engine#10797) flutter/engine@c5d3c90d9 Roll fuchsia/sdk/core/linux-amd64 from 9xVXx... to HsuAH... (flutter/engine#10806) flutter/engine@97326b242 Roll src/third_party/dart 3d9a356f6e..78ce916d82 (7 commits) flutter/engine@086c0d50e Roll src/third_party/skia b2151310068b..26cedb4c603b (1 commits) (flutter/engine#10804) flutter/engine@1c75ee7c0 Roll src/third_party/skia 059bf776c184..b2151310068b (3 commits) (flutter/engine#10803) flutter/engine@2611a6eef Roll src/third_party/dart f29f41f1a5..3d9a356f6e (65 commits) flutter/engine@09a05f8b9 Roll src/third_party/skia 4d557e3df433..059bf776c184 (1 commits) (flutter/engine#10801)

This reverts commit 8682aaf.

flutter/engine@be4c8338a Roll src/third_party/dart 20407e28db..45f892df68 (2 commits) flutter/engine@da352d0f7 Revert "Track detailed LibTxt metrics with LineMetrics(flutter/engine#10127)" (flutter/engine#10982) flutter/engine@23700fc1e Roll src/third_party/dart b9217efc77..20407e28db (6 commits) flutter/engine@07f2cfa0e Roll src/third_party/skia 6e1b7bb2b5df..cd8b6d5c1cb8 (10 commits) (flutter/engine#10980) flutter/engine@7711efbf0 Return an empty mapping for an empty file asset (flutter/engine#10815) flutter/engine@663f9a9e1 Roll src/third_party/skia 6ef31815a694..6e1b7bb2b5df (1 commits) (flutter/engine#10979) flutter/engine@0a0a14202 Roll fuchsia/sdk/core/linux-amd64 from z99ul... to j8BvV... (flutter/engine#10978) flutter/engine@e13df85ce Roll src/third_party/dart 9aea1f3489..b9217efc77 (7 commits) flutter/engine@d0698f275 Roll src/third_party/skia f99631100372..6ef31815a694 (3 commits) (flutter/engine#10976) flutter/engine@7dfdfc6fa Roll src/third_party/skia b875cc709c7f..f99631100372 (2 commits) (flutter/engine#10974) flutter/engine@274659564 Roll src/third_party/dart 06509e333d..9aea1f3489 (8 commits) flutter/engine@e22893966 Expose isolateId for engine (flutter/engine#10823) flutter/engine@a18fa3722 Roll src/third_party/dart baebba06af..06509e333d (7 commits) flutter/engine@5909cf930 Roll fuchsia/sdk/core/linux-amd64 from u8FN9... to z99ul... (flutter/engine#10972) flutter/engine@144007da5 Roll src/third_party/skia ef0406f04761..b875cc709c7f (4 commits) (flutter/engine#10970) flutter/engine@6a4dfa7aa include zx::clock from new location to fix Fuchsia autoroll. (flutter/engine#10968) flutter/engine@ed88d0a44 Change SemanticsNode#children lists to be non-null (flutter/engine#10952) flutter/engine@853464e36 Roll src/third_party/dart b31df28d72..baebba06af (5 commits) flutter/engine@fb317b085 Roll src/third_party/dart 896c053803..b31df28d72 (10 commits) flutter/engine@20b7ea234 Increase the license block scan from 5k to 6k (flutter/engine#10956) flutter/engine@a438469aa Fix format (flutter/engine#10955) flutter/engine@70dd606ba Roll src/third_party/skia 8d1b0bceedd6..ef0406f04761 (8 commits) (flutter/engine#10954) flutter/engine@cff3f3d66 Roll src/third_party/dart 32b70ce2a5..896c053803 (1 commits) flutter/engine@b5c198b69 Fix iOS references to PostPrerollResult (flutter/engine#10949) flutter/engine@971a63915 Allow for dynamic thread merging on IOS for embedded view mutations (flutter/engine#9819) flutter/engine@c92a0d992 Report JUnit test failures (flutter/engine#10941) flutter/engine@e95125aad Fix iOS keyboard crash (flutter/engine#10656) flutter/engine@a50ec07d5 Bump local podspec's iOS deployment target from 7.0 to 8.0 (flutter/engine#10662) flutter/engine@0ce28b100 Roll src/third_party/skia c2da70fbef53..8d1b0bceedd6 (2 commits) (flutter/engine#10937) flutter/engine@07ee128c2 Roll src/third_party/dart 9adf3c119e..32b70ce2a5 (3 commits) flutter/engine@79c50123a iOS JIT support and enhancements for scenarios app (flutter/engine#10820) flutter/engine@d3be62888 Roll src/third_party/dart 261fd6266b..9adf3c119e (2 commits) flutter/engine@68b749624 Roll src/third_party/skia da037b85352e..c2da70fbef53 (1 commits) (flutter/engine#10924) flutter/engine@d3523b9b7 Roll src/third_party/skia a47c48ef6bce..da037b85352e (3 commits) (flutter/engine#10909) flutter/engine@b39019d57 Roll src/third_party/dart e29d6d0ecb..261fd6266b (2 commits) flutter/engine@19f564acf Roll src/third_party/skia 9c23a9e790b2..a47c48ef6bce (1 commits) (flutter/engine#10896) flutter/engine@4099cb146 Roll src/third_party/skia 5e7271a4598e..9c23a9e790b2 (1 commits) (flutter/engine#10838) flutter/engine@a3fa41b3b Roll src/third_party/skia 3e7fa79f2c79..5e7271a4598e (1 commits) (flutter/engine#10827) flutter/engine@99355b086 Roll fuchsia/sdk/core/linux-amd64 from bvUCO... to xqsy5... (flutter/engine#10826) flutter/engine@8a068a1bd Roll src/third_party/skia 89e43889afd9..3e7fa79f2c79 (1 commits) (flutter/engine#10825) flutter/engine@ee6267d04 Roll src/third_party/skia fe19203eb7d7..89e43889afd9 (1 commits) (flutter/engine#10824) flutter/engine@7ad538dcc Roll fuchsia/sdk/core/linux-amd64 from HsuAH... to bvUCO... (flutter/engine#10822) flutter/engine@cbc2fe6e1 Roll src/third_party/dart ffefa124a7..e29d6d0ecb (4 commits) flutter/engine@0afba6b1f Roll src/third_party/skia af89d3972e0b..fe19203eb7d7 (5 commits) (flutter/engine#10819) flutter/engine@6c678bfca Roll src/third_party/dart 15a3bf82cb..ffefa124a7 (11 commits) flutter/engine@03caa67ac Roll Dart SDK 78ce916d82..15a3bf82cb (flutter/engine#10810) flutter/engine@e096f7830 Roll src/third_party/skia b7b0b3ad538c..af89d3972e0b (7 commits) (flutter/engine#10812) flutter/engine@cb1628986 Revert "Remove flutter_kernel_sdk dart script (flutter/engine#10808)" (flutter/engine#10811) flutter/engine@42afb1abb [dart:zircon] Porting Cache re-usable handle wait objects (flutter/engine#10809) flutter/engine@1b287524e Remove flutter_kernel_sdk dart script (flutter/engine#10808) flutter/engine@58f4f27a4 change add part to add child (flutter/engine#10787) flutter/engine@708fc625d rename stub_ui to web_ui (flutter/engine#10776) flutter/engine@70de3ec2c [fuchsia] Migrate from custom FuchsiaFontManager to SkFontMgr_fuchsia (flutter/engine#10700) flutter/engine@e0beaff61 Roll src/third_party/skia 26cedb4c603b..b7b0b3ad538c (11 commits) (flutter/engine#10807) flutter/engine@83b640d4b Rename flutter_java.jar and flutter_engine.jar so they match the Maven comvention (flutter/engine#10797) flutter/engine@c5d3c90d9 Roll fuchsia/sdk/core/linux-amd64 from 9xVXx... to HsuAH... (flutter/engine#10806) flutter/engine@97326b242 Roll src/third_party/dart 3d9a356f6e..78ce916d82 (7 commits) flutter/engine@086c0d50e Roll src/third_party/skia b2151310068b..26cedb4c603b (1 commits) (flutter/engine#10804) flutter/engine@1c75ee7c0 Roll src/third_party/skia 059bf776c184..b2151310068b (3 commits) (flutter/engine#10803) flutter/engine@2611a6eef Roll src/third_party/dart f29f41f1a5..3d9a356f6e (65 commits) flutter/engine@09a05f8b9 Roll src/third_party/skia 4d557e3df433..059bf776c184 (1 commits) (flutter/engine#10801)

googlebot added the cla: yes label Jul 25, 2019

GaryQian mentioned this pull request Jul 25, 2019

Expose enough of LibTxt to make custom text layout practical flutter/flutter#35994

Closed

5 tasks

GaryQian self-assigned this Jul 25, 2019

GaryQian added the Work in progress (WIP) Not ready (yet) for review! label Jul 25, 2019

Hixie reviewed Jul 29, 2019

View reviewed changes

GaryQian changed the title ~~Expose detailed LibTxt metrics~~ Track detailed LibTxt metrics Jul 31, 2019

GaryQian removed the Work in progress (WIP) Not ready (yet) for review! label Jul 31, 2019

GaryQian added 10 commits July 31, 2019 14:34

Created new fiels

93abd86

Begin transition to reworked metrics

4e7281f

working, add tests

a72a820

More work, more tests

9ea58fd

Make font metrics directly stored in line_metrics_

bb90abf

Transition all layout to use LineMetrics instead of individual vectors

f7532e6

Update licenses

cf02a3f

Additional tests

036116f

Simplify libtxt level API for LineMetrics

157fe01

Update docs, cleanup

8d5b5cf

GaryQian force-pushed the metrics branch from ecad411 to 8d5b5cf Compare July 31, 2019 21:35

GaryQian requested review from cbracken, chinmaygarde and jason-simmons July 31, 2019 21:37

chinmaygarde reviewed Jul 31, 2019

View reviewed changes

Address comments

5510ade

chinmaygarde approved these changes Jul 31, 2019

View reviewed changes

GaryQian added 3 commits August 1, 2019 17:09

Prevent indexing into unused line_metrics_

f282100

Remove extra erase

ebd0802

Compile on windows

b08d0bb

GaryQian merged commit 8682aaf into flutter:master Aug 2, 2019

cfontas pushed a commit to cfontas/engine that referenced this pull request Aug 8, 2019

Track detailed LibTxt metrics with LineMetrics(flutter#10127)

47d2cfe

dnfield added a commit that referenced this pull request Aug 13, 2019

Revert "Track detailed LibTxt metrics with LineMetrics(#10127)"

1f3edae

This reverts commit 8682aaf.

dnfield mentioned this pull request Aug 13, 2019

Revert "Track detailed LibTxt metrics" #10982

Merged

dnfield added a commit that referenced this pull request Aug 13, 2019

Revert "Track detailed LibTxt metrics with LineMetrics(#10127)" (#10982)

da352d0

This reverts commit 8682aaf.

stuartmorgan-g mentioned this pull request Aug 13, 2019

Roll engine ff49ca1c6e5b..be4c8338a6ab (61 commits) flutter/flutter#38495

Merged

9 tasks

dnfield added a commit to dnfield/engine that referenced this pull request Aug 13, 2019

Revert "Track detailed LibTxt metrics with LineMetrics(flutter#10127)"

5b1b778

This reverts commit 8682aaf.

GaryQian mentioned this pull request Aug 22, 2019

Reland "Track detailed LibTxt metrics with LineMetrics(#10127)" #11064

Merged

GaryQian added a commit that referenced this pull request Aug 22, 2019

Reland "Track detailed LibTxt metrics with LineMetrics(#10127)" (#11064)

a1fe6a6


		LineMetrics();

		LineMetrics(size_t s, size_t e, size_t eew, size_t ein, bool h)


		namespace txt {

		class LineMetrics {

Track detailed LibTxt metrics #10127

Track detailed LibTxt metrics #10127

Uh oh!

Conversation

GaryQian commented Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Jul 29, 2019

Uh oh!

GaryQian commented Jul 29, 2019

Uh oh!

GaryQian commented Jul 29, 2019

Uh oh!

GaryQian commented Jul 29, 2019

Uh oh!

GaryQian commented Jul 31, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GaryQian commented Jul 25, 2019 •

edited

Loading