-
-
Notifications
You must be signed in to change notification settings - Fork 45
h265: reimplementation #425
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
Conversation
Deploying scuffle-docrs with
|
| Latest commit: |
c729ec0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3f57a53a.scuffle-docrs.pages.dev |
| Branch Preview URL: | https://pr-425.scuffle-docrs.pages.dev |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #425 +/- ##
==========================================
- Coverage 84.15% 83.32% -0.83%
==========================================
Files 225 240 +15
Lines 15915 17007 +1092
==========================================
+ Hits 13393 14171 +778
- Misses 2522 2836 +314
... and 7 files with indirect coverage changes
|
|
@lennartkloock rebase this pr |
0d4fe96 to
5db8bfa
Compare
🛫 Startup details 🛫Revision main not found locally. Fetching from origin...Checking out commit 827bdd8 into "target/semver-baseline" HEAD is now at 827bdd8 Auto merge of #433 - lennart/new-nightly-clippy, r=TroyKomodo 📦 Processing crates 📦
Semver-checks summary🚩 2 ERRORS FOUND 🚩🔖 Error
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR reimplements H.265 SPS parsing and updates related modules with improved error handling, enum-based type safety, and enhanced tests and documentation.
- Introduces new modules for SPS profile tier level, PCM and conformance window parsing.
- Updates configuration record parsing/muxing to leverage new enums and refactors module exports.
- Adjusts tests (including FLV integration) and dependency metadata.
Reviewed Changes
Copilot reviewed 43 out of 45 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/h265/src/sps/profile_tier_level.rs | Adds new parsing logic for ProfileTierLevel and sub-layer profiles/levels. |
| crates/h265/src/sps/pcm.rs | Implements PCM parsing with range checking and exp-Golomb decoding. |
| crates/h265/src/sps/conformance_window.rs | Introduces conformance window parsing and byte stream building. |
| crates/h265/src/sps.rs | Removes the old SPS parsing implementation. |
| crates/h265/src/range_check.rs | Adds a macro to validate numeric ranges in the bitstream. |
| crates/h265/src/nal_unit_header.rs | Updates NAL unit header parsing with stricter temporal id and layer id checks. |
| crates/h265/src/lib.rs | Updates module exports and documentation; revises comments and metadata. |
| crates/h265/src/enums/* | Adds several enums (e.g., VideoFormat, ParallelismType, NumTemporalLayers, NALUnitType, ConstantFrameRate, AspectRatioIdc) to improve type safety. |
| crates/h265/src/config.rs | Reimplements the HEVCDecoderConfigurationRecord demux/mux logic using new enum types and clearer error checks. |
| crates/flv/src/lib.rs | Adjusts FLV integration tests to account for the updated HEVC configuration and SPS parsing. |
| crates/h265/Cargo.toml | Updates crate metadata and dependencies for the new implementation. |
| changes.d/pr-425.toml | Provides PR metadata summarizing the reimplementation of H.265 SPS parsing. |
Files not reviewed (2)
- crates/flv/src/snapshots/scuffle_flv__tests__demux_flv_hevc_aac.snap: Language not supported
- crates/h265/src/snapshots/scuffle_h265__config__tests__config_demux.snap: Language not supported
Comments suppressed due to low confidence (1)
crates/h265/src/config.rs:51
- [nitpick] The inline comment suggests that 'avg_frame_rate' might be better represented as a f64 rather than a u16; consider reviewing this type for consistency with expected frame rate precision.
pub avg_frame_rate: u16,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR reimplements the H.265 SPS parsing functionality while refactoring several related modules and updating documentation and tests.
- Introduces a new range_check! macro for cleaner boundary checks.
- Implements NAL unit header parsing using stronger type definitions and updated error handling.
- Refactors configuration demux/mux logic and updates tests to align with the new design.
Reviewed Changes
Copilot reviewed 48 out of 50 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/h265/src/range_check.rs | New macro for validating numeric ranges. |
| crates/h265/src/nal_unit_header.rs | Implements NAL unit header parsing with improved error messages. |
| crates/h265/src/lib.rs | Updates module organization and documentation for H.265 decoding. |
| crates/h265/src/config.rs | Refactors configuration record demux/mux with new enums and checks. |
| crates/h265/src/enums/*.rs | Introduces nutype_enum-based enums for various H.265 parameters. |
| crates/h264/src/* | Updates related SPS parsing module imports. |
| crates/flv/src/lib.rs | Adjusts tests to reflect changes in nal unit type naming and decoding. |
| crates/bytes-util/* | Adds/export NAL emulation prevention handling. |
| changes.d/pr-425.toml | Documents the refactor as a breaking change. |
Files not reviewed (2)
- crates/flv/src/snapshots/scuffle_flv__tests__demux_flv_hevc_aac.snap: Language not supported
- crates/h265/src/snapshots/scuffle_h265__config__tests__config_demux.snap: Language not supported
Comments suppressed due to low confidence (1)
crates/h265/src/range_check.rs:3
- [nitpick] Consider using a more descriptive variable name instead of 'n' in the macro to reduce the chance of name collisions or confusion in expanded contexts.
let n = $n;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR reimplements several aspects of H.265 header parsing and configuration handling while updating documentation, tests, and dependency management. Key changes include:
- A reimplementation of H.265 SPS parsing and associated NAL unit handling.
- Updates to the configuration record parsing/muxing with new enum types (e.g. ConstantFrameRate, NumTemporalLayers, ParallelismType) and conversion logic.
- Adjustments in related crates (H264, FLV, bytes-util) to align with the new H265 implementation.
Reviewed Changes
Copilot reviewed 50 out of 51 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/h265/src/rbsp_trailing_bits.rs | Added implementation for reading RBSP trailing bits. |
| crates/h265/src/range_check.rs | Introduced a macro for range checking with associated test case. |
| crates/h265/src/nal_unit_header.rs | Updated NAL unit header parsing with stricter error conditions. |
| crates/h265/src/lib.rs | Reorganized modules, exports and updated mux/demux logic for config. |
| crates/h265/src/enums/* | Added various enums (video format, nal unit type, parallelism, etc.). |
| crates/h265/src/config.rs | Extended and refined HEVC decoder configuration record handling. |
| crates/h264/src/{sps/mod.rs,lib.rs} | Adjusted imports in SPS parsing to account for refactoring in H265. |
| crates/flv/src/lib.rs | Updated test expectations to reflect changes in H265 unit naming. |
| crates/bytes-util/src/{nal_emulation_prevention.rs, lib.rs} | Added support for NAL emulation prevention I/O. |
| changes.d/pr-425.toml | Documented PR changes in changelog format. |
Files not reviewed (1)
- crates/flv/src/snapshots/scuffle_flv__tests__demux_flv_hevc_aac.snap: Language not supported
7a0ec37 to
241f3a0
Compare
| } | ||
|
|
||
| impl VuiParameters { | ||
| // TODO: Find a solution for this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in h264 I never had to pass in values like this. It was either passing in the parsed object or the reader of the bitstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think h264 is less complex, so there are less values which other values might depend on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see. I wonder if @TroyKomodo has any ideas for how to design/handle this?
|
Great work!! My suggestions are just nits. |
Co-authored-by: Copilot <[email protected]>
12e6b11 to
6f5a2f0
Compare
|
?brawl merge |
|
📌 Commit c729ec0 has been approved and added to the merge queue. Requested by: @TroyKomodo Approved by: @TroyKomodo |
|
🎉 Build successful! Approved by: @TroyKomodo |
Reimplementation of H.265 SPS NALU parsing with scuffle-h265.
CLOUD-30