-
Notifications
You must be signed in to change notification settings - Fork 430
Add Android TextureView #2540
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
Add Android TextureView #2540
Conversation
@dotnet-policy-service agree |
Thanks @jonmdev! I truly appreciate your contribution. This PR adds over 9,000 lines of code. Is it possible to reduce the scope of this contribution? A typical pull request target a few hundred (or less) lines of code. If all +9,000 lines are truly necessarily, I understand. However, it requires a herculean effort from us to review this PR which is time we cannot allocate to other issues and existing PRs. |
I meant to say something similar. I think the bulk of the change is a new sample application. Hopefully we could merge the changes into the current sample app and remove the noise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 38 out of 53 changed files in this pull request and generated no comments.
Files not reviewed (15)
- samples/AddAndroidTextureViewTest/AddAndroidTextureViewTest.csproj: Language not supported
- samples/AddAndroidTextureViewTest/App.xaml: Language not supported
- samples/AddAndroidTextureViewTest/AppShell.xaml: Language not supported
- samples/AddAndroidTextureViewTest/MainPage.xaml: Language not supported
- samples/AddAndroidTextureViewTest/Platforms/Android/AndroidManifest.xml: Language not supported
- samples/AddAndroidTextureViewTest/Platforms/Android/Resources/values/colors.xml: Language not supported
- samples/AddAndroidTextureViewTest/Platforms/MacCatalyst/Entitlements.plist: Language not supported
- samples/AddAndroidTextureViewTest/Platforms/MacCatalyst/Info.plist: Language not supported
- samples/AddAndroidTextureViewTest/Platforms/Tizen/tizen-manifest.xml: Language not supported
- samples/AddAndroidTextureViewTest/Platforms/Windows/App.xaml: Language not supported
- samples/AddAndroidTextureViewTest/Platforms/MacCatalyst/AppDelegate.cs: Evaluated as low risk
- samples/AddAndroidTextureViewTest/Platforms/Android/MainApplication.cs: Evaluated as low risk
- samples/AddAndroidTextureViewTest/MauiProgram.cs: Evaluated as low risk
- samples/AddAndroidTextureViewTest/Platforms/Android/MainActivity.cs: Evaluated as low risk
- samples/AddAndroidTextureViewTest/Platforms/Windows/App.xaml.cs: Evaluated as low risk
Comments suppressed due to low confidence (2)
samples/AddAndroidTextureViewTest/App.xaml.cs:34
- The null-conditional operator '!' is redundant here. It should be removed to simplify the code.
var platformView = ((CommunityToolkit.Maui.Core.Views.MauiMediaElement)mediaElement.ToPlatform(mediaElement.Handler!.MauiContext!));
samples/AddAndroidTextureViewTest/MainPage.xaml.cs:4
- [nitpick] The variable name 'count' is ambiguous. It should be renamed to 'clickCount'.
int count = 0;
Yes, as you can see almost all the added code is the test project. The test project is samples/AddAndroidTextureViewTest. This is a default Maui project which I then modified by altering app.xaml.cs as noted. If you look at the differences, you can scroll down 3 quarters of the way before you reach changes not relating to the test project. Ie. Scroll down until you stop seeing samples/AddAndroidTextureViewTest What I would recommend is that you just read those changes. It is a tiny change in the actual code base. Then if you are satisfied with the code changes and have finished using the test project to assess, I can delete the test project and push the update once more without it for actual merging. I have already spent more time than I have on this and I don't think there is any meaningful value in trying to merge the test project into the code base in any way. It was just there for my personal tests and I presume it might be helpful for yours too. Alternatively if you want me to remove the test project now I can but I suspect you will want to use it same as I did to verify or test the usage. |
Yes it is. As I commented above you can just scroll down on the differences until you stop seeing samples/AddAndroidTextureTest I have no intention for this test project to enter the Community Toolkit repository. It was a Maui default project I modified in App.xaml.cs. It can be deleted completely and it is my intention to do so. I left it there only in case you wanted to use it for easy testing of the code same as I used it when I wrote the changes. Please let me know when you want me to delete it and I will delete it. The actual code change to the CommunityToolkit is tiny and represents just the last little bit when you review the differences. |
If u want some help I can update the sample app for you and/or provide the code and you can add it yourself @jonmdev |
Yes please. If you want to delete the sample app yourself at this point ie. The entire path of samples/AddAndroidTextureViewTest And if needed integrate some other simple test into the official sample that would be great. You understand this issue as well as I do or better at this point so obviously I am certain you can do so. And you know the official test project which I know nothing about. So you would be better for that job if needed. Please then if so delete samples/AddAndroidTextureViewTest entirely but please leave my code on the actual changes at /src/ unchanged for now so that can get a proper full review in the current state. And feel free to add any formal example you like to the official test app as I am not familiar with that or what would be wanted. As noted I shared the three main intended user usage methods of the changes in the OP if that is any help and how to verify what is made in debugging. Thanks for your help as always. |
Without refactoring or reworking this PR it will not work with existing sample app. We either refactor the sample app or refactor this PR. You asked me not to refactor your PR so I won't. But the changes as is will prevent the sample app from running without significant refactoring of the sample app. I will seek input from other maintainers about that. |
I don't think you should keep my AddAndroidTextureViewTest. There is no need for it. It is an entire bloated Maui sample project and there would be no benefit to keeping it in the CommunityToolkit. I just used it as it was the easiest working method for me to develop with. I am not sure what you have permission to do or not on this. I am not familiar with how GitHub works or what was wanted for me to do. So I just deleted it now. 🙂 My ugly test project is now gone. I think that solves any further wasted energy on it easily enough. Now the push is only 15 changed files with the actual changes: https://github.com/CommunityToolkit/Maui/pull/2540/files The simple test code anyone can use to test is in the OP. I.e. The 3 user methods and the debug out of the view type code. |
This PR has breaking changes to |
Sorry, please check again, I already fixed that ~15 min ago. I realized as soon as I deleted my test project and ran the regular test project. I just needed to add a default constructor. It should work again now. |
I checked. It no longer crashes sample app. Good job. But you have altered the handler for every device type. Should we be altering handlers when we do not need to? I do not think there is a need to change the handler for ios, macos, or windows when we are targeting android changes. Have you considered any way to do this that does not require changing the handler for unaffected devices? |
The changes are universal because the
This was intentional and I think it is a good idea. The system should be implemented for all platforms so that this infrastructure does not need to be changed again after this and can be used for any range of purposes. As you can see, there are no breaking changes as it is fully safe. Not doing this now will just create confusion later in my opinion and fragmentation of the system which is not intended. I don't want people to think It is meant to be fully cross platform compatible. I hope that makes sense. Thanks. |
Please ignore my last comment. You added a default constructor. That works. No breaking changes now. That is great! I am testing now and adding changes to sample app and testing that. Should be able to repot back in a few minutes. |
I just copied the sample you provided for displaying the video. Anyone who can write better markup than me feel free to update it to something better. It works for sample app and shows how Texture view works. Anyone who wants to feel free to edit the changes I made. If you have a better way to display it please feel free to change it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jonmdev + @ne0rrmatrix! This PR very close and just needs a few updates:
- Add Unit Tests for
MediaElementOptions
andUseMauiCommunityToolkitMediaElement
- See
CommunityToolkit.Maui.UnitTests.Extensions.AppBuilderExtensionsTests
for existing examples
- See
-
MediaElementTextureViewPage
needs to be updated- See my comment below on
MediaElementTextureViewPage.xaml
for more info.
- See my comment below on
- The Popup on
MediaElementPage
flickers- See my comment on
MediaElementPage.xaml.cs
for more info
- See my comment on
Just FYI, I've made a few tweaks to this PR. A few of these changes will require updates to the docs.
- Fix Namespaces
- Fix file names
- Added
.shared.cs
suffix
- Added
- Move
AndroidViewType.shared.cs
from\Views\
to\Primatives\
folder - Remove
CommunityToolkit.Maui.Core.Primitives
namespace- We don't use
.Primatives
in our namespaces - See
CommunityToolkit.Maui.Core\Primatives\
for examples
- We don't use
- Refactored
MediaElementOptions
- It now aligns with the existing format of
CommunityToolkit.Maui.Options
andCommunityToolkit.Maui.Core.Options
- It now aligns with the existing format of
- Remove unnecessary constructors from
MediaElement
MediaElement.AndroidViewType
is now aninit
-only Property- This allows
AndroidViewType
to be set by users in XAML
- Add
StreamingVideoUrls
toCommunityToolkit.Maui.Sample
- Use
AndroidViewType.SurfaceView
for existing MediaElement pages in Sample App
samples/CommunityToolkit.Maui.Sample/Pages/Views/MediaElement/MediaElementTextureViewPage.cs
Outdated
Show resolved
Hide resolved
@@ -170,7 +170,7 @@ async void ChangeSourceClicked(Object sender, EventArgs e) | |||
MediaElement.MetadataArtworkUrl = botImageUrl; | |||
MediaElement.MetadataArtist = "Big Buck Bunny Album"; | |||
MediaElement.Source = | |||
MediaSource.FromUri(buckBunnyMp4Url); | |||
MediaSource.FromUri(StreamingVideoUrls.BuckBunny); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be a bug unrelated to this PR. If so, please open an Issue.
This also may be a bug on my emulator. If so, please let me know if you are unable to reproduce it on your Android device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As to the flickering specifically that is a known issue with emulator and I cannot replicate it on any physical device on any API.
As to the blank screen I will try to replicate that. I have seen in some cases when navigating to page that video does not load. It looks like that happens with main too. So unrelated to this PR. But further testing is needed to confirm that. I will get that done. User selecting a video from a picker always loads which is weird if initial load is failing. I will have to investigate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for constructing MediaElement with a TextureView on Android, allowing developers to choose between TextureView and SurfaceView during platform view construction. Key changes include:
- Adding a new global and per-instance override for the Android view type via MediaElementOptions.
- Creating a new MediaElementTextureView class and sample pages to demonstrate the TextureView usage.
- Updating sample projects and configuration to support the new view type.
Reviewed Changes
Copilot reviewed 37 out of 42 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/CommunityToolkit.Maui.MediaElement/AssemblyInfo.shared.cs | Removed an XML namespace mapping, likely as part of refactoring. |
src/CommunityToolkit.Maui.MediaElement/AppBuilderExtensions.shared.cs | Updated the extension to support a new MediaElementOptions parameter for platform-specific setup. |
samples/CommunityToolkit.Maui.Sample/ViewModels/Views/ViewsGalleryViewModel.cs | Added a new section for MediaElementTextureView view model. |
samples/CommunityToolkit.Maui.Sample/ViewModels/Views/MediaElement/MediaElementViewModel.cs | Removed an unused empty constructor. |
samples/CommunityToolkit.Maui.Sample/ViewModels/Views/MediaElement/MediaElementTextureViewViewModel.cs | Added a new view model for TextureView. |
samples/CommunityToolkit.Maui.Sample/ViewModels/Views/MediaElement/MediaElementCollectionViewViewModel.cs | Updated video URL references to use StreamingVideoUrls constants. |
samples/CommunityToolkit.Maui.Sample/ViewModels/Views/MediaElement/MediaElementCarouselViewViewModel.cs | Updated video URL references to use StreamingVideoUrls constants. |
samples/CommunityToolkit.Maui.Sample/Pages/Views/MediaElement/MediaElementTextureViewPage.cs | Created a new sample page to demonstrate MediaElement with TextureView. |
samples/CommunityToolkit.Maui.Sample/Pages/Views/MediaElement/MediaElementPage.xaml.cs | Updated video URLs and set AndroidViewType for a popup MediaElement. |
samples/CommunityToolkit.Maui.Sample/Pages/Views/MediaElement/MediaElementMultipleWindowsPage.cs | Updated Android view configurations and video URL references. |
samples/CommunityToolkit.Maui.Sample/MauiProgram.cs | Configured the default MediaElement Android view type to use TextureView. |
samples/CommunityToolkit.Maui.Sample/Constants/StreamingVideoUrls.cs | Added a new constants file for streaming video URLs. |
samples/CommunityToolkit.Maui.Sample/AppShell.xaml.cs | Mapped the new MediaElementTextureViewPage in the application shell. |
Files not reviewed (5)
- samples/CommunityToolkit.Maui.Sample/Pages/Views/MediaElement/MediaElementCarouselViewPage.xaml: Language not supported
- samples/CommunityToolkit.Maui.Sample/Pages/Views/MediaElement/MediaElementCollectionViewPage.xaml: Language not supported
- samples/CommunityToolkit.Maui.Sample/Pages/Views/MediaElement/MediaElementPage.xaml: Language not supported
- samples/CommunityToolkit.Maui.Sample/Platforms/Android/AndroidManifest.xml: Language not supported
- src/CommunityToolkit.Maui.MediaElement/CommunityToolkit.Maui.MediaElement.csproj: Language not supported
- Fix tizen invalid using statement
Implemented unit tests for `AppBuilderExtensions_MediaElement` and `MediaElementOptions` to validate the setting and retrieval of the default Android view type for media elements. The tests cover various scenarios to ensure expected behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey James! I've refactored the MediaElementTextureViewPage
to simplify the code and follow best practices for CommunityToolkit.Maui.Markup
.
I still am confused/concerned by the purpose of the MediaElementTextureViewPage
. You said that "It is not possible to have overlapping views using SurfaceView on Android". However, when I use AndroidViewType.SurfaceView
for the MediaElements on MediaElementTextureViewPage
, both videos still appear the same as the do when using Android.TextureView
and I am able to interact with both videos (screenshot below).
Could you please either update MediaElementTextureViewPage
to show a better use case for AndroidViewType.TextureView
and/or improve the explanations provided on the labels on MediaElementTextureViewPage
to help us understand what it is demonstrating?
Here's the checklist of items we are waiting on before we can merge this PR:
- Improve demonstration of TextureView vs SurfaceView in
CommunityToolkit.Maui.Sample
- Fix failing Unit Test
- Update documentation for TextureView (@bijington can help you with this)
Android.SurfaceView | Android.TextureView |
---|---|
![]() |
![]() |
Based on the changes you have made I am not sure how to proceed. Here is a image of what it looked like before changes. If you look closely the images overlap 50 percent. This behaviour is very different on texture view and surface view. On surface view it will only ever show one video. |
If you want to understand SurfaceView vs. TextureView issues, please review my post here: My demo project showing the limitation of SurfaceView and need for TextureView is still present as linked there also: https://github.com/jonmdev/MediaElementOverlapBug Here is also a good StackOverflow thread with many users explaining the difference: https://stackoverflow.com/questions/16916877/whats-differences-between-surfaceview-and-textureview SurfaceView and TextureView are also well documented by Android. There are many resources from Android about the difference. The distinction between SurfaceView and TextureView is more a matter of Android OS design choice, documented by Android accordingly. The difference in short is that SurfaceView does not respect things like clipping or overlay properly as it is not designed to. The analogy I have seen that is used is that SurfaceView is like a "hole" punched through the UI's screen. You cannot clip or overlay predictably with it. This is still used commonly as default as it renders more efficiently. TextureView renders more the way you would expect - you can overlay or clip it or affect opacity without difficulties. So whenever you need clipping or overlaying of videos you must use TextureView. Here are some posts explaining this further as linked in the OP: If you have any questions feel free to share and I can do my best to help clarify further. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the Unit Tests.
I've requested the necessary changes to the docs PR.
Finally, I've removed the MediaElementTextureView page as it's unneeded because I've updated the existing MediaElement sample pages to demonstrate using AndroidViewType.TextureView
. It's best to unblock this PR by removing that sample page.
Description of Change
Android MediaElement (ExoPlayer) can exist as TextureView or SurfaceView which have different properties and may be needed in different situations. This must be set on platform view construction.
Previously there was no method to build a MediaElement as a TextureView. This now adds this functionality in the following possible various ways:
(1) Global Setting By Builder:
And the default builder option is still of course working:
(2) C# Override Per Object:
Users can also override the global defaults per object this way:
(3) XAML or C# Override Per Object:
Or for both C# and xaml they can also do this by deriving:
The addition of the
MediaElementOptions
and this infrastructure will also allow other platform specific customizations that are needed during construction of any platform view to be added easily in the future if needed.There are no breaking changes. These additions will not hurt or negatively affect anyone's current projects or usage. It only adds function.
The code is all fast and safe with minimal computational cost (just a few null checks and reference assignments). I have been using the Android method of building TextureView here for 7-9 months with zero issues, so it is safe and effective.
Linked Issues
PR Checklist
approved
(bug) orChampioned
(feature/proposal)main
at time of PRAdditional information
The code should all guaranteed to be safe and foolproof now after a few minor fixes have been applied as below. The logic and implementation is discussed at length in this thread:
#2526
Testing Methods
The demo project was in
AddAndroidTextureViewTest
samples project but I have pruned that and @ne0rrmatrix added a sample to the official samples page.If you wish to run your own tests, you originally could debug out a constructed view's type in Android as in
App.xaml.cs
with:However I was instructed not to let the PlayerView be accessed publicly, so you will have to use Reflection or another method to do something similar.