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

Enable Evas GL renderer for all profile #312

Merged

Conversation

JSUYA
Copy link
Member

@JSUYA JSUYA commented Jul 12, 2022

For devices other than wearable, we can either use ElmFlutterView or EcoreWl2Window,
depending on apps. This commit is to avoid being device dependent.

Remove the build flag for evas_gl only and use the WEARABLE_PROFILE macro if necessary.
Before creating the engine, the embedding can select the renderer type in the engine property.
On runtime, it selects an appropriate renderer and external texture
according to the renderer type and renders it.

related issue : #301

@JSUYA JSUYA marked this pull request as draft July 12, 2022 02:48
@JSUYA JSUYA force-pushed the dev/remove_evas_gl_only branch 2 times, most recently from 3e321dd to f9a01c5 Compare July 12, 2022 04:46
shell/platform/tizen/BUILD.gn Outdated Show resolved Hide resolved
shell/platform/tizen/BUILD.gn Show resolved Hide resolved
JSUYA added a commit to JSUYA/flutter-tizen that referenced this pull request Jul 12, 2022
Set the renderer type before creating the engine.
The window is selected as EcoreWl2 or ElmWin
according to the renderer type.

This commit requires engine side commits.
(flutter-tizen/engine#312)
JSUYA added a commit to JSUYA/flutter-tizen that referenced this pull request Jul 12, 2022
Set the renderer type before creating the engine.
The window is selected as EcoreWl2 or ElmWin
according to the renderer type.

This commit requires engine side commits.
(flutter-tizen/engine#312)
@JSUYA JSUYA marked this pull request as ready for review July 12, 2022 05:14
shell/platform/tizen/public/flutter_tizen.h Outdated Show resolved Hide resolved
shell/platform/tizen/public/flutter_tizen.h Outdated Show resolved Hide resolved
@@ -74,6 +81,8 @@ typedef struct {
// Array of Dart entrypoint arguments. This is deep copied during the call
// to FlutterDesktopRunEngine.
const char** dart_entrypoint_argv;
// The renderer type of the engine.
FlutterDesktopEngineRendererType renderer_type;
Copy link
Member

Choose a reason for hiding this comment

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

Please consider moving this property to FlutterDesktopView(Window)Properties since it's not relevant for headless engine. You will need to refactor the code a little bit, for example,

  1. Move the renderer initialization code from FlutterTizenEngine's constructor to a separate method and call it from FlutterTizenView's SetEngine or CreateRenderSurface, or
  2. Let FlutterTizenView own the renderer instance.

Note: event_loop_ and render_loop_ can be initialized during RunEngine.

Does this make sense? Do you have any thought? @bbrto21

Copy link
Member Author

Choose a reason for hiding this comment

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

How about adding a None type in Type for headless?

I can move the renderer to FlutterTizenView as you said.
However, when FlutterEngine used in flutter-tizen, I think that if we create a renderer in the engine, it can be helpful for improvement of the launch time(?).

Copy link

@bbrto21 bbrto21 Jul 12, 2022

Choose a reason for hiding this comment

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

@swift-kim I think it make sense. all task runners are needed at the time of the run engine, so you can initialize them lazily.

The renderer depends on what the view is. So I think it's fair to associate renderer with view.
If the engine has a specific type of renderer, the available views are limited by that type.
So it seems more flexible and simpler for user to deal with the type of view in the API, not the renderer type.
Well, one more thing, I think adding a renderer type to the member of a flutter project is also a little inappropriate.

Copy link
Member Author

Choose a reason for hiding this comment

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

You guys are right. This way is a problem because the render type must be set when creating a FlutterEngine for ElmFlutterView.
I will move this to the view. I think there will be more code changes...

Copy link
Member

Choose a reason for hiding this comment

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

Is the renderer_type property only applicable to FlutterDesktopWindowProperties? How about FlutterDesktopViewProperties?

Regarding the launch time optimization, maybe we could remove RunEngine from the FlutterDesktopViewCreate family and provide another API that starts the engine associated with a view.

Copy link
Member

Choose a reason for hiding this comment

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

You might take renderer_type as an argument of FlutterDesktopViewCreateFromNewWindow or FlutterDesktopViewCreateFromElmParent if you don't want to add the property to both structs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added renderer_type only to FlutterDesktopWindowProperties. I didn't add it to FlutterDesktopViewProperties because for now only EvasGL is used. We can add them later if needed.

JSUYA added a commit to JSUYA/flutter-tizen that referenced this pull request Jul 13, 2022
Set the renderer type before creating the engine.
The window is selected as EcoreWl2 or ElmWin
according to the renderer type.

This commit requires engine side commits.
(flutter-tizen/engine#312)
shell/platform/tizen/BUILD.gn Outdated Show resolved Hide resolved
shell/platform/tizen/flutter_tizen_texture_registrar.cc Outdated Show resolved Hide resolved
shell/platform/tizen/external_texture_pixel_egl.cc Outdated Show resolved Hide resolved
@bbrto21 bbrto21 linked an issue Jul 14, 2022 that may be closed by this pull request
@JSUYA
Copy link
Member Author

JSUYA commented Jul 14, 2022

https://github.com/flutter-tizen/engine/runs/7334808882?check_suite_focus=true#step:6:53
@swift-kim Do you know FlutterTizenEngineTest.AddPluginRegistrarDestructionCallback test?
I can't found it in FlutterTizenEngineTest class.
CI goes into a loop on this test (maybe crash?)...

@swift-kim
Copy link
Member

@JSUYA The test has recently been added in #311. You might have to rebase your commits.

@JSUYA JSUYA force-pushed the dev/remove_evas_gl_only branch 2 times, most recently from e378ecf to 1e98c05 Compare July 14, 2022 07:42
shell/platform/tizen/external_texture_surface_evas_gl.h Outdated Show resolved Hide resolved
shell/platform/tizen/flutter_tizen.cc Show resolved Hide resolved
shell/platform/tizen/flutter_tizen.cc Outdated Show resolved Hide resolved
shell/platform/tizen/flutter_tizen_engine.cc Outdated Show resolved Hide resolved
shell/platform/tizen/flutter_tizen_engine.cc Outdated Show resolved Hide resolved
shell/platform/tizen/flutter_tizen_engine.cc Outdated Show resolved Hide resolved
shell/platform/tizen/flutter_tizen_engine.h Outdated Show resolved Hide resolved
shell/platform/tizen/flutter_tizen_engine_unittest.cc Outdated Show resolved Hide resolved
For devices other than wearable, we can either use ElmFlutterView or EcoreWl2Window,
depending on apps. This commit is to avoid being device dependent.

Remove the build flag for evas_gl only and use the WEARABLE_PROFILE macro if necessary.
Before creating the engine, the embedding can select the renderer type in the engine property.
On runtime, it selects an appropriate renderer and external texture
according to the renderer type and renders it.

related issue : flutter-tizen#301
JSUYA added a commit to JSUYA/flutter-tizen that referenced this pull request Jul 20, 2022
Set the renderer type before creating the engine.
The window is selected as EcoreWl2 or ElmWin
according to the renderer type.

This commit requires engine side commits.
(flutter-tizen/engine#312)
Copy link
Member

@swift-kim swift-kim left a comment

Choose a reason for hiding this comment

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

Only minor concerns. Otherwise looks good to me!

shell/platform/tizen/flutter_tizen_texture_registrar.cc Outdated Show resolved Hide resolved
shell/platform/tizen/flutter_tizen_texture_registrar.cc Outdated Show resolved Hide resolved
@swift-kim swift-kim merged commit 3d21ac7 into flutter-tizen:flutter-3.0.0-tizen Jul 25, 2022
swift-kim pushed a commit to swift-kim/flutter-tizen that referenced this pull request Jul 26, 2022
Set the renderer type before creating the engine.
The window is selected as EcoreWl2 or ElmWin
according to the renderer type.

This commit requires engine side commits.
(flutter-tizen/engine#312)

Remove renderer type in engine properties

If profile is wearable, the default of renderer type is kEvasGl

Add profile check for renderer in C#

Add profile check for renderer in cpp

fix package test

Apply review comments

Change file mode
swift-kim pushed a commit to swift-kim/flutter-tizen that referenced this pull request Jul 27, 2022
Set the renderer type before creating the engine.
The window is selected as EcoreWl2 or ElmWin
according to the renderer type.

This commit requires engine side commits.
(flutter-tizen/engine#312)

Remove renderer type in engine properties

If profile is wearable, the default of renderer type is kEvasGl

Add profile check for renderer in C#

Add profile check for renderer in cpp

fix package test

Apply review comments

Change file mode
JSUYA added a commit to JSUYA/flutter-tizen that referenced this pull request Aug 1, 2022
Set the renderer type before creating the engine.
The window is selected as EcoreWl2 or ElmWin
according to the renderer type.

This commit requires engine side commits.
(flutter-tizen/engine#312)
swift-kim pushed a commit to swift-kim/flutter-tizen that referenced this pull request Aug 1, 2022
Set the renderer type before creating the engine.
The window is selected as EcoreWl2 or ElmWin
according to the renderer type.

This commit requires engine side commits.
(flutter-tizen/engine#312)

Remove renderer type in engine properties

If profile is wearable, the default of renderer type is kEvasGl

Add profile check for renderer in C#

Add profile check for renderer in cpp

fix package test

Apply review comments

Change file mode
JSUYA added a commit to JSUYA/flutter-tizen that referenced this pull request Aug 1, 2022
Set the renderer type before creating the engine.
The window is selected as EcoreWl2 or ElmWin
according to the renderer type.

This commit requires engine side commits.
(flutter-tizen/engine#312)
swift-kim pushed a commit that referenced this pull request Aug 5, 2022
For devices other than wearable, we can use either ElmFlutterView or EcoreWl2Window,
depending on apps. This commit is to avoid being device dependent.

Remove the use_evas_gl_renderer build flag and use the WEARABLE_PROFILE macro if necessary.
Before creating the engine, the embedding can select the renderer type in the engine property.
At runtime, it selects an appropriate renderer and external texture
according to the renderer type and renders to it.
swift-kim pushed a commit that referenced this pull request Sep 1, 2022
For devices other than wearable, we can use either ElmFlutterView or EcoreWl2Window,
depending on apps. This commit is to avoid being device dependent.

Remove the use_evas_gl_renderer build flag and use the WEARABLE_PROFILE macro if necessary.
Before creating the engine, the embedding can select the renderer type in the engine property.
At runtime, it selects an appropriate renderer and external texture
according to the renderer type and renders to it.
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.

Enable Evas GL renderer for all profile
3 participants