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

Migrate to Media3 #4157

Closed
parneet-guraya opened this issue Sep 12, 2023 · 13 comments · Fixed by #4177
Closed

Migrate to Media3 #4157

parneet-guraya opened this issue Sep 12, 2023 · 13 comments · Fixed by #4177

Comments

@parneet-guraya
Copy link
Contributor

Media3 is the new home for Exoplayer and it is recommended to migrate to it. Exoplayer is now the part of this library.

Apps that are currently using the standalone com.google.android.exoplayer2 library and androidx.media should migrate to androidx.media3. Use the migration script to migrate gradle build files, Java and Kotlin source files, and XML layout files from ExoPlayer 2.19.1 to AndroidX Media3 1.1.1

Let me know about this and assign this to me, I will do it.

@JuancaG05
Copy link
Collaborator

Hi @parneet-guraya! Nice to see you again over here! 😃
Thanks for opening the issue. Seems a nice improvement 👍. I encourage you to make it and send a PR so that we can review it and hopefully merge it 🚀.
There's no need to assign the issue, just develop it and whenever it's ready we'll link the PR with the issue 👌.

@parneet-guraya
Copy link
Contributor Author

Nice to see you here too 😃, Working on it 🚀.

@parneet-guraya
Copy link
Contributor Author

Hi @JuancaG05, hope you're doing fine.

So, I was working on the migration and it was successful. But I would like you to read this first nextcloud/android#11977 (comment) ignore the specific file names mentioned there). I had pushed a PR regarding migration in the nextcloud's android client and had some question which are same for migration in owncloud's client. You can also see the migration I did there If you want to see how would Immersive mode look like if you want.

  • The reason to use immersive mode was pretty much the same but instead of launching Dialog here we are using another activity to go full screen.

So, do let me know what you think about having a single activity for media playback?
Thanks!

@JuancaG05
Copy link
Collaborator

Hi @parneet-guraya! Let's see if I can reply to your questions.

About the first question: I'm not very familiarised with the Media UI concepts, what do you mean with cut-out mode? And what effect would it have in other non-media previews? Could you post some screenshots to see it more visually?

About the second question: if you were trying and couldn't get what Google Drive has, it's ok, NEVER mode looks good enough (more than SHORT_EDGES). Maybe Google Drive is using another kind of toolbar, or more likely they are using Jetpack Compose, which allows much more flexibility than XML 😄. I hope we can introduce it veeeery soon in ownCloud.

I saw the before and after videos of your PR and they look great! 💯 When you have it for ownCloud, it would be so cool if you posted some videos as well!

Thanks a lot for your contributions, we're very happy with them 😺😍

@parneet-guraya
Copy link
Contributor Author

Thanks @JuancaG05, appreciate your feedback 😄 and yes surely would put a demo of before and after.

So, In order to have Immersive mode smoothly In/out. It needs (best practices for handling display cutout) to have either SHORT_EDGES or NEVER. Since, it takes effect in the Activity only not in Fragment since that is just a part of activity. I was asking to migrate the media play back code which is right now in Fragment to Activity, that way we can specify the cutout mode (basically it describes how UI should be rendered in the notch area on devices with notches) and since we are going to have separate Activity, It would not effect other non-media previews and only what we choose to render in the Media playback Activity. So, let me know and I will push a PR about migration to activity first then I can push the Media3 migration and I'm thinking to push these two changes as two separate PR's (Activity Migration and Media3 migration) so to avoid having too much changes in a single PR, is that right?

Yes, we can go with NEVER mode. It looks good than the SHORT_EDGES (unless used the approach what G-Drive using). However in future I still try to achieve that If I could 👍 .

@JuancaG05
Copy link
Collaborator

Hi @parneet-guraya! I was taking a look and it seems that some of the media preview fragments are in Java, and others are already in Kotlin. I think it's a great opportunity to kotlinize those classes, but on the other hand I see this could be a little bit complex. If I understood well, your idea is refactoring and moving the PreviewVideoFragment.java and PreviewImageFragment.kt to a same activity (let's say PreviewMediaActivity.kt?), right?

I see these two classes have much different content between them, and maybe trying to unify them is tricky since we'll have a very big new activity, which is not the aim, the architecture we follow says it's good to separate concerns and keep things simple. The solution I can think of is creating a new activity, parallel to FileDisplayActivity.kt, which is our main activity now, but just used for the media preview cases (instead of creating new fragments, you would switch to that activity). You can set that activity to the cutout mode you prefer, but it will continue hosting fragments. In that way, we can keep using the current media preview fragments and avoid a difficult refactor.

What do you think about this proposal? 🤔 Maybe I'm missing something in the solution you suggest and that's why I see it too complex, so just let me know 😀.

BTW, after taking a look at some other apps, I see SHORT_EDGES a good option too (see YouTube for example, with videos in full screen), the problem would be the toolbar, which keeps a weird appearance. But now it's up to you, you can choose whatever you want and we'll give our input 🤩.

Thanks a lot again for your involvement with the app! 🥇

@parneet-guraya
Copy link
Contributor Author

Okay let's see, we have these files for previews -->

  • Image -> PreviewImageActivity and PreviewImageFragment
  • Audio -> PreviewAudioFragment
  • Video -> PreviewVideoFragment and PreviewVideoActivity (For Full screen)

By migration I was talking about doing it for audio and video media. So having a file PreviewMediaActivity in which I can use media player and based on the media type (audio/video) we can show the appropriate layout. For the Image (from what I can see) we have a activity already PreviewImageActivity. So If we want to handle the display cutout, we can do it too in that activity easliy. But for this I was just talking about media (audio and video) (PS: I have also seen separate activity being used for media (audio/video) in nextcloud instead of fragments.).

And yes I did see the Youtube example before and I think it is usingNEVER for Video playing view and for Shorts as well. Attaching screenshots -->

As you can see the status bar is black. So it means it's not rendering in the notch area and high chances are NEVER mode is being used here.

Status Bar is black Status Bar is black
photo_6096098386500500036_y photo_6096098386500500035_y

The main view of the app has the base colored status bar as the background of the main view. So it is rendering in the notch area. It could be they are using SHORT_EDGES Or ALWAYS basically any mode which allows rendering into notch area.

photo_6096098386500500034_y

Also can you explain me more about the having different activity for media previews. How that would work like right now we have the files list in FileDisplayActivity and depending on the file type we launch appropriate screen. How having a different activity for media previews would look like?

@JuancaG05
Copy link
Collaborator

Hey @parneet-guraya!

Yes sorry, I meant the audio fragment when I talked about the image fragment in my previous comment (ExoPlayer is used for audio and video). About the activity structure I propose, just take a look at the start...Preview methods in FileDisplayActivity.kt. startImagePreview starts the new activity dedicated to images previews, while startAudioPreview and startVideoPreview are creating the new corresponding fragment instances. The idea is that these two last methods work like the first one but instantiating the new PreviewMediaActivity.kt and thus, being able to set its cutout mode. You could also maybe get more ideas from the changes that were proposed in this issue: #3144.

And about YouTube, I meant in the full screen video mode, something like this:

PXL_20230927_130100545.TS.mp4

In this case we can see that the notch area is being used to display the video as well, so it doesn't seem to be a NEVER cutout mode here but a SHORT_EDGE. That's why I told you I see both modes are used in conventional apps, so, whatever you choose will be ok 😃. BTW, I guess the audio preview is not a problem for this since we won't need an immersive mode there (nothing important to show on screen), is that right?

@parneet-guraya
Copy link
Contributor Author

About the activity structure I propose, just take a look at the start...Preview methods in FileDisplayActivity.kt. startImagePreview starts the new activity dedicated to images previews, while startAudioPreview and startVideoPreview are creating the new corresponding fragment instances. The idea is that these two last methods work like the first one but instantiating the new PreviewMediaActivity.kt and thus, being able to set its cutout mode.

So, making sure If we're on the same page. You are talking about having a PreviewMediaActivity (audio and video) and which can be launched from either of the start...preview method?

BTW, I guess the audio preview is not a problem for this since we won't need an immersive mode there (nothing important to show on screen), is that right?

And If I consider the above, yes, audio does not need to have a cutout mode (unless required to show the audio playback UI in a certain way). Primarly Video playback needs it for the going fullscreen. So, does that mean we should have a single Activity just for video (for ex: PreviewVideoActivity) and leave the audio preview PreviewAudioFragment as it is?

You could also maybe get more ideas from the changes that were proposed in this issue: #3144.
Yes, this is talking about having a single PreviewVideoFragment instead of having two files like we have currently, PreviewVideoFragment (for simple view) and PreviewVideoActivity (For Full screen). But, as discussed for having a cutout mode set, we need Activity.

Also, this is talking more about refactoring the code and having the duplicated logic in one file. Which indeed would happen if we go with Activity and implement immersive mode to have video functionality in a single place and yes kotlinization also.

@JuancaG05
Copy link
Collaborator

And If I consider the above, yes, audio does not need to have a cutout mode (unless required to show the audio playback UI in a certain way). Primarly Video playback needs it for the going fullscreen. So, does that mean we should have a single Activity just for video (for ex: PreviewVideoActivity) and leave the audio preview PreviewAudioFragment as it is?

Yes, I would go with this approach to make it simpler. Let's create a PreviewVideoActivity.kt (refactor the one we already have) and launch it from the startVideoPreview method, and for the moment we'll let the audio preview stay as it is 👍.

@parneet-guraya
Copy link
Contributor Author

Okay, great. Should I make that migration to activity in a separate PR first or keep it in the media migration?

@JuancaG05
Copy link
Collaborator

Since it is finally just one activity, I would make everything in a same PR, not too complex to separate it in 2 different PR, I think I'll be able to review it properly 👍

@parneet-guraya parneet-guraya mentioned this issue Oct 3, 2023
2 tasks
@JuancaG05 JuancaG05 self-assigned this Oct 4, 2023
@JuancaG05 JuancaG05 linked a pull request Oct 4, 2023 that will close this issue
2 tasks
@JuancaG05
Copy link
Collaborator

Some stuff affected in this PR that could be good to QA, in addition to the core feature (video play):

  • Filter for menu options in main file list is OK while previewing a video
  • If video file is removed while previewing it, go back to the main file list (app doesn't crash or behaves wrongly)
  • Menu options in the video preview are working properly

@jesmrec

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants