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

delay subtitles taking into account advertisements breaks #53

Merged
merged 1 commit into from
Sep 16, 2019

Conversation

mediaminister
Copy link
Contributor

This is a very experimental fix for https:/michaelarnauts/plugin.video.vtm.go/issues/51
It works as follows:

  1. get advertisement breaks start timestamps and durations from manifest json file
  2. load external WEBVTT subtitle file into memory
  3. delay subtitle timestamps with ad break duration if advertisement break starts before the subtitle starts
  4. save edited WEBVTT file with delayed timestamps to addon profile folder
  5. add local WEBVTT path to subtitle_info in ResolvedStream object

@dagwieers dagwieers added the bug Something isn't working label Sep 10, 2019
@dagwieers dagwieers added this to the v0.9.0 milestone Sep 10, 2019
@michaelarnauts
Copy link
Collaborator

I've noticed that the subtitle filename is displayed in the subtitle dialog, and I couldn't change it through the stream info. By saving it to disk, we might be able to name it Dutch.vtt or something and have Kodi parse the language correctly.

@michaelarnauts
Copy link
Collaborator

Is the add-on profile folder the preferred location to save temp files? When do we clean this up?

@mediaminister
Copy link
Contributor Author

The add-on profile folder it the only folder an add-on may use to write files to. (https://kodi.wiki/view/Add-on_rules). Cleaning up is not taken care of yet, subtitles are edited every time a stream is requested, so we can clean up with xbmcvfs.rmdir when calling get_stream.

Feel free to push your changes to this branch.

@dagwieers
Copy link
Collaborator

Personally if we need to process and write these files, I would propose to either use a dedicated directory, or create our own temp/ directory for these kind of files. Leave the root of the add-on for permanent files (e.g. the settings, and potentially other files).

For VRT we have this:

.
|-- cache
|   |-- categories.json
|   |-- category.cultuur.json
|   |-- ...
|   `-- schedule.tomorrow.json
|-- search_history.json
|-- settings.xml
`-- tokens
    |-- XVRTToken.tkn
    |-- live_vrtPlayerToken.tkn
    |-- ...
    `-- vrtloginrt.tkn

@mediaminister
Copy link
Contributor Author

mediaminister commented Sep 15, 2019

The currently used subtitles are now stored in addon_data/plugin.video.vtm.go/temp directory. Old vtt-files are deleted when a new subtitle is generated.

Please test and review.

@dagwieers
Copy link
Collaborator

I wish I could test, but it is not easy if you need a new Kodi and inputstream.adaptive, which are not available for ARM.

@mediaminister
Copy link
Contributor Author

mediaminister commented Sep 15, 2019

I think testing on ARM is possible with the test builds: https://jenkins.kodi.tv/blue/organizations/jenkins/peak3d%2Finputstream.adaptive/detail/master/112/artifacts

@michaelarnauts
Copy link
Collaborator

I don't think there are automated Linux arm builds... Is a new Kodi also required? I can test on Linux and windows

@mediaminister
Copy link
Contributor Author

No, you only need InputStream Adaptive with multiple periods support. Kodi 19 with IChapter support is nice to have, because periods play flawlessly from the first to the last second.

@michaelarnauts
Copy link
Collaborator

So periods play flawlessly with just the Inputstream adaptive update, and ichapter is only for the GUI?

Any idea where inputstream builds are made for arm? (I have osmc on a vero4k+)

@mediaminister
Copy link
Contributor Author

No, I mean that you can test this PR on Windows/Linux with only the InputStream Adaptive update from Jenkins. After a couple of seconds in a new Period everything should play fine.
But for a seamless experience you need Kodi 19 alpha with PR-16581, so you need to build Kodi yourself.

For ARM, there are no test builds yet that contain the latest commits from master branch:
https://discourse.osmc.tv/t/kodi-19-matrix-nightly-builds-for-vero-2-4k/79408/102

@michaelarnauts
Copy link
Collaborator

No, I mean that you can test this PR on Windows/Linux with only the InputStream Adaptive update from Jenkins. After a couple of seconds in a new Period everything should play fine.
But for a seamless experience you need Kodi 19 alpha with PR-16581, so you need to build Kodi yourself.

For ARM, there are no test builds yet that contain the latest commits from master branch:
https://discourse.osmc.tv/t/kodi-19-matrix-nightly-builds-for-vero-2-4k/79408/102

Okay, I was ready to give this a try, but now I notice that there aren't even normal linux binaries build for inputstream adaptive. Am I missing something? I've tested inpustream in the past, but I probably did this on my windows pc.

@mediaminister I assume you use windows for your development and/or testing?

@dagwieers
Copy link
Collaborator

I have tested this on Android. I could not find a newer Kodi build with IChapter support.
However playback worked quite well, the only issues I noticed:

  • On Android switching between periods causes a prolonged multi-second black screen (this does not affect subtitle synchronization)
  • When jumping forward/backward I have seen cases where the subtitle synchronization started to fail

I haven't tried longer than 5 minutes, so did not experience period-switches except at the very start. Without an ARM build, I wouldn't be able to test this for a longer time than just some short tests.

@mediaminister
Copy link
Contributor Author

mediaminister commented Sep 16, 2019

When jumping forward/backward I have seen cases where the subtitle synchronization started to fail

I also experienced this when seeking forward with the mouse pointer and I created an issue for this problem: xbmc/inputstream.adaptive#324

@michaelarnauts I use Linux and I compiled Kodi and InputStream Adaptive myself.

To compile InputStream Adaptive add-on from master branch for Kodi Leia, this should work:

git clone -b Leia https:/xbmc/xbmc.git
git clone https:/peak3d/inputstream.adaptive
cd ~/inputstream.adaptive && mkdir build && cd build
cmake -DADDONS_TO_BUILD=inputstream.adaptive -DADDON_SRC_PREFIX=../.. -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=../../xbmc/addons -DPACKAGE_ZIP=1 ../../xbmc/cmake/addons
make
cd ~/xbmc/addons/
zip -r inputstream.adaptive.zip inputstream.adaptive

@michaelarnauts
Copy link
Collaborator

@mediaminister I did a few tests and subtitles are working fine! Awesome job! I'll merge this. We can always improve later on.

@michaelarnauts michaelarnauts merged commit 64bc684 into add-ons:master Sep 16, 2019
@dagwieers
Copy link
Collaborator

Beware though. I would rather have inputstream.adaptive take care of this (if this is at all possible in a generic way). Because if this gets fixed in inputstream.adaptive our own fixes will no longer work. (And in fact by having this now added to VTM GO, it becomes less likely @peak3d can test a fix in inputstream.adaptive).

So maybe we have to make this optional or configurable so a fix upstream is facilitated?

@michaelarnauts
Copy link
Collaborator

@dagwieers It seems that the way vtm implemented this isn't according to any standards. They have a custom json that sets the breakpoints of periods. Their own (web)player injects the ads into the timeline without affecting the timeline of the original program. This is something that is a lot harder to do in kodi.

I don't see how this could be done in inputstream.

@dagwieers
Copy link
Collaborator

Ok, I remember @peak3d saying there was a subtitle in the manifest, so I assumed we were extracting it, changing it and providing it back to Kodi's ListItem.

@michaelarnauts
Copy link
Collaborator

That's right, but I think that "embedded" subtitle was empty. If it weren't, it would probably just work, since it would be tied to the Period instead.

@mediaminister
Copy link
Contributor Author

mediaminister commented Sep 16, 2019

VTM GO provides two subtitle formats: internal segmented WEBVTT in the MPEG-DASH manifest and an external WEBVTT url.
My implementation uses the external WEBVTT file as a source to generate an new WEBVTT file with edited timings. This was the easiest and fastest way to fix this.

The internal segmented WEBVTT in the MPEG-DASH manifest can be supported in InputStream Adaptive, but currently the implementation is incomplete or wrong. I don't know yet what the problem is and it will take more time to investigate what is needed to make WEBVTT subtitling work in InputStream Adaptive. I can confirm the internal segmented WEBVTT is not empty: https:/michaelarnauts/plugin.video.vtm.go/issues/51#issuecomment-529626570

Yes this is a quick fix, but it doesn't interfere with the subtitle stream in the MPEG-DASH manifest. When WEBVTT gets fixed in InputStream Adaptive the subtitle menu will just have two subtitle items and the user can select which one to use.

@michaelarnauts
Copy link
Collaborator

Okay, thanks for the explanation! I assume when inputstream adaptive fully supports the subtitles in the periods, we can drop our own implementation, since it will just be the same result.

@dagwieers
Copy link
Collaborator

@mediaminister Good to know. So we're not modifying the manifest either. (It is clear I didn't look to the specifics yet ;-))

@mediaminister
Copy link
Contributor Author

No, we get the external subtitle url directly from the videoplayer-service.api json (_get_stream_info in vtmgostream.py)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants