damus-io / damus

iOS nostr client
GNU General Public License v3.0
2k stars 289 forks source link

Damus Video Player (f/k/a Bug: Media Control disappeared in iOS18) #2458

Open tkhumush opened 2 months ago

tkhumush commented 2 months ago

What happens Video playback on iOS18 no longer shows any media control.

What I expect to happen I expect to see a play, pause, 15 +/-, seek bar, and airplay.

Link to noteID, npub N/A

Screenshots/video recording image

Versions Damus version: 1.10 (8) Operating system version: iOS18 Release Candidate. Device: e.g. iPhone 14 Pro mac

Steps To Reproduce Click in any video on iOS18.

Additional context

iOS18 is being release very soon.


Remaining issues

1.11(4) experimental build

alltheseas commented 2 months ago

Whats the noteID?

tkhumush commented 2 months ago

Here is a note.

note1qqqrwcpeckskkwgrjcwxrra9s4ax7m0mvzy3x83sptdq62k7022qr9vdvv

Though this is not note specific.

jb55 commented 2 months ago

I've noticed this as well, can't fullscreen

alltheseas commented 2 months ago

I've noticed this as well, can't fullscreen

One more data point: noticed same behavior on nostur as well (I am assuming nostur had full screen prior), as I've locked myself out of damus

cc @fabianfabian

danieldaquino commented 1 month ago

This is still an issue on the official (non-beta) iOS 18.0 release

Device: iPhone 13 mini (physical device) iOS: 18.0 Damus: Current tip of 1.10 dfa72fceb10dc0cd6867870acfc1f2b69c890722

danieldaquino commented 1 month ago

Ok, I haven't found a fix for this yet, but I did a lot of testing and digging and got much closer to the root cause, and there are some different ways we can fix this.

The thing is that, historically, TabView with .tabViewStyle(.page) causes video controls to become hidden. This is yet another SwiftUI quirk — which we have been living with for years.

The thing is, there used to be a hack around this, which is described here: https://stackoverflow.com/a/74401288 We use this hack, and it was working for a long time. But now this hack sadly no longer works on iOS 18.

Here is a minimally reproducible code to demonstrate this:

import SwiftUI
import AVFoundation
import AVKit

struct VideoView: View {
    @State var player = AVPlayer(url: URL(string: "https://www.w3schools.com/html/mov_bbb.mp4")!)
    @State var player2 = AVPlayer(url: URL(string: "https://www.w3schools.com/html/mov_bbb.mp4")!)

    var body: some View {
        TabView {
            DamusAVPlayerView(
                player: player,
                controller: AVPlayerViewController.init(),
                show_playback_controls: true
            )
            VideoPlayer(player: player2)
                .frame(width: 320, height: 180, alignment: .center)
                .clipped()
            Text("hi")
        }.tabViewStyle(.page)
    }
}

struct VideoViewWithNonPageTabBar: View {
    @State var player = AVPlayer(url: URL(string: "https://www.w3schools.com/html/mov_bbb.mp4")!)
    @State var player2 = AVPlayer(url: URL(string: "https://www.w3schools.com/html/mov_bbb.mp4")!)

    var body: some View {
        TabView {
            DamusAVPlayerView(
                player: player,
                controller: AVPlayerViewController.init(),
                show_playback_controls: true
            )
            .clipped() // The SwiftUI hack that no longer works
            VideoPlayer(player: player2)
                .frame(width: 320, height: 180, alignment: .center)
                .clipped()
            Text("test page")
        }
    }
}

struct DamusAVPlayerView: UIViewControllerRepresentable {

    let player: AVPlayer
    var controller: AVPlayerViewController
    let show_playback_controls: Bool

    func makeUIViewController(context: Context) -> AVPlayerViewController {

        return self.controller
    }

    func updateUIViewController(_ uiViewController: AVPlayerViewController, context: Context) {
        if uiViewController.player == nil {
            uiViewController.player = player
            player.play()
        }
        self.controller.showsPlaybackControls = show_playback_controls
    }

    static func dismantleUIViewController(_ uiViewController: AVPlayerViewController, coordinator: ()) {
        uiViewController.player?.pause()
        uiViewController.player = nil
    }
}

#Preview {
    VStack {
        VideoView()
        VideoViewWithNonPageTabBar()
    }
}

Possible solutions:

  1. Build custom video controls
  2. Use something else other than a page tab bar on the full screen carousel view (Maybe an equivalent implementation of it)
    • I am thinking of making it so that I apply some arrow buttons and page indicator at the top. There would be no swipe though.
    • Or maybe a scroll view?
  3. Keep looking for another hack

@jb55, @alltheseas, two questions:

danieldaquino commented 1 month ago
  • Should this be a blocker for the 1.10 release?

@jb55, @alltheseas, in this decision it is worth bearing in mind that this issue is also present on the current app store release (1.9.1) on iOS 18, so releasing 1.10 without this fix does not make the app store version "worse".

jb55 commented 1 month ago

let's fix this after 1.10. it's annoying but this is going to hold up the release for too long.

alltheseas commented 1 month ago

let's fix this after 1.10. it's annoying but this is going to hold up the release for too long.

Sned

https://media.tenor.com/MWTaw30tPWYAAAAC/cbz.gif

alltheseas commented 1 month ago

@ericholguin advised custom video controls/player may be way to go. Tradeoff is more work to implement.

@danieldaquino supports @ericholguin view in order to improve UX.

alltheseas commented 1 month ago

added to 1.11 milestone in view of improving customer UX

danieldaquino commented 1 month ago

@alltheseas @robagreda I tagged this issue with the "design" label because since we can't use the normal iOS video controls on the full screen carousel anymore, this could be an opportunity to make a better custom UX/UI design and reimagine what this could look like

robagreda commented 1 month ago

good, this is great opportunity to fix the design, I am glad we can add custom controls/icons

alltheseas commented 1 month ago

one customer request: video scrubber for moving forward, and backward in time

alltheseas commented 1 month ago

Via twtr https://github.com/user-attachments/assets/593ca50b-b243-4dc0-bd42-5b614f753c1a

robagreda commented 1 month ago

do we wanna replicate the user behaviour by having the actions to repost, like, share, comment etc on the video?

robagreda commented 1 month ago

one customer request: video scrubber for moving forward, and backward in time

We can also add double tap to back/for-ward 5s,10s?

jb55 commented 1 month ago

On Mon, Sep 23, 2024 at 10:30:56AM -0700, Daniel D’Aquino wrote:

@alltheseas @robagreda I tagged this issue with the "design" label because since we can't use the normal iOS video controls on the full screen carousel anymore, this could be an opportunity to make a better custom UX/UI design and reimagine what this could look like

if we literally just copy what twitter does that would be ideal. its pretty much the perfect ui and has really clean transitions from the timeline.

alltheseas commented 1 month ago

Report of video player issues 1.10.1: https://github.com/damus-io/damus/issues/2566

alltheseas commented 1 month ago

Timestamps in video links

https://damus.io/nevent1qqsqqvpxss2jdwyhhygxmypug7l0rz9w2l7ss5t9hqecevle00crdkcpz4mhxue69uhhwmm59ehx7um5wgh8qctjw3uszrthwden5te0dehhxtnvdakqz9thwden5te0dp5hxapwdehhxarj9ekxzmnyqys8wumn8ghj7un9d3shjtt2wqhxummnw3ezuamfwfjkgmn9wshx5uqzuswr2

alltheseas commented 3 weeks ago

moving from sprint 25 to 26 (starting oct 14 2024)

alltheseas commented 3 weeks ago

one customer request: video scrubber for moving forward, and backward in time

one more customer request for scrubber from today @danieldaquino

alltheseas commented 3 weeks ago

Twtr video options

image

alltheseas commented 3 weeks ago

future: remember my video posiition: https://github.com/damus-io/damus/issues/2078

alltheseas commented 2 weeks ago

bonus points save video: https://github.com/damus-io/damus/issues/1370

danieldaquino commented 2 weeks ago

@jb55, @alltheseas, I have done some more research on the seamless transitioning. It is possible, but seems quite involved, so I am deferring it to this ticket, where I also wrote my findings — so that we can hopefully save some time when we get to work on that.

alltheseas commented 2 weeks ago

@jb55, @alltheseas, I have done some more research on the seamless transitioning. It is possible, but seems quite involved, so I am deferring it to this ticket, where I also wrote my findings — so that we can hopefully save some time when we get to work on that.

Thanks for identifying and descoping complexity in favor of fixing the basics first @danieldaquino 💪

danieldaquino commented 2 weeks ago

@jb55 @alltheseas, I uploaded an internal testflight build with the latest WIP on this, for further testing and evaluation.

Here is what to currently expect:

  1. Basic video controls implemented and working robustly
  2. Time syncing between different instances of the same video (e.g. timeline vs. full screen) working well, plus or minus roughly 5 seconds of max de-sync
  3. Landscape and portrait videos on full screen (and switching) should be working well, without unintended video switching or without leaving full screen mode
  4. Video coordination working well, videos playing and pausing on timeline as you scroll, no double-playing video, etc
  5. On full screen you should be able to hide controls by tapping the video
  6. If it is a carousel, you should be able to swipe between the videos/pictures

If there are any bugs or flakiness with the above, please let me know! I will do some testing as I use it too.

Also published the existing WIP code for this here: https://github.com/damus-io/damus/pull/2613 if needed for reference.

danieldaquino commented 2 weeks ago

Actually, there seems to be some bizarre bug with this new build, so I removed the build for now. Sorry for the inconvenience! I will upload a new one when that's sorted out

alltheseas commented 2 weeks ago

https://github.com/damus-io/damus/issues/2620

danieldaquino commented 2 weeks ago

Actually, there seems to be some bizarre bug with this new build, so I removed the build for now. Sorry for the inconvenience! I will upload a new one when that's sorted out

This issue seems to be resolved now, uploaded new internal TestFlight build

danieldaquino commented 1 week ago

@jb55 @alltheseas new experimental build dropped on internal TestFlight 1.11(5):

  1. Rebased on top of master to include other new features cc @ericholguin
  2. Fixed portrait videos not being full width on timeline, and tried to improve overall media carousel state management
  3. Fixed a gesture bug where playback controls would not disappear on tap when in full screen mode

I am trying to track remaining items to be done on the description of this ticket

danieldaquino commented 1 week ago

@jb55 you may notice a (hopefully tiny) bit of extra lag on the timeline. I need to re-add the media size caching to reduce sizing computations, (which I have removed due to a big refactor, and haven't had the time to add it back again).

danieldaquino commented 1 week ago

@jb55, @alltheseas, finally and hopefully wrapped up all of these changes! I marked the PR as ready for review: https://github.com/damus-io/damus/pull/2613

It's fairly large, but I have split it into logical commits, and extensively documented things so hopefully that makes it easier to review.

Please let me know if you have any questions or concerns. Thank you!