Media Playlist Set* methods modifying wrong segments

[bradleyfalzon]

I believe there maybe an issue with the Set* methods when the media playlist capacity is not a power of 2. It appears that once the playlist is full, a Set* (such as SetDiscontinuity() or SetKey()) sets the correct value on the wrong segment.

The following should demonstrate this, ignore the fact I'm ignoring errors, even when checked the problem still occurs. The SetKey() method best shows the issue as I can set the key URI (and IV) to be the same as the segment URI to better illustrate the problem, but the same occurs with SetDiscontinuity() (and likely other methods that use the p.Segments[(p.tail-1)%p.capacity] calculation).

package main

import (
    "fmt"

    "github.com/grafov/m3u8"
)

func main() {
    // OK when size is 1,2,4,8,16....
    // Not OK when size is 3,5,6,7,9,10...
    size := uint(5)
    pls, _ := m3u8.NewMediaPlaylist(size, size)

    for i := uint(0); i < size; i++ {
        uri := fmt.Sprintf("uri-%d", i)
        _ = pls.Append(uri+".ts", 4, "")
        _ = pls.SetKey("AES-128", uri+".key", fmt.Sprintf("%d", i), "", "")
    }

    fmt.Print(pls)
}

Output

#EXTM3U
#EXT-X-VERSION:3
#EXT-X-MEDIA-SEQUENCE:1
#EXT-X-TARGETDURATION:4
#EXT-X-KEY:METHOD=AES-128,URI="uri-4.key",IV=4 <- the 5th iteration overwrote the 1st's
#EXTINF:4.000,
uri-0.ts
#EXT-X-KEY:METHOD=AES-128,URI="uri-1.key",IV=1 <- 2nd iteration correct
#EXTINF:4.000,
uri-1.ts
#EXT-X-KEY:METHOD=AES-128,URI="uri-2.key",IV=2 <- 3rd iteration correct
#EXTINF:4.000,
uri-2.ts
#EXT-X-KEY:METHOD=AES-128,URI="uri-3.key",IV=3 <- 4th iteration correct
#EXTINF:4.000,
uri-3.ts
#EXTINF:4.000,
uri-4.ts  <- 5th iteration is missing the EXT-X-KEY

Generally, it looks like the problem is that the .Append() method sets playlist tail property (p.tail) to be modulo capacity and stores it back in p.tail (p.tail is always between 0 and p.capacity). Whereas the Set* methods assume this is a incrementing integer (based on they're calculation of (p.tail-1)%p.capacity). Essentially, because p.tail wraps to zero and because it is a uint, when p.tail is 0 the result of p.tail-1 is 18446744073709551615. Changing this to an int doesn't quite fix things either, at least not without still requiring further changes within the Set* methods (but this may also be the preferred solution) and more casting.

I believe a simple change maybe:

@@ -275,8 +276,8 @@ func (p *MediaPlaylist) Append(uri string, duration float64, title string) error
        seg.URI = uri
        seg.Duration = duration
        seg.Title = title
-       p.Segments[p.tail] = seg
-       p.tail = (p.tail + 1) % p.capacity
+       p.Segments[p.tail%p.capacity] = seg
+       p.tail++
        p.count++
        if p.TargetDuration < duration {
                p.TargetDuration = math.Ceil(duration)

There's other methods to fix it in each Set* method, via (something similar to, but perhaps using a private tail() method to obtain the correct tail):

+               tail := p.tail - 1
+               if p.tail == 0 {
+                       tail = p.capacity - 1
+               }
+               // OR: tail := int(math.Min(float64(p.tail-1), float64(p.capacity-1)))
-               p.Segments[(p.tail-1)%p.capacity].Key = &Key{method, uri, iv, keyformat, keyformatversions}
+               p.Segments[tail].Key = &Key{method, uri, iv, keyformat, keyformatversions}

I wanted to get hear others' thoughts on this before I go too much further, as this maybe a misunderstanding, maybe another solution, or this proposed change may cause other unintended consequences. This isn't a straight forward issue as there's multiple causes (depending on your opinion) and multiple solutions.

I'll be happy to send in a PR + tests and also address the Remove() methods (as well as the corresponding head variable), just focusing on the Append() and Set* methods first.

Note, I can't reproduce the issue when there capacity is a power of 2, e.g.: 1,2,4,8,16 - this may explain why the issue hasn't been discussed before?

I think there's two ways to solve this, move tail/head to ints and handle negatives in Set* methods, or use the above method (tail to always be incrementing). I'll continue to have a bit of a think about this, but I want to hear others' opinions.

[bradleyfalzon]

I had considered proposing alternatives to this exact FIFO method (looking at these examples: https://gist.github.com/moraes/2141121), where we could have auto extending playlists (see rasky's suggestion), but this would potentially be a breaking change, as others may depend on the playlist full behaviour, either case, this version should still be fixed (if it isn't intentional behaviour).

[bradleyfalzon]

@grafov when you have time, do you have any thoughts on this? Importantly, am I doing something wrong here, or is this an actual problem?

[grafov]

Finally I looked this bug, it reproduced as explained above. I fully agree with @bradleyfalzon, my usage of uint without check was wrong. Bradley please merge your commit it fix problem properly.

[grafov]

Well I found pull request and merged it. Thank you!

[bradleyfalzon]

Thanks @grafov, much appreciated