androidx / media

Jetpack Media3 support libraries for media use cases, including ExoPlayer, an extensible media player for Android
https://developer.android.com/media/media3
Apache License 2.0
1.73k stars 415 forks source link

A few video in a list overlap each other #1107

Open kotya341 opened 9 months ago

kotya341 commented 9 months ago

Version

Media3 1.2.1

More version details

We are designing a social media feed for our app and we are facing a problem with with our compose implementation in a feed we are using androidx.media3.exoplayer.ExoPlayer and via AndroidView( modifier = Modifier.align(Alignment.BottomCenter), factory =PlayerView(content) where I set a player per an item like this this.player = player and to fit the screen size I use AspectRatioFrameLayout.RESIZE_MODE_ZOOM
it looks like a player takes all of the player Views to show a video

as a result, you can see the same video in several list items

it looks like the same issue was reported for old exo-player, however without a working solution unfortunately

Please help me to provide the best experience to our users

Devices that reproduce the issue

Pixel 5

Reproduction steps

  1. create a lazy column list
  2. add several videos in a row
  3. start playing them one by one

Expected result

The video should take a frame size and don't interact with a neighbour item

Actual result

Current video plays in several list items

Media

https://github.com/androidx/media/assets/2707599/62880d83-fee2-4e4c-b599-c2fcd60e1d47

Bug Report

oceanjules commented 9 months ago

Thank you for your report, @kotya341

While we are still exploring the interoperability of Media3 and Compose, I could look into this issue for you if you provide a complete sample code for us to reproduce the issue.

When I played around with landscape/portrait orientation, I needed to switch between RESIZE_MODE_ZOOM and RESIZE_MODE_FIT, also use a more fine grained modifier (e.g. fillMaxSize(), fillMaxWidth(), wrapContentSize() and aspectRatio())

kotya341 commented 9 months ago

Thanks for the quick reply this is an example app that I've managed to create its a simple list, kind of instagram feed with videos

if you remove my custom frame size from here, issues is still reproducible

oceanjules commented 9 months ago

I think the repository might be private - the link navigates to 404 sadly

kotya341 commented 9 months ago

sorry my bad, I just changed settings

oceanjules commented 9 months ago

Great, thanks!

It seems like the main problem is the resize mode.

By default, PlayerView has RESIZE_MODE_FIT and if you change your default value here, it will indeed work

When you set RESIZE_MODE_ZOOM, the video is stretched such that the width is fully occupied, while the height will be whatever it needs to be to maintain the correct aspect ratio. That means the surface of AspectRatioFrameLayout is larger than the parent component (MyStyledPlayerView). Ideally, the parent component should crop the view to the boundaries of the parent, but I guess it doesn't.

To see the overlap more clearly, I added red spacers between the 3 items like:

Scaffold(modifier = Modifier.fillMaxSize()) { innerPadding ->
    LazyColumn(
        modifier = Modifier
            .padding(innerPadding)
            .fillMaxSize(),
    ) {
        items(
            count = data.size * 2,
        ) { index ->
            if (index % 2 == 0) {
                VideoPlayer(modifier = Modifier, data[index/2])
            } else {
                Spacer(modifier = Modifier.height(200.dp).fillMaxWidth().background(color = Color.Red))
            }

        }
    }
}

From the LayoutInspector, we can see that:

Screenshot 2024-02-20 at 15 27 35

Screenshot 2024-02-20 at 15 28 02

Screenshot 2024-02-20 at 15 28 41

Then if we look at the next item, the two AspectRatioFrameLayout will overlap inside the spacer, but at least not in the PlayerView component

Screenshot 2024-02-20 at 15 29 16


My conclusion is to either use FIT or remove the height of the Box (420dp). You seem to have two nested Boxes:

Also, if LazyColumn doesn't feel like the right choice, you might want to consider VerticalPager (for a video to snap to the middle), but I haven't explored it myself yet.

Let me know if I understood you and your requirements correctly!

kotya341 commented 9 months ago

Thanks a lot for such a details analysis. This is what I thought about a layout and ideally, as you said, it would be ideal to set a max height of AspectRatioFrameLayout or crop a video in some way. It looks like I have to talk to our designer to align if it is possible to exclude max cell size it would solve the problem and the video would be just bigger.

VerticalPager is the same as increasing the size of the cell, both of the solutions would lead to an increase in the size of the element.

Maybe this case worth to create a bug in MediaPlayer just to avoid this issue for the future?

kotya341 commented 9 months ago

btw I've played around with some configs for aspect ration layout and found out that if you set app:surface_type="texture_view" view looks as expected

oceanjules commented 9 months ago

When I played around with it, I also noticed that TextureView works and I raised the issue internally about the difference of SurfaceView vs TextureView clipping inside FrameLayout. We do prefer you to be using SurfaceView, so that should be followed up as a proper bug in Compose. I would suggest filing an issue in this tracker. Looks like other people are having similar problems, like here

Could I ask you how you injected the xml attribute? (I manually changed the variable in PlayerView, but I'm assuming you had to inflate a layout in a hacky way?)

On a different note, the current timeline is to aim for a more native Compose solution (no AndroidView interoperability), hence we are unlikely to try to make this work -- given that it's better to concentrate our efforts on a longer-term stable vision. I will close the issue, since it is not related to Media3 library as such.

kotya341 commented 9 months ago

you are correct about my inflation way

    AndroidView(
        modifier = modifier.fillMaxSize(),
        factory = {
        val view = LayoutInflater.from(context).inflate(R.layout.feed_list_video_view, null, false)```
            view.apply {}}

I'll create the bug anyway and hope that you will find some time to fix an issue. it would be awesome to have a fully composed view for the video player in the future and I understand that it should be a prior topic for you. Thanks for support :)

oceanjules commented 6 months ago

Duplicate of https://github.com/androidx/media/issues/1237 Tracking internal bug: b/334901521

icbaker commented 4 months ago

Re-opening and un-duping based on https://github.com/androidx/media/issues/1237#issuecomment-2256022016

oceanjules commented 4 months ago

Hi @kotya341, Given that it's been a while, let's bring each other up to date. Is it still correct that:

kotya341 commented 3 months ago

Thanks a lot for helping me with the topic. I've just tried all of your suggestions and can confirm that only with TextureView there is no problem. You can pull for updates in my sample app.

in my production app, I just use this custom inflate from xml method that is commented in my example app.

However, there are some performance issues with TextureView, it flickers when I switch between bottom tabs in the real app. it looks like this is a high-load issue for low-performance devices.

And I had to find a balance, which problem I could live with and I chose flickering :(

kotya341 commented 3 months ago

@oceanjules could you please share some updates with me? looking forward to get any reply from you

freeboub commented 1 month ago

Same issue reported on react native video implementation. I tried clipping view, but it just put white square in hidden zones and do not reveal the other video.