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

[Tizen] Support EmbedderExternalTextureGL for impeller #368

Merged
merged 5 commits into from
Jul 8, 2024

Conversation

xiaowei-guan
Copy link

@xiaowei-guan xiaowei-guan commented Jun 18, 2024

flutter-tizen/embedder#15
To reduce modifying the flutter engine source code, try to just add code.
Actually, we can change file "embedder_external_texture_gl" to "embedder_external_texture_gl_skia"

@xiaowei-guan xiaowei-guan marked this pull request as draft June 18, 2024 10:24
@xiaowei-guan xiaowei-guan marked this pull request as ready for review June 19, 2024 08:57
Copy link
Member

@JSUYA JSUYA left a comment

Choose a reason for hiding this comment

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

I thought about declaring emebedder_tizen.h and proceeded with POC, but it actually made the code more complicated. I think this PR is probably the best(?).
I was thinking in minimizing code diffs. My review may not be about code readability or efficiency.

My hope is that this commit will be cherry-picked without conflict when migrating to the next version. :)

@xiaowei-guan
Copy link
Author

@swift-kim Do you have any suggestion?

@swift-kim
Copy link
Member

@xiaowei-guan Nope. The current structure looks okay to me. IMO readability is more important than minimizing the code diff.

@JSUYA
Copy link
Member

JSUYA commented Jun 24, 2024

@xiaowei-guan Nope. The current structure looks okay to me. IMO readability is more important than minimizing the code diff.

Hi @swift-kim
How about adding embedder_tizen.h?
We can only add a structure dedicated to the tizen platform to the existing code and use it in user_data.
I expect it to have a significant less impact on readability. #368 (comment)
If you think that way will affect readability, we can apply this PR as is.
This PR worked well when I tested it.

@swift-kim
Copy link
Member

I expect it to have a significant less impact on readability.

I don't quite agree on this but either way should be okay. I'm just afraid that the texture-related code is already complicated and the FlutterOpenGLTextureTizen extension will add even more complexity.

Copy link
Member

@JSUYA JSUYA left a comment

Choose a reason for hiding this comment

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

@xiaowei-guan I think, it is better to reconsider adding embedder_tizen.h by looking at conflicts that arise due to code diff when upgrading the engine and embedder later.

@xiaowei-guan
Copy link
Author

@xiaowei-guan I think, it is better to reconsider adding embedder_tizen.h by looking at conflicts that arise due to code diff when upgrading the engine and embedder later.

Yes, we need consider this problem. I think we can also consider trying to submit the code to the official repository.

@JSUYA
Copy link
Member

JSUYA commented Jun 27, 2024

I think we can also consider trying to submit the code to the official repository.

Do you have any plan?

@xiaowei-guan
Copy link
Author

I think we can also consider trying to submit the code to the official repository.

Do you have any plan?

This is my plan:

  1. We can investigate Windows and Linux embedder code.
  2. Design common interface for Windows, Linux and Tizen.
  3. Create a new PR.
    But If we only submit the interface without any implementation for Windows or Linux, I don't konw whether the flutter official will agree.

Copy link
Member

@JSUYA JSUYA left a comment

Choose a reason for hiding this comment

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

It doesn't look easy. I think contributing to the official repository is a very good. I don't think you really need embedder implementations for windows and linux if you just add EmbedderExternalTextureGL. I think it's okay if there is an appropriate test case.(embedder_gl_unittests.cc)
How about you suggest EmbedderExternalTextureGL first?

shell/platform/embedder/BUILD.gn Outdated Show resolved Hide resolved
@xiaowei-guan
Copy link
Author

. I think contributing to the official repository is a very good. I don't think you really need embedder

Ok, we can try just add EmbedderExternalTextureGL for impeller. I will add test case later.

@swift-kim
Copy link
Member

@JSUYA Contributing to the upstream is not very easy as they don't usually accept pull requests that are not used by any of the 1st-party platforms. I'm not sure if this policy has changed.

@JSUYA JSUYA merged commit 0d9ed7b into flutter-tizen:flutter-3.22.2 Jul 8, 2024
17 checks passed
JSUYA pushed a commit to JSUYA/engine that referenced this pull request Jul 22, 2024
JSUYA pushed a commit to JSUYA/engine that referenced this pull request Aug 23, 2024
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