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

Port to Python 3 #36

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Port to Python 3 #36

wants to merge 17 commits into from

Conversation

FichteFoll
Copy link

@FichteFoll FichteFoll commented Oct 1, 2019

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:

  1. Python 2 support is removed, because unicode/str is a hassle.
  2. Please verify the macOS installation instructions.
  3. I didn't actually run this besides the tests, because I never used it. This was requested by people I know.
  4. I made slight style adjustments to satisfy a very basic flake8 config (file included), most notably adding whitespace around operators.
  5. No idea whether this will work on Travis CI for all specified versions (let's see what it does for this PR). Solved.
  6. Now uses setuptools, which is the recommended tool by the PyPA.
  7. Also uses a proper package structure and relative imports instead of having all source files in the root.
  8. I did not attempt to build a Windows binary, nor do I plan to.

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.

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.
@ndrwy
Copy link

ndrwy commented Oct 15, 2019

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)

@DeadSix27
Copy link

He forgot to decode the byte strings or add the byte prefix.

@FichteFoll
Copy link
Author

FichteFoll commented Oct 16, 2019

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?)

@DeadSix27
Copy link

DeadSix27 commented Oct 16, 2019

if chunk.getname() == 'fmt ':

chunk.getname() returns bytes so every comparison has to be updated to compare against bytes not string.
Or you create a variable of chunk.getname() and decode that then use that to compare.

(That is why it returns the File does not start with RIFF id error, the header checks fail due to the wrong comparison)

@FichteFoll
Copy link
Author

Thanks for the pointer. 8573e31 should fix this.

@al3xtjames
Copy link

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```

@Subarashii-no-Fansub
Copy link

Hello, it's maybe an extra-fix, but can you deal in this port with error like

  File "sushi/__main__.py", line 139, in parse_args_and_run
    run(args)
  File "sushi/__init__.py", line 635, in run
    calculate_shifts(src_stream, dst_stream, search_groups,
  File "sushi/__init__.py", line 430, in calculate_shifts
    shift = new_time - original_time
TypeError: unsupported operand type(s) for -: 'NoneType' and 'float'

(like issues #33, #32)

Thanks

@FichteFoll
Copy link
Author

FichteFoll commented Apr 25, 2020

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.
@FichteFoll
Copy link
Author

I published my fork under the name sushi-sub on pypi, so you can install it with pip, since @tp7 seems to be AWOL.

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 master branch. I'm also looking for a tester on Windows to see if it can actually be used there and whether my installation intructions are correct.

@tp7
Copy link
Owner

tp7 commented Jul 19, 2020

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.

@FichteFoll
Copy link
Author

FichteFoll commented Jul 21, 2020

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.

@DjangoDurano
Copy link

process = subprocess.Popen(['ffmpeg', '-hide_banner', '-i', path], stderr=subprocess.PIPE, universal_newlines=True)

universal_newlines use in windows per default "Windows-1252" encoding-page.

This ends in UnicodeError if there are some special characters in the file.

I delete the option universal_newlines=True and changed in Line 24 return err to return err.decode('utf-8').

This works for me. This also should work in linux.
Maybe there is another better way to fix this.

@FichteFoll
Copy link
Author

Fixed and included in 0.6.2, thanks for the report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants