TTML to VTT style migration #48

Avishek-Gulshan · 2021-07-09T15:06:25Z

Update TTM L parser to push parent style to child tags
Creating new custom style classes by combining inline style and style class
Added additional style propagation from TTML to VTT

- Added style migration from TTML to WEBVTT

- Added support for STYLE classes

- Pushing parent style and inline style to child <span> - Creating new custom style class after combing inline style and style class as multiple format doesn't support inline style + style

coveralls · 2021-07-09T15:11:25Z

Coverage decreased (-3.2%) to 73.198% when pulling b86cb77 on EurosportDigital:TTML_TO_VTT_STYLE_MIGRATION into 8c108a5 on asticode:master.

Avishek-Gulshan · 2021-07-09T15:17:54Z

Hi,

Can you please have a look at this PR. See if this makes sense

Avishek-Gulshan · 2021-07-13T11:17:30Z

Hi, Please take a look at this PR

asticode · 2021-07-14T07:32:57Z

This is a big PR and I'm pretty busy right now, I'll need some time to review it

asticode

This is a huge PR, I've not reviewed everything but let start from here.

You need to fix tests as well.

asticode · 2021-07-14T07:29:39Z

subtitles.go

+	WebVTTVertical        string
+	WebVTTViewportAnchor  string
+	WebVTTWidth           string
+	WebVTTBackgroundColor string


Could you order new fields alphabetically?

asticode · 2021-08-11T07:33:47Z

ttml.go

@@ -196,7 +213,7 @@ func (i *TTMLInItems) UnmarshalXML(d *xml.Decoder, start xml.StartElement) (err
 // TTMLInItem represents an input TTML item
 type TTMLInItem struct {
 	Style string `xml:"style,attr,omitempty"`
-	Text  string `xml:",chardata"`
+	Text  string `xml:",innerxml"`


I'm not familiar with this tag, what difference does it make ?

asticode · 2021-08-11T07:36:39Z

ttml.go

@@ -326,34 +343,53 @@ func ReadFromTTML(i io.Reader) (o *Subtitles, err error) {
 		s.Style = o.Styles[id]
 	}

+	//take care of body style
+	defaultBodyStyle := ""


Could you rename this variable to bodyStyle?

asticode · 2021-08-11T07:36:46Z

ttml.go

@@ -326,34 +343,53 @@ func ReadFromTTML(i io.Reader) (o *Subtitles, err error) {
 		s.Style = o.Styles[id]
 	}

+	//take care of body style
+	defaultBodyStyle := ""
+	defaultRegion := ""


Could you rename this variable to bodyRegion?

asticode · 2021-08-11T07:37:48Z

ttml.go

+		defaultRegion = ttml.Body.Region
+	}
+	//if true create a new default body style and push it in o.Styles
+	if ttml.Body.TTMLInStyleAttributes != (TTMLInStyleAttributes{}) {


I'd rather make ttml.Body.TTMLInStyleAttributes a pointer in the struct so that it checks != nil here

asticode · 2021-08-11T08:02:53Z

ttml.go

+}
+
+//combineInlineStyles combines two StyleAttributes keeping everything from destination and adding missing data from source to destination
+func combineInlineStyles(source *StyleAttributes, destination StyleAttributes) *StyleAttributes {


I'd rather make this a func (s *StyleAttributes) merge(in *StyleAttributes) (out *StyleAttributes) and move it to subtitles.go

asticode · 2021-08-11T08:04:32Z

ttml.go

 	return
 }

+func checkIfCustomStyleExists(styleMap map[Style]string, style *Style) (bool, string) {


Could you transform this to func (ss ttmlCustomStyles) exists(s *Style) (id string, ok bool) instead ?

asticode · 2021-08-11T08:05:07Z

ttml.go

+						newStyle.Style = lineItem.Style.Style
+					}
+					newStyle.InlineStyle.propagateTTMLAttributes()
+					ok, id := checkIfCustomStyleExists(customStyleMap, newStyle)


Could you make this a one liner : if ok, id := checkIfCustomStyleExists(customStyleMap, newStyle); ok {

asticode · 2021-08-11T08:06:45Z

ttml.go

+
+	//Pushing Inline style from parent tag to every child tag then combining it with child tag's Style to create a new style and then assigning that style to the child tag.
+	customStyleNumber := 1
+	var customStyleMap = make(map[Style]string)


I feel it should be the other way around and with a pointer var customStyleMap = make(map[string]*Style), and you should rename this variables to customStyles

asticode · 2021-08-11T08:08:10Z

ttml.go

+
+	//Pushing Inline style from parent tag to every child tag then combining it with child tag's Style to create a new style and then assigning that style to the child tag.
+	customStyleNumber := 1
+	var customStyleMap = make(map[Style]string)


I'd rather you create a specific type for this called type ttmlCustomStyles map[string]*Style

NhanNguyen700 · 2023-11-09T09:06:06Z

This one looks interesting, any further work?

asticode · 2023-11-09T13:16:14Z

Requested changes need to be addressed (this was the first part of my review) but based on the fact that this PR is pretty old, I doubt the OP will ever do it.

If you feel like it, you can pick it up 👍

discovery-avishekgulshan and others added 9 commits June 14, 2021 22:36

propagating region style separately than rest of the styles

c590a35

reverting changes directly from master

e0dec33

Merge branch 'asticode:master' into master

0e23f6c

Migrating Style from ttml to vtt

4806b1d

Migrating Style from ttml to vtt

734eaaa

adding style migrations

11ed74a

updated propagateTTMLAttributes()

c605109

- Added style migration from TTML to WEBVTT

- Fixed Region

79e4958

- Added support for STYLE classes

- Setting parent <p> position and alignment as Players process TTML

aed04cf

- Pushing parent style and inline style to child <span> - Creating new custom style class after combing inline style and style class as multiple format doesn't support inline style + style

removing commented code

24cd423

Avishek-Gulshan added 5 commits July 21, 2021 23:04

fixing additional space issue while reading ttml

e48e299

fixing additional space issue while reading ttml

0cbc006

sorting style

f0b99c7

nil check for style on webvtt

114d3ae

nil check for style on webvtt

b86cb77

asticode requested changes Aug 11, 2021

View reviewed changes

asticode force-pushed the master branch 6 times, most recently from 1a6c252 to 012fd3a Compare July 27, 2023 09:40

asticode force-pushed the master branch from 012fd3a to 04eba85 Compare August 8, 2023 09:01

Avishek-Gulshan closed this Feb 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TTML to VTT style migration #48

TTML to VTT style migration #48

Avishek-Gulshan commented Jul 9, 2021

coveralls commented Jul 9, 2021 •

edited

Avishek-Gulshan commented Jul 9, 2021

Avishek-Gulshan commented Jul 13, 2021

asticode commented Jul 14, 2021

asticode left a comment

asticode Jul 14, 2021

asticode Aug 11, 2021

asticode Aug 11, 2021

asticode Aug 11, 2021

asticode Aug 11, 2021

asticode Aug 11, 2021

asticode Aug 11, 2021

asticode Aug 11, 2021

asticode Aug 11, 2021

asticode Aug 11, 2021

NhanNguyen700 commented Nov 9, 2023

asticode commented Nov 9, 2023

TTML to VTT style migration #48

TTML to VTT style migration #48

Conversation

Avishek-Gulshan commented Jul 9, 2021

coveralls commented Jul 9, 2021 • edited

Avishek-Gulshan commented Jul 9, 2021

Avishek-Gulshan commented Jul 13, 2021

asticode commented Jul 14, 2021

asticode left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NhanNguyen700 commented Nov 9, 2023

asticode commented Nov 9, 2023

coveralls commented Jul 9, 2021 •

edited