Bounds checks for many fields #66
Conversation
dholroyd
force-pushed
the
bitstream-restrictions-bounds-checks
branch
from
549d4e6 to
902104e
Compare
dholroyd
changed the title
dholroyd
force-pushed
the
bitstream-restrictions-bounds-checks
branch
from
902104e to
86a0ff9
Compare
dholroyd
force-pushed
the
bitstream-restrictions-bounds-checks
branch
from
86a0ff9 to
506ee23
Compare
|
| Report | Fri, March 1, 2024 at 07:28:22 UTC |
| Project | h264-reader |
| Branch | 66/merge |
| Testbed | ubuntu-22.04 |
| Benchmark | Estimated Cycles | Estimated Cycles Results estimated cycles | Instructions | Instructions Results instructions | L1 Accesses | L1 Accesses Results accesses | L2 Accesses | L2 Accesses Results accesses | RAM Accesses | RAM Accesses Results accesses | Total Accesses | Total Accesses Results total-accesses |
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| ci_bench::ci::reader read:setup_video("big_buck_bunny_1080p_24f... | ➖ (view plot) | 16653199.000 | ➖ (view plot) | 8205418.000 | ➖ (view plot) | 10769774.000 | ➖ (view plot) | 16841.000 | ➖ (view plot) | 165692.000 | ➖ (view plot) | 10952307.000 |
Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help
dholroyd
force-pushed
the
bitstream-restrictions-bounds-checks
branch
2 times, most recently
from
bacebcc to
ab5c4cb
Compare
|
| Report | Fri, March 1, 2024 at 18:56:06 UTC |
| Project | h264-reader |
| Branch | 66/merge |
| Testbed | localhost |
| Benchmark | Estimated Cycles | Estimated Cycles Results estimated cycles | (Δ%) | Estimated Cycles Lower Boundary estimated cycles | (%) | Instructions | Instructions Results instructions | L1 Accesses | L1 Accesses Results accesses | L2 Accesses | L2 Accesses Results accesses | RAM Accesses | RAM Accesses Results accesses | Total Accesses | Total Accesses Results total-accesses |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| ci_bench::ci::reader read:setup_video("big_buck_bunny_1080p_24f... | ✅ (view plot) | 16648266.000 (+0.04%) | 14976912.000 (89.96%) | ➖ (view plot) | 8205445.000 | ➖ (view plot) | 10771081.000 | ➖ (view plot) | 15558.000 | ➖ (view plot) | 165697.000 | ➖ (view plot) | 10952336.000 |
Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help
src/nal/sps.rs
Outdated
| // "The value of max_dec_frame_buffering shall be greater than or equal to | ||
| // max_num_ref_frames." | ||
| if max_dec_frame_buffering < sps.max_num_ref_frames { | ||
| return Err(SpsError::FieldValueTooLarge { |
There was a problem hiding this comment.
Should this be FieldValueTooSmall?
src/nal/sps.rs
Outdated
| }) | ||
| } else { | ||
| None | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // calculates the maximum allowed value for the max_dec_frame_buffering field | ||
| fn max_val_for_max_dec_frame_buffering(sps: &SeqParameterSet) -> u32 { |
There was a problem hiding this comment.
This introduces panics on unknown level/profile. At least I'd want it to return Err instead. What are your thoughts on how important max_dec_frame_buffering bounds enforcement is at all? I'm not sure it's necessary to avoid overflows as say the pic_init_qs_minus26 check is (#58).
There was a problem hiding this comment.
I left this in a half finished state.
Currently the value would only be in avoiding possible bounds check issues in calling code, since there is no local use of this field.
I'll back this code out in the interests of getting this merged sooner, and it can return at a later date.
| max_mvs_per2mb: Option<NonZeroU8>, | ||
| } | ||
|
|
||
| lazy_static! { |
There was a problem hiding this comment.
What are your feelings on using std::sync::LazyLock (msrp 1.80) or std::sync::OnceLock (msrp 1.70) instead? I don't see any particular need to support old Rust versions and personally would rather not have the extra dep.
Alternatively, could use a static sorted array + bisection I suppose.
There was a problem hiding this comment.
I'll take a look at one of those alternatives in a separate PR and back this block out along with the the max_val_for_max_dec_frame_buffering() check for now.
dholroyd
force-pushed
the
bitstream-restrictions-bounds-checks
branch
2 times, most recently
from
4d867c6 to
463bcae
Compare
dholroyd
force-pushed
the
bitstream-restrictions-bounds-checks
branch
from
463bcae to
ac44ecd
Compare
Turns out to be more complicated than I expected due to the need to refer to a table of values from the spec when calculating the max value for
max_dec_frame_buffering.log2_max_mv_length_horizontalandlog2_max_mv_length_verticalare not checked to the [0, 15] limits in the most recent spec because older specs said the restriction was [0, 16] and real world test data appears to use the value 16.Other changes included,
Fixes #55 and #58