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

fix(present): black video flashes and other bugs linked to Qt (original PR) #464

Merged
merged 3 commits into from
Aug 26, 2024

Conversation

PeculiarProgrammer
Copy link
Contributor

This pull request fixes the screen going black after the video ends in versions of PySide6 v6.5.3 and newer. This allows for other errors (like #315) that require a newer version of PySide6 to also be fixed.

Fixes Issue

Closes #315

Description

I was attempting to get manim-slides to work with the new versions of PySide6, as to stop the Device loss detected in Present() error, and found that at the end of the video, a frame of size (-1, -1) would attempt to appear. So, to prevent this, I added a requirement that the frame should only be displayed if it is the same size as the frame before it. Upon adding, the video no longer disappeared once it ended.

Check List

Check all the applicable boxes:

  • I understand that my contributions needs to pass the checks;
  • If I created new functions / methods, I documented them and add type hints;
  • If I modified already existing code, I updated the documentation accordingly;
  • The title of my pull request is a short description of the requested changes.

PeculiarProgrammer and others added 2 commits August 23, 2024 17:05
This commit fixes the screen going black after the video ends in versions of PySide6 v6.5.3 and newer. This allows for other errors (like jeertmans#315) that require a newer version of PySide6 to also be fixed.
Copy link

codecov bot commented Aug 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.54%. Comparing base (c3e1aa0) to head (3b23c77).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #464      +/-   ##
==========================================
+ Coverage   80.41%   80.54%   +0.12%     
==========================================
  Files          22       22              
  Lines        1828     1835       +7     
==========================================
+ Hits         1470     1478       +8     
+ Misses        358      357       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jeertmans
Copy link
Owner

Hi @PeculiarProgrammer, thanks for your great work! I would be very happy to have a solution to this problem :)

Small question for now: why not just check if the size is not (-1,-1)? Checking if the frame size changes might not be compatible with manual window resizing, no?

@jeertmans jeertmans added bug Something isn't working present Related to the main "present" feature qt Related to Qt (or its Python binding) labels Aug 24, 2024
@PeculiarProgrammer
Copy link
Contributor Author

I was considering making it check for (-1, -1), however, I thought that it was possible for frames of other sizes to also be dead. It could just be unnecessary future-proofing, but at the worst, it would mean one frame is skipped if the frame changes size. Plus, I have found, at least in my testing, that the frame size stays constant despite adjusting the size of the window.

While testing to see if the frame size changes with window size changes, I also found that dead frames have a format of Invalid, while normal frames have that of NV12. Implementing a check to make sure the frame is not of format Invalid is probably optimal then. I'm not by my computer now, but I'll make that change once I am.

@jeertmans
Copy link
Owner

Thanks for your analysis! I think I would rather prefer checking for the format validity then, rather for the size as it does not convey any meaningful information (or is probably a consequence of the format being good or not).

@PeculiarProgrammer
Copy link
Contributor Author

I swapped the reliance to be on pixel format, so it should be good now.

@jeertmans jeertmans linked an issue Aug 26, 2024 that may be closed by this pull request
@jeertmans jeertmans merged commit e6df908 into jeertmans:main Aug 26, 2024
2 of 3 checks passed
@jeertmans
Copy link
Owner

Oops, I didn't want to push on my main branch... trying to fix that asap

@jeertmans
Copy link
Owner

Sorry but I could not re-open your PR, especially as I have no more write access to your repository :/

I re-use your commits and opened a new PR, see #464.

Note: next time, please use a separate branch (instead of main) for pull-requests, as it makes it harder for me to make changes.

@jeertmans jeertmans changed the title Fix for #293 and similar errors fix(present): black video flashes and other bugs linked to Qt (original PR) Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working present Related to the main "present" feature qt Related to Qt (or its Python binding)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Device loss detected in Present() [BUG] Slides go black when video finishes
2 participants