Sushi icon indicating copy to clipboard operation
Sushi copied to clipboard

Port to Python 3

Open FichteFoll opened this issue 4 years ago • 13 comments

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.

FichteFoll avatar Oct 01 '19 20:10 FichteFoll

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)

ndrwy avatar Oct 15 '19 19:10 ndrwy

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

DeadSix27 avatar Oct 15 '19 19:10 DeadSix27

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

FichteFoll avatar Oct 16 '19 01:10 FichteFoll

https://github.com/tp7/Sushi/blob/eb359e045ef4345539d9dd056e14f4bbcca90979/sushi/wav.py#L40

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)

DeadSix27 avatar Oct 16 '19 01:10 DeadSix27

Thanks for the pointer. https://github.com/tp7/Sushi/pull/36/commits/8573e31392ce97392a156023c6c07e3f06c85ccc should fix this.

FichteFoll avatar Oct 16 '19 01:10 FichteFoll

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

al3xtjames avatar Feb 01 '20 04:02 al3xtjames

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

Subarashii-no-Fansub avatar Apr 25 '20 08:04 Subarashii-no-Fansub

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.

FichteFoll avatar Apr 25 '20 18:04 FichteFoll

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.

FichteFoll avatar Jul 19 '20 12:07 FichteFoll

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.

tp7 avatar Jul 19 '20 13:07 tp7

If by "styling changes" you're referring to https://github.com/tp7/Sushi/pull/36/commits/0d28fb5a7d07592719b88bb868a727ac3ca5deb7 (which are really just following PEP8 outside of the line length, see https://github.com/tp7/Sushi/pull/36/commits/424d8e050ceeea9ac583ab5af4cf83f1f37bdca9, 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.

FichteFoll avatar Jul 21 '20 00:07 FichteFoll

https://github.com/tp7/Sushi/blob/eb359e045ef4345539d9dd056e14f4bbcca90979/sushi/demux.py#L21

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.

DjangoDurano avatar Jul 16 '22 16:07 DjangoDurano

Fixed and included in 0.6.2, thanks for the report.

FichteFoll avatar Jul 17 '22 18:07 FichteFoll