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 nalu parsing for 3-byte & 4-byte start seqs #301

Merged
merged 1 commit into from
Mar 11, 2025

Conversation

rileym-arcules
Copy link
Contributor

@rileym-arcules rileym-arcules commented Mar 10, 2025

Description

There is an issue in the current emitNalu function where the implementation can skip over 3-byte start sequences.

The emitNalu function was changed in commit bfe92b9 which introduced this regression: bfe92b9#diff-5eca440e933baa4c5491dcd9ccc7c1f4d807994e6c5b54c304228c34fcc3c963L40

From analysis of the current emitNalu function,

func emitNalus(nals []byte, emit func([]byte)) {
start := 0
length := len(nals)
for start < length {
end := bytes.Index(nals[start:], annexbNALUStartCode)
offset := 4
if end == -1 {
end = bytes.Index(nals[start:], naluStartCode)
offset = 3
}
if end == -1 {
emit(nals[start:])
break
}
emit(nals[start : start+end])
// next NAL start position
start += end + offset
}
}

one can observe that NALUs with 3-byte start sequences may be skipped over if it is the first NALU, or if it is between two NALUs with 4-byte start sequences (in this case: at L52 end != -1)

This PR aims to resolve this issue by searching for 3-byte start sequences and accounting for 4-byte sequences afterward.

@rileym-arcules rileym-arcules force-pushed the rileym/nalu-parsing-fix branch from 491e7d7 to f9158ee Compare March 10, 2025 23:11
@rileym-arcules rileym-arcules force-pushed the rileym/nalu-parsing-fix branch 2 times, most recently from 44250ac to 853b7fa Compare March 10, 2025 23:15
@rileym-arcules rileym-arcules force-pushed the rileym/nalu-parsing-fix branch from 853b7fa to 5662773 Compare March 10, 2025 23:17
Copy link

codecov bot commented Mar 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.76%. Comparing base (ee5524b) to head (5662773).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #301      +/-   ##
==========================================
+ Coverage   86.69%   86.76%   +0.07%     
==========================================
  Files          26       26              
  Lines        3043     3061      +18     
==========================================
+ Hits         2638     2656      +18     
  Misses        348      348              
  Partials       57       57              
Flag Coverage Δ
go 86.76% <100.00%> (+0.07%) ⬆️
wasm 86.21% <100.00%> (+0.08%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 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.

@Sean-Der Sean-Der merged commit 9bbc0f6 into pion:master Mar 11, 2025
14 checks passed
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

2 participants