Skip to content

Conversation

@xiaowei-guan
Copy link

  • No need wait for flutter engine destruct callback to destory TBM buffer.

@xiaowei-guan xiaowei-guan requested a review from a team March 11, 2022 12:55
* No need wait for flutter engine destruct callback to destory TBM
  buffer.
@swift-kim
Copy link
Member

swift-kim commented Mar 14, 2022

The release_callback interface has been added to the upstream's FlutterDesktopPixelBuffer (flutter#28298).

Should we update our implementation (FlutterDesktopGPUBufferTextureConfig and FlutterDesktopGpuBuffer) accordingly? It will be a breaking API change and the video_player_tizen plugin will need to be updated as well.

(Please also take a look at Stuart's comment in flutter#28298 (comment) if you're to make the change.)

p.s. FlutterDesktopGPUBufferTextureConfig (capitalized GPU) should also be renamed to FlutterDesktopGpuBufferTextureConfig for consistency.

@xiaowei-guan
Copy link
Author

The release_callback interface has been added to the upstream's FlutterDesktopPixelBuffer (flutter#28298).

Yes, For the release callback, we can refer to the implementation of pixel buffer. Thanks for your suggestion. I will apply it.

Copy link

@wb0716 wb0716 left a comment

Choose a reason for hiding this comment

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

Good for me

Copy link

@zhouleonlei zhouleonlei left a comment

Choose a reason for hiding this comment

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

lgtm

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.

Thank you for taking my suggestion into account and the change generally looks good to me.

Will this change require any client side (i.e. camera_tizen and video_player_tizen) changes?

@xiaowei-guan
Copy link
Author

Thank you for taking my suggestion into account and the change generally looks good to me.

Will this change require any client side (i.e. camera_tizen and video_player_tizen) changes?

Yes, the plugin which uses texture api, need update.

@bwikbs
Copy link
Member

bwikbs commented Mar 15, 2022

@xiaowei-guan
I have a question (Plz understand that I have never seen driver behavior or egl extension implementations before..).
I'm not sure because I'm not in the office right now.. so I can't test this with my target...
According to my foggy memory, the reason that destroying tbm is performed in destruciton callback is because this caused a problem in using multiple webviews (like animation or map). And by some inaccurate experiments, I thought that contents of tbm were not copied when creating the egl image.

So..can you explain a little bit more about whether it is safe to create an egl image and delete the tbm right away? 😄

@xiaowei-guan
Copy link
Author

@xiaowei-guan I have a question (Plz understand that I have never seen driver behavior or egl extension implementations before..). I'm not sure because I'm not in the office right now.. so I can't test this with my target... According to my foggy memory, the reason that destroying tbm is performed in destruciton callback is because this caused a problem in using multiple webviews (like animation or map). And by some inaccurate experiments, I thought that contents of tbm were not copied when creating the egl image.

So..can you explain a little bit more about whether it is safe to create an egl image and delete the tbm right away? 😄

I just refer to eglCreateImageKHR API guide, I didn't find need wait for destruction callback, and I have tested with video player plugin. But I didn't test others plugin which use texture. I start to check the soruce code of eglCreateImageKHR,I will reply it later.

@xiaowei-guan
Copy link
Author

I have a question (Plz understand that I have never seen driver behavior or egl extension implementations before..).
I'm not sure because I'm not in the office right now.. so I can't test this with my target...

@bwikbs
I have downloaded TV GPU driver source code, and add some log. I have tested Video player plugin,there is no problem.
Could you please guide me how to test multiple webviews?

@bwikbs
Copy link
Member

bwikbs commented Mar 23, 2022

@xiaowei-guan

Could you please guide me how to test multiple webviews?

Here is sample
Thanks 😄 I'll check it too!


update)
In my case, It works ok at TV. 👍 I guess there is some misunderstanding.
Plz, double check it your side.

@swift-kim swift-kim added the no merge This PR is not ready for merge yet label Mar 24, 2022
@xiaowei-guan
Copy link
Author

I have run webview plugin sample on TV device,also no problem.

@swift-kim
Copy link
Member

The client side (camera_tizen and video_player_tizen) changes should be prepared first before we can publish this change.

@xiaowei-guan
Copy link
Author

@swift-kim swift-kim removed the no merge This PR is not ready for merge yet label Mar 25, 2022
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.

Thank you!

@swift-kim swift-kim merged commit f1e1b01 into flutter-tizen:flutter-2.10.1-tizen Mar 25, 2022
bwikbs added a commit to bwikbs/engine that referenced this pull request Apr 10, 2022
bwikbs added a commit to bwikbs/engine that referenced this pull request Apr 12, 2022
swift-kim pushed a commit that referenced this pull request May 12, 2022
* Release buffer after creating egl image

* No need wait for flutter engine destruct callback to destory TBM
  buffer.

* Apply common release callback
swift-kim pushed a commit that referenced this pull request Aug 5, 2022
* Release buffer after creating egl image

* No need wait for flutter engine destruct callback to destory TBM
  buffer.

* Apply common release callback
swift-kim pushed a commit that referenced this pull request Sep 1, 2022
* Release buffer after creating egl image

* No need wait for flutter engine destruct callback to destory TBM
  buffer.

* Apply common release callback
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.

5 participants