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

Fix opus silence potential to generate huge files #2250

Merged
merged 7 commits into from
Jun 30, 2020

Conversation

neilkinnish
Copy link
Contributor

This may not be the most optimal way to address the issue and I may be missing something. But this seems to work and resolve all the issues I have encountered where huge (example: 28gb) opus files can be generated.

@neilkinnish
Copy link
Contributor Author

neilkinnish commented Jun 28, 2020

Looking deeper at this I think I can improve by checking next rather than passing the count, but I'd be interested in opinion / downsides of the general approach.

@lminiero
Copy link
Member

I think it's already helpful to avoid those issues. Not sure about the impact on recordings, as sometimes they're caused by timestamp resets at unfortunate places and may mean some missing audio packets when the additional command line flags are not used, but it should be very few of them anyway. This looks fine to me already, but if you think this can be improved before we merge please do let me know.

@neilkinnish
Copy link
Contributor Author

Updated to use tmp->next rather than pass the count.

@neilkinnish
Copy link
Contributor Author

Not sure about the impact on recordings, as sometimes they're caused by timestamp resets at unfortunate places and may mean some missing audio packets when the additional command line flags are not used

I have tested across audio recordings that are fine and also 3 with issues.

It definitely resolves the issue of massive files which can impact the server / service quite badly and that I think is key (at least for me).

In all the results the audio was fine and playable. In one case of the 3 with issues it reported an issue with granulepos but played back okay and could be trasncoded to another format fine - I'm assuming this is because it didn't resolve some data issues with PTS at the beginning. Using -i and setting an arbitrary n also works and resolved the issue as it drops the frames with the issue I suppose, but you have to kind of guess the n and I would prefer not to.

postprocessing/pp-opus.c Outdated Show resolved Hide resolved
@lminiero
Copy link
Member

Thanks! I'll have to double check how this behaves with "normal" losses to compensate with silence, since they'd be much more frequent than GB files and we don't want to break those.

@neilkinnish
Copy link
Contributor Author

@lminiero 👍

@lminiero
Copy link
Member

Seems to be working as the current janus-pp-rec on an Opus recording with some occasional losses, so this looks good to me. Thanks, 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.

2 participants