Parsing SCTE35 tag from playlist #40

vishal24tuniki · 2016-03-09T14:54:17Z

Adding feature as per SCTE standards
Ref : http://www.scte.org/documents/pdf/standards/SCTE%2067%202014.pdf 12.1.6.1 HLS Cue Tags

vishal24tuniki · 2016-03-13T05:43:30Z

Hey @grafov any plan on merging this code ? Anything more needed from my end ?

bradleyfalzon · 2016-03-13T07:34:09Z

We'd also like to see this get merged, but a few personal queries/comments:

Acronyms such as SCTE should be written as SCTE instead of Scte (eg URI is used in other variables, not Uri), such as SetScte and MediaSegment.ScteData
Where there's ScteData, perhaps it should be renamed to SCTECue - as (I believe, but correct me if I'm wrong) it contains the SCTE Cue attribute, as the SCTE standard also supports ID and Time attributes, and may contain other tags in the future, therefore SCTECue maybe a more suitable name.
Missing some unit tests for the reader and writer, I know this package is missing many, but I think this PR would easily support some (I can assist but not for a few days).

vishal24tuniki · 2016-03-13T08:06:08Z

sure @bradleyfalzon I will do whatever i can from my end

grafov · 2016-03-13T21:06:07Z

Sorry for delays too busy. I will merge it after small check ASAP.

bradleyfalzon · 2016-03-15T19:45:51Z

I'm also thinking that these SCTE tags should be added to it's own struct, such as:

type MediaSegment struct {
    SeqId           uint64
    Title           string
    URI             string
    Duration        float64
    Limit           int64
    Offset          int64
    Key             *Key
    Map             *Map
    SCTE            *SCTE
    Discontinuity   bool
    ProgramDateTime time.Time
}

type SCTE struct {
    Cue string
    ID string
    Time time.Time
}

This way it's easier to add extra tags in the future, and contains all SCTE in the same way it's done for Key and Map. This is a slightly more substantial change to the code, but I think it's straight forward to do and makes sense in relation to how other optional tags are handled.

Thoughts?

vishal24tuniki · 2016-03-16T05:54:14Z

@bradleyfalzon makes perfect sense. Will do these changes in an hour or two and update the pull request

vishal24tuniki · 2016-03-16T07:07:58Z

Hey guys @bradleyfalzon @grafov done withe changes as discussed above

bradleyfalzon · 2016-03-16T07:17:49Z

reader.go

-		for _, sep := range scteData {
+		SCTEData := strings.Split(line[12:], ",")
+		state.scte = new(SCTE)
+		for _, sep := range SCTEData {


Can this for loop use the decodeParamsLine, similar to the EXT-X-KEY on line 396? Or is there a quoting issue? As I think this loop expects an example line to look like #EXT-SCTE35: CUE=SOMEDATA,ID=SOMEID,TIME=3.142, whereas the example in http://www.scte.org/documents/pdf/standards/SCTE%2067%202014.pdf shows the fields should be quoted.

I guess so, i will look up that function after some time and make use of it/ write something similar for our purpose

My apologies, the for loop does correctly handle quotes for strings, and no quotes for float, which is expected - sorry for the confusion. But I think decodeParamsLine may still be suitable, such as:

case !state.tagSCTE35 && strings.HasPrefix(line, "#EXT-SCTE35:"): state.tagSCTE35 = true state.listType = MEDIA state.scte = new(SCTE) for k, v := range decodeParamsLine(line[12:]) { switch k { case "CUE": state.scte.Cue = v case "ID": state.scte.ID = v case "TIME": state.scte.Time, _ = strconv.ParseFloat(v, 64) } }

Which handles a sample playlist like this nicely:

#EXTM3U #EXT-X-TARGETDURATION:10 #EXT-X-VERSION:3 #EXT-X-MEDIA-SEQUENCE:00001 #EXTINF:9.9, http://server-host/path/file44.ts #EXTINF:4.2, http://server-host/path/file45.ts #EXT-SCTE35: CUE="/DAIAAAAAAAAAAAQAAZ/I0VniQAQAgBDVUVJQAAAAH+cAAAAAA==" #EXTINF:10 stream_med_00001.ts #EXTINF:4, stream_med_00002.ts #EXT-SCTE35: CUE="/DAIAAAAAAAAAAAQAAZ/I0VniQAQAgBDVUVJQAAAAH+cAAAAAA==", ID="someidentifier1", TIME=3.142 #EXTINF:6, stream_med_00003.ts #EXTINF:4, stream_med_00004.ts #EXT-SCTE35: CUE="/DAIAAAAAAAAAAAQAAZ/I0VniQAQAgBDVUVJQAAAAH+cAAAAAA==", ID="someidentifier2", TIME="3.142" #EXTINF:6, stream_med_00005.ts #EXTINF:6, stream_med_00006.ts #EXT-SCTE35:CUE="/DAIAAAAAAAAAAAQAAZ/I0VniQAQAgBDVUVJQAAAAH+cAAAAAA==",ID="someidentifier3",TIME="3.142" #EXTINF:6, stream_med_00007.ts #EXT-X-ENDLIST

Thanks again for these minor changes, the PR is useful and we are thankful for it, feel free to add yourself to the AUTHORS file too, if not, we'll add it at a later stage.

bradleyfalzon · 2016-03-16T07:27:02Z

New PR looks good, I've added a comment re decoding.

I'd love to see a good unit test for this as well for the reader and writer changes as I think it would provide a lot of value to see how this is expected to be used. Perhaps adding an example in sample-playlists folder (just a couple EXT-SCTE35 tags with just CUE tag and one with CUE+ID+TIME) and adding a test to reader_test.go and writer_test.go to verify it's behaving correctly. I can assist an example once you clarify the decoding comment (so I know that quoting should be required).

…in a line

vishal24tuniki · 2016-03-16T10:36:22Z

Hey @bradleyfalzon done with the changes :)

bradleyfalzon · 2016-03-16T21:35:32Z

Brilliant, looks good will test and merge hopefully within the next 12 hours.

I didn't notice before, but it looks like this is just reader methods, not writer, I can add them in at a later stage, something like the following (can you double check my suggestion) but with tests. Did you have plans to add that in the future, if not, I can.

--- a/writer.go+++ b/writer.go
@@ -449,6 +449,22 @@ func (p *MediaPlaylist) Encode() *bytes.Buffer {
                if p.winsize > 0 { // skip for VOD playlists, where winsize = 0
                        i++
                }
+               if seg.SCTE != nil {
+                       p.buf.WriteString("#EXT-X-SCTE:")
+                       p.buf.WriteString("CUE=\"")
+                       p.buf.WriteString(seg.SCTE.Cue)
+                       p.buf.WriteRune('"')
+                       if seg.SCTE.ID != "" {
+                               p.buf.WriteString(",ID=\"")
+                               p.buf.WriteString(seg.SCTE.ID)
+                               p.buf.WriteRune('"')
+                       }
+                       if seg.SCTE.Time != 0 {
+                               p.buf.WriteString(",TIME=")
+                               p.buf.WriteString(strconv.FormatFloat(seg.SCTE.Time, 'f', 3, 64))
+                       }
+                       p.buf.WriteRune('\n')
+               }
                // check for key change
                if seg.Key != nil && p.Key != seg.Key {
                        p.buf.WriteString("#EXT-X-KEY:")

vishal24tuniki · 2016-03-17T01:22:12Z

@bradleyfalzon Yeah, please go ahead and add the writer code. I just didn't go much through the writer methods, but your code gave me a good idea. Just 1 suggestion, i feel you should use precision as -1 instead of 3, which would help to reform the float64 variable Time back

Refer : https://golang.org/pkg/strconv/#FormatFloat
(The special precision -1 uses the smallest number of digits necessary such that ParseFloat will return f exactly.)

I can create another PR for writer test cases later

Parsing SCTE35 tag from playlist

bradleyfalzon · 2016-04-05T22:31:31Z

@vishal24tuniki Did you check out the updated HLS draft RFC's SCTE35 support? https://www.ietf.org/rfcdiff?url1=draft-pantos-http-live-streaming-18&url2=draft-pantos-http-live-streaming-19

vishal24tuniki · 2016-04-06T05:26:58Z

@bradleyfalzon interesting ! Will go through them in next couple of days

Parsing SCTE35 tag from playlist

Parsing SCTE35 tag from playlist

07eeb53

grafov added the enhancement label Mar 9, 2016

Vishal added 2 commits March 16, 2016 06:35

Using a different struct for all SCTE attributes

3146aef

len func applied on correct value

394d315

Removing unnecessary wrapping

336e130

bradleyfalzon reviewed Mar 16, 2016
View reviewed changes

Vishal added 3 commits March 16, 2016 08:32

Making use of decodeParamsLine func to extract attributes and values …

25d13fe

…in a line

Added test cases for reader for scte35

0114bd5

Adding in AUTHORS file

db4ff05

Merge branch 'master' of https://github.com/grafov/m3u8

7009ef2

bradleyfalzon added a commit that referenced this pull request Mar 17, 2016

Merge pull request #40 from amagimedia/master

027badc

Parsing SCTE35 tag from playlist

bradleyfalzon merged commit 027badc into grafov:master Mar 17, 2016

bradleyfalzon mentioned this pull request Mar 20, 2016

Support EXT-X-SCTE when encoding playlists #41

Merged

bradleyfalzon mentioned this pull request May 31, 2016

Add support for multiple SCTE-35 tag syntax #60

Closed

grafov pushed a commit that referenced this pull request Nov 22, 2016

Merge pull request #40 from amagimedia/master

4a78874

Parsing SCTE35 tag from playlist

Parsing SCTE35 tag from playlist #40

Parsing SCTE35 tag from playlist #40

Uh oh!

Conversation

vishal24tuniki commented Mar 9, 2016

Uh oh!

vishal24tuniki commented Mar 13, 2016

Uh oh!

bradleyfalzon commented Mar 13, 2016

Uh oh!

vishal24tuniki commented Mar 13, 2016

Uh oh!

grafov commented Mar 13, 2016

Uh oh!

bradleyfalzon commented Mar 15, 2016

Uh oh!

vishal24tuniki commented Mar 16, 2016

Uh oh!

vishal24tuniki commented Mar 16, 2016

Uh oh!

bradleyfalzon Mar 16, 2016

Choose a reason for hiding this comment

Uh oh!

vishal24tuniki Mar 16, 2016

Choose a reason for hiding this comment

Uh oh!

bradleyfalzon Mar 16, 2016

Choose a reason for hiding this comment

Uh oh!

bradleyfalzon commented Mar 16, 2016

Uh oh!

vishal24tuniki commented Mar 16, 2016

Uh oh!

bradleyfalzon commented Mar 16, 2016

Uh oh!

vishal24tuniki commented Mar 17, 2016

Uh oh!

bradleyfalzon commented Apr 5, 2016

Uh oh!

vishal24tuniki commented Apr 6, 2016

Uh oh!

Uh oh!