Texture api change #82

bwikbs · 2021-05-07T03:12:33Z

To build, you need:
https://github.com/flutter-tizen/tizen_tools/pull/12

media_packet sample:
flutter-tizen/plugins#90

bwikbs · 2021-05-07T03:42:58Z

The video plugin was also briefly tested with the following code.

VideoPlayer::VideoPlayer(flutter::PluginRegistrar *pluginRegistrar,
                         FlutterTextureRegistrar *textureRegistrar,
                         const std::string &uri, VideoPlayerOptions &options) {
  isInitialized_ = false;
  textureRegistrar_ = textureRegistrar;

  LOG_INFO("[VideoPlayer] register texture");
  textureId_ = FlutterRegisterExternalTexture(textureRegistrar_,TizenTextureType::MediaPacket);

...

void VideoPlayer::onVideoFrameDecoded(media_packet_h packet, void *data) {
  VideoPlayer *player = (VideoPlayer *)data;
  // tbm_surface_h surface;
  // int ret = media_packet_get_tbm_surface(packet, &surface);
  // if (ret != MEDIA_PACKET_ERROR_NONE) {
  //   LOG_ERROR(
  //       "[VideoPlayer.onVideoFrameDecoded] media_packet_get_tbm_surface "
  //       "failed, error: %d",
  //       ret);
  //   media_packet_destroy(packet);
  //   return;
  // }
  FlutterMarkExternalTextureFrameAvailable(player->textureRegistrar_,
                                           player->textureId_, packet);
  // media_packet_destroy(packet);
}

bwikbs · 2021-05-07T03:47:23Z

It looks like a kind of pool is applied to the packet related API(at least camera_set_media_packet_preview_cb) , so if we do not discard packet quickly, we will have a problem.

xuelian-bai · 2021-05-07T05:50:06Z

I'm not sure if it's a good way, it can bypass using tbm internal APIs, but, it's specifically for media scenario, texture implementation is supposed to be common.

bwikbs · 2021-05-07T06:06:35Z

texture implementation is supposed to be common.

Does this mean you think we should accept one type? Could you explain a little bit more about what you're worried about?
And (as you already know..) It is also quite important to remove internal api. Without that, we have to take engine implementation separately.

xuelian-bai · 2021-05-07T06:07:24Z

shell/platform/tizen/external_texture_gl.cc

+
+static void UnmarkMediaPacketToUse(void* surface) {
+  FT_ASSERT(surface);
+  media_packet_destroy((media_packet_h)surface);


I think media_packet_ref should be called to prevent this handle from destroy. The same thing happen for tbm surface.

I'm a little confusing... you think calling media_packet_destroy at here is problem?
Is it correct that I understand? Why?

I mean, if you call media_packet_destroy directly, that media handle may be really destroyed, media player need to allocate another media handle, but if you call media_packet_ref when mark and call media_packet_destroy when unmark, that handle won't be really destroyed, media player may reuse it later.

Whatever External texture sources were used, it now seems only Tizen embedder is responsible for destroy them.
I think this is a good approach. Now we don't need to use ref/unref anymore.

xuelian-bai · 2021-05-07T06:37:22Z

texture implementation is supposed to be common.

Does this mean you think we should accept one type? Could you explain a little bit more about what you're worried about?
And (as you already know..) It is also quite important to remove internal api. Without that, we have to take engine implementation separately.

I mean the implementation is supposed to be common, not only for media packet, on the other hand, media packet can only handle mmplayer case, there are other cases such as webview, can webview use media packet too?
You may notice texture implementation for other platform, they even use mandantary GL APIs instead of our GL_TEXTURE_EXTERNAL_OES, I think the reason is to be common, since not all GPU driver support GL_TEXTURE_EXTERNAL_OES extension.
In my opinion, If only for tbm_surface_internal_ref/unref API, I suggest to use dlsym to get from libtbm.so

bbrto21 · 2021-05-07T07:04:38Z

Please rebase

bwikbs · 2021-05-07T07:05:44Z

I mean the implementation is supposed to be common, not only for media packet, on the other hand,

I totally agree with you !

on the other hand, media packet can only handle mmplayer case, there are other cases such as webview, can webview use media packet too?

Um... There are currently 3 cases where we use external textures... 2 of 3 are getting data through media packets. I just want to add it for convenience. Webview will continue to use tbm.
Internal api can disappear at any time (That's out platform!) and dlopen can cause unexpected problems.

xuelian-bai · 2021-05-08T02:58:21Z

I checked media_packet and muse_player source code, it turns out that mmplayer won't reuse tbm_surface in every frame_cb(), it will create a new tbm surface for every frame(I have suggested them to reuse...), everytime frame_cb() is called, a new media_packet handler is created and a new tbm surface is attached, when media_packet_destroy is called, both media_packet and tbm surface will be destroyed. So, I suggest:

We don't call tbm_surface_internal_ref/unref inside embedder
Inside video player, when onVideoFrameDecoded is called, we don't call media_packet_destroy immediately, we need a callback in emebedder to notify video player that texture render is done, in that new callback, we call media_packet_destroy.
For other possible cases, the same thing could happen to keep the tbm surface from rewritten or destroyed.
Inspired by this patch, I think we also need to support more types, such as CPU data to be imported as texture, to other possible users.

bwikbs · 2021-05-08T05:40:50Z

I strongly support your suggestion. 💪
In fact, it was the way I initially considered, but the current way was proposed because there are many changes on the plugin side with that way.

@flutter-tizen/maintainers plz, comment on this suggestion. Especially, who use external textures api!

swift-kim · 2021-05-08T07:21:02Z

Just a side note from an API design perspective:

Inside video player, when onVideoFrameDecoded is called, we don't call media_packet_destroy immediately, we need a callback in emebedder to notify video player that texture render is done, in that new callback, we call media_packet_destroy.

If we're to make breaking changes in our texture APIs, that is, if we're to add a new API or change existing ones, I think this is a good time to organize the overall API set cleanly. As you may remember, we unfortunately didn't do that during the last Flutter 2.0 upgrade.

We have these two sets of texture APIs in common/cpp and tizen respectively:

and we need to remove duplicates (such as FlutterPluginRegistrarGetTexture) and leave only Tizen-specific APIs in flutter_tizen_texture_registrar.h. You may also consider changing the API prefix to FlutterDesktop as suggested by #83. Then we will ultimately have

Common

(flutter_plugin_registrar.h) FlutterDesktopRegistrarGetTextureRegistrar
(flutter_texture_registrar.h) FlutterDesktopTextureType (unused by Tizen)
(flutter_texture_registrar.h) FlutterDesktopTextureInfo (unused by Tizen)
(flutter_texture_registrar.h) FlutterDesktopTextureRegistrarRegisterExternalTexture (unused by Tizen)
(flutter_texture_registrar.h) FlutterDesktopTextureRegistrarUnregisterExternalTexture (unused by Tizen)
(flutter_texture_registrar.h) FlutterDesktopTextureRegistrarMarkExternalTextureFrameAvailable (unused by Tizen)

Tizen-only

(flutter_tizen_texture_registrar.h) FlutterDesktopTizenTextureType
(flutter_tizen_texture_registrar.h) FlutterDesktopTizenTextureInfo (used for passing a callback pointer that @xuelian-bai suggested)
(flutter_tizen_texture_registrar.h) FlutterDesktopRegisterExternalTexture
(flutter_tizen_texture_registrar.h) FlutterDesktopUnregisterExternalTexture
(flutter_tizen_texture_registrar.h) FlutterDesktopMarkExternalTextureFrameAvailable

Note that the names and structure of the above Tizen-only APIs are tentative and can be changed as you prefer.

* Add interface for media_packet_h Signed-off-by: MuHong Byun <mh.byun@samsung.com>

bwikbs · 2021-05-10T00:52:54Z

@swift-kim That's a good idea. I'll check it out and give my opinion. 👀

bwikbs · 2021-05-10T06:54:51Z

@flutter-tizen/maintainers
Can we assume that everyone agrees on the callback approach?

@xuelian-bai
Will you prepare a patch for your suggestion? If so, webview and camera plug-in will be applied our side.

@swift-kim
Roughly speaking, it seems possible to no longer use flutter_tizen_texture_registrar.h.. but I'm not sure..
I will do this once after this task is over.

bwikbs · 2021-05-11T00:17:37Z

I'll close this PR when @xiaowei-guan 's patch is ready.

bwikbs · 2021-05-11T11:32:11Z

Continued on #86

Added in flutter#32050

bwikbs marked this pull request as draft May 7, 2021 03:13

bwikbs mentioned this pull request May 7, 2021

Fix a crash in ExternalTextureGL #81

Merged

bwikbs force-pushed the texture_api_change branch 2 times, most recently from f6ca6e3 to aab41e9 Compare May 7, 2021 05:29

bwikbs marked this pull request as ready for review May 7, 2021 05:37

xuelian-bai reviewed May 7, 2021

View reviewed changes

bwikbs added the under discussion label May 8, 2021

Change api of external texture

8b98607

* Add interface for media_packet_h Signed-off-by: MuHong Byun <mh.byun@samsung.com>

bwikbs force-pushed the texture_api_change branch from aab41e9 to 8b98607 Compare May 10, 2021 00:42

xiaowei-guan self-requested a review May 10, 2021 08:41

This was referenced May 11, 2021

[webview_flutter] Apply changes in public api flutter-tizen/plugins#89

Closed

[camera] Apply changes in public api flutter-tizen/plugins#90

Closed

bwikbs removed the under discussion label May 11, 2021

bwikbs marked this pull request as draft May 11, 2021 00:15

bwikbs closed this May 11, 2021

swift-kim mentioned this pull request May 13, 2021

Some unresolved issues #53

Closed

12 tasks

swift-kim pushed a commit that referenced this pull request Sep 1, 2022

Impelement DisplayListDispatcher::transformReset. (#82)

e381974

Added in flutter#32050

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Texture api change #82

Texture api change #82

bwikbs commented May 7, 2021 •

edited

Loading

bwikbs commented May 7, 2021

bwikbs commented May 7, 2021

xuelian-bai commented May 7, 2021

bwikbs commented May 7, 2021

xuelian-bai May 7, 2021

bwikbs May 7, 2021

xuelian-bai May 7, 2021

bbrto21 May 10, 2021

xuelian-bai commented May 7, 2021

bbrto21 commented May 7, 2021

bwikbs commented May 7, 2021 •

edited

Loading

xuelian-bai commented May 8, 2021

bwikbs commented May 8, 2021

swift-kim commented May 8, 2021 •

edited

Loading

bwikbs commented May 10, 2021

bwikbs commented May 10, 2021 •

edited

Loading

bwikbs commented May 11, 2021

bwikbs commented May 11, 2021

Texture api change #82

Texture api change #82

Conversation

bwikbs commented May 7, 2021 • edited Loading

bwikbs commented May 7, 2021

bwikbs commented May 7, 2021

xuelian-bai commented May 7, 2021

bwikbs commented May 7, 2021

xuelian-bai May 7, 2021

Choose a reason for hiding this comment

bwikbs May 7, 2021

Choose a reason for hiding this comment

xuelian-bai May 7, 2021

Choose a reason for hiding this comment

bbrto21 May 10, 2021

Choose a reason for hiding this comment

xuelian-bai commented May 7, 2021

bbrto21 commented May 7, 2021

bwikbs commented May 7, 2021 • edited Loading

xuelian-bai commented May 8, 2021

bwikbs commented May 8, 2021

swift-kim commented May 8, 2021 • edited Loading

bwikbs commented May 10, 2021

bwikbs commented May 10, 2021 • edited Loading

bwikbs commented May 11, 2021

bwikbs commented May 11, 2021

bwikbs commented May 7, 2021 •

edited

Loading

bwikbs commented May 7, 2021 •

edited

Loading

swift-kim commented May 8, 2021 •

edited

Loading

bwikbs commented May 10, 2021 •

edited

Loading