Skip to content
This repository was archived by the owner on Dec 8, 2024. It is now read-only.
This repository was archived by the owner on Dec 8, 2024. It is now read-only.

Media Playlist Set* methods modifying wrong segments #36

@bradleyfalzon

Description

@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.

Metadata

Metadata

Assignees

Labels

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions