Skip to content
This repository has been archived by the owner on Jun 8, 2020. It is now read-only.

Ios support #144

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

Ios support #144

wants to merge 8 commits into from

Conversation

NatalyaKovalova
Copy link

@NatalyaKovalova NatalyaKovalova commented Oct 17, 2017

ability to build Skia for iOS platform have been added in this PR.
For building could be used:
cargo build --target aarch64-apple-ios
@larsbergstrom , could you, please, make code review and accept changes?

There are stubs in this PR. I need to discuss how to do it better add AEGL support.
I am temporary using this solution and use servo-egl interface for android, it works but I'm not sure that it is the best way for PR. @larsbergstrom , could you please redirect me to person who worked with egl for android to discuss it?

@larsbergstrom, I'll commit Azure tomorrow - it depends on Skia and will ask you again accept new PR.


This change is Reviewable

@NatalyaKovalova
Copy link
Author

Hello @larsbergstrom , could you, please, write me if I should provide something else to your accepting of my PR?

@larsbergstrom
Copy link
Contributor

Hi, @NatalyaKovalova!

@MortimerGoro Could you please help do a first pass? Otherwise, I'll take a look tomorrow. Sorry - my calendar is horrific in the middle of my week.

@NatalyaKovalova
Copy link
Author

@larsbergstrom
ok. Thanks a lot!

@MortimerGoro
Copy link
Contributor

@larsbergstrom @NatalyaKovalova ok, I'll take a look.

@NatalyaKovalova
Copy link
Author

@larsbergstrom @MortimerGoro
There are changes only to pass a building process. There is no implementation.

For pulling implementation it would be better to define how to add AEGL support better. I'm temporary using this solution and use servo-egl interface for android, it works but I'm not sure that it is the best way for PR.
What would be better to use:

  • Create separate crate for ios' eagl?
  • Make fork and use temporary desition with egl to eagl convertor?( in this case less changes in code because of existing egl implementation )
    ?

It is not an immediate question but important for me.
Thanks!

}

impl Drop for GLRasterizationContext {
fn drop(&mut self) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove empty drop method

CGDataProviderRef SkCreateDataProviderFromData(SkData* data) {
data->ref();
return CGDataProviderCreateWithData(data, data->data(), data->size(),
unref_proc);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: align unref_proc with data or put it in the same line as the other parameters

@MortimerGoro
Copy link
Contributor

MortimerGoro commented Oct 19, 2017

The stub implementation looks good to me.

About AEGL support, I don't like the libegl and servo-egl solution for a final PR. It would be better not to add the iOS libegl transpiler dependency (last commit is from 4 years ago).

AEGL is a pretty straightforward api. I don't think it's necessary to create a separate crate. You could perform the API calls using the rust-objc crate (already included in servo) and maybe another like core-foundation or core-graphics crates depending on the API requirements.

I'm working on adding WebGL 2.0 support (servo/surfman#108) and did some tests with MacOS and iOS GLContext backends. I will share some code once I clean a bit the PR.

As a long-term plan we could share the gl-context implementations used by WebGL and skia. I think that rust-offscreen-rendering-context crate has all that is needed for skia-contexts. But this is out of scope for this PR (it would involve modifications in all the skia backends to do it right)

@NatalyaKovalova
Copy link
Author

@MortimerGoro
Yes, this way is much better. Thanks a lot for your detailed answer.

@MortimerGoro
Copy link
Contributor

@NatalyaKovalova I cleaned some of the code and did the iOS GLContext PR to rust-offscreen-context servo/surfman#109.

You could use native_gl_context.rs as a reference to implement the skia context for iOS.

Any chance that you could include the implementation in this PR or are you blocked by this to pass the building process?

@NatalyaKovalova
Copy link
Author

@MortimerGoro
Great! Thanks!
Yes, I'll change implementation to this solution and update this PR.

@NatalyaKovalova
Copy link
Author

Hello.
Please, don't merge follow commits temporarily.

@bors-servo
Copy link

☔ The latest upstream changes (presumably #148) made this pull request unmergeable. Please resolve the merge conflicts.

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

Successfully merging this pull request may close these issues.

None yet

4 participants