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

revert samplebuilder #1874

Closed
wants to merge 3 commits into from
Closed

revert samplebuilder #1874

wants to merge 3 commits into from

Conversation

jech
Copy link
Member

@jech jech commented Jul 7, 2021

This revers the samplebuilder changes, which are uselessly complex
and break existing software.

While it is true that the samplewriter could use a rewrite, a new
version should be a complete rewrite that uses data structures with
better cache locality. The new APIs introduced in this version would
impede writing such a better version.

Fixes #1839. Probably also #1870.

  • Add test to check whether the samplebuilder drops data
  • Revert "Fix infinitate loop possible in sample builder"
  • Revert "Refactored samplebuilder logic"

jech added 3 commits July 7, 2021 20:16
This reverts commit 7d97c9b.

Reason for revert: the new code is way too complex, it adds
new functionality of dubious utility, and is broken.
@jech
Copy link
Member Author

jech commented Jul 7, 2021

CC @robin-raymond

@robin-raymond
Copy link
Contributor

See #1873 -- There's an issue with the assumptions in the test and how the sample builder works.

@jech
Copy link
Member Author

jech commented Jul 7, 2021

The sample builder used to have a certain behaviour. Your patch breaks this behaviour, and in doing so has broken working software.

Perhaps you're right, and the semantics you propose is better. In that case, you should distribute a separate library, or submit a different package to Pion, and perhaps propose that it replace the samplebuilder in a future v4. What you have done amounts to sneaking in a change in behaviour into a stable version of Pion which caused people's working software to break.

@robin-raymond
Copy link
Contributor

If a behaviour was expected then it needs to be codified into a unit test to demonstrate that behaviour as part of the contract. There was no such test to pass. The previous code was buggy. The solution at this point I think is to codify specific start of buffer behaviours for start/end of buffer ranges rather than a revert.

@robin-raymond
Copy link
Contributor

Your test passes. You do not need to revert. The issue was rather easy to fix. You can kill this PR.

@Sean-Der Sean-Der closed this Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Samplebuilder is broken in v3.0.29
3 participants