-
Notifications
You must be signed in to change notification settings - Fork 424
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
base: main
Are you sure you want to change the base?
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. |
@jonmdev @ne0rrmatrix looks like I manage to present the video without the xml reader can you try out the code and see if that works for you? Here's the snippet that I used inside var inflater = LayoutInflater.From(MauiContext.Context)!;
var layout = inflater.Inflate(Microsoft.Maui.Resource.Layout.textureview, null)!;
// Find the PlayerView in the inflated layout
PlayerView = layout.FindViewById<PlayerView>(Microsoft.Maui.Resource.Id.texture_view_media_element);
if (PlayerView == null)
{
throw new InvalidOperationException("PlayerView cannot be null");
}
PlayerView.Player = Player;
PlayerView.UseController = false;
PlayerView.ControllerAutoShow = false; as a mobile developer my approach looks like more android-sh |
That works great @pictos I am glad you found a good solution. At this time I have only really added the code for for sample app. I was unable to find a better solution but it looks like you have solved it. Anyways I appreciate the help. I hope @jonmdev implements it. I am just assisting him with some issues. It is their PR. |
My only wish list now is to remove the multiple constructors that should not be needed. I have shown several ways of doing that. I provided an example of options class and have mentioned using a The code could be further reduced if we just used the option class like we do with the rest of the toolkit. There are multiple examples of how to do that. Otherwise if you want to set it at runtime you can alternatively set the option class with a BindableProperty as another way of doing it? @jonmdev If you need help implementing that I can help with that. I will write up a sample and link it here so that you can see how it is done. |
Neat. So this is just the same method (we are still inflating from the xml) but just by a different method. I am fine with either. It is the same thing either way. I don't know which is faster but I would assume there is no significant difference. So if people like this syntax better I will change it to that way. The only downside to this method is you now have two "resource" names to maintain in the code - one to the textureview xml and another to the id of that xml object. But since these won't change over time and we should get errors if they are changed it is no issue really. This is just stylistic from my perspective. |
I believe what you are suggesting is that the usage should be:
ie. Where we pass in:
In my opinion this is likely less efficient, but it would stylistically bring it more in line with the other systems and it is not a significant cost. So I don't have any major feelings either way. I am happy to add that if you would like the look of this better. Questions:
This I entirely leave to your preference as you have spent way more time working on this project than I have and it doesn't change anything meaningful to me except again stylistically as long as it is safe. I think the current system is good because it is simple and I always prefer simplicity. We are also being clear about what you have control over. But that is just personal preference. It makes no difference otherwise to me. If people prefer the extra complexity and ambiguity for stylistic reasons, I don't think it matters too much and we can do that. Am I understanding what you are asking? Regarding constructors, the entire point of this design is to have a mechanism to set in information before the platform constructors run in a safe and predictable way. The only way to do that is by setting it in as a global setting first (as we have done) and for individual element overrides, passing it into the object constructor (as we have done). So there is no way around that. But it is optional of course and no one needs to use it if they don't like or need any of these functions. Thanks. |
UPDATEI just pushed another update. I added the new xml build method @pictos suggested in I kept the old one there also in case any more testing is needed or there is any debate on which is better. As I said I don't really care as long as they both work. Ether is then fine. I think the original may be better as the
Is likely slower (FindByViewId) and theoretically could provoke an error though not likely. I think I will stopwatch them and see. I thought some more about the builder method and my best thought was to just add a new builder constructor so now both of these stylistic approaches work: Builder - Option 1:
Builder - Option 2:
The first takes If it is just a matter of stylistic consistency either is fine. Technically the original version is safer as people could do damaging things with an |
On a tangential note, does anyone know why I can't build the existing demo project to Android? I can still run my own test project but whenever i try to run the existing sample project to Android I get:
I tried deleting bin and obj but I cannot get this existing samples project to run on Android. It does run on Windows however. Thoughts? |
Just as a matter of curiosity, I tested the two methods of creating the ExoPlayer. I just started a StopWatch and stopped it at the end of each, tested one at a time (with the other commented out). My original method was slightly faster (which is what I guessed) but it is not meaningful. This is likely because it avoids the Find which is probably suboptimal and we never usually want to be running Find type operations if we don't have to. But no major difference. We get (Debug to Pixel 5 simulator):
|
UPDATEFor anyone interested, I pushed another update as we need three builder options if we want all to work. Currently now all three of these work: Using Builder to Set Default Options:
I actually think it is kind of nice having all three, but again there is no difference to me. The last option is faster and safer than the second option but the second option better matches the common aesthetic convention. I also went back to the original Exoplayer construction code as stopwatch showed no improvement (and possibly worsening) with the newly offered alternative code. I can also say I have tested the original Exoplayer construction code extensively over the past 9 months so I can be confident it is stable. I left the new code as a comment and I suggest it stay there as a comment just in case the methods change in the future. It is not easy to find this stuff. It is good to keep a backup method in my opinion. Please let me know if you would like to delete any of these builder options or if we can leave all three or if there are any other ideas or suggestions. Currently in addition to the above 3 builder options for setting global defaults (or not setting them as per your wish), we also still now have the individual object overrides: C# Overriding Default Options Per Object
C# or XAML Overriding Defaults Per Object
I think this is now very comprehensive and versatile. |
Ok I have spent the last few hours testing. It is working well in android at least. I will mention I did notice a huge bug. But not with this PR. I discovered that when navigating back and forth to any media element page a second time we are getting some weird flickering. This is on main too. I will be investigating that and trying to figure out if it is just the sample app. edit: |
Great to know! Thanks. I don't know if I have also found a bug. I cannot build the standard samples project in Android at all. I am getting this every time whether to device or simulator: #2540 (comment) Have you ever seen that before? Should I make a new bug report? I am currently having to keep adding and removing my own test project just to see what is going on as I can't use the real one which is strange to me. 🫤 Thanks. |
If you removed your project in visual studio you need to delete the files from the project you added and then delete obj/bin folders in Sample. After doing that try building sample project again. If it fails again build again. It should work then. |
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.
Looks very good. I like that you are using an options class. But I would prefer to have one option instead of three that do the same thing.
src/CommunityToolkit.Maui.MediaElement/Views/MauiMediaElement.android.cs
Outdated
Show resolved
Hide resolved
Tangential to off topic, but just in case anyone else experiences the bug I was describing with the samples project, for documentation, it was due to Android AAPT2 not supporting long files in the same way that Windows can. When I had this project nested in a deep folder structure, plus the long file names like "drawable-hdpi-v4_common_google_signin_btn_icon_dark_normal_background.9.png" it was exceeding AAPT2's max file length and breaking. Solution was just to shorten the file path to the project and I can now build the sample project okay. |
I think you are correct. I have deleted the extra two constructors for
It is probably best to avoid clutter or bloat where it is not necessary and this better matches the other builder syntax so looks more visually consistent and keeps the code more streamlined. Thanks. |
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
#2526
#1891
#1773
#1610
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.