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

Transcode stream refactor #609

Merged
merged 24 commits into from
Jul 23, 2020
Merged

Conversation

WithoutPants
Copy link
Collaborator

@WithoutPants WithoutPants commented Jun 11, 2020

This is a significant refactor of the transcode streaming code.

The forceMKV and forceHEVC flags have been removed, and is_streamable has been removed from the scene graphql type. Instead, there is a new IsSceneStreamable graphql query that accepts a list of supported video codecs. Similarly, the stream.mp4 route has been changed to accept a list of codecs as well. The supported video codecs are obtained using a custom Modernizr build that only tests for video support. There is an additional test for mkv support, but it is hard-coded to check if the client browser is Chromium-based.

On the backend, the transcode code is changed so that it chooses the codec to use based on the supported codecs sent to the request. It is currently hard-coded to prefer VP9, VP8, HEVC then H264. This should probably be further refined to accept the supported codecs in order of preference.

This doesn't fix the Safari bug referenced in #600 but is a decent first step. This obsoletes a lot of the changes in #522 which will need to be reworked if we go ahead with this.

Fixes #600

@WithoutPants WithoutPants added the improvement Something needed tweaking. label Jun 11, 2020
@WithoutPants WithoutPants added this to the Version 0.3.0 milestone Jun 11, 2020
@WithoutPants
Copy link
Collaborator Author

Incidentally, this should eliminate the performance impact of not having the scene containers in the database.

pkg/ffmpeg/stream.go Outdated Show resolved Hide resolved
@bnkai
Copy link
Collaborator

bnkai commented Jun 11, 2020

Mp4 as a container is not suited for live streaming.
Try forcing h264, mp4 as the codec , container and try to play a scene that needs transcoding
Try a long scene (not a small one that can be converted at once). When you seek through it and then let it play it plays for a little while and then stops. Even playing without seeking has the same issue.
h264 and hevc is possible but only when mkv is the container (#522 makes use of that)
If the issue of h264 and mp4 is worked out or h264 and mkv selected instead there are a couple more h264 settings that need to be tuned eg crf reduced to 25, pixfmt set to yuv420p so that 10bit encoded files are convertered to 8 bit , tune zerolatency and so on that can also be taken from #522

@bnkai
Copy link
Collaborator

bnkai commented Jun 11, 2020

Incidentally, this should eliminate the performance impact of not having the scene containers in the database.

Yeah i would imagine everyone at some point would have finished a new scan to update the containers but the is_streamableneeded to go anyway.

#522 will be changed back to draft till we sort this out.
Is it possible to use the UI and getSupportedformats to whitelist, blacklist some codecs depending on user options ? If yes then #522 wouldn't be needed.

@WithoutPants
Copy link
Collaborator Author

MP4 appears to work correctly on Chrome, but not on Firefox. I'm not sure if it's worth spending the time and effort fixing it when Firefox supports webm anyway.

The latest commit adds support for HLS for Safari. Initial testing looks good, but I need to do a more extension test run. HLS is supported in Edge but playback didn't work for me, so I've made it so that it won't use HLS unless other options aren't available.

The HLS support was assisted with knowledge from a fork of Golang HLS Streamer. The fork was a lot more basic than the original, so there's possibly some improvements to be done with further study of that project. For example, this file looks like it might have some refinements that might improve it.

pkg/ffmpeg/stream.go Outdated Show resolved Hide resolved
pkg/ffmpeg/stream.go Outdated Show resolved Hide resolved
pkg/ffmpeg/stream.go Outdated Show resolved Hide resolved
pkg/ffmpeg/stream.go Outdated Show resolved Hide resolved
pkg/ffmpeg/stream.go Outdated Show resolved Hide resolved
@bnkai
Copy link
Collaborator

bnkai commented Jun 18, 2020

Some quick feedback from testing on an Android Smartphone.

Microsoft Edge ( got the hint from discord ) is the only browser that i found which supports hevc natively. This can be direct streamed (no transcode) when Force Hevc was checked in the dev build. In this version HEVC is not detected as supported.

Chrome and Edge ( again in android ) show Choosing transcode codec from: h264,vp8,vp9,mkv,hls when transcoding which implies that hls is supported in Android ? I tried forcing hls as a first choice when transcoding but it doesn't seem to play , it returns the infamous file not supported message from JWplayer. I don't know if the detection of hls as supported or if the implementation is lacking. HLS support is ok imo even if it only plays in safari.

@ghost
Copy link

ghost commented Jun 18, 2020

@bnkai Android does support HLS, see https://support.jwplayer.com/articles/adaptive-streaming-reference#hls-support

However according to jwplayer it only supports an older spec, so some tweaks might be required. I can see if I can get it working.

EDIT: Couldn't get anywhere with HLS in Android. Probably not worth messing with for now.

@bnkai
Copy link
Collaborator

bnkai commented Jun 19, 2020

@InfiniteTF i found this https://alexzambelli.com/blog/2016/05/04/understanding-hls-versions-and-client-compatibility/ regarding hls specs. I think for the job we need it even the oldest version is enough.
Wouldn't it be a lot easier to just limit hls to safari only ? Imo webm for the rest or even a mp4/mkv with h264 extra choice for chrome based browsers should be enough. The only problem with webm/vp9 is the higher cpu usage requirements ( problem for older, low specs machines ) that can not be addressed even with the realtime setting.
EDIT: Just saw the edit. Agreed.

@WithoutPants
Copy link
Collaborator Author

What is the consequence of leaving the HLS compatibility code as is if Android Chrome only uses it if forced? I'd rather not add Safari-detection code if we can avoid it, since it's apparently very difficult to do and is brittle. I'm thinking we should consider a move away from jwplayer if we can maintain the same features.

@bnkai
Copy link
Collaborator

bnkai commented Jun 21, 2020

With the code as it is it doesn't really matter since you have to alter the code yourself to reach hls in other browsers. The safari detection was mainly for cosmetic reasons and if it was easy to implement.
The only issue left is the Microsoft Edge in android and hevc. It might be easier to leave an advanced option in the UI for manual codec overrides.
EDIT Hevc isn't also detected for a custom hevc enabled old chromium build i use in linux (not that it should ) . Having an override would be needed imo to have feature parity with the current dev

@WithoutPants WithoutPants marked this pull request as draft July 9, 2020 06:13
@WithoutPants WithoutPants marked this pull request as ready for review July 9, 2020 09:37
@WithoutPants
Copy link
Collaborator Author

I've done a major refactor of this PR. The supported parameters are gone. Instead, I have exposed new endpoints in the server:

/stream
/stream.mkv
/stream.webm
/stream.m3u8
/stream.ts
/stream.mp4

The first endpoint is a direct stream of the source file. The mkv endpoint is a video copy stream and the remaining transcode to the applicable type. m3u8 gives the HLS manifest, with ts giving the individual streams.

I have replaced isSceneStreamable with sceneStreams. This returns applicable stream endpoints for the scene. Direct stream endpoints are filtered out of the returned values if the audio codec is invalid for the container, so the client doesn't attempt to direct stream. The mkv stream endpoint is only included if the scene file is an mkv file.

On the client end, I have changed it so that jwplayer receives a list of source urls, allowing it to choose the correct compatible stream. In addition, if the stream fails due to an incompatible stream, then it will fail over to the next source. I am hoping that this will make the force options redundant and I have removed them once again.

@WithoutPants
Copy link
Collaborator Author

WithoutPants commented Jul 10, 2020

From Discord, hearing that hevc/aac MKV files are not streaming correctly on iPad safari. On Chrome these files also appear to only be outputting audio only.

Files were encoded using: https://github.com/FallingSnow/h265ize

@bnkai
Copy link
Collaborator

bnkai commented Jul 10, 2020

The same thing happens to me in chrome/linux.
For the same file in firefox i get the infamous 224003 error so i guess something is wrong with the codec detection or the error retrieval from jwplayer.
In chrome mkv is supported so it plays the audio and because of that it doesn't throw an error even though the video codec isn't supported.
Any mkv hevc files with aac audio should behave the same

EDIT hevc/aac mp4 files have the same issue. Audio is supported and played thus jwplayer doesn't show an error , even if the video codec is not supported and played through

EDIT2 from a quick look i think this behaviour can be explained if the below code is somehow not executed/behaving correctly

 this.player.on("error", (err: any) => {
      if (err && err.code === 224003) {
        if (this.tryNextStream()) {
          this.player.load(this.playlist);
          this.player.play();

With the latest refactor how is container/codec compatibilty checked exactly? WMV and AVI files seem to transcode fine so if the above code works for them it should also work at least for the firefox/hevc/aac/mkv case

@WithoutPants
Copy link
Collaborator Author

The failover code was broken which is why you saw the 224003 error. For the no video issue, I've added some code that detects if the video metadata width and height are zero, and if so fails over to the next source.

pkg/api/routes_scene.go Outdated Show resolved Hide resolved
@bnkai
Copy link
Collaborator

bnkai commented Jul 11, 2020

The error retrieval seems to work now but VP8 is used instead of VP9 so it should be changed to VP9.
Hevc detection works now for most of the cases i tried except Edge on android were instead of direct streaming hevc it converts to webm (custom hevc enabled linux chromium build direct streams hevc files without a problem).

@victorhooi
Copy link

victorhooi commented Jul 11, 2020

I've been testing this on Linux x64 - these builds seem to remove the thumbnail functionality you normally get when hovering over the scrub bar.

For example, on YouTube:

image

But yes on the builds for this PR, I'm no longer seeing that.

@WithoutPants
Copy link
Collaborator Author

Corrected the webm codec and fixed the thumbnail sprite regression.

I've removed stream.mp4 from the sceneStreams return list, since having the second mp4 stream appeared to be confusing jwplayer on iPad, causing it to try that stream over the HLS one.

@bnkai
Copy link
Collaborator

bnkai commented Jul 18, 2020

Everything looks ok except the minor issue with microsoft edge and hevc in android mentioned above.

@WithoutPants
Copy link
Collaborator Author

The error retrieval seems to work now but VP8 is used instead of VP9 so it should be changed to VP9.
Hevc detection works now for most of the cases i tried except Edge on android were instead of direct streaming hevc it converts to webm (custom hevc enabled linux chromium build direct streams hevc files without a problem).

Tested the current develop version on Edge Android. Confirmed that when I set forceHEVC and tried to stream an hevc file, I got the 224003 error. Also tried streaming directly (not using jwplayer) and got the same issue. I don't think Android Edge actually has reliable support for hevc files.

@bnkai
Copy link
Collaborator

bnkai commented Jul 19, 2020

The hevc in android support should be a combination of browser/android version/hardware.
I have a xiaomi mi note 3 and also a user with a oneplus reported in the channel that it was working for him. In theory the other browsers should support that but only Edge was direct streaming it.
I am not sure if something can be done about it and how many people use it.

@WithoutPants
Copy link
Collaborator Author

I think I might have to chalk this up as a limitation at the moment:

The issue was introduced by the no video detection added in 3b816fe. If this code is removed, then it streams directly in Android Edge.

The no video detection works by hooking into the meta jwplayer event, and causes a failover if the video's width and height are 0. It appears that on Android Edge, the width/height are 0, but the video is still able to be played.

One alternative would be to detect Android Edge and disable the no video detection when on that client, but I just don't think it's worth the hassle.

@bnkai
Copy link
Collaborator

bnkai commented Jul 22, 2020

I think we can leave that as a limitation especially if #679 can be used instead in android.

@WithoutPants WithoutPants merged commit 37be146 into stashapp:develop Jul 23, 2020
Tweeticoats pushed a commit to Tweeticoats/stash that referenced this pull request Feb 1, 2021
* Remove forceMkv and forceHEVC
* Add HLS support and refactor
* Add new streaming endpoints
@WithoutPants WithoutPants deleted the stream-detection branch February 4, 2021 03:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Something needed tweaking.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug Report] Cannot transcode stream videos on Safari
4 participants