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 passing of integer arguments in Python 3.10+ #4527

Merged
merged 6 commits into from Nov 25, 2021
Merged

Conversation

ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented Nov 7, 2021

Update: Downloadable packages

Hot off the project build servers, here for testing purposes (see below) are links to OpenShot packages built off this branch. They're stored in my personal Google Drive space. When you initially follow the link, you may get a spinny icon for a while until Google decides it can't preview the file and shows you a download button. You don't have to wait for that, though — there's already a download icon at the top of the page. Whenever/however you click Download, after that you'll likely get a warning that Google can't scan the file for viruses, you'll have to click past that to actually download the file.

Linux: OpenShot-v2.6.1-dev-daily-8590-aee3a44b-73c519f0-x86_64.AppImage
Mac: OpenShot-v2.6.1-dev-daily-8590-aee3a44b-73c519f0-x86_64.dmg
I'm not actually sure Windows users will be able to easily test this (they'd need to run openshot-qt-cli.exe to see console output, and even then I'm not sure the environment setting will work correctly), but here are links anyway...
Win64: OpenShot-v2.6.1-dev-daily-8590-aee3a44b-73c519f0-x86_64.exe
Win32: OpenShot-v2.6.1-dev-daily-8590-aee3a44b-73c519f0-x86.exe

Oh, yes. Fixes #4358.

Original PR message

This is a PR I've put off submitting for too long already. Hopefully it's complete, but one of the things we really need to do is try to verify that.

The integer problem

Python 3.10 removes automatic conversion of floats to integers, when passing arguments to functions that require integer arguments. The Python developers had been warning of their plans to do that for quite some time, and we weren't really listening. But, now they've pulled that trigger, and as a result OpenShot (even 2.6.1) fails to run under Python 3.10.

That's because it turns out we unknowingly (ab)use integer promotion a lot, when calling PyQt5 functions for integer Qt classes like QSize, QPoint, QRect, QColor, and QTimer (among others). All of those require integer arguments, and under Python 3.10 throw an exception if you pass them floating-point arguments.

The (developing) solution

This PR attempts to fix the problem, by wrapping all values being passed as integer arguments to functions that require them with int(). These places were discovered by reading the code, by searching the code, and by running OpenShot under Python 3.9 with the environment variable PYTHONWARNINGS=always set.

Setting that environment variable causes Python to display a message whenever a function that takes an integer argument is passed one that isn't an integer. (Which, under 3.9, does not yet throw an exception.) Fixing those warnings gets the code ready for Python 3.10. Hopefully I've found all of them, but the only way to be sure is to use every single piece of code so that it can be confirmed not to emit any warnings. There's no way I've managed to do that.

How YOU can help

This branch has been created in the project repo, so it will shortly result in packages being built for it. When those packages are available, running them under the included Python 3.x will automatically set PYTHONWARNINGS=always in the process environment, meaning OpenShot will output messages on the console if there are any remaining uses of integer promotion. (That means OpenShot must be launched from the command line, so that it has a terminal to output to. I don't believe the warnings will make it into our logs, but perhaps they will.) If any of those messages are displayed, PLEASE report them ASAP so that they can be fixed, because under Python 3.10 they will cause program crashes.

Other fixes

This PR also fixes a few other issues:

  • The thumbnail server code was leaving filehandles dangling when it opened thumbnail files (which also causes a warning message, when PYTHONWARNINGS is set to "always"), they are now properly closed.
  • When updating transform properties, any properties missing in the project data (like origin_x/origin_y) would lead to exceptions, now it should be able to handle creating any properties it needs to.
  • getTransformMode() in the VideoWidget code is replaced with a rewritten checkTransformMode() that has much less repetitious code because it loops over the handle regions instead of checking each one in a hand-coded chain of elseifs.
  • Much of the other region-handling code is rewritten to be briefer and less repetitive. This was done by adding some variables to hold frequently-calculated values, and by arithmetically reducing equations where possible. the transform functionality all works exactly the same after these changes, which is a positive sign that I didn't screw any of them up.

@ferdnyc ferdnyc added needs testing Issue or PR needs some help testing from the community or other developers code Source code cleanup, streamlining, or style tweaks labels Nov 7, 2021
Comment on lines -116 to +151
self.topLeftHandle = QRectF(-cs/sx/2.0, -cs/sy/2.0, cs/sx, cs/sy)
self.topRightHandle = QRectF(source_width - (cs/sx) + cs/sx/2.0, -cs/sy/2.0, cs/sx, cs/sy)
self.bottomLeftHandle = QRectF(-cs/sx/2.0, source_height - (cs/sy) + cs/sy/2.0, cs/sx, cs/sy)
self.bottomRightHandle = QRectF(source_width - (cs/sx) + cs/sx/2.0, source_height - (cs/sy) + cs/sy/2.0, cs/sx, cs/sy)
self.topLeftHandle = QRectF(
-csx / 2.0, -csy / 2.0, csx, csy)
self.topRightHandle = QRectF(
source_width - csx / 2.0, -csy / 2.0, csx, csy)
self.bottomLeftHandle = QRectF(
-csx / 2.0, source_height - csy / 2.0, csx, csy)
self.bottomRightHandle = QRectF(
source_width - csx / 2.0, source_height - csy / 2.0, csx, csy)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a perfect example of that last point in my intro. I noticed early on that cs/sx and cs/sy were being computed dozens of times over and over again, so I added two variables csx and csy that held those values, respectively. Then I replaced every instance of cs/sx with csx, and the same for cs/sy => csy.

Once I'd made those changes, it was obvious that source_width - (csx) + (csx / 2.0) is the same thing as source_width - csx / 2.0, so I made that substitution. And the same for a lot of other computations.

# There's one a bit farther down that uses 
((x1*source_width+x2*source_width) / 2.0) - (cs/sx/2.0)
# That reduces to
((x1 + x2) * source_width - csx) / 2.0
# so that's what I replaced it with.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Nov 7, 2021

@jonoomph Once this is merged I think there should probably be a new OpenShot 2.6.2 release for Python 3.10 compatibility. Maybe, if I manage to get the JUCE update together quickly enough, including that as well.

@jonoomph
Copy link
Member

@ferdnyc This looks good to me, and it makes sense! However, it's a very difficult one to review (as a lot of subtle changes are in here). But assuming you have tested this one, and there are no obvious breakages, let's go ahead and merge away.

We'll get more testing eyeballs after merging it.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Nov 25, 2021

@jonoomph Yeah, makes sense. I was wrong about the builds auto-enabling warnings, anyway, that'd require a launch script because doing it from inside Python code is too late. So, it takes a fair amount of effort to test this, unfortunately.

Meanwhile, I've updated to Fedora 35 which includes Python 3.10, so I keep having to merge this branch into all of my others just so I can test them locally!

I fixed a couple last things and removed the attempts to set PYTHONWARNINGS uselessly, when CI is done chewing on it I'll look it over one last time and merge.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Nov 25, 2021

Hopefully this'll solve Python 3.10 fully. Time will tell...

@ferdnyc ferdnyc merged commit fff785e into develop Nov 25, 2021
@ferdnyc ferdnyc deleted the py310-numeric-types branch November 25, 2021 19:38
@jonoomph
Copy link
Member

jonoomph commented Dec 8, 2021

@ferdnyc Regression found: the "Tracker" effect is broken by this PR. When you drop the Tracker effect, and attempt to draw a rectangle in the video_widget, it fails to draw. I'm looking at this now, but some of the changes to video_widget are a bit tricky to see clearly.

@jonoomph
Copy link
Member

jonoomph commented Dec 8, 2021

Looks like zooming into the video_widget is also broken by this PR. Oops.

@jonoomph
Copy link
Member

jonoomph commented Dec 8, 2021

I'm leaning towards reverting this entire PR, as it's so complex, I'll need to go back over every single line of it, and possibly need to refactor portions of it. Unless @ferdnyc can see a quick fix, lol.

@jonoomph
Copy link
Member

jonoomph commented Dec 8, 2021

Found the issues, will be merging a revert of just the tiny sections causing the bugs: #4583

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code Source code cleanup, streamlining, or style tweaks needs testing Issue or PR needs some help testing from the community or other developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

openshot freezes on startup
2 participants