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.

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

Implement SkImages::AdoptTextureFrom bindings #1019

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Luna0x01
Copy link

@Luna0x01 Luna0x01 commented Feb 3, 2025

I've tested it and it works completely fine under OpenGL.

Copy link
Member

@MatkovIvan MatkovIvan left a comment

Choose a reason for hiding this comment

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

Aside from JNI, it should contain K/N bindings to work outside JVM too

@Luna0x01
Copy link
Author

Luna0x01 commented Feb 4, 2025

I've implemented the requested changes.

Copy link
Member

@MatkovIvan MatkovIvan left a comment

Choose a reason for hiding this comment

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

Thanks for implementing the missing part.

Currently I'm not sure that creating a new GrBackendTexture should be a part of Image API, but I'm OK to leave this part as TODO for later.

PS I'll be on vacation next week so cc @igordmn for taking care of it

GrBackendTexture backendTexture = GrBackendTextures::MakeGL(
width,
height,
skgpu::Mipmapped::kYes,
Copy link
Member

Choose a reason for hiding this comment

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

Such a thing shouldn't be hardcoded

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's provide it as a parameter (boolean suffice if it has 2 options)

textureInfo.fTarget = static_cast<GrGLenum>(target);
textureInfo.fFormat = static_cast<GrGLenum>(format);

GrBackendTexture backendTexture = GrBackendTextures::MakeGL(
Copy link
Member

Choose a reason for hiding this comment

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

It seems that we need to expose this separately and pass it as an argument

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is a GL object that is used in a non-GL method. Because of that we have to move it out of this method and create a real BackendTexture kotlin class with BackendTexture.MakeGL function.

@MatkovIvan MatkovIvan dismissed their stale review February 4, 2025 11:41

Addressed

@MatkovIvan MatkovIvan requested a review from igordmn February 4, 2025 11:41
@Luna0x01
Copy link
Author

Luna0x01 commented Feb 9, 2025

@igordmn Any idea when this will be reviewed?

GrBackendTexture backendTexture = GrBackendTextures::MakeGL(
width,
height,
skgpu::Mipmapped::kYes,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's provide it as a parameter (boolean suffice if it has 2 options)

textureInfo.fTarget = static_cast<GrGLenum>(target);
textureInfo.fFormat = static_cast<GrGLenum>(format);

GrBackendTexture backendTexture = GrBackendTextures::MakeGL(
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is a GL object that is used in a non-GL method. Because of that we have to move it out of this method and create a real BackendTexture kotlin class with BackendTexture.MakeGL function.

@@ -138,6 +138,36 @@ class Image internal constructor(ptr: NativePointer) : RefCnt(ptr), IHasImageInf
return Image(ptr)
}

/** Creates GPU-backed [org.jetbrains.skia.Image] from backendTexture associated with context.
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Please use KDoc format of the comment:
/**
 *
 *
 */
  • describe the parameters as in the original documentation in Skia

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants