-
Notifications
You must be signed in to change notification settings - Fork 42
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
Add Cython and Password Support #21
Conversation
This combines the password and Cython PRs into one.
@Theelgirl you forget about README.md:
But one needs a ffmpeg executable for this to work, anyway, so you should explicitly say it in there, may be? |
So, I tried patch-1 on Win8 machine. It seems it compiled ok:
But when I tried to use it:
|
Also, it seems I cannot uninstall it:
|
Ok, I'll try and find a way to install cython without distutils. However, the "ImportError: cannot import name 'main' from 'fvid'" shouldn't be coming from cython, cython doesn't change anything there. Also, try compiling the cython directly with |
Here is a log:
Archive with built files: |
That log looks like it worked. Try building this new version from scratch now and let me know how it works. Edit: Windows is the root of all evil. |
lol You forget about a comma before The log:
I don't work either:
New build files in the directory: |
Try changing main.py to import sys
from fvid.fvid import main
if __name__ == '__main__':
sys.exit(main()) Edit: No, that's dumb don't do that. |
@dobrosketchkun Try now? I figure it could have been a conflict with the package name and file name on Windows, as it works fine on Ubuntu. |
Well, actually, I just went to another machine with Win10 and tried as it is and it's doesn't work, so I change main.py in |
Should I try your new commits? |
Try the new commits, because the one I told you to do doesn't work on my Ubuntu machine, this might work on both systems. |
New commits:
maybe we just should check with the system with os module and import what we need in each case? |
Did you rename fvid.py to fvid_python.py? I'll try and add detection of the os in main.py and adjust accordingly if this doesn't work. |
Ehm, no I did not. I installed it as it is. Anyway, the result is the same:
|
Ok, I'll update main.py with detection of os now. |
@dobrosketchkun Try now on windows, it may need to be tested on MacOS also (@AlfredoSequeida) |
@dobrosketchkun I just tried running this on 3.8.6, and I installed pycrypto, but I'm getting this error:
I dont know why it suddenly stopped working, but I checked the github and it hasn't been updated since 2014. Shouldn't we be using a newer module that's actively updated for security and maintainability and such? Edit: I realized pycryptodome will work drop-in instead, so I'll add that to setup.py |
@AlfredoSequeida any news? |
@dobrosketchkun sorry, I'm really busy lately with school work. |
@AlfredoSequeida Do you think you can give one of us pushing and releasing permissions? |
For good for all of us, it better be @Theelgirl because I know very little about pulls, pushes, and branches. |
Ah, I'm really bad with branches but I can manage pulls and pushes if needed. However, this would also need to be released to PyPi on important updates, so if he wants to hedge any minute risk of one of us going rogue, he could let me manage PRs/pushes, and you manage PyPi releases. |
@Theelgirl Yes! I've been pondering this for a while because I see you guys are so active. And I was actually going to ask you about it. Let me look into how to get that done and I would be glad to see what you do with it. |
Here's a useful link for PyPi: |
@Theelgirl I sent you a request to add you to the project. If you wish to accept it I would be happy to have you manage it until I have more time to work on it. And I can definitely try to fit some time in when we need to update the PyPi package. Let me know what you think. Also thank you for taking interest in the project, I'm glad you like it enough to contribute the amount you have so far. |
Thanks for the invitation! I actually found this project off a Reddit link from the Python Discord, and I just happened to be using Pillow and Cython extensively, so I figured I'd drop in and help make a few optimizations, but I didn't initially expect to end up contributing as much as I have. However, even though I didn't initially plan for it, I'd love to help continue development and optimize this project, so I accepted the invitation! |
Removing a python call to all(), which forced conversion of a ctuple to a python tuple, and removing white_diff and black_diff ctuple creation because I can fit all of their use cases into a single if statement.
Edit: I did a dumb and measured the pure python performance with my line profiler on, and Cython without it on, Cython actually only gives a 8-9x boost compared to the current python version, and a 3-4x boost compared to a speedier python fvid that I'm about to push :( |
More than doubled the python decoding speed for get_bits_from_image, and sped up ffmpeg about 33-50% by tuning for zerolatency.
@dobrosketchkun Can you test my most recent changes to fvid on Windows and/or OS X? These work on Ubuntu 20.04, and if they work on both of those platforms, I can merge it. |
Yeah, it's working quite well and fast. But. I tried to check, for once,
Then I tried 30, and there was another error:
The same was for This is the same for Win and Arch. |
I tried on one file, and it is working from default up to Maybe the deal is about codec and its quality settings? |
Btw without the |
Hm ok, so I'll remove the tune flag and squash merge it. Thanks for testing! |
Password commits make it incompatible with 0.0.2 and 0.0.1.
This combines the password and Cython PRs into one.