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

Fix resume not working on LG WebOS #3724

Merged
merged 2 commits into from
Jun 27, 2022

Conversation

samcon
Copy link

@samcon samcon commented Jun 20, 2022

Resume wasn't working for me on LG WebOS (via jellyfin-webos) on Jellyfin 10.8.0, jellyfin-webos 1.0.1 and latest jellyfin-web (master)

Description
I believe the issue is that:

  • ?t=XXX is ignored on the video element in the WebOS browser
  • Flawed logic in setCurrentTimeIfNeeded
  • For some reason, changing currentTime in the 'playing' event (of the HTML video element) doesn't seem to do anything. I also tried 'loadeddata', 'loadedmetadata', 'canplay' but none of them worked. Only thing that worked is to delay the execution with setTimeout.
  • Also possible that this only effects DirectStream MKV files

Changes

  • I'm not sure what the original intention was, but Math.abs(element.currentTime || 0, seconds) is actually Math.abs(element.currentTime || 0). I interpreted the logic was meant to not skip if the diff is smaller than 1 sec.
  • Maybe someone has a less hack-y way to change (i.e. without using setTimeout) currentTime

Issues
Probably solves jellyfin/jellyfin-webos#46

src/components/htmlMediaHelper.js Outdated Show resolved Hide resolved
src/components/htmlMediaHelper.js Outdated Show resolved Hide resolved
@dmitrylyzo
Copy link
Contributor

dmitrylyzo commented Jun 20, 2022

What webOS version are you testing?
Older versions can behave oddly, so timeouts are not unusual.

Since this is a bug, please rebase this branch on release-10.8.z and edit target branch to the same (without reopening a new PR).

@dmitrylyzo dmitrylyzo added bug Something isn't working p: webos This PR or issue mainly concerns WebOS clients labels Jun 20, 2022
@samcon
Copy link
Author

samcon commented Jun 20, 2022

What webOS version are you testing? Older versions can behave oddly, so timeouts are not unusual.

Since this is a bug, please rebase this branch on release-10.8.z and edit target branch to the same (without reopening a new PR).

release-10.8.z

WebOS 4.5

Will do

@samcon samcon changed the base branch from master to release-10.8.z June 20, 2022 19:34
@samcon samcon requested a review from a team as a code owner June 20, 2022 19:34
@samcon samcon changed the base branch from release-10.8.z to master June 20, 2022 19:55
@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Jun 20, 2022
@samcon samcon changed the base branch from master to release-10.8.z June 20, 2022 20:02
@dmitrylyzo
Copy link
Contributor

Don't forget to reapply suggestions. You are free to squash commits to a first one.

@dmitrylyzo dmitrylyzo added the stable backport Backport into the next stable release label Jun 20, 2022
@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Jun 20, 2022
Copy link
Contributor

@dmitrylyzo dmitrylyzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have played with timeouts: 50ms worked too (once), but 300 worked more often. So there's a chance that 500ms may fail too. It probably depends on buffering time.

I have tried events durationchange, loadeddata, play, loadedmetadata: they all go before seekOnPlaybackStartIfNeeded (play goes first).
I haven't tried timeupdate though 🤔 Something like: seek if needed on the first time update.

src/components/htmlMediaHelper.js Outdated Show resolved Hide resolved
src/components/htmlMediaHelper.js Outdated Show resolved Hide resolved
src/components/htmlMediaHelper.js Outdated Show resolved Hide resolved
Copy link
Contributor

@dmitrylyzo dmitrylyzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Maybe also add HACK: somewhere to mark it as an unfinished/non-ideal solution?

@samcon
Copy link
Author

samcon commented Jun 23, 2022

I did some more testing and noticed subtitles aren't synced to the resumed time.
If subs aren't supposed to sync automatically when currentTime is set, any pointers on how to "manually" invoke sub sync?

@dmitrylyzo
Copy link
Contributor

Tested unmodified jf-web on emulators of webOS 1.2, 2, 4 and 5 - only 4 fails 🤷‍♂️
If this is true for real TVs, we need to check the web0sVersion.

I did some more testing and noticed subtitles aren't synced to the resumed time. If subs aren't supposed to sync automatically when currentTime is set, any pointers on how to "manually" invoke sub sync?

What subs? SRT or ASS, embedded or external.

@samcon
Copy link
Author

samcon commented Jun 23, 2022

External SRT file

@dmitrylyzo
Copy link
Contributor

External SRT file

On webOS this block should be called on timeupdate event:

let timeMs = time * 1000;
timeMs += ((currentPlayOptions.transcodingOffsetTicks || 0) / 10000);
this.updateSubtitleText(timeMs);

@dmitrylyzo
Copy link
Contributor

dmitrylyzo commented Jun 23, 2022

In webOS 4 emulator currentTime and subtitles are synchronized and updates normally, but the video updates slowly.
Progress line (and its text) is out of sync too. Do you have the same?
I tried it from 0s position. With timeout and without - slow video.

Test assets

jf-gray_1080p_h264_m40.mp4

jf-gray_1080p_h264_m40.time.srt.txt

@samcon
Copy link
Author

samcon commented Jun 26, 2022

Alright, I did some more digging and it turns out that if you change 'metadata' here:

if (!appHost.supports('htmlvideoautoplay')) {
html += '<video class="' + cssClass + '" preload="metadata" autoplay="autoplay" controls="controls" webkit-playsinline playsinline>';

to 'auto' then resume simply (i.e. w/o the timer) works 🤷‍♂️.

Subtitles still don't sync to the correct position though

@dmitrylyzo
Copy link
Contributor

I like hack-free solutions. 👍

Does it work with transcoding?

autoplay is supposed to force a full preload, but taking into account quirks of browsers, it is better to specify a full preload explicitly. Moreover, I think we can set it to auto for all cases (need testing), because we are definitely going to play it.

Subtitles still don't sync to the correct position though

Have you tried the sample above? In the emulator subtitles are out of sync even from the beginning.

Co-authored-by: Dmitry Lyzo <56478732+dmitrylyzo@users.noreply.github.com>
@sonarcloud
Copy link

sonarcloud bot commented Jun 27, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@joshuaboniface joshuaboniface merged commit bc48691 into jellyfin:release-10.8.z Jun 27, 2022
thornbill pushed a commit that referenced this pull request Jun 29, 2022
(cherry picked from commit bc48691)
Signed-off-by: Bill Thornton <billt2006@gmail.com>
@jellyfin-bot jellyfin-bot removed the stable backport Backport into the next stable release label Jun 29, 2022
@samcon samcon deleted the fix_resume_webos branch June 30, 2022 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working p: webos This PR or issue mainly concerns WebOS clients
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants