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

[Multimedia] Add MediaPacket TBM surface APIs #6278

Merged
merged 3 commits into from
Aug 28, 2024

Conversation

hsgwon
Copy link
Contributor

@hsgwon hsgwon commented Aug 20, 2024

Description of Change

  • Add TbmSurface property on MediaPacket to get native tbm surface handle
  • Add HasTbmSurface property to check whether the packet have TBM surface or not

API Changes

@hsgwon hsgwon added ACR Required API12 Platform : Tizen 9.0 / TFM: net6.0-tizen9.0 labels Aug 20, 2024
@hsgwon hsgwon requested a review from myroot August 20, 2024 06:56
@TizenAPI-Bot
Copy link
Collaborator

Public API Changed

Please follow the ACR process for the changed API below.

Added: 2, Removed: 0, Changed: 0

Added

+ /// <since_tizen>12</since_tizen
+ System.Boolean Tizen.Multimedia.MediaPacket::HasTbmSurface()

+ /// <since_tizen>12</since_tizen
+ System.IntPtr Tizen.Multimedia.MediaPacket::TbmSurface()

Copy link
Contributor

@myroot myroot left a comment

Choose a reason for hiding this comment

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

PR감사합니다.

src/Tizen.Multimedia/MediaTool/MediaPacket.cs Outdated Show resolved Hide resolved
@TizenAPI-Bot
Copy link
Collaborator

Public API Changed

Please follow the ACR process for the changed API below.

Added: 2, Removed: 0, Changed: 0

Added

+ /// <since_tizen>12</since_tizen
+ System.Boolean Tizen.Multimedia.MediaPacket::HasTbmSurface()

+ /// <since_tizen>12</since_tizen
+ System.IntPtr Tizen.Multimedia.MediaPacket::TbmSurface()

Copy link
Member

@wiertel wiertel left a comment

Choose a reason for hiding this comment

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

The API is simple, the description is good, but IntPtr should be avoided in public API. Some solutions would be: wrap the pointer in SafeHandle (or derivative) - it is still a pointer, but this way it would at least have a type (like SafeNativeWindowHandle), another idea is to expose an interface/class that would provide the pointer internally and the other side would consume that pointer internally so that the user would not touch the pointer - that one is possible only if the consuming part is somewhere in TizenFX.
Please share how the pointer is used / what API consumes it / what the app developer will need to do with it. I'm wondering if the usage scenario justifies having pointer in the API.

@myroot
Copy link
Contributor

myroot commented Aug 21, 2024

The API is simple, the description is good, but IntPtr should be avoided in public API. Some solutions would be: wrap the pointer in SafeHandle (or derivative) - it is still a pointer, but this way it would at least have a type (like SafeNativeWindowHandle), another idea is to expose an interface/class that would provide the pointer internally and the other side would consume that pointer internally so that the user would not touch the pointer - that one is possible only if the consuming part is somewhere in TizenFX. Please share how the pointer is used / what API consumes it / what the app developer will need to do with it. I'm wondering if the usage scenario justifies having pointer in the API.

Yes, I understand your concern, but It can't wrapping with managed type. because it directly used on Interop calls
We already have IntPtr type in public API
for example, EvasObject.Handle, IWindowProvider.WindowHandle
It is same reason why IntPtr API was provided

We can consider, A managed TbmSurface class if we providing functionality of TbmSurface on C# API, but not now.

@myroot
Copy link
Contributor

myroot commented Aug 21, 2024

We plan to provide a view that supporing tbm surface. Here

We can update content of view with tbm_surface handle with below API

        /// <summary>
        /// Sets the source of the TbmSurfaceView to the given tbm_surface_h.
        /// </summary>
        /// <param name="tbmSurface">The handle to the tbm_surface_h to set as the source.</param>
        /// <remarks>
        ///  Note that this method must be called on the UI thread only during the first call. Subsequent calls can be made from any thread.
        /// </remarks>
        public void SetSource(IntPtr tbmSurface);

@wiertel
Copy link
Member

wiertel commented Aug 26, 2024

Wanted to ensure that alternatives are considered. The chosen solution seems justified.

@TizenAPI-Bot
Copy link
Collaborator

Public API Changed

Please follow the ACR process for the changed API below.

Added: 2, Removed: 0, Changed: 0

Added

+ /// <since_tizen>12</since_tizen
+ System.Boolean Tizen.Multimedia.MediaPacket::HasTbmSurface()

+ /// <since_tizen>12</since_tizen
+ System.IntPtr Tizen.Multimedia.MediaPacket::TbmSurface()

@hsgwon hsgwon merged commit 63f4b54 into Samsung:master Aug 28, 2024
3 checks passed
@hsgwon hsgwon deleted the api12.multimedia.AddTbmSurfaceAPI branch August 28, 2024 00:03
everLEEst pushed a commit to everLEEst/TizenFX that referenced this pull request Sep 4, 2024
* [Multimedia] Add MediaPacket TBM surface API
everLEEst pushed a commit that referenced this pull request Sep 4, 2024
* [Multimedia] Add MediaPacket TBM surface API
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACR Required API12 Platform : Tizen 9.0 / TFM: net6.0-tizen9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants