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

gots.InsertPTS improvements #121

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

tmm1
Copy link
Contributor

@tmm1 tmm1 commented May 8, 2019

cleaned up the tests a bit, and fixed what appears to be a bug in the marker bit insertion.

@@ -129,7 +129,7 @@ func InsertPTS(b []byte, pts uint64) {
b[4] = byte(pts&0xff) << 1 // PTS[7..0]

// Set the marker bits as appropriate
b[0] |= 0x21
b[0] |= 0x01
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not really sure about this change. The tests pass with and without it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I'm glad you commented on this, as we don't want to change from 0x21 to 0x01. 0x21 sets the PTS_DTS_flags field to "has PTS". However, I will agree that's a bit sneaky and we should probably include a clarifying comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, so that means this function is not suitable for writing DTS. Don't the number of marker bits required vary depending on whether DTS is being written also?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You could use this function to write DTS, but you would have to set the mask on b[0] to 0x11 for the DTS portion of the PTS_DTS field and then append the two generated byte buffers together. Realistically I don't think that's the best plan if you need to write both, but it would be your code, not mine :) The actual marker_bits are the same between the two fields, and they don't change position or anything like that if you have both PTS and DTS.

If you need to write a PTS_DTS field, rather than just PTS, I would probably recommend implementing a InsertPTSDTS(b []byte, pts uint64, dts uint64) where you could call InsertPTS(b[:5], pts) and then insert the DTS and modify the PTS_DTS_flag (b[0]) accordingly.

@@ -129,7 +129,7 @@ func InsertPTS(b []byte, pts uint64) {
b[4] = byte(pts&0xff) << 1 // PTS[7..0]

// Set the marker bits as appropriate
b[0] |= 0x21
b[0] |= 0x01
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I'm glad you commented on this, as we don't want to change from 0x21 to 0x01. 0x21 sets the PTS_DTS_flags field to "has PTS". However, I will agree that's a bit sneaky and we should probably include a clarifying comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants