Add (seg MediaSegment)String()

[keizo042]

related #98

[coveralls]

Coverage decreased (-5.05%) to 66.278% when pulling 178325b on keizo042:media_segment_string into d137fcd on grafov:master.

[bradleyfalzon]

I'm concerned that we'd now have two different methods to write a segment.

Can we not move the following to an unexported function, which takes a *m3u8.MediaSegment and *bytes.Buffer and writes to the provided buffer?

https://github.com/grafov/m3u8/blob/master/writer.go#L483-L600

As long as performance isn't drastically affected.

[coveralls]

Coverage decreased (-4.7%) to 66.589% when pulling 9177d90 on keizo042:media_segment_string into d137fcd on grafov:master.

[coveralls]

Coverage decreased (-0.5%) to 70.83% when pulling dde4c97 on keizo042:media_segment_string into d137fcd on grafov:master.

[keizo042]

@bradleyfalzon
Thank you for your review.
I'd fix that the functions writing XXX use provided buffer and replace duplicated code to unexported function.

[bradleyfalzon]

Thanks @keizo042 that change is better, but there's still some duplication of the if statements. We've created a few new unexported functions, which is good, but I'd think there should just be one function that both MediaSegment.String() and *MediaPlaylist.Encode() call.

Unless there's a specific reason we can't do that?

[keizo042]

@bradleyfalzon

I'd think there should just be one function that both MediaSegment.String() and *MediaPlaylist.Encode() call.
Unless there's a specific reason we can't do that?

I agree but there is two difference.
in (p *MediaPlaylist) String, writing m3u8.Map and m3u8.Key depends on previous media segments.
but in the function I request, I think it is good to write buffer if they exist.

In addition, (p *MediaPlaylist) String() have previous durations cache.
but (seg MediaSegment) String() is stateless.
I'd like to remove caching procedure in (seg MediaSegment) String()`.

so it needs writeKey and writeMap.
if I shoud not break order of segments attribute, writeSCTE also.

I feel need to create unexported function as rest part of writing media segment in order to remove duplicated code.
I don't have good idea how it maanges caching yet.

[bradleyfalzon]

depends on previous media segments.

Ah yes, I see. This is unfortunate.

have previous durations cache

Yeah, so this is so for very large playlists, we don't continue to call the intensive strconv.FormatFloat.

I then understand why you've chosen this method, but I do prefer if we could remove the duplication completely. I don't mind duplicate code, but there's a lot of logic here that's being duplicated and that's what concerns me.

What if there was a function like

// writeSegment writes a string representation of seg to buf.
//
// durationCache is required to reduce the number of calls to repetitive strconv formats. 
// If playlist is non-nil, additional context is derived from the playlist.
func writeSegment(seg MediaSegment, buf bytes.Buffer, durationCache map[float]string, playlist *MediaPlaylist, buf bytes.Buffer)

Then the same if statements that used information from the playlist would first check if playlist is non-nil.

-if p.Map == nil && seg.Map != nil {
+if p != nil && p.Map == nil && seg.Map != nil {

Or similar? I'm not 100% for my suggestion, just asking your thoughts.

[keizo042]

That's good idea.
I'd like to show all infomation when the context is not provided.
I think better like this.

func (seg MediaSegment) write(buf bytes.Buffer, p *MediaPlaylist, durationCache map[string]float
) {
...  
      if p != nil {
                if p.Map == nil && seg.Map != nil {
                        writeMap(buf, seg.Map)
                }
        } else {
                writeMap(buf, seg.Map)
        }
...
        if p != nil {
                // original caching and conversion
        } else {
                buf.WriteString(strconv.FormatFloat(seg.Duration, 'f', 3, 32))
        }
...
}

[coveralls]

Coverage increased (+0.3%) to 71.644% when pulling 6255234 on keizo042:media_segment_string into d137fcd on grafov:master.

[bradleyfalzon]

This is looking good to me I think.

Could you run the benchmarks before and after to check for performance regressions?

[keizo042]

benchmark resutls

Env

• CentOS 7.4
• go version go1.8.3 linux/amd64

result

[m3u8]$ go test -bench=.
BenchmarkDecodeMasterPlaylist-12           50000             29740 ns/op
BenchmarkDecodeMediaPlaylist-12              100          21899250 ns/op
BenchmarkEncodeMasterPlaylist-12         1000000              1426 ns/op
BenchmarkEncodeMediaPlaylist-12              200           6463614 ns/op
PASS
ok      github.com/grafov/m3u8  7.455s


[m3u8]$ git checkout media_segment_string
Switched to branch 'media_segment_string'
[m3u8]$ go test -bench=.
BenchmarkDecodeMasterPlaylist-12           50000             29644 ns/op
BenchmarkDecodeMediaPlaylist-12              100          21823718 ns/op
BenchmarkEncodeMasterPlaylist-12         1000000              1420 ns/op
BenchmarkEncodeMediaPlaylist-12              200           6974774 ns/op
PASS
ok      github.com/grafov/m3u8  7.569s

only Encode MediaPlaylist benchmark

[m3u8]$ git checkout master
Already on 'master'
m3u8]$ go test -bench=BenchmarkEncodeMediaPlaylist
BenchmarkEncodeMediaPlaylist-12              200           6456721 ns/op
PASS
ok      github.com/grafov/m3u8  2.000s

[m3u8]$ git checkout media_segment_string
Switched to branch 'media_segment_string'

[m3u8]$ go test -bench=BenchmarkEncodeMediaPlaylist
BenchmarkEncodeMediaPlaylist-12              200           6951293 ns/op
PASS
ok      github.com/grafov/m3u8  2.131s
[m3u8]$

well... I think we prefer that peformance regression is under 0.1sec.
I take inlining writeSCTE and invesitage in detail.