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

Muxer: increase Continuity Counter of PAT and PMT #29

Merged
merged 1 commit into from
Jul 5, 2021
Merged

Muxer: increase Continuity Counter of PAT and PMT #29

merged 1 commit into from
Jul 5, 2021

Conversation

aler9
Copy link
Contributor

@aler9 aler9 commented Jul 3, 2021

Hello, this library allowed rtsp-simple-server to convert seamlessly from RTSP to HLS during the last months, hence a big thank you from the entire community.

As you may already know, HLS is a video/audio streaming format that uses MPEG-TS segments, containing H264 and AAC.

The only change i had to do to generate valid HLS was hacking a couple of functions in order to increase the continuity counter of PAT and PMT after every delivery.

Although the specification is not clear enough about the continuity counter of PAT and PMT, when it is not increased, VLC starts printing these kind of errors:

[00007f80ec001180] ts demux error: libdvbpsi error (PSI decoder): TS duplicate (received 0, expected 1) for PID 0
[00007f80ec001180] ts demux error: libdvbpsi error (PSI decoder): TS duplicate (received 0, expected 1) for PID 4096

At the same time, hls.js (JS library that converts from MPEG-TS to browser-compatible fMP4) is unable to generate a valid video stream.

PAT and PMT buffers are reused in order to save RAM.

Countinuity counter of PAT and PMT must be increased after every
delivery in order to avoid errors when playing TS files with VLC
or hls.js.

PAT and PMT buffers are reused in order to save RAM.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 77.174% when pulling a2617d6 on aler9:patch-cc into 7a46f0b on asticode:master.

Copy link
Owner

@asticode asticode left a comment

Choose a reason for hiding this comment

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

Nice PR, just need some clarifications

if err := m.generatePAT(); err != nil {
return bytesWritten, err
}
if err := m.generatePAT(); err != nil {
Copy link
Owner

Choose a reason for hiding this comment

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

What's the reasoning behind removing this condition ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello, to increase the continuity counter, PAT and PMT needs to be regenerated during every WriteTables(), therefore they can't be cached anymore. That condition allowed to cache PAT and PMT, this is not possible anymore.

if err := m.generatePMT(); err != nil {
return bytesWritten, err
}
if err := m.generatePMT(); err != nil {
Copy link
Owner

Choose a reason for hiding this comment

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

What's the reasoning behind removing this condition ?

@asticode asticode merged commit ddb96a7 into asticode:master Jul 5, 2021
@asticode
Copy link
Owner

asticode commented Jul 5, 2021

FYI I've created a v1.9.0 tag

Cheers

@aler9 aler9 deleted the patch-cc branch July 5, 2021 11:02
@barbashov
Copy link
Contributor

It seems that this PR introduced a bug: now tables version increase every time tables are written. Which doesn't go along with specification AFAIR: we should increase tables version only if they change

@tmm1
Copy link
Contributor

tmm1 commented Mar 16, 2022

I noticed the same and had to disable it: fancybits@0156013

@asticode
Copy link
Owner

asticode commented Mar 17, 2022

What do you think about this fix : #38 ?

@aler9
Copy link
Contributor Author

aler9 commented Apr 21, 2022

I didn't notice that the version number was increasing every time tables are written - my only aim was increasing the continuity counter, as the specification says. So thanks for the fix.

@barbashov
Copy link
Contributor

No worries, bugs happen :)

@asticode
Copy link
Owner

@aler9 yeah no worries 👍

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.

5 participants