Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

update to skia m126 #969

Merged
merged 15 commits into from
Aug 12, 2024
Merged

update to skia m126 #969

merged 15 commits into from
Aug 12, 2024

Conversation

eymar
Copy link
Member

@eymar eymar commented Jul 26, 2024 •
edited
Loading

@eymar eymar changed the title update to m122 WIP update to m126 WIP Aug 2, 2024
@eymar eymar force-pushed the ok/skiko_update_july24 branch from c66d97d to 9cbf641 Compare August 5, 2024 09:31
@eymar eymar mentioned this pull request Aug 5, 2024
Merged
@eymar
Copy link
Member Author

eymar commented Aug 5, 2024 •
edited
Loading

The tests passed.
Skiko 0.8.10.1 was built from this branch.
The version was update in Compose here: JetBrains/compose-multiplatform-core#1486 (waiting for CI to run the tests)

@eymar eymar changed the title update to m126 WIP update to skia m126 Aug 5, 2024
@eymar eymar marked this pull request as ready for review August 5, 2024 10:53
MatkovIvan
MatkovIvan reviewed Aug 5, 2024
View reviewed changes
skiko/src/awtMain/objectiveC/macos/MetalApi.mm Outdated
#include "ganesh/mtl/GrMtlBackendContext.h"
#include "ganesh/mtl/GrMtlDirectContext.h"
#include "ganesh/mtl/GrMtlBackendSurface.h"
#include "ganesh/mtl/GrMtlTypes.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO for later: try to switch from Ganesh to Graphite

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it up to us?
I mean we should use what skia provides, no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skia provides a few render backends. Yes, it's up to us what to use

skiko/src/awtMain/objectiveC/macos/MetalRedrawer.mm Outdated
@@ -10,8 +10,9 @@

#import <GrBackendSurface.h>
#import <GrDirectContext.h>
#import <mtl/GrMtlBackendContext.h>
#import <mtl/GrMtlTypes.h>
#include "ganesh/mtl/GrMtlBackendContext.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please align header importing style

<> - for system and library headers
"" - for local files, sometimes does recursive search

#include - C++ one
#import - Objective-C one

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

skiko/src/jvmMain/cpp/common/BackendRenderTarget.cc Outdated
@@ -25,7 +31,8 @@ extern "C" JNIEXPORT jlong JNICALL Java_org_jetbrains_skia_BackendRenderTargetKt
GrMTLHandle texture = reinterpret_cast<GrMTLHandle>(static_cast<uintptr_t>(texturePtr));
GrMtlTextureInfo fbInfo;
fbInfo.fTexture.retain(texture);
GrBackendRenderTarget* instance = new GrBackendRenderTarget(width, height, fbInfo);
GrBackendRenderTarget obj = GrBackendRenderTargets::MakeMtl(width, height, fbInfo);
auto instance = new GrBackendRenderTarget(obj);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent auto usage. I'm ok to use it, but it's better to align the approach because now it uses both ways in similar situations

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed. Using explicit types in this PR now.

skiko/src/nativeJsMain/cpp/shaper/Shaper.cc Outdated
}

SKIKO_EXPORT KNativePointer org_jetbrains_skia_shaper_Shaper__1nMakeCoreText() {
#ifdef SK_SHAPER_CORETEXT_AVAILABLE
return reinterpret_cast<KNativePointer>(SkShaper::MakeCoreText().release());
return nullptr;
// TODO: build skia with `skia_use_fonthost_mac=true` to have SkShaper::MakeCoreText
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it a regression?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

skia_use_fonthost_mac used to be not needed. So the compilation error was noticed only after skia was built (>3h).
The method was not used anywhere, so I added a TODO in case someone has an issue with it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm, I'll rebuild skia. We postponed alpha03 and we have time now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@eymar eymar force-pushed the ok/skiko_update_july24 branch from 8a6f7bd to 9102e5b Compare August 9, 2024 09:09
@eymar
Copy link
Member Author

eymar commented Aug 12, 2024

Published skiko 0.8.11 with the above suggestions/improvements/fixes.

@eymar eymar merged commit d04654c into master Aug 12, 2024
6 checks passed
@eymar eymar deleted the ok/skiko_update_july24 branch August 12, 2024 08:09
@MatkovIvan MatkovIvan mentioned this pull request Aug 12, 2024
Closed
eymar added a commit to JetBrains/compose-multiplatform-core that referenced this pull request Aug 12, 2024
@eymar
Update skiko to 0.8.11 (#1486)
0d891b8 
It's based on skia m126
Skiko update PR: JetBrains/skiko#969

## Testing
<!-- Optional -->
- The demo runs on all platforms
- The tests passed

<!-- Optional -->
This should be tested by QA

## Release Notes
### Highlights - Multiple Platforms
- Skiko version was updated to 0.8.11 (It is based on skia m126 release)
mazunin-v-jb pushed a commit to JetBrains/compose-multiplatform-core that referenced this pull request Aug 14, 2024
@eymar @mazunin-v-jb
Update skiko to 0.8.11 (#1486)
4ec5441 
It's based on skia m126
Skiko update PR: JetBrains/skiko#969

## Testing
<!-- Optional -->
- The demo runs on all platforms
- The tests passed

<!-- Optional -->
This should be tested by QA

## Release Notes
### Highlights - Multiple Platforms
- Skiko version was updated to 0.8.11 (It is based on skia m126 release)
SergeevPavel
SergeevPavel reviewed Sep 24, 2024
View reviewed changes
skiko/src/jvmMain/cpp/common/shaper/Shaper.cc
@@ -84,7 +93,7 @@ extern "C" JNIEXPORT jlong JNICALL Java_org_jetbrains_skia_shaper_ShaperKt__1nSh
text.c_str(),
text.size(),
*font,
SkFontMgr::RefDefault(),
SkFontMgrSkikoDefault(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've noticed that this call might be quite costly. And it's performed on every TextLine.make which makes every call last for ~15ms even for short strings.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is profile
image

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eymar wdyt should we cache the fontMgr here? Other option may be just getting rid of this Shaper API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igordmn igordmn igordmn left review comments

@SergeevPavel SergeevPavel SergeevPavel left review comments

@MatkovIvan MatkovIvan MatkovIvan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@eymar @MatkovIvan @SergeevPavel @igordmn