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

rbsp module changes #81

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

scottlamb
Copy link
Collaborator

See commit descriptions.

You might have seen that I did something dumb earlier—accidentally pushed a version of this directly to the dholroyd/h264-reader repo's master branch. I force-pushed over it a few minutes later—generally not a good idea either but seemed like the least bad thing under the circumstances. Sorry for the mess. Might be worth setting up branch protection to prevent the accident-prone person (me) from doing something like that again...

These improve suitability for parsing H.265 as well as H.264.

The changes to `ByteReader` construction are because H.265 has
two-byte NAL headers, so automatically skipping one byte is super
confusing. Arguably it was confusing even with H.264. Make the skip
explicit.

Additionally, `BitRead::read_to` is nice for loading stuff directly
into primitives such as `[u8; 11]`. That's the representation that
makes the most sense for the H.265 profile (as defined in section 7.3.3,
`profile_tier_level`, `if( profilePresentFlag )` block). When
constructing a `HEVCDecoderConfigurationRecord`, we need that full
11-byte span; when constructing the RFC 6381 codec string, we need
a 6-byte span. So parsing the whole thing into individual fields means
reconstructing these later.

And finally, I renamed methods to more closely match `bitstream-io`,
which is what `h264-reader` is using internally now and is
overwhelmingly the commonly used crate for bitstream parsing in general.
@scottlamb scottlamb requested a review from dholroyd August 12, 2024 23:57
Copy link

🐰Bencher

ReportMon, August 12, 2024 at 23:58:52 UTC
Projecth264-reader
Branch81/merge
Testbedlocalhost

⚠️ WARNING: The following Measures do not have a Threshold. Without a Threshold, no Alerts will ever be generated!

  • Instructions (instructions)
  • L1 Hits (l1-hits)
  • L2 Hits (l2-hits)
  • RAM Hits (ram-hits)
  • Total read+write (total-read-write)

Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the --ci-only-thresholds CLI flag.

Click to view all benchmark results
BenchmarkEstimated CyclesEstimated Cycles Results
estimated cycles | (Δ%)
InstructionsInstructions Results
instructions
L1 HitsL1 Hits Results
hits
L2 HitsL2 Hits Results
hits
RAM HitsRAM Hits Results
hits
Total read+writeTotal read+write Results
reads/writes
ci_bench::ci::reader read:setup_video("big_buck_bunny_1080p_24fps_h264.h264"...✅ (view plot)16,619,076.00➖ (view plot)8,162,349.00➖ (view plot)10,750,146.00➖ (view plot)14,943.00➖ (view plot)165,549.00➖ (view plot)10,930,638.00

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

@scottlamb
Copy link
Collaborator Author

@dholroyd would you like to take a look at this before I merge? I'll need your help anyway publishing to cargo, as I don't have permissions for that.

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.

1 participant