nova-video-player / aos-AVP

NOVA opeN sOurce Video plAyer: main repository to build them all
Apache License 2.0
3.5k stars 201 forks source link

[FEAT]: Switch from jcifs-ng to smbj to increase SMB throughput #835

Closed Pentaphon closed 1 year ago

Pentaphon commented 1 year ago

Description

A long term solution would be to switch to https://github.com/hierynomus/smbj because the dev of jcifs-ng, our current smb backend is limited to 30MB/s which is the main reason why 4K and Bluray-bitrate video has been struggling to keep up for Nova users for some time. Now, admittedly, the new configurable buffer setting has been helpful by allowing more buffer to be stored, but eventually, as videos get higher quality (8K and so forth or higher bit-rate streaming becomes a thing to keep up with faster ISP speeds) Nova will begin to hit a stumbling block due to the current 30MB/s limit that will put it as a major disadvantage.

Now, the jcifs-ng dev has stated in the aforementioned issue that multi-credit requests can possibly double our current speed but even he cannot exactly predict what improvement it will make in real world use, and that any meaningful performance increases on par with Samba would require a full re-write of jcifs-ng and implied that such a re-write will never happen.

Therefore, in order to futureproof Nova and avoid any issues in the future, I suggest we make it a high priority to switch to smbj, or whatever implementation has the highest SMB performance on Android.

Additional information

N/A

ghost commented 1 year ago

if that's the indicator the new option is definitely not working for me. Still showing smb everywhere.

For clarification sake, ticking the option in nova will make smb:// links processed by newer smbj SMB library instead of jcifs-ng one. In other words it does not require to have smbj:// links in this case.

So what Pentaphon said is not correct? Is there any way to check whether the new option actually works?

courville commented 1 year ago

@jaqarll, you can either trust me or check my code activate logs and check via adb that the right code paths are activated.

In reality, I think debug logs are still active, thus just play with the app and see w/o the option if via adb logcat you see different things (hint search for smbj).

Pentaphon commented 1 year ago

So what Pentaphon said is not correct? Is there any way to check whether the new option actually works?

If you want to be absolutely sure and see "smbj://" in the file properties on all your videos, delete your old SMB share, go to network shortcuts > browse a network share: and create a new share with the "SMBJ" option from the dropdown list, index that folder and use that from now on. It's pretty straightforward.

ghost commented 1 year ago

@jaqarll, you can either trust me or check my code activate logs and check via adb that the right code paths are activated.

In reality, I think debug logs are still active, thus just play with the app and see w/o the option if via adb logcat you see different things (hint search for smbj).

My trust in you and your app suffices.

One last question: there's two SMB options concerning SMB2 and j now, I can imagine that for someone not following this thread this could be a bit confusing + does it even make sense in an scenario to deactivate SMB2 but activate smbj? Shouldn't SMB2 be always active for the newer option? Or does the SMB2+ option override the SMB2 option anyway? Does it make sense that SMB2+ comes first in the options list and then the SMB2 option?

Just some thoughts and questions I had.

courville commented 1 year ago

@jaqarll you are right: I will deactivate the SMB2 option selection when selecting newer faster SMB2 implementation.

ghost commented 1 year ago

@jaqarll you are right: I will deactivate the SMB2 option selection when selecting newer faster SMB2 implementation.

Great! Not trying to be nitpicky, it's just easy as someone following development rather closely to understand all the new aspects of it, but there's always people who are new or just use the app as it is and they might not understand every new option by just seeing it appear.

I dearly love your app and everyone's effort being put into it, especially yours of course.

courville commented 1 year ago

Here you go: v6.2.8 available with previous suggestions implemented (options clarification and deal with empty path).

ghost commented 1 year ago

Here you go: v6.2.8 available with previous suggestions implemented (options clarification and deal with empty path).

Not sure if connected, but unlike my pixel7 my Samsung Tab A7 encounters issues playing smb now. After 5-10min the app either crashes or in most cases the video just ends and I'm back at the nova library

courville commented 1 year ago

@jaqarll, I found a stacktrace on sentry crashnalytics matching your samsung tab a7 (SM-T505):

org.bouncycastle.crypto.InvalidCipherTextException: mac check in GCM failed
    at org.bouncycastle.crypto.modes.GCMBlockCipher.doFinal
    at com.hierynomus.security.bc.BCAEADCipherFactory$BCAEADBlockCipher.doFinal(BCAEADCipherFactory.java:114)
    at com.hierynomus.smbj.connection.PacketEncryptor.decrypt(PacketEncryptor.java:74)
    at com.hierynomus.smbj.connection.packet.SMB3DecryptingPacketHandler.doHandle(SMB3DecryptingPacketHandler.java:117)
    at com.hierynomus.smbj.connection.packet.AbstractIncomingPacketHandler.handle(AbstractIncomingPacketHandler.java:27)
    at com.hierynomus.smbj.connection.Connection.handle(Connection.java:281)
    at com.hierynomus.smbj.connection.Connection.handle(Connection.java:73)
    at com.hierynomus.smbj.transport.PacketReader.readPacket(PacketReader.java:72)
    at com.hierynomus.smbj.transport.PacketReader.run(PacketReader.java:48)
    at java.lang.Thread.run(Thread.java:1012)
com.hierynomus.security.SecurityException: org.bouncycastle.crypto.InvalidCipherTextException: mac check in GCM failed
    at com.hierynomus.security.bc.BCAEADCipherFactory$BCAEADBlockCipher.doFinal(BCAEADCipherFactory.java:116)
    at com.hierynomus.smbj.connection.PacketEncryptor.decrypt(PacketEncryptor.java:74)
    at com.hierynomus.smbj.connection.packet.SMB3DecryptingPacketHandler.doHandle(SMB3DecryptingPacketHandler.java:117)
    at com.hierynomus.smbj.connection.packet.AbstractIncomingPacketHandler.handle(AbstractIncomingPacketHandler.java:27)
    at com.hierynomus.smbj.connection.Connection.handle(Connection.java:281)
    at com.hierynomus.smbj.connection.Connection.handle(Connection.java:73)
    at com.hierynomus.smbj.transport.PacketReader.readPacket(PacketReader.java:72)
    at com.hierynomus.smbj.transport.PacketReader.run(PacketReader.java:48)
    at java.lang.Thread.run(Thread.java:1012)
com.hierynomus.smbj.common.SMBRuntimeException: com.hierynomus.security.SecurityException: org.bouncycastle.crypto.InvalidCipherTextException: mac check in GCM failed
    at com.hierynomus.smbj.connection.PacketEncryptor.decrypt(PacketEncryptor.java:85)
    at com.hierynomus.smbj.connection.packet.SMB3DecryptingPacketHandler.doHandle(SMB3DecryptingPacketHandler.java:117)
    at com.hierynomus.smbj.connection.packet.AbstractIncomingPacketHandler.handle(AbstractIncomingPacketHandler.java:27)
    at com.hierynomus.smbj.connection.Connection.handle(Connection.java:281)
    at com.hierynomus.smbj.connection.Connection.handle(Connection.java:73)
    at com.hierynomus.smbj.transport.PacketReader.readPacket(PacketReader.java:72)
    at com.hierynomus.smbj.transport.PacketReader.run(PacketReader.java:48)
    at java.lang.Thread.run(Thread.java:1012)

Note to self: perhaps related to setupBouncyCastle() in CustomApplication but check as well https://github.com/hierynomus/smbj/issues/617 and https://github.com/hierynomus/smbj/issues/479.

ghost commented 1 year ago

Back again: not sure if it's connected to the newest release (6.2.8), but currently I'm unable to delete smb files and folders within the app

Edit: seemed to be connected to my smb drive, not nova. Please ignore

courville commented 1 year ago

@jaqarll thx for reporting: it will be fixed in next release.

Pentaphon commented 1 year ago

@courville v6.2.8 gives me an "Empty Folder" on Fire TV 4K Stick whereas on earlier releases my folder showed just fine. If I remove the path, it says "Todo: Credential Required". Not sure what is going on. Had to roll back to v6.2.7 and everything works perfectly again.

I use a Windows SMB share and I have the following filled in on v6.2.7 which works perfectly

SMBJ: 192.168.4.45 Port (Empty - defaults to 445 anyway) Path (my share folder) Username (name of my Windows machine on the network) Password (blank)

Same credentials do not work on with a clean install of v6.2.8 but DO work with a clean install of v6.2.7

courville commented 1 year ago

@Pentaphon thanks for reporting, will try to reproduce with same setup, the "Todo: Credential Required" provides a hint in the code path used. EDIT2: Reviewing the code, it could be linked to my attempt to fix the samsung tab a7 cyphering issue from @jaqarll. EDIT3: this cannot be it since v6.2.8 does not contain the boucycastle fix.

Pentaphon commented 1 year ago

@courville I did an experiment where I upgraded a clean install of v.6.2.7 (with only my library loaded to it and no settings changed) to v6.2.8 and I noticed that my library would still work but videos would take longer to load when playing them so I think something "broke" that negatively impacted connectivity between v6.2.7 and v6.2.8. Not sure if you can reproduce this. I'm sticking to v6.2.7 since that is working better than any version so far and you should probably consider v6.2.7 a reference point for how SMBJ should perform.

Does anybody else have a similar experience to mine?

courville commented 1 year ago

6.2.10 is out. @jaqarll could you please test this version to check that your ciphering issue is solved on your samsung tablet? @Pentaphon this version should not solve the problem you have (but I still do not know what triggered it, perhaps it is linked to empty password but I do not know what has changed in this matter).

ghost commented 1 year ago

6.2.10 is out. @jaqarll could you please test this version to check that your ciphering issue is solved on your samsung tablet?

No deleting issues as of yet, but I also did not delete much. Encountered one crash when I started a video.

courville commented 1 year ago

@Pentaphon, ok I reproduced the "empty folder" issue and fixed it on my side: please try out the 6.2.11 pre-release on your side.

Pentaphon commented 1 year ago

ok I reproduced the "empty folder" issue and fixed it on my side: please try out the 6.2.11 pre-release on your side.

Upgraded to v6.2.11 on the Onn 4K and did a clean install of v6.2.11 to the FireTV 4K and the empty folder issue is now gone after a clean install and the performance is solid after a regular upgrade. This is now the best version so far! Great work!

courville commented 1 year ago

@Pentaphon thanks for the feedback. I still can see some crashes SMBRuntimeException DiskShare has already been closed happening sometimes though I check that smbShare.isConnected()...

Pentaphon commented 1 year ago

thanks for the feedback. I still can see some crashes SMBRuntimeException DiskShare has already been closed happening sometimes though I check that smbShare.isConnected()...

Yes, I just saw a crash to home screen while browsing the "all movies" and "all TV shows" menus on v6.2.11 Not sure if it was a low memory error on the FireTV 4K or something else.

ghost commented 1 year ago

6.2.10 is out. @jaqarll could you please test this version to check that your ciphering issue is solved on your samsung tablet?

No deleting issues as of yet, but I also did not delete much. Encountered one crash when I started a video.

Still encountering random deleting issues, but it's inconsistent.

Pentaphon commented 1 year ago

Just tried connecting to my Windows share from a Motorola phone on Android 11 running v6.2.11 and whenever I "browse a network share" via SMBJ, entered the credentials I mentioned before ITT and pressed OK, I would see my folder contents for a second and then Nova would crash to home screen and I would get the Android system message "Nova keeps stopping" when it crashed. It did this about 4 times just 20 minutes ago. It stopped doing it after those crashes...for now. Might have to do with the random crashes I saw when browsing "all movies and tv" menus on my FireTV 4K.

Not sure you can see those crashes from your Sentry console, @courville

courville commented 1 year ago

@Pentaphon I see on sentry the following:

SMBApiException STATUS_BAD_NETWORK_NAME (0xc00000cc): Could not connect to \\192.168.4.45\Downloads.tbn

com.hierynomus.mssmb2.SMBApiException: STATUS_BAD_NETWORK_NAME (0xc00000cc): Could not connect to \\192.168.4.45\Downloads.tbn
    at com.hierynomus.smbj.session.Session.connectTree(Session.java:151)
    at com.hierynomus.smbj.session.Session.connectShare(Session.java:113)
    at com.archos.filecorelibrary.smbj.SmbjUtils.getSmbShare(SmbjUtils.java:127)
    at com.archos.filecorelibrary.smbj.SmbjFileEditor.exists(SmbjFileEditor.java:138)
    at com.archos.mediascraper.LocalImages.getIfAvailable(LocalImages.java:318)
    at com.archos.mediascraper.LocalImages.findPoster(LocalImages.java:156)
    at com.archos.mediascraper.LocalImages.generateLocalPoster(LocalImages.java:99)
    at com.archos.mediacenter.video.browser.ThumbnailEngineVideo.computeThumbnail(ThumbnailEngineVideo.java:238)
    at com.archos.mediacenter.utils.ThumbnailEngine$ThumbnailThread.process(ThumbnailEngine.java:397)
    at com.archos.mediacenter.utils.ThumbnailEngine$ThumbnailThread.run(ThumbnailEngine.java:431)

I will try to reproduce. @Pentaphon do you confirm that the network share name is correct? EDIT: TODO catch SMBApiException in SmbjFileEditor EDIT2: in JcifsUtils there is:

        // disable dfs makes win10 shares with ms account work
        prop.put("jcifs.smb.client.dfs.disabled", "true");

EDIT3: DFS is already set to false in smbj (opposite boolean logic than jcifs)

Pentaphon commented 1 year ago

do you confirm that the network share name is correct?

Yes the network share name is correct and I just reproduced it again with another clean install of v6.2.11 on the same Motorola device. Just got it to crash 4 times. Android system message I get right after all 4 crashes below. The crashes get even worse if I try to view a video's info with the network browser and it scrapes the video automatically.

Screenshot_20230525-020358-052

courville commented 1 year ago

@Pentaphon could you confirm that the behavior is specific to the motorola device and not experienced with the fireTV?

courville commented 1 year ago

Just pushed 6.2.12 with couple of fixes for testing (should not solve the windows issue completely).

Pentaphon commented 1 year ago

could you confirm that the behavior is specific to the motorola device and not experienced with the fireTV?

Yes, they both crashed to homescreen but the Motorola phone repeatedly crashed over and over when I "browse a network share" while the FireTV 4K would only occasionally crash when browsing the "all movies and TV" menus for a library connected via SMBJ. The Motorola phone would keep crashing even when I wiped all data from Nova and tried to "browse a network share" over SMBJ by typing in the same credentials, so it was much more reproducible.

Just pushed 6.2.12 with couple of fixes for testing (should not solve the windows issue completely).

I'm happy to report that the crashes when I "browse a network share" on the Motorola phone are now gone when I stress-tested v6.2.12 just now. I even wiped all data and tried reproducing the steps over again to see if it would crash but it did not crash even once. I will start flicking around the "all movies and TV" menus on my FireTV 4K later today to see if I still get those occasional crashes. Nice work!

courville commented 1 year ago

FYI 6.2.13 fixes subtitles download for videos located on smbj shares.

Pentaphon commented 1 year ago

@courville On 6.2.13 I'm getting a totally black screen when a video finishes instead of playing the next video. When I back out by pressing the back button and then try to play the episode by pressing the play button, I get a toast notification that says "cannot play video". I have to wait about 10-15 seconds, and then the video plays just fine when I press the play button again. This has happened to me 3 times today in the exact same way.

This is on my Onn 4K which didn't have this issue on regular SMB. I double checked my wifi just to be sure it wasn't my connection and my router is in the same room as the Onn 4K. Do you think it could have to do with SMBJ connection robustness combined with playing videos in sequence? I'm using "folder mode".

Pentaphon commented 1 year ago

@courville still getting the black screen on 6.2.17 when playing videos sequentially over SMBJ (even low bitrate files only 300MB in size) and when I back out, I get a toast saying "cannot play video" but then if I wait 10-15 seconds, everything is fine and I can select and play the next video/episode again. Do you see any issues when playing videos in sequence in folder mode?

courville commented 1 year ago

The only issues reported on the sentry backend are:

Pentaphon commented 1 year ago

The only issues reported on the sentry backend are:

Yeah, I wouldn't expect it to be there because it doesn't seem to be a crash but it just is unable to play the next video and just stops at a black screen.

courville commented 1 year ago

How long are your videos? After how many videos played in series it happens?

Pentaphon commented 1 year ago

How long are your videos? After how many videos played in series it happens?

Mostly shorter than 30 min because they are mostly kids or family shows without commercials. Although I will admit this is TV so an episode is always less than an hour. It happens after anywhere from 4 to 6 videos from the season menu and then the black screen shows up. Most of the time it doesn't happen at all but it's happened enough for me to notice it because my family comes rushing to let me know "the TV went black!" and I have to run in and get it playing again. I don't see it nearly as often as my family does because I tend to watch new episodes of current shows so I watch shows 1 episode at a time or movies. It's definitely happened at least 3 times in front of me, probably 4 or much more in total because my family uses our TVs more than I do to play children's and family TV shows. They tend to be x265 encodes in mkv anywhere from 150MB to 500MB in size to save space but they should play just fine.

I've had to switch them to Kodi to make sure it doesn't happen anymore when I am not home but when I am home I use Nova to test builds like the newest v6.2.18. I will let you know if v6.2.18 gives me any trouble.

Pentaphon commented 1 year ago

@courville I am closing this because 1) I strongly suspect 6.2.18 has fixed the "cannot play video" issue when playing episodes in sequence. Not 100% sure it's totally gone but I've been watching out for this issue and asked my family to let me know if they see it when I'm not at home and it seems to be gone since I installed it on the 17th.

2) SMBJ implementation seems to be complete so this issue is no longer needed. Thanks for SMBJ!