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

Black window if video is paused and toggled out of PiP #4268

Closed
1 task done
svobs opened this issue Mar 13, 2023 · 18 comments
Closed
1 task done

Black window if video is paused and toggled out of PiP #4268

svobs opened this issue Mar 13, 2023 · 18 comments

Comments

@svobs
Copy link
Contributor

svobs commented Mar 13, 2023

System and IINA version:

Steps to reproduce:

  1. Enable UI > Picture-in-Picture > Toggle Picture-in-Picture by minimizing/un-minimizing the window. The other PiP settings don't seem to make a difference.
  2. Open a video and start playing. Pause.
  3. Enter Picture-in-Picture.
  4. Wait a second to make sure it's settled.
  5. Click the Toggle PiP button inside the PiP window to toggle out of PiP.

Expected behavior:

Assuming Accessibility > Reduce motion is disabled in System Settings, smooth scaling animation into and out of PiP, displaying the content of the video with no breaks.

Actual behavior:

There is some flickering when coming out of PiP and the main window is displayed a bit too early, and when animation is done, the main video window is black (specifically, the VideoView appears to be hidden).

Bonus issue: Reduce motion does not appear to be honored when enabled.

  • MPV does not have this problem.

How often does this happen?

Always

@svobs svobs changed the title Black video if video is paused and toggling in and out of PiP Black video if video is paused and toggling out of PiP Mar 13, 2023
@svobs svobs changed the title Black video if video is paused and toggling out of PiP Black window if video is paused and toggling out of PiP Mar 13, 2023
@low-batt
Copy link
Contributor

The change to shutdown the display link while the video is paused in order to not waste energy has caused a number of problems. I'm working on that issue right now. That might be the root cause of this issue. I will look into it.

On Reduce motion not being honored, I'll double check, but I'm pretty certain the animation is coming from Apple's code.

@low-batt low-batt self-assigned this Mar 13, 2023
@svobs
Copy link
Contributor Author

svobs commented Mar 14, 2023

On Reduce motion not being honored, I'll double check, but I'm pretty certain the animation is coming from Apple's code.

I've found that you sometimes need to check for AccessibilityPreferences.motionReductionEnabled, and there is a way to make a call to disable implicit animations. But...I'm unable to figure out where the actual resize is occurring. It looks like it might be inside PIPViewController.presentAsPicture(inPicture: pipVideo)? Is that in Objective-C code somewhere...?

@svobs svobs changed the title Black window if video is paused and toggling out of PiP Black window if video is paused and toggled out of PiP Mar 14, 2023
@low-batt
Copy link
Contributor

The class AccessibilityPreferences was added by me. There are definitely still some places in IINA where AppKit provides an animate parameter that should be keying off the Reduce motion setting.

I would expect we need to probe what MainWindowController.enterPIP is doing. I vaguely remember being told the PiP stuff is tricky and used undocumented methods.

I see a some references to animation in PIPViewController.h a source from the project PiPHack, a PiP proof of concept. Possibly something can be learned from that project.

There is also sliding animations in Settings caused by disclosure triangles. I remember reading something about how to control that, but have not had time to look into it.

By the way, if I am non-responsive for a while it may be due to network access and/or power being cutoff by a Winter storm. Heavy wet snow is predicted starting tomorrow.

@low-batt
Copy link
Contributor

Reproduced.

@svobs
Copy link
Contributor Author

svobs commented Mar 15, 2023

The class AccessibilityPreferences was added by me.

Oh ha, didn't notice that! Nice.

I would expect we need to probe what MainWindowController.enterPIP is doing. I vaguely remember being told the PiP stuff is tricky and used undocumented methods.

I will look into this more at some point, but I never use PiP and so it's a bit low-priority for me. I don't know why it resorted to a hack. I've been playing around with MacOS animation more and more and it's ugly but it's not crazy-complicated...

Regarding the pause issue, from my experience with OpenGL (many years ago) it's likely writing to a separate buffer than the other views when it does the drawing, so if you give it any chance to drop the data (like maybe when detaching it from a view, or sending it off screen) it might discard it without warning. I've been thinking it might be a good idea to basically capture a screenshot the video when it pauses and display that in a separate layer which is above the video. It's apparently standard practice in Cocoa development to do things like this. Older examples mention bitmapImageRepForCachingDisplayInRect and it's unclear whether it still works, but I also see CGWindowListCreateImage , If done properly it could be very fast and wouldn't be noticeable to the user, and in fact might help hide issues like flickering, and avoid hacky stuff like calling videoLayer.draw() pendingRedrawsAfterEnteringPIP number of times.

But so many things to work on. Hopefully you can find a simpler solution. But these redraw issues turn out to be a whack-a-mole kind of situation, it might be worth thinking about.

@low-batt
Copy link
Contributor

Analysis

Fixing this regression requires fixing two problems.

First the fix in PR #4249 for issue #4055 must be applied to correct a regression in drawing.

Once drawing is corrected then there is a problem with commits from PR #3973 as mentioned here that must be addressed.

Adding logging showed that VideoView.layout is not always called twice when entering PiP. At least that is what testing on my MacBookPro18,2 under macOS Ventura 13.2.1 showed. This leaves VideoView.pendingRedrawsAfterEnteringPIP set to 1. When layout is called when exiting PiP the code in VideoView.layout is calling videoLayer.draw(forced: true) which results in a black screen.

As a quick test I added this line to MainWindowController.prepareForPIPClosure:

videoView.pendingRedrawsAfterEnteringPIP = 0

In branch fix-4055 which contains the changes in PR #4249 to correct drawing. With that line added the code to force drawing in VideoView.layout is not executed when exiting PiP and the screen displays the frame the video is paused on.

While working on this I noticed this comment about pipShouldClose:

// it seems the system doesn't call this function since macOS 10.15
- (BOOL)pipShouldClose:(PIPViewController *)pip __OSX_AVAILABLE_STARTING(__MAC_10_12,__IPHONE_NA);
// instead this is added in macOS 10.15
- (void)pipWillClose:(PIPViewController *)pip __OSX_AVAILABLE_STARTING(__MAC_10_12,__IPHONE_NA);
- (void)pipDidClose:(PIPViewController *)pip __OSX_AVAILABLE_STARTING(__MAC_10_12,__IPHONE_NA);

Is incorrect. This debug logging I added as a test shows both pipShouldClose and pipWillClose being called:

15:12:58.144 [iina][d] ##### pipShouldClose
15:12:58.144 [iina][d] ######### prepareForPIPClosure
15:12:58.145 [iina][d] ##### pipWillClose
15:12:58.146 [iina][d] ######### prepareForPIPClosure
15:12:58.400 [iina][d] ##### pipDidClose

At least that is what is happening under macOS Ventura.

Need to think on this some more as to whether clearing pendingRedrawsAfterEnteringPIP in prepareForPIPClosure is the correct fix. I'm thinking that might be too late. In other words is pendingRedrawsAfterEnteringPIP being non-zero after entering PiP causing other problems?

@svobs
Copy link
Contributor Author

svobs commented Mar 21, 2023

Do we have any idea why it takes 2 forced redraws after entering PiP? Such an odd number. A number like that smells like a race condition.

...Or maybe there was a race condition, and the other PRs fixed it, which means this "2 forced redraws" workaround is now breaking the code? Intuitively it seems like it would be bad forpendingRedrawsAfterEnteringPIP to linger with a non-zero value, because drawing should not be "forced" unless it has to...I think? What exactly are the side effects of a "forced" render and what distinguishes it from a non-forced render, anyway?

@low-batt
Copy link
Contributor

Those are some of the reasons why I did not post a PR with a fix and have turned by attention to fixing filter related problems. I may hand this one off to @uiryuu.

@uiryuu uiryuu assigned uiryuu and unassigned low-batt Mar 23, 2023
uiryuu added a commit that referenced this issue Mar 23, 2023
@uiryuu

This comment was marked as outdated.

@uiryuu
Copy link
Member

uiryuu commented Mar 23, 2023

I cannot reproduce the problem with #4249 applied. Am I missing anything? I've tried making Toggle Picture-in-Picture by minimizing/un-minimizing the window on and off.

@low-batt
Copy link
Contributor

I had not brought up this issue yet as I was thinking it was best to merge the pending PRs for other display related issues first and then see if this is still an issue. Let's get those changes into the develop branch and then come back to this issue.

@uiryuu
Copy link
Member

uiryuu commented Mar 26, 2023

I cannot reproduce using the latest develop build anymore.

@low-batt
Copy link
Contributor

I've been waiting for merges before testing this again. I just pulled the latest develop branch, ran a clean build, tested and the problem is still there for me. For the test I:

  • Started IINA
  • Opened a downloaded copy of Big Buck Bunny 60fps 4K - Official Blender Foundation Short Film
  • Paused playback
  • Clicked the PiP button in the OSC
  • The main window went to the dock
  • The PiP window opened and displayed the frame
  • Clicked the button in the PiP window to exit PiP
  • The main window transitioned from the dock
  • The main window displayed a black screen

This test was run on my MacBookPro18,2 under macOS Ventura 13.2.1.

I rebuilt it for Intel and took it to my MacBook Air running macOS Catalina. IINA acts slightly differently in that the main window does not minimize to the dock. When it transitions back from PiP the main window shows a frame for a moment and then goes black.

I'm confused as to why it did not reproduce for you.

@uiryuu
Copy link
Member

uiryuu commented Mar 29, 2023

It’s weird. I followed the exact steps you provided (except for the video, which I don't think that will make a difference), but it didn't reproduce for me. The behavior I got is when it transitioned back, the window will show a black frame for a moment and then the video frame come back. This has been existing for a long time if I remembered correctly.

So this must be a race condition, where sometimes the forced redraw happens first (which will lead to your behavior), sometimes the forced redraw happens latter (which will lead to mine).

@svobs
Copy link
Contributor Author

svobs commented Mar 29, 2023

I still see it too, 100% of the time. On my Macbook Pro M1 Pro, running MacOS 13.1, IINA develop ac6b964:

Screen.Recording.2023-03-28.at.22.06.14.downscaled.mov

I tried toggling OSD on and off, different OSC positions, via an external display and without an external display.

@low-batt
Copy link
Contributor

I'm experimenting with an alternative fix. At the moment looking good. Need to test some more…

low-batt added a commit that referenced this issue Mar 30, 2023
This commit will:
- Add a VideoPIPViewController class that extends PIPViewController
- This class will force drawing when entering and exiting PiP
- Change MainWindowController to use VideoPIPViewController for the PiP
  controller
- Remove the pendingRedrawsAfterEnteringPIP property from VideoView
- Remove the layout method from VideoView
- Remove code that set pendingRedrawsAfterEnteringPIP from the
  MainWindowController.enterPIP method
@low-batt
Copy link
Contributor

I've posted an alternative fix for the white window/black window PiP related problems. This is working for me under macOS 13.3 and 10.15.7.

I'm unsure of this fix. Please test it out and try and break it.

low-batt added a commit that referenced this issue Mar 30, 2023
This commit will:
- Add a VideoPIPViewController class that extends PIPViewController
- This class will force drawing when entering and exiting PiP
- Change MainWindowController to use VideoPIPViewController for the PiP
  controller
- Remove the pendingRedrawsAfterEnteringPIP property from VideoView
- Remove the layout method from VideoView
- Remove code that set pendingRedrawsAfterEnteringPIP from the
  MainWindowController.enterPIP method

Updated to address review comments and change VideoPIPViewController,
adding a private forceDraw method that only forces drawing when
required.
uiryuu pushed a commit that referenced this issue Apr 3, 2023
* Fix black window after exiting PiP, #4268

This commit will:
- Add a VideoPIPViewController class that extends PIPViewController
- This class will force drawing when entering and exiting PiP
- Change MainWindowController to use VideoPIPViewController for the PiP
  controller
- Remove the pendingRedrawsAfterEnteringPIP property from VideoView
- Remove the layout method from VideoView
- Remove code that set pendingRedrawsAfterEnteringPIP from the
  MainWindowController.enterPIP method

Updated to address review comments and change VideoPIPViewController,
adding a private forceDraw method that only forces drawing when
required.
@low-batt
Copy link
Contributor

IINA 1.3.2 contains the fix for this issue.

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.

3 participants