Replace SampleBuilder with jech's implementation

[Sean-Der]

This has been on my TODO list for a long time.

@jech would you be in support of this? Do you have any concerns/things I should be aware of?

I may have to add some small additional APIs, but I don't anticipate much. I spent some time reading the Pion samplebuilder code we have today and it is very difficult to follow.

[jech]

I fully support it.

Do you have any concerns/things I should be aware of?

The main difference is that sample durations are off by one: my samplebuilder returns a sample's duration (or 0 if unavailable), while the one currently in Pion returns the duration of the previous sample. My personal opinion is that we should deprecate the Duration field in the Sample struct, since it is difficult to compute correctly in all cases, and document that the timestamp is the right thing to use. (We should also fix the examples.)

[midepeter]

@Sean-Der Hello

This is peter i was the one who showed interest in contributing tho pion and helping out with some tasks. Not sure if i could pick this up so i can work on it if possible

@jech could you point me to your implementation if i could pick it up:slightly_smiling_face:

[jech]

https://github.com/jech/samplebuilder

[Sean-Der]

Amazing! That would be great @midepeter

We have two additional options. I am not sure if we need to delete them OR add it to the implementation we are importing.

• WithMaxTimeDelay
• WithPacketHeadHandler

[midepeter]

@Sean-Der

I think we could keep them with so it makes the impelmentation more robust enough and i am not sure if will be keeping backward compaibility also because it will be of help that way

[Sean-Der]

That's great @midepeter!

I added you to the Pion organization. I would start with a small PR of deleting all our code (and using @jech instead) and we can iterate and add more features back. Would love to see this better ASAP :)

[midepeter]

Alright @Sean-Der

That's great. I will continue to monitor and you can also tag me when the PR is up.

[Sean-Der]

@midepeter Would you like to create the PR? You can create a branch on this repo and start making changes. I am happy to review/help as soon as you have questions

[midepeter]

@Sean-Der Alright I will do that

[Sean-Der]

We ended up doing something slightly different.

We now have a JitterBuffer that is just targeted at de-jittering RTP packets. You can use it directly, in the future we will also provide an interceptor.

If users wish to build samples the SampleBuilder is still available. We will work on simplifying it/fixing bugs.

@thatsnotright did I explain that right/sound like a good plan? @jech What else needs to change/be added to the JitterBuffer implementation to be useful for your purposes. I would love to solve this problem for everyone!

[jech]

@jech What else needs to change/be added to the JitterBuffer implementation to be useful for your purposes.

I'm not sure how I'm supposed to depacketise the output of the jitterbuffer. I'd also like to undestand what are the advantages.

I would love to solve this problem for everyone!

While I wouldn't necessarily be opposed to accepting a patch that rewrites the disk writing code in Galene to use the jitter buffer, you'll understand that I have limited time to spend on Galene, and that rewriting the current working code is pretty low on my list of priorities, so I'm not likely to do it myself in the near future.

[Sean-Der]

@jech If you want depacketise media SampleBuilder is the right choice.

I had users ask to add PopRTP and equivalent APIs to the SampleBuilder. To make those users happy we now have that JitterBuffer instead.

I am just going to get the Sample durations fixed, and make it so the Pion one is useful enough again. For Galene I want things to be as simple as changing import paths