Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed bug of writing vod playlist #38

Merged
merged 3 commits into from
Jul 31, 2016

Conversation

hori-ryota
Copy link
Contributor

@@ -443,7 +443,7 @@ func (p *MediaPlaylist) Encode() *bytes.Buffer {

head := p.head
count := p.count
for i := uint(0); i < p.winsize && count > 0; count-- {
for i := uint(0); (i < p.winsize || p.winsize == 0) && count > 0; count-- {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the behaviour here is to support VoD where winsize == 0, perhaps it might be better to write as:

    for i := uint(0); p.Closed || i < p.winsize && count > 0; count-- {

Then a live playlist can still support winsize of 0, but perhaps if the content is VoD, winsize should've been count?

I don't also fully understand (haven't looked yet), why this would get into this state, seems like if it is VoD - winsize should just match count.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p.Closed || i < p.winsize && count > 0 means (p.Closed || i < p.winsize) && count > 0 ?

lgtm, but I feel like having the demand that wants to show everything in live.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, you're right, it should have be written as:

    for i := uint(0); (p.Closed || i < p.winsize && count > 0); count-- {

The original proposal (i < p.winsize || p.winsize == 0) && count > 0 would ignore the winsize when it is zero, but didn't express the fact we're doing this because VoD has a winsize of zero. So I proposed we switch this to (i < p.winsize || p.Closed) && count > 0 which expresses the previous statement a little more clearly. But this would then ignore the winsize of any VoD content, which almost certainly isn't what anyone wants (it's OK for the moment as the winsize cannot be changed, but this behaviour may change in future versions).

So we need to check why the winsize is getting to 0 for VoD content in the first place, and perhaps have it set to count instead, as I believe that's where the root cause is (but I haven't looked closely at this either, seeing an example would help).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, regardless of where the underlying issue it, it appears the docs require winsize to be 0 for VoD:

winsize uint // max number of segments displayed in an encoded playlist; need set to zero for VOD playlists

So I think your original proposal is likely the best option for backwards compatibility.

@bradleyfalzon
Copy link
Collaborator

In the original issue #22, what's wrong with the originally submitted PR? What makes this PR different?

@hori-ryota
Copy link
Contributor Author

originally submitted PR write chunk more 1 than the winsize.

@grafov grafov added the bug label Feb 21, 2016
@hori-ryota
Copy link
Contributor Author

Would you merge this?

@bradleyfalzon
Copy link
Collaborator

Hi @hori-ryota

Sorry for the delay in this, can you let me know exactly how to reproduce (ideally via an additional test) and I'll verify and merge.

@hori-ryota
Copy link
Contributor Author

@bradleyfalzon sorry for delay. I added test. how about this?

@grafov
Copy link
Owner

grafov commented Jul 31, 2016

Sorry for delay too. I checked the code and test and think the bugfix is ok. Thank you @hori-ryota!

@grafov grafov merged commit 2244be1 into grafov:master Jul 31, 2016
grafov added a commit that referenced this pull request Nov 22, 2016
Fix bug of writing vod playlist when winsize=0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants