[skia] Fix include path #32660

xiaozhuai · 2023-07-20T08:36:55Z

Changes comply with the maintainer guide
SHA512s are updated for each updated download
The "supports" clause reflects platforms that may be fixed by this new version
Any fixed CI baseline entries are removed from that file.
Any patches that are no longer applied are deleted from the port's directory.
The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
Only one version is added to each modified port's versions file.

Fix #32657 and #28591
It was a mistake in the first place, and now it's time to fix it.

dg0yt · 2023-07-20T17:06:48Z

It was a mistake in the first place, and now it's time to fix it.

Opinion, not fact.

BillyONeal · 2023-07-20T19:42:39Z

I don't have a horse in this race, but I note there appears to be only one downstream consumer in the repo, msdfgen, and they say #include <skia/core/SkPath.h>.

xiaozhuai · 2023-07-21T03:26:06Z

It was a mistake in the first place, and now it's time to fix it.

Opinion, not fact.

There have been debates on this issue before, so I don't want to repeat myself.
I don't care if this PR ends up being merged.
But I'm sure there will be more reports about the same problem.

BillyONeal · 2023-07-21T16:02:18Z

@xiaozhuai can you clarify the problem that this is actually fixing? I did the 'data gathering' above but as I said I don't have strong opinions here; I'm concerned that this is going to break existing users for no obvious benefit. (Both #include <skia/blah.h> and #include <blah.h> work with the status quo)

xiaozhuai · 2023-07-23T06:03:49Z

@BillyONeal

I'm concerned that this is going to break existing users for no obvious benefit.

It won't. As you said, Both #include <skia/blah.h> and #include <blah.h> work after this change.
I don't want to argue about which way is better. But we should offer both options.

Consider the following scenario

#include "skia/core/SkCanvas.h"

This clearly indicates that the header file is from skia

And

#include "core/SkCanvas.h"

This is much worse than the first. Not only is it not clear which library the header file came from.
If user project has a directory named core (There is a very high probability of this). This leads to confusion and pollution of the inclusion path. There are a lot of directories in Skia, config, codec, utils, encode... They are likely to appear in user projects as well.

Considering the IDE's autocompletion, the first method is obviously more friendly.
When I type #include <core/xxx>, the IDE knows I want include headers come from the current project, not skia's.

Finally, take into account not breaking existing users. This PR does not remove the second way.
At the same time, those who prefer the first way are given the right to choose.

BillyONeal · 2023-07-24T00:27:16Z

As you said, Both #include <skia/blah.h> and #include <blah.h> work after this change.

Hmmm that was not my understanding of the change; my understanding was that today they both work, and after the change only <skia/blah.h> works. If they both worked before and both work now what is actually changed here?

Finally, take into account not breaking existing users. This PR does not remove the second way.
At the same time, those who prefer the first way are given the right to choose.

I'm confused, how does the user get to 'choose' as a result of this change?

xiaozhuai · 2023-07-24T02:53:06Z

Hmmm that was not my understanding of the change; my understanding was that today they both work, and after the change only <skia/blah.h> works.

No, currentlly only blah.h works. This pr make them both work. Please review this pr again.

dg0yt · 2023-07-24T05:39:52Z

@BillyONeal The port installs two configs: The recommended config with the unofficial prefix only supports include/skia. The legacy config also supported include, to not break legacy users. This PR move this legacy property to the recomended config.

What I dislike about this change is that it basically makes all headers in include available to consumers which link to skia.
This is not breaking users or ports now but it may hide dependencies, causing unexpected problems later.
The problem is not so much with port skia (which resides in include/skia) but with all those other ports which install headers directly to include. For example, we recently fixed libkml issues around include/zip.h being used unexpectedly instead of include/minizip/zip.h.

Skias's public headers are sufficiently distinct by using an Sk filename component. This is somewhat similar to the Q in Qt's headers. That's why I think we should avoid adding the general include path unless it becomes the official pattern to use skia headers. Users which want to refer to headers with a skia prefix may add this special behavior in their projects by inspecing the properties of the unofficial cmake config.

That's why I say the current implementation of the unofficial config is not by mistake, or a bug. But I don't claim I know the official pattern, or even the most popular one. And even if the official pattern would be #include <skia/core/SkSurface.h>, the best fix might be to install the headers to include/skia/skia instead - if there weren't user which don't use cmake config at all...

xiaozhuai · 2023-07-24T06:01:56Z

What I dislike about this change is that it basically makes all headers in include available to consumers which link to skia.

There are many ports behave like this. If this becomes a problem, then the vcpkg installation directory should be redesigned.

This is not breaking users or ports now but it may hide dependencies, causing unexpected problems later.

What problem?

For example, we recently fixed libkml issues around include/zip.h being used unexpectedly instead of include/minizip/zip.h

Good example. Use skia/blah.h solve this. This why we should keep skia prefix.

if there weren't user which don't use cmake config at all

Currently, user with cmake can only use blah.h and user without cmake can only use skia/blah.h. Totally different behavior.

That's why I think we should avoid adding the general include path unless it becomes the official pattern to use skia headers.

Yes, both of them are unofficial. Let users choose what they want.

dg0yt · 2023-07-24T06:06:24Z

What problem?

I named a real example.
I had another input to move forward to a good decision, but I prefer to leave this discussion at this point.

xiaozhuai · 2023-07-24T06:47:10Z

There is no point in arguing any more, and I will not reply to anything more on this.

Let the team decide.

BillyONeal · 2023-07-25T18:23:34Z

@dg0yt :

The port installs two configs: The recommended config with the unofficial prefix only supports include/skia. The legacy config also supported include, to not break legacy users. This PR move this legacy property to the recomended config.
[...]
That's why I say the current implementation of the unofficial config is not by mistake, or a bug. But I don't claim I know the official pattern, or even the most popular one. And even if the official pattern would be #include <skia/core/SkSurface.h>, the best fix might be to install the headers to include/skia/skia instead - if there weren't user which don't use cmake config at all...

I see, thank you for clarifying.

What I dislike about this change is that it basically makes all headers in include available to consumers which link to skia.

I think this is going to be the case regardless. Most ports expect 'one include directory to rule them all', and this is in fact the way a lot of downstream customers can get to dependencies at all. So I think this ship has sailed.

The problem is not so much with port skia (which resides in include/skia) but with all those other ports which install headers directly to include. For example, we recently fixed libkml issues around include/zip.h being used unexpectedly instead of include/minizip/zip.h.

But if they said #include <minizip/zip.h>, wouldn't that be a better resolution of this problem? With the proposed behavior libkml is still toast if more than one thing tries to provide zip.h. On the contrary, the behavior here can hide the problem while vcpkg's normal 'only one owner' file checks would catch this.

@xiaozhuai :

There are many ports behave like this. If this becomes a problem, then the vcpkg installation directory should be redesigned.

This is not a vcpkg problem, this is a C++ problem. No 2 entities can own the same #include <stuff> name; the C++ compiler is only going to pick one of them.

dg0yt · 2023-07-26T06:38:34Z

Most ports expect 'one include directory to rule them all', and this is in fact the way a lot of downstream customers can get to dependencies at all. So I think this ship has sailed.

At least this ship hasn't sailed yet for the unofficial-skia config package.
And in this particular context, it should be noted that skia/gn/bazel try hard to do the opposite of 'one include directory to rule them all', to avoid problems vcpkg has to fight with.

But if they said #include <minizip/zip.h>, wouldn't that be a better resolution of this problem?

This is the ship which has sailed: Official usage is #include <zip.h>. The generla pattern of this problem is the lack of disambiguation by unique components. This would be mitigated by avoiding the 'one include directory to rule them all' approach.
But skia headers have a unique component: the Sk name prefix.

No 2 entities can own the same #include <stuff> name; the C++ compiler is only going to pick one of them.

I would say that a single translation unit cannot use two different instances of <stuff>. But it is not to hard to use different <stuff> for different CMake targets, in a controlled way.

Last not least: The skia path component is a proprietary vcpkg port property.

Upstream doesn't support installation at all. gn or bazel projects use subdirectories for dependencies (as does skia, making it more difficult to devendor dependencies).
Upstream keeps public headers in include.
(Unfortunately, some headers are needed also from modules.)

So expected usage patters are more like

#include "include/codec/SkCodec.h"

as in convert-to-nia.cpp which is used for the test port.
And that's why the port's primary installation dir is include/skia/include. This content is duplicated into include/skia to support the legacy config as long as we want to maintain support for it. Once we drop the legacy config, I would drop the duplication. (Consumers which didn't use the include/ path component can be supported by adding include/skia/include as another item in the interface include directories.)

FTR Chrome uses

#include "third_party/skia/include/core/SkBitmap.h"

where skia is the source code directory (parent dir of include), not a subdir of include. So if users want to work with a skia path component (after this PR!), they should actually spell it out like

#include <skia/include/codec/SkBitmap.h>

To sum it up, I don't mean to block a change. But I would like to make it clear that

the current config is not a bug or mistake but was chosen for good reasons,
the existence of a skia path component is more or less specific to vcpkg.
the proposed usage of #include <skia/core/SkBitmap.h> will make it impossible to eventually drop the duplicated installation.

xiaozhuai · 2023-07-26T07:10:50Z

the proposed usage of #include <skia/core/SkBitmap.h> will make it impossible to eventually drop the duplicated installation.

@BillyONeal @dg0yt
It was fixed.
5370a20

And usage tests passed on x64-osx that use the following demo code.
https://github.com/xiaozhuai/vcpkg_demo/tree/master/src/skia

And a downstream port msdfgen also passed.

BillyONeal · 2023-07-27T21:12:28Z

@AugP @markle11m @dan-shaw @JavierMatosD @ras0219-msft and I discussed this today.

The vcpkg team considers 'hiding' headers with additional -Is, as seemed to be @dg0yt's design
intent here, a non-goal.
@ras0219-msft did a bunch of github searches which seem to indicate the most common use is
1. forks of skia itself
2. #include "include/core/skcanvas.h"
  which suggests that, as @dg0yt says, the subdirectory <skia is a vcpkg fabrication and that we
  should actually be installing skia's headers to ${CURRENT_PACKAGES_DIR}/include/include (as in,
  make "${CURRENT_PACKAGES_DIR}/include/include/core/SkCanvas.h")
Yes, this means we should fix msdfgen to say #include <include/core/SkPath.h> rather than
#include <skia/core/SkPath.h>
It would be nice if we have some way to contact upstream and see what they think. Unfortunately it seems they only care about Bazel :(

xiaozhuai · 2023-07-28T03:19:34Z

@BillyONeal

Yes, this means we should fix msdfgen to say #include <include/core/SkPath.h> rather than
#include <skia/core/SkPath.h>

Since this PR can't be merged, let's leave the status quo, at least without breaking downstream users.
Before there is an official statement, #include <include/core/SkPath.h> is also a speculation.
IMO, change to #include <include/core/SkPath.h> won't help much. Instead, it can lead to misunderstandings.

So be it, I'm going to close this PR.

This change introduces Skia painter available under a flag. It's not yet match capabilities of Gfx::Painter and is not ready to replace it yet. Motivation: - The current CPU painter is a performance bottleneck on most websites. Our own GPU painter implementation is very immature and has received relatively little attention. - There is ongoing effort on building painter that supports affine transforms (AffineCommandExecutorCPU) but it is far from being on par with the default CPU painter. Skia will allow us to immediately get full transformation support for all painting commands. GPU painting: I experimented with Ganesh GPU-backend, however profiling revealed that without sharing viewport texture between WebContent and Browser processes, it won't bring much performance improvement compared to CPU-backend. Therefore, I decided to keep this PR focused on implementing most of painting commands and switch to GPU-backend in future changes. Text rendring: Skia painter uses glyph bitmaps produced by LibGfx. Using Skia for text rendering will require large refactoring of the font rendering subsystem. Currently, it's impossible to construct SkFont right before rendering because Gfx::VectorFont can't be serialized back into sequence of bytes. Couple annoying things worth to mention: - vcpkg doesn't provide latest version of Skia and there's a very visible performance difference on ARM Mac CPU-backend between latest build from master and vcpkg. - Ugly include paths like `#include <core/SkBitmap.h>`. I would prefer to have skia prefix in the path. There was an attempt to fix that but PR was rejected microsoft/vcpkg#32660 Regressions compared to Gfx::Painter: - DrawText is not implemented - PaintTextShadow is not implemented - PaintRadialGradient and PaintLinearGradient do not support "transition hints" and repeat length - PaintConicGradient is not implemented - DrawTriangleWave is not implemented - DrawLine does not account for line style property - DrawScaledBitmap and DrawScaledImmutableBitmap do not account for scaling mode property

This change introduces Skia painter available under a flag. It's not yet match capabilities of Gfx::Painter and is not ready to replace it. Motivation: - The current CPU painter is a performance bottleneck on most websites. Our own GPU painter implementation is very immature and has received relatively little attention. - There is ongoing effort on building painter that supports affine transforms (AffineCommandExecutorCPU) but it is far from being on par with the default CPU painter. Skia will allow us to immediately get full transformation support for all painting commands. GPU painting: I experimented with Ganesh GPU-backend, however profiling revealed that without sharing viewport texture between WebContent and Browser processes, it won't bring much performance improvement compared to CPU-backend. Therefore, I decided to keep this PR focused on implementing most of painting commands and switch to GPU-backend in future changes. Text rendring: Skia painter uses glyph bitmaps produced by LibGfx. Using Skia for text rendering will require large refactoring of the font rendering subsystem. Currently, it's impossible to construct SkFont right before rendering because Gfx::VectorFont can't be serialized back into sequence of bytes. Couple annoying things worth to mention: - vcpkg doesn't provide latest version of Skia and there's a very visible performance difference on ARM Mac CPU-backend between latest build from master and vcpkg. - Ugly include paths like `#include <core/SkBitmap.h>`. I would prefer to have skia prefix in the path. There was an attempt to fix that but PR was rejected microsoft/vcpkg#32660 Regressions compared to Gfx::Painter: - DrawText is not implemented - PaintTextShadow is not implemented - PaintRadialGradient and PaintLinearGradient do not support "transition hints" and repeat length - PaintConicGradient is not implemented - DrawTriangleWave is not implemented - DrawLine does not account for line style property - DrawScaledBitmap and DrawScaledImmutableBitmap do not account for scaling mode property

This change introduces Skia painter available under a flag. It's not yet match capabilities of Gfx::Painter and is not ready to replace it. Motivation: - The current CPU painter is a performance bottleneck on most websites. Our own GPU painter implementation is very immature and has received relatively little attention. - There is ongoing effort on building painter that supports affine transforms (AffineCommandExecutorCPU) but it is far from being on par with the default CPU painter. Skia will allow us to immediately get full transformation support for all painting commands. GPU painting: I experimented with Ganesh GPU-backend, however profiling revealed that without sharing viewport texture between WebContent and Browser processes, it won't bring much performance improvement compared to CPU-backend. Therefore, I decided to keep this PR focused on implementing most of painting commands and switch to GPU-backend in future changes. Text rendring: Skia painter uses glyph bitmaps produced by LibGfx. Using Skia for text rendering will require large refactoring of the font rendering subsystem. Currently, it's impossible to construct SkFont right before rendering because Gfx::VectorFont can't be serialized back into sequence of bytes. There is a problem with ugly include paths like: `#include <core/SkBitmap.h>`. I would prefer to have skia prefix in the path. There was an attempt to fix that but PR was rejected microsoft/vcpkg#32660 Regressions compared to Gfx::Painter: - DrawText is not implemented - PaintTextShadow is not implemented - PaintRadialGradient and PaintLinearGradient do not support "transition hints" and repeat length - PaintConicGradient is not implemented - DrawTriangleWave is not implemented - DrawLine does not account for line style property - DrawScaledBitmap and DrawScaledImmutableBitmap do not account for scaling mode property

xiaozhuai marked this pull request as draft July 20, 2023 08:50

jimwang118 assigned Adela0814 Jul 20, 2023

xiaozhuai force-pushed the dev-skia branch from a44ce79 to 9fc8305 Compare July 20, 2023 09:21

Adela0814 added the category:port-bug The issue is with a library, which is something the port should already support label Jul 20, 2023

xiaozhuai marked this pull request as ready for review July 21, 2023 03:07

Adela0814 added category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist and removed category:port-bug The issue is with a library, which is something the port should already support labels Jul 21, 2023

Adela0814 previously approved these changes Jul 21, 2023

View reviewed changes

Adela0814 added the info:reviewed Pull Request changes follow basic guidelines label Jul 21, 2023

BillyONeal removed the info:reviewed Pull Request changes follow basic guidelines label Jul 21, 2023

BillyONeal marked this pull request as draft July 21, 2023 16:02

xiaozhuai marked this pull request as ready for review July 25, 2023 07:25

BillyONeal added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jul 25, 2023

xiaozhuai dismissed Adela0814’s stale review via 8995122 July 26, 2023 07:06

xiaozhuai added 2 commits July 27, 2023 11:03

[skia] Fix include path

7abb484

drop the duplicated headers installation

5370a20

update version database

d768b26

xiaozhuai force-pushed the dev-skia branch from 8995122 to d768b26 Compare July 27, 2023 03:03

xiaozhuai closed this Jul 28, 2023

xiaozhuai deleted the dev-skia branch August 2, 2023 13:22

kalenikaliaksandr mentioned this pull request Jun 16, 2024

LibWeb: Add Skia painting command executor LadybirdBrowser/ladybird#182

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[skia] Fix include path #32660

[skia] Fix include path #32660

xiaozhuai commented Jul 20, 2023 •

edited

Loading

dg0yt commented Jul 20, 2023

BillyONeal commented Jul 20, 2023

xiaozhuai commented Jul 21, 2023

BillyONeal commented Jul 21, 2023

xiaozhuai commented Jul 23, 2023 •

edited

Loading

BillyONeal commented Jul 24, 2023

xiaozhuai commented Jul 24, 2023

dg0yt commented Jul 24, 2023 •

edited

Loading

xiaozhuai commented Jul 24, 2023 •

edited

Loading

dg0yt commented Jul 24, 2023

xiaozhuai commented Jul 24, 2023

BillyONeal commented Jul 25, 2023

dg0yt commented Jul 26, 2023

xiaozhuai commented Jul 26, 2023 •

edited

Loading

BillyONeal commented Jul 27, 2023

xiaozhuai commented Jul 28, 2023

[skia] Fix include path #32660

[skia] Fix include path #32660

Conversation

xiaozhuai commented Jul 20, 2023 • edited Loading

dg0yt commented Jul 20, 2023

BillyONeal commented Jul 20, 2023

xiaozhuai commented Jul 21, 2023

BillyONeal commented Jul 21, 2023

xiaozhuai commented Jul 23, 2023 • edited Loading

BillyONeal commented Jul 24, 2023

xiaozhuai commented Jul 24, 2023

dg0yt commented Jul 24, 2023 • edited Loading

xiaozhuai commented Jul 24, 2023 • edited Loading

dg0yt commented Jul 24, 2023

xiaozhuai commented Jul 24, 2023

BillyONeal commented Jul 25, 2023

dg0yt commented Jul 26, 2023

xiaozhuai commented Jul 26, 2023 • edited Loading

BillyONeal commented Jul 27, 2023

xiaozhuai commented Jul 28, 2023

xiaozhuai commented Jul 20, 2023 •

edited

Loading

xiaozhuai commented Jul 23, 2023 •

edited

Loading

dg0yt commented Jul 24, 2023 •

edited

Loading

xiaozhuai commented Jul 24, 2023 •

edited

Loading

xiaozhuai commented Jul 26, 2023 •

edited

Loading