Add CLOSED-CAPTIONS= to writer for (STREAM-INF) #79

mbowBC · 2017-03-02T15:53:22Z

No description provided.

coveralls · 2017-03-02T16:04:55Z

Coverage decreased (-0.2%) to 58.845% when pulling 8201bad on mbowBC:ADD_CC_WRITE into f55aa7a on grafov:master.

bradleyfalzon · 2017-03-03T00:15:10Z

Would you be able to add reader support and tests?

EDIT: sorry, there is reader support already, but can you add a test for both reader and writer?

coveralls · 2017-03-03T17:41:28Z

Coverage increased (+4.8%) to 63.78% when pulling 81e7f23 on mbowBC:ADD_CC_WRITE into f55aa7a on grafov:master.

coveralls · 2017-03-03T17:43:09Z

Coverage increased (+4.8%) to 63.78% when pulling 5566280 on mbowBC:ADD_CC_WRITE into f55aa7a on grafov:master.

bradleyfalzon

Thanks for the extra tests, I've added some feedback, we should be able to simplify some of these and still achieve a decent test.

bradleyfalzon · 2017-03-04T02:20:20Z

reader_test.go

+	}
+
+	for i, v := range p.Variants {
+		//spew.Dump(p.Variants)


Comment can be removed

bradleyfalzon · 2017-03-04T02:21:49Z

reader_test.go

+		if i == 0 && len(v.Alternatives) != 5 {
+			t.Fatalf("not all alternatives from #EXT-X-MEDIA parsed (has %d but should be 5", len(v.Alternatives))
+		}
+		if i > 0 && len(v.Alternatives) > 0 {


Neither of these if blocks belong here, looks like they were from the previous test testing Alternatives, feel free to just have the one if v.Captions ~!= ..., that's all you need to test for.

bradleyfalzon · 2017-03-04T02:24:38Z

reader_test.go

@@ -96,7 +96,40 @@ func TestDecodeMasterPlaylistWithAlternatives(t *testing.T) {
 			t.Fatal("should not be alternatives for this variant")
 		}
 	}
-	// fmt.Println(p.Encode().String())
+	//fmt.Println(p.Encode().String())


This line probably didn't need to be changed. We can remove it in another cleanup.

bradleyfalzon · 2017-03-04T02:24:58Z

reader_test.go

+			t.Fatal("variant field for CLOSED-CAPTIONS should be equal to NONE but it equals", v.Captions)
+		}
+	}
+	//fmt.Println(p.Encode().String())


This can be removed, it appears to have just come from a copy/paste.

bradleyfalzon · 2017-03-04T02:29:51Z

writer_test.go

+}
+
+// Create new master playlist supporting closed-caption=none
+func TestNewMasterPlaylistWithClosedCaptionEqNone(t *testing.T) {


This is a very comprehensive test, but I don't think it really needs to be. Can the same writing test be achieved by just having a single master playlist, with one variant setting the Alternative Closed Captions to none?

And then the test can be a straight forward strings.Contains, such as

m3u8/writer_test.go

Lines 192 to 221 in f55aa7a

func TestSetSCTEForMediaPlaylist(t *testing.T) {

tests := []struct {

Cue string

ID string

Time float64

Expected string

}{

{"CueData1", "", 0, `#EXT-SCTE35:CUE="CueData1"` + "\n"},

{"CueData2", "ID2", 0, `#EXT-SCTE35:CUE="CueData2",ID="ID2"` + "\n"},

{"CueData3", "ID3", 3.141, `#EXT-SCTE35:CUE="CueData3",ID="ID3",TIME=3.141` + "\n"},

{"CueData4", "", 3.1, `#EXT-SCTE35:CUE="CueData4",TIME=3.1` + "\n"},

{"CueData5", "", 3.0, `#EXT-SCTE35:CUE="CueData5",TIME=3` + "\n"},

}

for _, test := range tests {

p, e := NewMediaPlaylist(1, 1)

if e != nil {

t.Fatalf("Create media playlist failed: %s", e)

}

if e = p.Append("test01.ts", 5.0, ""); e != nil {

t.Errorf("Add 1st segment to a media playlist failed: %s", e)

}

if e := p.SetSCTE(test.Cue, test.ID, test.Time); e != nil {

t.Errorf("SetSCTE to a media playlist failed: %s", e)

}

if !strings.Contains(p.String(), test.Expected) {

t.Errorf("Test %+v did not contain: %q, playlist: %v", test, test.Expected, p.String())

}

}

}

?

ok will do.

bradleyfalzon · 2017-03-04T02:33:19Z

writer_test.go

+	if m.ver != 4 {
+		t.Fatalf("Expected version 4, actual, %d", m.ver)
+	}
+	// diff with master file using reader.


See comment on test, but I don't think this test needs to be any more complicated than a strings.Contains.

If we do want this comprehensive test (@grafov thoughts?), then this test should be renamed to something like TestNewMasterPlaylistWithAlternates and a simpler string comparison should be made instead of bringing in difflib.

I think as the library code is simple and doesn't need external deps so the tests should avoid external deps too.

coveralls · 2017-03-06T12:13:55Z

Coverage increased (+6.2%) to 65.205% when pulling e4e2dd5 on mbowBC:ADD_CC_WRITE into f55aa7a on grafov:master.

coveralls · 2017-03-06T13:38:05Z

Coverage increased (+6.0%) to 65.019% when pulling 796b493 on mbowBC:ADD_CC_WRITE into f55aa7a on grafov:master.

coveralls · 2017-03-06T14:09:18Z

Coverage increased (+6.0%) to 65.019% when pulling bc89f66 on mbowBC:ADD_CC_WRITE into f55aa7a on grafov:master.

bradleyfalzon · 2017-03-06T21:34:15Z

I can see more commits, and they're looking good, but you haven't stated you're finished, so I'll await your confirmation before reviewing.

mbowBC · 2017-03-06T21:44:49Z

I think that's it, let me know if I've missed anything.

bradleyfalzon · 2017-03-06T22:02:31Z

writer_test.go

+// Create new master playlist supporting closed-caption=none
+func TestNewMasterPlaylistWithClosedCaptionEqNone(t *testing.T) {
+	m := NewMasterPlaylist()
+	var alts = []*Alternative{


One more thing, is this still required? If it's just to test for captions == none, I don't think we need an alternative for it? I'm not familiar with alternative or captions myself in m3u8 spec, so if an Alternative is required to set Captions to none then I'm OK with leaving it in, but if it's not required, then I think we can remove this from here too.

bradleyfalzon · 2017-03-06T22:03:07Z

This is fine, one minor comment, but then I think we can merge.

mbowBC · 2017-03-07T11:06:54Z

Thanks yes looks like don't need set up Alts, so have removed it.

coveralls · 2017-03-07T11:17:50Z

Coverage increased (+6.0%) to 65.019% when pulling 4d17293 on mbowBC:ADD_CC_WRITE into f55aa7a on grafov:master.

bradleyfalzon · 2017-03-07T11:20:57Z

You're a champion, thanks for the time, looks good to me but the CI failure seems to be a gofmt issue, did you not have a chance to run that - could you?

coveralls · 2017-03-07T21:00:03Z

Coverage increased (+6.0%) to 65.019% when pulling 1febd27 on mbowBC:ADD_CC_WRITE into f55aa7a on grafov:master.

mbowBC · 2017-03-07T21:00:35Z

apologies no idea why Gogland IDE didn't do that, cli way works fine.

bradleyfalzon · 2017-03-07T21:09:39Z

Thanks for the contribution @mbowBC

add closed captions to writer.

8201bad

mbowBC added 2 commits March 3, 2017 17:34

add tests

81e7f23

add new playlist

5566280

bradleyfalzon suggested changes Mar 4, 2017

View reviewed changes

grafov added the enhancement label Mar 4, 2017

trim tests down.

e4e2dd5

remove printf

796b493

cant use t.name in older version of go, replace.

bc89f66

bradleyfalzon approved these changes Mar 6, 2017

View reviewed changes

bradleyfalzon reviewed Mar 6, 2017

View reviewed changes

Alternatives are not needed remove.

4d17293

go fmt.

1febd27

bradleyfalzon merged commit e521730 into grafov:master Mar 7, 2017

	func TestSetSCTEForMediaPlaylist(t *testing.T) {
	tests := []struct {
	Cue string
	ID string
	Time float64
	Expected string
	}{
	{"CueData1", "", 0, `#EXT-SCTE35:CUE="CueData1"` + "\n"},
	{"CueData2", "ID2", 0, `#EXT-SCTE35:CUE="CueData2",ID="ID2"` + "\n"},
	{"CueData3", "ID3", 3.141, `#EXT-SCTE35:CUE="CueData3",ID="ID3",TIME=3.141` + "\n"},
	{"CueData4", "", 3.1, `#EXT-SCTE35:CUE="CueData4",TIME=3.1` + "\n"},
	{"CueData5", "", 3.0, `#EXT-SCTE35:CUE="CueData5",TIME=3` + "\n"},
	}

	for _, test := range tests {
	p, e := NewMediaPlaylist(1, 1)
	if e != nil {
	t.Fatalf("Create media playlist failed: %s", e)
	}
	if e = p.Append("test01.ts", 5.0, ""); e != nil {
	t.Errorf("Add 1st segment to a media playlist failed: %s", e)
	}
	if e := p.SetSCTE(test.Cue, test.ID, test.Time); e != nil {
	t.Errorf("SetSCTE to a media playlist failed: %s", e)
	}
	if !strings.Contains(p.String(), test.Expected) {
	t.Errorf("Test %+v did not contain: %q, playlist: %v", test, test.Expected, p.String())
	}
	}
	}

Add CLOSED-CAPTIONS= to writer for (STREAM-INF) #79

Add CLOSED-CAPTIONS= to writer for (STREAM-INF) #79

Uh oh!

Conversation

mbowBC commented Mar 2, 2017

Uh oh!

coveralls commented Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bradleyfalzon commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bradleyfalzon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bradleyfalzon commented Mar 6, 2017

Uh oh!

mbowBC commented Mar 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradleyfalzon commented Mar 6, 2017

Uh oh!

mbowBC commented Mar 7, 2017

Uh oh!

coveralls commented Mar 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bradleyfalzon commented Mar 7, 2017

Uh oh!

coveralls commented Mar 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbowBC commented Mar 7, 2017

Uh oh!

bradleyfalzon commented Mar 7, 2017

Uh oh!

Uh oh!

coveralls commented Mar 2, 2017 •

edited

Loading

bradleyfalzon commented Mar 3, 2017 •

edited

Loading

coveralls commented Mar 3, 2017 •

edited

Loading

coveralls commented Mar 3, 2017 •

edited

Loading

coveralls commented Mar 6, 2017 •

edited

Loading

coveralls commented Mar 6, 2017 •

edited

Loading

coveralls commented Mar 6, 2017 •

edited

Loading

coveralls commented Mar 7, 2017 •

edited

Loading

coveralls commented Mar 7, 2017 •

edited

Loading