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 androidx-media3 video player #3857

Merged
merged 46 commits into from
Aug 10, 2023
Merged

Migrate to androidx-media3 video player #3857

merged 46 commits into from
Aug 10, 2023

Conversation

nikclayton
Copy link
Contributor

@nikclayton nikclayton commented Jul 16, 2023

Behaviour is consistent with previous player except that:

  • Swapping apps while a video is playing, and then returning to Tusky, will keep the seek position in the video instead of returning to the start
  • The controls/media description can be shown by tapping anywhere, not just on the video itself
  • The media description is on-screen for the same duration as the player controls (5 seconds here, 3 seconds in the previous code)
  • The user has options to control the playback speed
  • Rotating the device does not squash/stretch the video
  • Show the media preview when playing audio-only files

Fixes #3329, #3141, #3126, #2753, #3508, #3291

mcclure and others added 30 commits February 22, 2023 14:24
…yback works. Play, ff, rew buttons are from inconsistent versions of the product (play is circular, rew and ff aren't). Video caption flickers weirdly.
… use dash (consider rls if we add peertube later)
Previous code didn't distinguish between a down and up motion event, so the description was being shown on the down and immediately removed on the up.
This also means the player has to be recreated as necessary

- Make `player` property
- Release `player` in `onPause`
- Add initializePlayer() to do the initialisation and binding
@mcclure
Copy link
Collaborator

mcclure commented Jul 19, 2023

Tested the patch, works great. Excited to see this moving forward. Many thumbs up 👍👍👍👍

I noticed some quirks.

  1. I do notice that the "controls time"— I don't know what to call it, the block of time where the controls display atop the video before auto-hiding— is much longer with the 668e84b version of the patch. In my testing previously released versions of Tusky showed the controls for 2 seconds, and this PR it stays up for 5 seconds. This is especially noticeable and maybe even negative since in exoplayer tusky while controls are visible we show a black overlay on the video whereas this was not true before. (The black overlay may be necessary given otherwise the white controls will be invisible on videos showing white— personally I might prefer to either pick custom controls that are visible on any type of background or show a dark band rather than darkening the whole screen, but I'm not objecting at this moment to the black overlay, only that it lasts pretty long now.) I don't know if this change was on purpose but I think 5 seconds is definitely too long. For short videos, 5 seconds may span the entire video.

  2. I tried testing the Image thumbnails for audio attachments #3291 feature. I did run into a problem. If I open the https://mastodon.social/@mcc/109836577896256495 test post, it works and I see both the preview and hear the audio, BUT if I then hit "back" to return to the post or other view I was looking at before, something… bad happens. The entire screen blinks fullscreen and I am dumped out to the most recently displayed view at the top, my place in the scroll is lost. If I check logcat at this moment, I see what looks like a crash.

2023-07-19 11:05:28.614 26919-26919 AndroidRuntime          com.keylesspalace.tusky.test         E  FATAL EXCEPTION: main
                                                                                                    Process: com.keylesspalace.tusky.test, PID: 26919
                                                                                                    java.lang.RuntimeException: Unable to destroy activity {com.keylesspalace.tusky.test/com.keylesspalace.tusky.ViewMediaActivity}: java.lang.RuntimeException: Failed to call observer method
                                                                                                    	at android.app.ActivityThread.performDestroyActivity(ActivityThread.java:5397)
                                                                                                    	at android.app.ActivityThread.handleDestroyActivity(ActivityThread.java:5430)
                                                                                                    	at android.app.servertransaction.DestroyActivityItem.execute(DestroyActivityItem.java:47)
                                                                                                    	at android.app.servertransaction.ActivityTransactionItem.execute(ActivityTransactionItem.java:45)
                                                                                                    	at android.app.servertransaction.TransactionExecutor.executeLifecycleState(TransactionExecutor.java:176)
                                                                                                    	at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:97)
                                                                                                    	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2215)
                                                                                                    	at android.os.Handler.dispatchMessage(Handler.java:106)
                                                                                                    	at android.os.Looper.loopOnce(Looper.java:346)
                                                                                                    	at android.os.Looper.loop(Looper.java:475)
                                                                                                    	at android.app.ActivityThread.main(ActivityThread.java:7889)
                                                                                                    	at java.lang.reflect.Method.invoke(Native Method)
                                                                                                    	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)
                                                                                                    	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1009)
                                                                                                    Caused by: java.lang.RuntimeException: Failed to call observer method
                                                                                                    	at androidx.lifecycle.ClassesInfoCache$MethodReference.invokeCallback(ClassesInfoCache.java:232)
                                                                                                    	at androidx.lifecycle.ClassesInfoCache$CallbackInfo.invokeMethodsForEvent(ClassesInfoCache.java:199)
                                                                                                    	at androidx.lifecycle.ClassesInfoCache$CallbackInfo.invokeCallbacks(ClassesInfoCache.java:190)
                                                                                                    	at androidx.lifecycle.ReflectiveGenericLifecycleObserver.onStateChanged(ReflectiveGenericLifecycleObserver.java:40)
                                                                                                    	at androidx.lifecycle.LifecycleRegistry$ObserverWithState.dispatchEvent(LifecycleRegistry.kt:314)
                                                                                                    	at androidx.lifecycle.LifecycleRegistry.backwardPass(LifecycleRegistry.kt:266)
                                                                                                    	at androidx.lifecycle.LifecycleRegistry.sync(LifecycleRegistry.kt:283)
                                                                                                    	at androidx.lifecycle.LifecycleRegistry.moveToState(LifecycleRegistry.kt:136)
                                                                                                    	at androidx.lifecycle.LifecycleRegistry.handleLifecycleEvent(LifecycleRegistry.kt:119)
                                                                                                    	at androidx.fragment.app.Fragment.performDestroy(Fragment.java:3372)
                                                                                                    	at androidx.fragment.app.FragmentStateManager.destroy(FragmentStateManager.java:811)
                                                                                                    	at androidx.fragment.app.FragmentStateManager.moveToExpectedState(FragmentStateManager.java:346)
                                                                                                    	at androidx.fragment.app.FragmentStore.moveToExpectedState(FragmentStore.java:114)
                                                                                                    	at androidx.fragment.app.FragmentManager.moveToState(FragmentManager.java:1455)
                                                                                                    	at androidx.fragment.app.FragmentManager.dispatchStateChange(FragmentManager.java:3034)
                                                                                                    	at androidx.fragment.app.FragmentManager.dispatchDestroy(FragmentManager.java:2988)
                                                                                                    	at androidx.fragment.app.FragmentController.dispatchDestroy(FragmentController.java:346)
                                                                                                    	at androidx.fragment.app.FragmentActivity.onDestroy(FragmentActivity.java:258)
                                                                                                    	at androidx.appcompat.app.AppCompatActivity.onDestroy(AppCompatActivity.java:283)
                                                                                                    	at android.app.Activity.performDestroy(Activity.java:8315)
                                                                                                    	at android.app.Instrumentation.callActivityOnDestroy(Instrumentation.java:1365)
                                                                                                    	at android.app.ActivityThread.performDestroyActivity(ActivityThread.java:5384)
                                                                                                    	at android.app.ActivityThread.handleDestroyActivity(ActivityThread.java:5430) 
                                                                                                    	at android.app.servertransaction.DestroyActivityItem.execute(DestroyActivityItem.java:47) 
                                                                                                    	at android.app.servertransaction.ActivityTransactionItem.execute(ActivityTransactionItem.java:45) 
                                                                                                    	at android.app.servertransaction.TransactionExecutor.executeLifecycleState(TransactionExecutor.java:176) 
                                                                                                    	at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:97) 
                                                                                                    	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2215) 
                                                                                                    	at android.os.Handler.dispatchMessage(Handler.java:106) 
                                                                                                    	at android.os.Looper.loopOnce(Looper.java:346) 
                                                                                                    	at android.os.Looper.loop(Looper.java:475) 
                                                                                                    	at android.app.ActivityThread.main(ActivityThread.java:7889) 
                                                                                                    	at java.lang.reflect.Method.invoke(Native Method) 
                                                                                                    	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548) 
                                                                                                    	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1009) 
                                                                                                    Caused by: java.lang.IllegalStateException: Can't access the Fragment View's LifecycleOwner for ViewVideoFragment{9edea1a} (f0a5693b-83c0-4ec2-86fd-5f378111fad3 tag=f0) when getView() is null i.e., before onCreateView() or after onDestroyView()
                                                                                                    	at androidx.fragment.app.Fragment.getViewLifecycleOwner(Fragment.java:385)
                                                                                                    	at com.keylesspalace.tusky.util.FragmentViewBindingDelegate.getValue(ViewBindingExtensions.kt:60)
                                                                                                    	at com.keylesspalace.tusky.fragment.ViewVideoFragment.getBinding(ViewVideoFragment.kt:75)
                                                                                                    	at com.keylesspalace.tusky.fragment.ViewVideoFragment.access$getBinding(ViewVideoFragment.kt:66)
                                                                                                    	at com.keylesspalace.tusky.fragment.ViewVideoFragment$initializePlayer$2$1.onLoadCleared(ViewVideoFragment.kt:336)
2023-07-19 11:05:28.615 26919-26919 AndroidRuntime          com.keylesspalace.tusky.test         E  	at com.bumptech.glide.request.SingleRequest.clear(SingleRequest.java:337)
                                                                                                    	at com.bumptech.glide.manager.RequestTracker.clearAndRemove(RequestTracker.java:70)
                                                                                                    	at com.bumptech.glide.RequestManager.untrack(RequestManager.java:662)
                                                                                                    	at com.bumptech.glide.RequestManager.untrackOrDelegate(RequestManager.java:630)
                                                                                                    	at com.bumptech.glide.RequestManager.clear(RequestManager.java:626)
                                                                                                    	at com.bumptech.glide.RequestManager.onDestroy(RequestManager.java:373)
                                                                                                    	at com.bumptech.glide.manager.LifecycleLifecycle.onDestroy(LifecycleLifecycle.java:42)
                                                                                                    	at java.lang.reflect.Method.invoke(Native Method)
                                                                                                    	at androidx.lifecycle.ClassesInfoCache$MethodReference.invokeCallback(ClassesInfoCache.java:225)
                                                                                                    	... 34 more

This reproduces 100%.

  1. In addition, something I notice with audio-posts-with-previews is that although we show the mp3 preview when playing the mp3 (great!), when simply displaying the post (eg showing the text, or reading in a timeline) we still show only the name "ufouria.mp3" and a little paper clip. Although I don't think it's mandatory for merging this patch, it would be great if we could show a "thumbnail" with a play icon in the regular post view iff the audio post has a preview, the same way we would for the thumbnail of a proper video.

  2. I have been informally collecting videos which failed to playback in Tusky on either my phone or others' phones. When I learn about such a video I boost it as @[email protected]. In my testing, some of the videos that did not work in Tusky 21 work now, and some of the videos that did work in Tusky 21 no longer work. I found two (one, two) videos which work in 21 but not this PR, which have a single especially interesting shared failure where an actual error message is printed on the screen. The videos are both notably very low resolution.

    We probably should not expect EVERY video to work before merging this PR (although the "decoder init failed" error looks more debuggable than most), especially because some fixes may be beyond our control (problem is internal to exoplayer or video is actually corrupt). But, post-merge we should think about some way of systematically tracking / keeping a list of known "Tusky can't play" videos (even if it's an informal system, like "project members know to send a URL to mcc_test_tusky when they encounter this")

I think problem 2 is the only true showstopper here. If we either fix problem 2, or roll back 668e84b (which I assume caused problem 2), I'd say this is mergeable.

@mcclure
Copy link
Collaborator

mcclure commented Jul 19, 2023

Screenshot of above-described ~phenomena~

@nikclayton
Copy link
Contributor Author

Thanks for the detailed feedback @mcclure

  1. The "controls time" in media3 defaults to 5 seconds (https://developer.android.com/reference/androidx/media3/ui/PlayerControlView#DEFAULT_SHOW_TIMEOUT_MS())

Having the timeout for the description be different for that (especially in the face of user feedback that the existing timeout of 3 seconds is too short, e.g., #3141) didn't seem like a good idea.

The darker background is R.id.exo_controls_background (https://github.com/androidx/media/blob/release/libraries/ui/src/main/res/layout/exo_player_control_view.xml#L23-L26). While that could be shrunk in code to cover just the area taken by the controls, it's also needed to ensure that the media description text is visible.

[ Changing the video player to use a bottom sheet to show the media description the way the image viewer does is out of scope for this PR ]

  1. That's a bug, I'll investigate.

  2. Noted, saved as Show preview image for audio attachments, if there is one #3866

  3. After this PR videos that can't play should be verified as being a media3/exoplayer problem, and if they are, reported to the media3 project. They have a demo app which can be used to confirm that it's a media3/exoplayer problem. [Bug] Some videos only have audio when they first play, are muted after looping #3145 (comment) has instructions for doing that.

Note that there's a couple of different dimensions to the problem that can make this tricky to reproduce.

First, the physical hardware that the user is using.

Second, the Mastodon server the user is using. It occurred to me earlier that if you're on server A and I'm on server B and you send me a link to a post that's not working, and it works for me, that doesn't prove anything (I think), because when I view the post the media will be fetched/proxied by server B.

So if server A is misconfigured (e.g,. the Range bug, see the comment thread starting at #1310 (comment)) the video might refuse to play because of a server problem. So any time videos are collected it's important to get the video from the same server that the user is reporting the problem from.

@nikclayton
Copy link
Contributor Author

@mcclure I couldn't reproduce the crash in 2, but the stacktrace looked sufficient. Please try a version that includes 26aef1a and let me know if that resolves the problem for you.

@mcclure
Copy link
Collaborator

mcclure commented Jul 20, 2023

Activity crash bug is gone with latest commit! If I were a reviewer I would approve this branch at this point.

@mcclure
Copy link
Collaborator

mcclure commented Jul 20, 2023

After this PR videos that can't play should be verified as being a media3/exoplayer problem, and if they are, reported to the media3 project…

Yeah, that sounds right. What I am thinking is once we're on media3 we should try to set up some kind of systematic process for gathering and submitting such reports. (Yes, I am volunteering to do the organizing work on that.) You make an interesting point about the depth of information we'd need though. Maybe this would require us to move toward in-app reporting infrastructure first.

@nikclayton
Copy link
Contributor Author

What I am thinking is once we're on media3 we should try to set up some kind of systematic process for gathering and submitting such reports.

I think we probably want to be as far away from that as possible, since we're not in a position to resolve the issues.

I'm thinking of:

  1. Patching the Exoplayer demo app so that the user can paste in an arbitrary URL (at the moment it's got a hardcoded list of videos)
  2. Convince that team to include the PR
  3. Convince that team to publish the Exoplayer demo app on the Play store
  4. Update the Tusky FAQ with a section that looks something like this:
# A video in a post doesn't play

Tusky uses the Android media3 framework to play videos, so this is almost certainly an issue that they will need to resolve.

To confirm that this is not a Tusky problem:

- Install <link to exoplayer demo app>
- Open it, and include the link to the Mastodon post with the video that doesn't work
- If the video doesn't work in the demo app, open an issues with the media3 team at <link to GitHub issue page>
- If the video does work in the demo app, open a Tusky issue at <link to GitHub issue page>

@nikclayton nikclayton requested review from connyduck and mcclure and removed request for Tak, charlag and connyduck July 26, 2023 21:37
@nikclayton
Copy link
Contributor Author

@mcclure - ready for final review, thanks.

@nikclayton nikclayton self-assigned this Aug 5, 2023
@mcclure
Copy link
Collaborator

mcclure commented Aug 6, 2023

LGTM.

I tested and the prior crashes with audio I saw are indeed fixed.

I clicked the "Review" button and it didn't do anything.

@nikclayton nikclayton merged commit 8529f30 into tuskyapp:develop Aug 10, 2023
1 of 3 checks passed
@nikclayton nikclayton deleted the media3 branch August 10, 2023 17:31
mcclure added a commit that referenced this pull request Oct 24, 2023
In the new Tusky version, we switched from "old and busted" Android
video player to new "Exoplayer" (PR #3857). This introduced a dark
"curtain" covering the entire screen. This patch restores the 23.0 look.

This is done by creating a "magic" exo_player_control_view.xml override file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ogg videos display broken
3 participants