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

A few video in a list overlap each other #1107

Open
1 task done
kotya341 opened this issue Feb 19, 2024 · 15 comments
Open
1 task done

A few video in a list overlap each other #1107

kotya341 opened this issue Feb 19, 2024 · 15 comments
Assignees

Comments

@kotya341
Copy link

kotya341 commented Feb 19, 2024

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

Screen_recording_20240219_142945.mp4

Bug Report

@oceanjules
Copy link
Contributor

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
Copy link
Author

kotya341 commented Feb 19, 2024

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
Copy link
Contributor

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

@kotya341
Copy link
Author

sorry my bad, I just changed settings

@oceanjules
Copy link
Contributor

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

  • MyStyledPlayerView is 380dp, just like you are setting

Screenshot 2024-02-20 at 15 28 02

  • Box and VideoPlayer are 420dp, hence the 40dp green background difference

Screenshot 2024-02-20 at 15 28 41

  • AspectRatioFrameLayout is almost 700dp which explains the overlap
    700 = 120 (out of the box) + 40 (green) + 380 (centrally aligned player) + 160 (out of the box, overlaps with a Spacer)

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:

  • VideoPlayer(Video, Modifier) IS a BOX that contains VideoPlayer(Player, Modifier, resizeMode) that IS a BOX. Let the outer Box not have the set height, then you won't have to override onMeasure() with 380dp.

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
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

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.

@oceanjules oceanjules closed this as not planned Won't fix, can't repro, duplicate, stale Feb 27, 2024
@kotya341
Copy link
Author

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 :)

@androidx androidx locked and limited conversation to collaborators Apr 28, 2024
@oceanjules
Copy link
Contributor

Duplicate of #1237
Tracking internal bug: b/334901521

@icbaker
Copy link
Collaborator

icbaker commented Jul 29, 2024

Re-opening and un-duping based on #1237 (comment)

@oceanjules
Copy link
Contributor

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

@androidx androidx unlocked this conversation Jul 30, 2024
@kotya341
Copy link
Author

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
Copy link
Author

kotya341 commented Aug 8, 2024

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

@freeboub
Copy link

freeboub commented Oct 5, 2024

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.

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

No branches or pull requests

5 participants