Change subtitle renderer from Roku embedded to a custom task #982

jkim2492 · 2023-01-27T00:16:27Z

Changes

Add an option to disable Roku's global captions and use custom captions renderer for CJK fonts
Issues

Related to issue #514

Crude subtitle renderer piggybacking on the JFVideo component

sevenrats · 2023-01-27T01:16:47Z

~~This is very cool but you have to download the fonts from somewhere at runtime. Roku will not publish any app over 4 mb.~~
sorry, it's clear from context you already know this.

components/captionTask.brs

components/JFVideo.brs

components/captionTask.brs

1hitsong · 2023-01-27T01:37:22Z

We should honor a user's closed captioning styles as set in the Roku global captionStyle: https://developer.roku.com/docs/references/scenegraph/media-playback-nodes/video.md#closed-caption-fields

1hitsong · 2023-01-27T01:39:01Z

We can't include the font files, just 1 of them is larger than our max allowed app size.

But, the API does provide us a method to transfer fonts from the server over to the client and store them in a temp folder.

https://api.jellyfin.org/#tag/Subtitle/operation/GetFallbackFont

1hitsong · 2023-01-27T01:39:47Z

That's all my notes for the first pass. Very cool enhancement!

JFVideo updates playerState for captionTask Moved pkg:/fonts to pkg:/components/fonts

jkim2492 · 2023-01-27T03:01:05Z

We can't include the font files, just 1 of them is larger than our max allowed app size.

But, the API does provide us a method to transfer fonts from the server over to the client and store them in a temp folder.

https://api.jellyfin.org/#tag/Subtitle/operation/GetFallbackFont

This is great! I had considered downloading it from the web on boot but this would be more bandwidth friendly

sevenrats · 2023-01-27T19:49:25Z

Is it possible to change the position of the labels? If so, it would be nice to eventually extend this for full ssa/ass support as well.

jkim2492 · 2023-01-28T03:02:16Z

Ya it shouldn't be too hard. As it is, jellyfin server seems to convert SRT to VTT before sending it to client. I'm not sure if that's the case for ASS as well

…

On Fri, 27 Jan 2023, 14:49 Austin Crandall, ***@***.***> wrote: Is it possible to change the position of the labels? If so, it would be nice to eventually extend this for full ssa/ass support as well. — Reply to this email directly, view it on GitHub <#982 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AM7RJUET7YCL2MJ5VNOQVA3WUQRFBANCNFSM6AAAAAAUIDPQPE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

Lorde627 · 2023-01-28T04:34:10Z

We stan!

The project should validate now

Now it downloads from the fallback folder if available. Defaults to LargeBoldSystemFont. Subtitles are displayed with an outline instead of a background rectangle TODO : take captionstyle into account

1hitsong · 2023-02-02T02:12:19Z

I selected a movie and within Roku settings enabled subtitles. It turned on the original subtitles, not the new ones.

1hitsong · 2023-02-02T02:28:22Z

When both global subtitles and new subtitles are enabled, they overlap.

1hitsong · 2023-02-02T02:32:31Z

We should clear the displayed subtitle when the track changes. When I changed tracks it kept the original one until it updated several seconds later.

Disable Roku subtitles on subtitle load Subtitles are cleared as soon as subtitle track changes

Subtitle now follows Roku config Subtitle now updates immediately when changing languages

…into unstable

jkim2492 · 2023-02-02T06:59:28Z

We should clear the displayed subtitle when the track changes. When I changed tracks it kept the original one until it updated several seconds later.

The subtitles should clear instantly now. Subtitle visibility will now follow Roku's settings and will not overlap i.e. if Roku settings is always on, it will be always on

Lorde627 · 2023-02-02T07:11:11Z

Merge! Merge! Merge!

1hitsong · 2023-02-02T22:59:26Z

Do you mind changing the name of your feature branch? It's causing git to act very funky due to your feature branch also being named unstable.

1hitsong · 2023-02-02T23:04:31Z

Play movie, subtitles showing
Press Back
Play movie again, no subtitles

Console error: *** ERROR: Missing or invalid PHY: 'noto.otf'

After several seconds, subtitles show

1hitsong · 2023-02-02T23:05:32Z

If you click on a movie you've already watched part of and from the resume popup select Resume from %time% the subtitles start from the beginning and not from the resume point.

jkim2492 · 2023-02-08T21:37:20Z

What if we drop use of background color and opacity and instead support window color and opacity?

That would mean we support the rectangle "window" around the lines of text, and the text itself. The Background settings, which are an odd background-within-a-background setting to begin with, we ignore.

Perhaps we can implement both : window settings for the rectangle and background settings for the outline (or vice versa)

1hitsong · 2023-02-08T21:57:03Z

I suppose it's worth a shot, but at least on my Roku, that text outline looks bad.

jkim2492 · 2023-02-08T22:15:04Z

Hm in that case we can hold back on the outlines until a solution is found

…

I suppose it's worth a shot, but at least on my Roku, that text outline looks bad. — Reply to this email directly, view it on GitHub <#982 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AM7RJUELHYVSRQ3PSKZHG7TWWQJDVANCNFSM6AAAAAAUIDPQPE> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

Remove text outline Add background rectangle

jkim2492 · 2023-02-10T17:20:51Z

We are using background rectangles instead of outlines now. They can be adjusted with Background/* settings

1hitsong · 2023-02-10T21:44:24Z

Is the expectation for this PR that this only work for external subtitle files, or should this work for subtitles that are part of the video file as well?

I ask because it works for my videos with external files, but the files with encoded subtitles get the "old" build-in subtitle render that don't support the CJK font.

jkim2492 · 2023-02-12T05:11:06Z

Is the expectation for this PR that this only work for external subtitle files, or should this work for subtitles that are part of the video file as well?

I ask because it works for my videos with external files, but the files with encoded subtitles get the "old" build-in subtitle render that don't support the CJK font.

The intent was to make it for external files only but we can probably handle encoded ones in a separate PR later!

1hitsong · 2023-02-12T11:59:21Z

we can probably handle encoded ones in a separate PR later!

Excellent. Cuts my testing in half!

I hope to have functional testing completed tonight and will do a code review pass after that.

1hitsong · 2023-02-18T19:13:54Z

Functionality works great! Here are some updates we need to make for users.

The fallback font API endpoint will always return any fonts, even if fallback fonts are disabled. We should only grab a fallback font if fallback fonts are enabled on the server

This is an awesome update, but there WILL be people who complain we're not using the Roku default function. Can you add a user setting that allows users to opt-in to use custom subtitle functionality instead of default?

1hitsong · 2023-02-18T19:15:33Z

components/JFVideo.brs

+
+sub toggleCaption()
+    m.captionTask.playerState = m.top.state + m.top.globalCaptionMode
+    if m.top.globalCaptionMode = "On"


Suggested change

if m.top.globalCaptionMode = "On"

if LCase(m.top.globalCaptionMode) = "on"

String comparisons should always force a case.

1hitsong · 2023-02-18T19:18:50Z

components/JFVideo.brs

+end sub
+
+sub updateCaption ()
+    while m.captionGroup.removeChildIndex(0)


Instead of using a while loop to remove all the children, could we use the removechildrenindex function?

m.captionGroup.removeChildrenIndex(m.captionGroup.getChildCount(), 0)

1hitsong · 2023-02-18T19:19:46Z

components/JFVideo.brs

-    if m.top.content.contenttype <> 4
-        m.top.unobserveField("position")
-    end if
+    ' if m.top.content.contenttype <> 4


Commented out code should just be removed

1hitsong · 2023-02-18T19:21:15Z

components/JFVideo.brs

-        m.showNextEpisodeButtonAnimation.control = "start"
-        m.nextEpisodeButton.setFocus(true)
-        m.nextEpisodeButton.visible = true
+    if m.top.content.contenttype = 4


Instead of nesting this clause, let's just escape early.

At the top of showNextEpisodeButton() do if m.top.content.contenttype <> 4 then return

1hitsong · 2023-02-18T19:21:55Z

components/JFVideo.brs

-        updateCount()
-        return
-    end if
+    if m.top.content.contenttype = 4


Same as above. Let's exit early.

if m.top.content.contenttype <> 4 then return

1hitsong · 2023-02-18T19:24:00Z

components/captionTask.brs

+    end if
+end sub
+
+function newlabel(txt):


Suggested change

function newlabel(txt):

function newlabel(txt)

1hitsong · 2023-02-18T19:24:44Z

components/captionTask.brs

+
+sub updateCaption ()
+    m.top.currentCaption = []
+    if m.top.playerState = "playingOn"


Suggested change

if m.top.playerState = "playingOn"

if LCase(m.top.playerState) = "playingon"

String comparisons should force case

1hitsong · 2023-02-18T19:25:17Z

components/captionTask.brs

+        lines = newLayoutGroup(labels)
+        rect = newRect(lines)
+        m.top.currentCaption = [rect, lines]
+    else if right(m.top.playerState, 4) = "Wait"


Suggested change

else if right(m.top.playerState, 4) = "Wait"

else if LCase(right(m.top.playerState, 4)) = "wait"

1hitsong · 2023-02-18T19:26:06Z

source/utils/Subtitles.brs

@@ -87,6 +86,8 @@ function setupSubtitle(video, subtitles, subtitle_idx = -1) as integer

    if selectedSubtitle.IsEncoded
        ' With encoded subtitles, turn off captions
+        ?"Check2"


1hitsong · 2023-02-18T19:27:55Z

source/utils/Subtitles.brs

@@ -177,6 +178,7 @@ sub turnoffSubtitles()
    video = m.scene.focusedChild.focusedChild
    current = video.SelectedSubtitle
    video.SelectedSubtitle = -1
+    ?"Check3"


Custom subtitles can be enabled via * > Settings > Playback > Use Custom Subtitles Fallback font downloaded only if it is enabled in system settings Cleaned up various code

jkim2492 · 2023-02-19T18:33:42Z

Functionality works great! Here are some updates we need to make for users.

The fallback font API endpoint will always return any fonts, even if fallback fonts are disabled. We should only grab a fallback font if fallback fonts are enabled on the server

This is an awesome update, but there WILL be people who complain we're not using the Roku default function. Can you add a user setting that allows users to opt-in to use custom subtitle functionality instead of default?

This and all of the change requests should be fixed now. Hope there aren't new bugs!

settings/settings.json

1hitsong · 2023-02-21T01:54:30Z

@jkim2492 Double check the translations file. Github shows the entire file as modified.

42d3e5d#diff-0c39691ce17a12d65440b15c63dc5f7c12b526d1a105e0c99f568a5e9c2313bc

Added en-US translations for custom subtitle option

jkim2492 · 2023-02-21T02:45:08Z

@1hitsong Seems the line endings had changed for some reason. It should correctly show diffs now

1hitsong · 2023-02-21T13:45:41Z

@jkim2492 Approved. Can you resolve the merge conflicts?

jkim2492 · 2023-02-21T17:44:04Z

@1hitsong Yup it should be good now

1hitsong · 2023-02-21T17:54:06Z

Merged. For the follow up, please use a different name for the branch 😉

depctg · 2023-03-03T22:26:07Z

Is it possible to change the position of the labels? If so, it would be nice to eventually extend this for full ssa/ass support as well.

As a follow up simple ass support is implemented in #1075

jkim2492 added 2 commits January 26, 2023 19:03

Subtitle rendering using a task node

2fc121f

Crude subtitle renderer piggybacking on the JFVideo component

Replace global caption with JF video caption

0bee595

1hitsong suggested changes Jan 27, 2023

View reviewed changes

components/captionTask.brs Outdated Show resolved Hide resolved

components/JFVideo.brs Show resolved Hide resolved

components/captionTask.brs Show resolved Hide resolved

1hitsong added the enhancement New feature or request label Jan 27, 2023

Fix playerState

ec1bc7e

JFVideo updates playerState for captionTask Moved pkg:/fonts to pkg:/components/fonts

This comment was marked as resolved.

Sign in to view

jkim2492 and others added 2 commits January 29, 2023 19:52

remove "then" from if statements

2b56ed5

The project should validate now

Remove font files from pkg

0bcb779

Now it downloads from the fallback folder if available. Defaults to LargeBoldSystemFont. Subtitles are displayed with an outline instead of a background rectangle TODO : take captionstyle into account

jkim2492 requested a review from 1hitsong February 1, 2023 01:32

1hitsong added the needs work label Feb 2, 2023

jkim2492 added 3 commits February 1, 2023 22:22

Disable Roku subtitles on subtitle load

2576162

Disable Roku subtitles on subtitle load Subtitles are cleared as soon as subtitle track changes

Change subtitle visibility to follow Roku

09045c3

Subtitle now follows Roku config Subtitle now updates immediately when changing languages

Merge branch 'unstable' of https://github.com/jkim2492/jellyfin-roku …

7a9fbb4

…into unstable

jkim2492 deleted the branch jellyfin:unstable February 3, 2023 00:23

Change Background/* settings to affect rectangle

facec48

Remove text outline Add background rectangle

jkim2492 requested a review from 1hitsong February 10, 2023 17:21

1hitsong suggested changes Feb 18, 2023

View reviewed changes

Custom subtitles added to options

bd5a055

Custom subtitles can be enabled via * > Settings > Playback > Use Custom Subtitles Fallback font downloaded only if it is enabled in system settings Cleaned up various code

jkim2492 requested a review from 1hitsong February 19, 2023 18:34

1hitsong suggested changes Feb 19, 2023

View reviewed changes

settings/settings.json Outdated Show resolved Hide resolved

jkim2492 requested a review from 1hitsong February 20, 2023 22:40

jkim2492 force-pushed the unstable branch from 42d3e5d to 71c4294 Compare February 21, 2023 02:34

Updated custom subtitle option in settings.json

1fd21b7

Added en-US translations for custom subtitle option

jkim2492 force-pushed the unstable branch from 71c4294 to 1fd21b7 Compare February 21, 2023 02:43

1hitsong approved these changes Feb 21, 2023

View reviewed changes

1hitsong removed the needs work label Feb 21, 2023

Merge branch 'unstable' into unstable

45dc9e5

1hitsong merged commit d10556e into jellyfin:unstable Feb 21, 2023

1hitsong mentioned this pull request Apr 11, 2023

After watching an episode, the next episode isn't properly reported by NextUp API endpoint #1186

Closed

	if m.top.globalCaptionMode = "On"
	if LCase(m.top.globalCaptionMode) = "on"

	if m.top.playerState = "playingOn"
	if LCase(m.top.playerState) = "playingon"

	else if right(m.top.playerState, 4) = "Wait"
	else if LCase(right(m.top.playerState, 4)) = "wait"

Change subtitle renderer from Roku embedded to a custom task #982

Change subtitle renderer from Roku embedded to a custom task #982

Conversation

jkim2492 commented Jan 27, 2023 • edited Loading

sevenrats commented Jan 27, 2023 • edited Loading

1hitsong commented Jan 27, 2023

1hitsong commented Jan 27, 2023

1hitsong commented Jan 27, 2023

jkim2492 commented Jan 27, 2023

This comment was marked as resolved.

sevenrats commented Jan 27, 2023

jkim2492 commented Jan 28, 2023 via email

Lorde627 commented Jan 28, 2023

1hitsong commented Feb 2, 2023 • edited Loading

1hitsong commented Feb 2, 2023

1hitsong commented Feb 2, 2023

jkim2492 commented Feb 2, 2023

Lorde627 commented Feb 2, 2023

1hitsong commented Feb 2, 2023 • edited Loading

1hitsong commented Feb 2, 2023

1hitsong commented Feb 2, 2023

jkim2492 commented Feb 8, 2023

1hitsong commented Feb 8, 2023

jkim2492 commented Feb 8, 2023 via email • edited Loading

jkim2492 commented Feb 10, 2023

1hitsong commented Feb 10, 2023

jkim2492 commented Feb 12, 2023

1hitsong commented Feb 12, 2023

1hitsong commented Feb 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkim2492 commented Feb 19, 2023

1hitsong commented Feb 21, 2023 • edited Loading

jkim2492 commented Feb 21, 2023

1hitsong commented Feb 21, 2023

jkim2492 commented Feb 21, 2023

1hitsong commented Feb 21, 2023

depctg commented Mar 3, 2023

jkim2492 commented Jan 27, 2023 •

edited

Loading

sevenrats commented Jan 27, 2023 •

edited

Loading

1hitsong commented Feb 2, 2023 •

edited

Loading

1hitsong commented Feb 2, 2023 •

edited

Loading

jkim2492 commented Feb 8, 2023 via email •

edited

Loading

1hitsong commented Feb 21, 2023 •

edited

Loading