New issue
Advanced search Search tips

Issue 816445 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 19
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Revert PaintTypeface Implementation?

Project Member Reported by drott@chromium.org, Feb 26 2018

Issue description

In https://chromium-review.googlesource.com/c/chromium/src/+/756213 PaintTypeface was introduced as one possible serialization approach for sending Typefaces over to the GPU process. 

After various meetings and discussions, I believe we decided that following down this road is going to be complicated as all font creation mechanisms on all platforms would have to be covered in Blink to make this serialization approach succeed. 

In my understanding, the alternative that was discussed would be to implement serialization at the Skia level, sharing the glyph cache and potentially scaler contexts in Skia between renderer and GPU process. Blink would not require much changes. The fonts OOP raster implementation would be in a central location, with less cross cutting effects on Blink and more efficient in terms of memory sharing across processes.

vmpstr@, am I covering the discussion correctly?

If so, can we consider reverting the introduction of PaintTypeface? If not yet, for how long do we still need it? Thanks for taking a look.

 

Comment 1 by drott@chromium.org, Feb 26 2018

Sorry, I think the CL that introduced PaintTypeface was https://chromium-review.googlesource.com/730568

Comment 2 by vmp...@chromium.org, Feb 26 2018

Cc: enne@chromium.org
We're currently using these for performance testing before the alternative approaches are implemented. I'm unsure whether this path is still going to be useful, but we have also considered keeping this path for things like Linux and Android system fonts where it does work properly. I think this all depends on the performance characteristics of Skia's remotable fonts.


Comment 3 by drott@chromium.org, Feb 26 2018

Thank you for commenting. Okay, there is no rush. But could you get back here if you have the alternative approach working or at least something testable using the new aproach? 

Currently the PaintTypeface overlay introduces an additional level of complexity in the platform/font code which I would ideally like to reduce back from. 

Comment 4 by drott@chromium.org, May 31 2018

enne@, vmpstr@, since I saw https://chromium-review.googlesource.com/c/chromium/src/+/1077661 and heard about the other ongoing work for OOP fonts, can we reconsider this and revert the PaintTypeface abstraction, or at least, move the font creation back to renderer/platform/fonts?


Comment 5 by enne@chromium.org, May 31 2018

Yes, we will get to this eventually once we ship oop-r (issue 757605).  It's a lot of churn to remove this and is not our highest priority at the moment.

Comment 6 by drott@chromium.org, May 31 2018

Thanks for commenting, is there something that you still need this for? Otherwise I could probably take a look at moving this back, since it gets in the way of other Blink font work occasionally.

Comment 7 by enne@chromium.org, May 31 2018

I think the reason we had been wanting to keep it is that we only recently got a font implementation landed and we were concerned about performance.  Having this as an option to ship fonts on some platforms was an option we have been keeping around until we shipped.

I am much more confident that we don't need this, but have not reached 100% confidence yet.  Maybe give us another few weeks here to do some more measuring and perf analysis.

Comment 8 by drott@chromium.org, May 31 2018

Got it, okay.
Owner: khushals...@chromium.org
This can be reverted, assigning to khushalsagar to prioritize.
Thanks for the update, I can help with moving this back to platform/fonts.

Thanks for waiting for this drott, feel free to take this up if you'd like. I think we can now revert PaintTypeface, PaintTextBlob, PaintFont, PaintTypefaceTransferCacheEntry and go back to corresponding skia types.
Cc: dgozman@chromium.org
Owner: drott@chromium.org
Status: Started (was: Assigned)
WIP CL here https://chromium-review.googlesource.com/c/chromium/src/+/1238540

I am not sure how to revert PaintFont, and PaintTextBlob yet - any advice? Also, I am trying to keep the initial revert CL small. If you have suggestions on how to further break it up, please let me know.

Project Member

Comment 15 by bugdroid1@chromium.org, Sep 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/36b37765ba1eb67dc45ead83490fbd1e9c823dfb

commit 36b37765ba1eb67dc45ead83490fbd1e9c823dfb
Author: Dominik Röttsches <drott@chromium.org>
Date: Mon Sep 24 09:18:19 2018

Revert PaintTypeface GPU typeface serialization abstraction

In https://chromium-review.googlesource.com/c/chromium/src/+/756213
PaintTypeface was introduced as one possible serialization approach for
sending Typefaces over to the GPU process. This approach has been
abandonded in favor of a new mechanism for serializing SkTypefaces over
to the GPU process.

Revert the PaintTypeface abstraction and introduce a SkTypeface_Factory
class in Blink to centralize SkTypeface instantiation.

No functional changes.

Bug:  816445 
Tbr: vmpstr@chromium.org
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_layout_tests_layout_ng;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ieb8843fb1888050f4c0d4bef3e99c6d4ffc85380
Reviewed-on: https://chromium-review.googlesource.com/1238540
Commit-Queue: Dominik Röttsches <drott@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593493}
[modify] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/cc/paint/BUILD.gn
[modify] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/cc/paint/oop_pixeltest.cc
[modify] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/cc/paint/paint_font.cc
[modify] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/cc/paint/paint_font.h
[modify] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/cc/paint/paint_op_buffer_unittest.cc
[modify] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/cc/paint/paint_op_perftest.cc
[modify] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/cc/paint/paint_op_reader.cc
[modify] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/cc/paint/paint_op_writer.cc
[modify] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/cc/paint/paint_text_blob.cc
[modify] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/cc/paint/paint_text_blob.h
[modify] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/cc/paint/paint_text_blob_builder.h
[delete] https://crrev.com/a3f28c81b8ceb9f8cc6edc985305c87a123007bc/cc/paint/paint_typeface.cc
[delete] https://crrev.com/a3f28c81b8ceb9f8cc6edc985305c87a123007bc/cc/paint/paint_typeface.h
[delete] https://crrev.com/a3f28c81b8ceb9f8cc6edc985305c87a123007bc/cc/paint/paint_typeface_transfer_cache_entry.cc
[delete] https://crrev.com/a3f28c81b8ceb9f8cc6edc985305c87a123007bc/cc/paint/paint_typeface_transfer_cache_entry.h
[modify] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/cc/paint/transfer_cache_entry.cc
[modify] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/cc/paint/transfer_cache_entry.h
[modify] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/third_party/blink/renderer/core/css/css_font_face_source_test.cc
[modify] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/third_party/blink/renderer/platform/BUILD.gn
[modify] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/third_party/blink/renderer/platform/fonts/font_cache.h
[modify] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/third_party/blink/renderer/platform/fonts/font_custom_platform_data.cc
[modify] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/third_party/blink/renderer/platform/fonts/font_platform_data.cc
[modify] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/third_party/blink/renderer/platform/fonts/font_platform_data.h
[modify] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/third_party/blink/renderer/platform/fonts/linux/font_unique_name_lookup_linux.cc
[modify] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/third_party/blink/renderer/platform/fonts/mac/font_platform_data_mac.mm
[delete] https://crrev.com/a3f28c81b8ceb9f8cc6edc985305c87a123007bc/third_party/blink/renderer/platform/fonts/paint_typeface.h
[modify] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/third_party/blink/renderer/platform/fonts/shaping/shape_result_bloberizer.h
[modify] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/third_party/blink/renderer/platform/fonts/shaping/shape_result_bloberizer_test.cc
[modify] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/third_party/blink/renderer/platform/fonts/skia/font_cache_skia.cc
[add] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/third_party/blink/renderer/platform/fonts/skia/sktypeface_factory.cc
[add] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/third_party/blink/renderer/platform/fonts/skia/sktypeface_factory.h
[modify] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/third_party/blink/renderer/platform/fonts/win/font_cache_skia_win.cc
[modify] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/third_party/blink/renderer/platform/fonts/win/font_platform_data_win.cc
[modify] https://crrev.com/36b37765ba1eb67dc45ead83490fbd1e9c823dfb/ui/gfx/render_text.cc

Owner: khushals...@chromium.org
Status: Assigned (was: Started)
khushal, could you take care of the remaining ones? I did PaintTypeface and PaintTypefaceTransferCacheEntry. PaintTextBlob, PaintFont seem to be left - it'd be great if we can strip those, too.



Project Member

Comment 17 by bugdroid1@chromium.org, Sep 25

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1ae09508a680f4b01af4773ecbc74369518acca8

commit 1ae09508a680f4b01af4773ecbc74369518acca8
Author: Wez <wez@chromium.org>
Date: Tue Sep 25 20:40:54 2018

Disable SkTypeface FromFontConfigInterfaceIdAndTtcIndex on Fuchsia.

Previously this method of creating an SkTypeface was not-implemented
under Fuchsia, but that was changed during the refactoring in
https://chromium-review.googlesource.com/c/chromium/src/+/1238540

We restore OS_FUCHSIA as a platform on which this method is not-
implemented, to avoid the SkFontConfigInterface::RefGlobal() reference
breaking our Debug builds.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:fuchsia-fyi-x64-dbg

Bug:  816445 
Change-Id: Ib14023478e65a8b9c59886a579d60a5be7e00aff
Reviewed-on: https://chromium-review.googlesource.com/1243571
Reviewed-by: Dominik Röttsches <drott@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594086}
[modify] https://crrev.com/1ae09508a680f4b01af4773ecbc74369518acca8/third_party/blink/renderer/platform/fonts/skia/sktypeface_factory.cc

Project Member

Comment 18 by bugdroid1@chromium.org, Oct 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/464808c91afd87d4531e1073d6de03367b383e7c

commit 464808c91afd87d4531e1073d6de03367b383e7c
Author: Khushal <khushalsagar@chromium.org>
Date: Fri Oct 19 23:45:59 2018

cc/paint: Remove paint wrapping of SkTextBlob and friends.

This was initially added to ensure that the typefaces embedded in a blob
were stored in a sidelist on the blob, in order to enable serialization
of typefaces for OOP raster. This is no longer required and the wrapping
is just unnecessary overhead.

R=drott@chromium.org,enne@chromium.org

Bug:  816445 , 894200 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_layout_tests_layout_ng;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I76f106d13ce1aa04a50aadb0d4faa98dafe7a688
Reviewed-on: https://chromium-review.googlesource.com/c/1289571
Commit-Queue: Khushal <khushalsagar@chromium.org>
Reviewed-by: enne <enne@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Dominik Röttsches <drott@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601359}
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/cc/paint/BUILD.gn
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/cc/paint/oop_pixeltest.cc
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/cc/paint/paint_canvas.h
[delete] https://crrev.com/31ba7eade907515bc50df8e564c75372b1b2f969/cc/paint/paint_font.cc
[delete] https://crrev.com/31ba7eade907515bc50df8e564c75372b1b2f969/cc/paint/paint_font.h
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/cc/paint/paint_op_buffer.cc
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/cc/paint/paint_op_buffer.h
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/cc/paint/paint_op_buffer_unittest.cc
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/cc/paint/paint_op_perftest.cc
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/cc/paint/paint_op_reader.cc
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/cc/paint/paint_op_reader.h
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/cc/paint/paint_op_writer.cc
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/cc/paint/paint_op_writer.h
[delete] https://crrev.com/31ba7eade907515bc50df8e564c75372b1b2f969/cc/paint/paint_text_blob.cc
[delete] https://crrev.com/31ba7eade907515bc50df8e564c75372b1b2f969/cc/paint/paint_text_blob.h
[delete] https://crrev.com/31ba7eade907515bc50df8e564c75372b1b2f969/cc/paint/paint_text_blob_builder.cc
[delete] https://crrev.com/31ba7eade907515bc50df8e564c75372b1b2f969/cc/paint/paint_text_blob_builder.h
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/cc/paint/record_paint_canvas.cc
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/cc/paint/record_paint_canvas.h
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/cc/paint/skia_paint_canvas.cc
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/cc/paint/skia_paint_canvas.h
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/cc/test/paint_op_helper.h
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/third_party/blink/public/platform/web_font_render_style.h
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/third_party/blink/renderer/platform/BUILD.gn
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/third_party/blink/renderer/platform/fonts/font.cc
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/third_party/blink/renderer/platform/fonts/font_platform_data.cc
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/third_party/blink/renderer/platform/fonts/font_platform_data.h
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/third_party/blink/renderer/platform/fonts/mac/font_platform_data_mac.mm
[delete] https://crrev.com/31ba7eade907515bc50df8e564c75372b1b2f969/third_party/blink/renderer/platform/fonts/paint_font.h
[delete] https://crrev.com/31ba7eade907515bc50df8e564c75372b1b2f969/third_party/blink/renderer/platform/fonts/paint_text_blob.h
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/third_party/blink/renderer/platform/fonts/shaping/harfbuzz_face.cc
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/third_party/blink/renderer/platform/fonts/shaping/shape_result_bloberizer.cc
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/third_party/blink/renderer/platform/fonts/shaping/shape_result_bloberizer.h
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/third_party/blink/renderer/platform/fonts/simple_font_data.cc
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/third_party/blink/renderer/platform/fonts/web_font_render_style.cc
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/third_party/blink/renderer/platform/fonts/win/font_platform_data_win.cc
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/third_party/blink/renderer/platform/graphics/test/mock_paint_canvas.h
[modify] https://crrev.com/464808c91afd87d4531e1073d6de03367b383e7c/ui/gfx/render_text.cc

Status: Fixed (was: Assigned)

Sign in to add a comment