Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

feat: Support custom ImageAsset#1079

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
dmtrKovalenko wants to merge1 commit intorust-skia:master
base:master
Choose a base branch
Loading
fromdmtrKovalenko:feat/custom-image-asset

Conversation

@dmtrKovalenko
Copy link
Contributor

@dmtrKovalenkodmtrKovalenko commentedDec 20, 2024•
edited
Loading

Here is an implementation that allows to return your custom implementation of ImageAsset wrapping the rust trait to the skia world using ResourceProvider. ImageAssets are overridable so I added an ability to override it using rust trait object just like you already do for ResourceProvider.

My use case: I have already available decoded images in the memory so I just manually creating the frame_data from the pointer. This is a code I use:

structFFramesSkiaImage(Image);implFFramesSkiaImage{fnnew(image:&ImageData) ->Option<Self>{let image_info =ImageInfo::new(ISize::new(image.metadata.widthasi32, image.metadata.heightasi32),ColorType::RGBA8888,AlphaType::Premul,None,);let sk_image =raster_from_data(&image_info,unsafe{Data::new_bytes(&image.get_bytes())},            image.metadata.widthasusize*4,)?;Some(Self(sk_image))}}implCustomImageAssetforFFramesSkiaImage{fnis_multi_frame(&self) ->bool{false}fnget_frame_data(&self,t:f32) ->ImageFrameData{create_image_frame_data(&self.0,Matrix::default(),SamplingOptions::default(),ImageSizeFit::kFill,)}}implResourceProviderforSkiaFFramesProvider{fnload_image_asset(&self,_resource_path:&str,resource_name:&str,_resource_id:&str,) ->Option<ImageAsset>{ifself.0.is_null(){// TODO log the errorreturnNone;}let image =self.0.media_source?.resolve_image(resource_name)?;let skia_image =FFramesSkiaImage::new(image)?;ImageAsset::from_custom_image_asset(skia_image)}fnload(&self,_resource_path:&str,_resource_name:&str) ->Option<skia_safe::Data>{None}fnload_typeface(&self,_name:&str,_url:&str) ->Option<skia_safe::Typeface>{None}fnfont_mgr(&self) ->FontMgr{FontMgr::empty()}}

Please let me know if you like the implementation and if you do I'll add the tests covering this API.

pub type ImageSizeFit = skresources_ImageAsset_SizeFit;
pub type ImageFrameData = skresources_ImageAsset_FrameData;

pub fn create_image_frame_data(
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I thought it would be possible to create the FrameData struct as a rust struct but it uses skia shared pointer and it looks like there is no way to get the shared pointer from theRCHandle without getting into the skia world. Correct me if I wrong

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we could provide a generic function for that, but this would in turn mean that we directly handle sp typed smartpointers in Rust, which we don't. There might be a way to create a compatible struct in Rust and placing a RCHandle at the same field position, but this feels scary. Speaking of that, FrameData should be wrapped and made opaque to the Rust world (with functions wrapping access to the fields), otherwise clients could mess around with the smartpointer.

Copy link
Member

@pragmatrixpragmatrix left a comment

Choose a reason for hiding this comment

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

Thank you a lot for that. I like the addition.

Here are the things I like to be changed:

  • Put the image_asset.rs into a new subdirectory named modules/resources and refer it from resources.rs (because it is not a module).
  • Theskresources_ImageAsset_SizeFit enum variants are prefixed with ak. Add avariant_name! check, and addSizeFit toskia_bindgen.rsENUM_REWRITES, this should fix it.
  • Wrap theFrameData in aHandle<>, and offer access to the members through functions (the simple members don't need C wrappers). This is to prevent clients from messing with the sp and also fixes a bug with the current implementation, where the reference count is not reduced when dropped.
  • Add a test case based on a reasonable use case if possible.

pub type ImageSizeFit = skresources_ImageAsset_SizeFit;
pub type ImageFrameData = skresources_ImageAsset_FrameData;

pub fn create_image_frame_data(
Copy link
Member

Choose a reason for hiding this comment

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

Yes, we could provide a generic function for that, but this would in turn mean that we directly handle sp typed smartpointers in Rust, which we don't. There might be a way to create a compatible struct in Rust and placing a RCHandle at the same field position, but this feels scary. Speaking of that, FrameData should be wrapped and made opaque to the Rust world (with functions wrapping access to the fields), otherwise clients could mess around with the smartpointer.

@dmtrKovalenko
Copy link
ContributorAuthor

Thank you for review I have a question though about this

Wrap the FrameData in a Handle<>, and offer access to the members through functions (the simple members don't need C wrappers). This is to prevent clients from messing with the sp and also fixes a bug with the cur

the problem is that it should not be a handle it is stack allocated struct that is used by skia so it needs to be the owned struct when we return it to the skia world. That's why there is no need to drop it or even have access to the fields. If Correct me if I'm wrong

@dmtrKovalenko
Copy link
ContributorAuthor

hey@pragmatrix gentle pin on my previous question when you'll had a chance

@pragmatrix
Copy link
Member

the problem is that it should not be a handle it is stack allocated struct that is used by skia so it needs to be the owned struct when we return it to the skia world. That's why there is no need to drop it or even have access to the fields. If Correct me if I'm wrong

TheHandle<T> does not make assumptions about where the value is allocated (compared toRefHandle orRCHandle which are heap only), and it's#[repr(transparent)], meaning that it exists only as a conceptual abstraction and has no effect on the bits moving around. It's just a way to wrap a C / C++ value so that its contents are hidden from Rust and supports a drop handler that should call back into Skia world and invoke the destructor of that C++ type.

If it is only passed from Rust to C/C++ (Skia world) it works, but after callingcreate_image_frame_data you are free to drop or modify it, and usingget_frame_data you can ask for it from the Rust part before converting it to a NativeImageAsset. So as long ImageFrameData gets exported as a public type, this loophole continues to exist and there is no place in skia-safe as far as I know where ask_sp<> is directly exposed.

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

Reviewers

@pragmatrixpragmatrixpragmatrix requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dmtrKovalenko@pragmatrix
[8]ページ先頭
©2009-2025 Movatter.jp