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

[janus-pp-rec] Enhance timestamp assignment and add payload-type option to CLI. #2345

Merged
merged 10 commits into from
Nov 30, 2020

Conversation

atoppi
Copy link
Member

@atoppi atoppi commented Sep 1, 2020

In janus-pp-rec the timestamp assigned to each packet is a uint64 that is used to monotonically order the packets in a recording.
This PR introduces a new algorithm in janus-pp-rec to assign packet timestamps in order to avoid erratic re-orderings that might lead to wrongly generated media files.

Since today that part of janus-pp-rec has relied on the "post reset trigger" mechanism to avoid messing media files (e.g. huge files) when particular orderings of timestamps were present.
This approach worked quite well so far but still it does not solve all corner cases. Furthermore it is in my opinion difficult to read, confusing to use and over-engineered.

This patch makes that mechanism more straightforward:

  • remove the parameter postreset-trigger (you can not use it anymore in the CLI)
  • instantly detects a timestamp reset (aka wrap-around) without the need of a trigger threshold
  • use wrap-around aware checks to understand when a new timestamp is before or after the highest one

It has been tested with some mjrs containing special cases (timestamp reset in the first packets, out of order packets near the wrap around etc.) and it seemed to work as expected.

UPDATE: this PR also includes the changes proposed in #2412.

@lminiero
Copy link
Member

lminiero commented Sep 1, 2020

Pinging @tgabi333 as I know he uses mjr files a lot.

@tgabi333
Copy link
Contributor

tgabi333 commented Sep 1, 2020

Seems ok for me, thanks for letting me know

@tgabi333
Copy link
Contributor

tgabi333 commented Sep 1, 2020

What about JANUS_PPREC_IGNOREFIRST?

@atoppi
Copy link
Member Author

atoppi commented Sep 1, 2020

That part is unchanged.
I suspect that some of the issues that needed that setting could be solved by the new approach without using that parameter.

@lminiero
Copy link
Member

lminiero commented Sep 1, 2020

I don't think the "ignore first" stuff is needed anymore with this fix.

@tgabi333
Copy link
Contributor

@atoppi do you think that "ignore first" could be dropped out as well with this pull request?

@atoppi
Copy link
Member Author

atoppi commented Oct 19, 2020

@tgabi333 not in this PR.
Skipping the first packets of a mjr might still find a motivation in the future.

@atoppi atoppi changed the title Simplify and enhance janus-pp-rec timestamp assignement. [janus-pp-rec] Enhance timestamp assignment and add payload-type option to CLI. Oct 30, 2020
@lminiero
Copy link
Member

Merging.

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.

None yet

3 participants