Skip to content

Optimize SampleBuilder and reset cross-sample state on Flush#3445

Open
solos wants to merge 3 commits into
pion:mainfrom
solos:optimize-SampleBuilder
Open

Optimize SampleBuilder and reset cross-sample state on Flush#3445
solos wants to merge 3 commits into
pion:mainfrom
solos:optimize-SampleBuilder

Conversation

@solos

@solos solos commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Description

Improve SampleBuilder performance and make Flush() a clean boundary for a new RTP stream.
Performance:

  • Replace lastSampleTimestamp *uint32 with uint32 + hasLastSampleTimestamp to avoid a heap allocation on every emitted sample
  • Pre-size sample payload buffers from the total RTP payload length
  • Compute Sample.Duration with integer arithmetic instead of float64 Flush semantics:
  • After purging buffered packets, Flush() now clears lastSampleTimestamp, droppedPackets, and paddingPackets
  • Prevents post-flush packets from being misclassified as padding from a prior stream when timestamps collide
  • Prevents PrevDroppedPackets on the next sample from including drops that occurred during the flush of leftover buffer data Samples built during Flush() still capture PrevDroppedPackets at build time; only state for packets received after Flush() is reset. Tests:
  • Add TestSampleBuilder_FlushResetsState
  • Add TestSampleBuilder_ValidSampleSameTimestampAfterFlush

Reference issue

None

@codecov

codecov Bot commented Jun 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.35294% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.46%. Comparing base (a323cae) to head (9b109df).

Files with missing lines Patch % Lines
pkg/media/samplebuilder/samplebuilder.go 82.35% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3445      +/-   ##
==========================================
- Coverage   85.60%   85.46%   -0.14%     
==========================================
  Files          81       81              
  Lines        9856     9868      +12     
==========================================
- Hits         8437     8434       -3     
- Misses       1003     1011       +8     
- Partials      416      423       +7     
Flag Coverage Δ
go 85.46% <82.35%> (-0.14%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@solos solos force-pushed the optimize-SampleBuilder branch 3 times, most recently from f6b2f17 to d0e74e7 Compare June 5, 2026 10:21

@JoTurk JoTurk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Change looks good to me, but we'll have to manually test it because samplebuilder has many moving parts.

sample := &media.Sample{
Data: data,
Duration: time.Duration((float64(samples)/float64(s.sampleRate))*secondToNanoseconds) * time.Nanosecond,
Duration: time.Duration(samples) * time.Second / time.Duration(s.sampleRate),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The old behavior returned invalid value (+Inf) if the sample-rate is 0.00f, this will just panic, both aren't ideal, but if there is a setup where users can pass sample rate to applications, this can be used to trigger panic/DoS.

Maybe we can check and return an error early if the user created a samplebuilder with 0 sample rate?

@solos solos Jun 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right. If we return an error, the caller will need to change too. What about set the Duration to 0.

@solos solos force-pushed the optimize-SampleBuilder branch 4 times, most recently from bb3bcc9 to be7010a Compare June 7, 2026 12:36
solos added 3 commits June 7, 2026 20:37
- Replace pointer-based lastSampleTimestamp with uint32+bool flag
- Preallocate sample data slice
- Compute duration using integer arithmetic
- Reset cross-sample state in Flush (lastTimestamp, droppedPackets,
  paddingPackets) to avoid contamination between streams.
- Add tests to verify flush resets padding detection and does not
  misclassify valid frames.
Fix range loop in test cases
check invalid sampleRate
@solos solos force-pushed the optimize-SampleBuilder branch from be7010a to 9b109df Compare June 7, 2026 12:37
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.

2 participants