-
Notifications
You must be signed in to change notification settings - Fork 45
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
Port to Python 3 #36
base: master
Are you sure you want to change the base?
Port to Python 3 #36
Conversation
Ignoring line length.
I don't know how to make an entry point with distutils – it might not even be supported.
Someone please verify macOS for me because I have no clue about that. I also removed the opencv Windows binary, because that's for 2.7. It can be re-added later.
There is currently no environment on Travis with 3.7. Also fix opencv package being installed.
Thanks for this, I'm getting "sushi.common.SushiError: File does not start with RIFF id". When running the original Sushi (Python 2), it processes the subtitles without any issue. (Linux Mint 18.3 64-bit) |
He forgot to decode the byte strings or add the byte prefix. |
Could you give an example invocation so I can try to reproduce (and add a test case, maybe)? Does it happen with any files or only with specific ones? (If only with specific, could you upload them somewhere?) |
Line 40 in eb359e0
(That is why it returns the |
Thanks for the pointer. 8573e31 should fix this. |
One minor fix is needed (otherwise it appears to work OK): diff --git a/sushi/__init__.py b/sushi/__init__.py
index 524aaff..7847ea8 100644
--- a/sushi/__init__.py
+++ b/sushi/__init__.py
@@ -405,7 +405,7 @@ def calculate_shifts(src_stream, dst_stream, groups_list, normal_window, max_win
idx += 1
continue
- left_audio_half, right_audio_half = np.split(tv_audio, [len(tv_audio[0]) / 2], axis=1)
+ left_audio_half, right_audio_half = np.split(tv_audio, [len(tv_audio[0]) // 2], axis=1)
right_half_offset = len(left_audio_half[0]) / float(src_stream.sample_rate)
terminate = False
# searching from last committed shift``` |
Co-Authored-By: al3xtjames <[email protected]>
Hello, it's maybe an extra-fix, but can you deal in this port with error like
Thanks |
I don't fully grasp the issue, but it seems it appears when the target audio is shorter than the source subtitles due to missing scenes. It's really not an issue I'm trying to solve here and should be fixed elsewhere (i.e. in a separate PR), imo. |
Untested, but might be enough to get it working.
I published my fork under the name https://pypi.org/project/sushi-sub/ I do not intend to maintain the package other than keeping it running, but I will accept PRs to the |
Tbh I thought about merging this PR but then I'm really against merging the styling changes alongside the py3 support - those are your own preferences and I've no idea why you decided to mix everything into a single PR. At some point I'll probably get around to cherry-pick some of the comments from here, just not right now. Separating it into a different project might be the best idea. |
If by "styling changes" you're referring to 0d28fb5 (which are really just following PEP8 outside of the line length, see 424d8e0, but I agree that my personal preference aligns with that (and most of your code does as well)), then I can understand that. I don't particularly see a strong argument not to merge the addition of a couple spaces, but that's why I made it into separate commits in the first place so they could be easily reviewed and reverted or not-cherry-picked. I can drop these two commits from the PR, but if you think there might be other things you don't agree with, I'll just wait. If you'd like to maintain the package on pypi then I would be more than happy to hand that over to as well, since I don't want to maintain a fork. I just made it usable with Python 3. |
Line 21 in eb359e0
This ends in I delete the option This works for me. This also should work in linux. |
Fixed and included in 0.6.2, thanks for the report. |
This is a port of Sushi from Python 2.7 to 3. All tests pass for me (Arch, Python 3.7), but I did not test this on any other system or version because opencv is usually only installed for your system Python version and I don't have VMs lying around. It should work down to 3.5, however, which is the latest still supported.
Things to note, in no particular order:
No idea whether this will work on Travis CI for all specified versions (let's see what it does for this PR).Solved.Please make use of this as you want. Python 2 will be EOL'd in 3 months and porting this to a more recent version wasn't that much work.