Parse scaling lists properly. #90

jerzywilczek · 2024-11-13T16:09:43Z

Scaling lists were skipped previously, I implemented proper parsing, which was mostly coming up with the types to store them in.

I hope the API with const-sized arrays is ok, it seemed whoever implemented skipping the lists wanted to use Vecs initially. I found the generic const more appealing, but can change back to Vecs if needed.

github-actions · 2024-11-13T16:11:15Z

Bencher Report

Branch	90/merge
Testbed	localhost

⚠️ WARNING: No Threshold found!
Without a Threshold, no Alerts will ever be generated.
Total read+write (reads/writes)
L1 Hits (hits)
L2 Hits (hits)
Estimated Cycles (estimated cycles)
RAM Hits (hits)
Instructions (instructions)
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

Benchmark	Estimated Cycles	estimated cycles	Instructions	instructions	L1 Hits	hits	L2 Hits	hits	RAM Hits	hits	Total read+write	reads/writes
ci_bench::ci::reader read:setup_video("big_buck_bunny_1080p_24f...	📈 view plot ⚠️ NO THRESHOLD	16,755,068.00	📈 view plot ⚠️ NO THRESHOLD	8,286,933.00	📈 view plot ⚠️ NO THRESHOLD	10,881,698.00	📈 view plot ⚠️ NO THRESHOLD	15,565.00	📈 view plot ⚠️ NO THRESHOLD	165,587.00	📈 view plot ⚠️ NO THRESHOLD	11,062,850.00

🐰 View full continuous benchmarking report in Bencher

jerzywilczek · 2024-12-10T14:53:07Z

@dholroyd I really don't want to be annoying, but any chance of looking at this at some point? It's ok if the answer is no, I'd just like to know if it makes sense to wait for this. Once again, I really don't want to be annoying or bother you, sorry.

scottlamb · 2025-01-25T18:06:22Z

src/nal/sps.rs

+pub enum ScalingList<const S: usize> {
+    NotPresent,
+    UseDefault,
+    List(Box<[i32; S]>),


It looks like the possible bounds of scalingList[ j ] are [1, 256); is that right? If so, perhaps a NonZeroU8 would be appropriate rather than an i32?

I've had a look at "Scaling list syntax" in the spec, and agree it seems to make sense to use NonZeroU8 here 👍

scottlamb · 2025-01-25T18:16:18Z

src/nal/pps.rs

@@ -142,7 +142,8 @@ impl SliceGroup {

 #[derive(Debug, Clone)]
 pub struct PicScalingMatrix {
-    // TODO
+    pub scaling_list4x4: Vec<ScalingList<16>>,
+    pub scaling_list8x8: Vec<ScalingList<64>>,


Do you think it'd be more clear to make this Option<Vec<ScalingList<64>> based on the transform_8x8_mode_flag, as opposed to representing this by scaling_list8x8.empty()? I could go either way; probably should have a comment in the latter case though.

After some thinking, representing it as Option<...> makes more sense, it seems more intuitive if someone wants to extract the flag.

scottlamb · 2025-01-25T18:23:00Z

src/nal/sps.rs

-        let mut scaling_list = vec![];
+
+impl<const S: usize> ScalingList<S> {
+    pub fn read<R: BitRead>(


Regarding this comment in your PR description:

I hope the API with const-sized arrays is ok, it seemed whoever implemented skipping the lists wanted to use Vecs initially. I found the generic const more appealing, but can change back to Vecs if needed.

I think the only concern I have about the const-size array approach is doubling the code size of this read method, in combination with the existing multiplicative factor of the BitRead monomorphization. What would you say about avoiding that via delegation, something like the following:

/// Returns `use_default_scaling_matrix_flag` fn fill_scaling_list<R: BitRead>(r: &mut R, scaling_list: &mut [NonZeroU8]) -> Result<bool, ScalingMatrixError> { // ... } impl<const S: usize> ScalingList<S> { pub fn read<R: BitRead>(r: &mut R, present: bool) -> Result<ScalingList<S>, ScalingMatrixError> { if !present { /* ... */ } let mut scaling_list = Box::new([0; S]); let use_default_scaling_matrix_flag = fill_scaling_list(r, &mut scaling_list[..])?; if use_default_scaling_matrix_flag { Ok(ScalingList::UseDefault) } else { Ok(ScalingList::List(scaling_list)) } } }

Oh, that's a very nice catch. Super cool you noticed that.

scottlamb · 2025-01-25T18:24:26Z

src/nal/sps.rs

+                        ScalingList::NotPresent,
+                        ScalingList::NotPresent,
+                    ]
+                }),
                ..ChromaInfo::default()


It'd be nice to have a test case that exercises the new code paths more fully.

I modified the test case a bit, are there things I forgot to include there that you wish were covered?

scottlamb · 2025-01-25T21:25:17Z

src/nal/sps.rs

-pub struct ScalingList {
-    // TODO
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub enum ScalingList<const S: usize> {


I had another thought about representation, and @dholroyd I'd love to get your take also.

Is it important to convey exactly how the file specified the scaling matrices, or are the callers always going to want to apply the relevant rules to come up with a matrix? The rules appear to be something like the following:

seq_scaling_matrix_present_flag == 0 => use Flat_...

seq_scaling_list_present_flag[ i ] == 0 => use fall-back rule set A

pic_scaling_matrix_present_flag == 0 => match the sps

pic_scaling_list_present_flag[ i ] == 0 => use fall-back rule set B

UseDefaultScalingMatrix...Flag[ i ] == 0 => use Default_..._Intra

If the latter, I think it's better to just represent a dense matrix of integers, rather than a vec of enums to these defaults or a box of integers.

It's always been my ambition to use this code to produce a diagnostic dump of the bitstream content, so I lean toward the former, bitstream-oriented representation. I think there is also a place for utility functions to produce the latter representation on demand, so that callers don't all duplicate the logic you've outlined.

Thanks; that makes the existing structure clearly the right way to go.

dholroyd · 2025-01-27T01:37:59Z

@jerzywilczek thanks for this, bardzo ładne! Also, sorry for being slow getting to the PR.

github-actions · 2025-01-27T11:38:43Z

Bencher Report

Branch	scaling-lists-optional
Testbed	localhost

⚠️ WARNING: No Threshold found!
Without a Threshold, no Alerts will ever be generated.
L1 Hits (hits)
RAM Hits (hits)
Instructions (instructions)
Total read+write (reads/writes)
L2 Hits (hits)
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

Benchmark	Estimated Cycles	Benchmark Result 1e6 x estimated cycles (Result Δ%)	Upper Boundary 1e6 x estimated cycles (Limit %)	Instructions	1e6 x instructions	L1 Hits	1e6 x hits	L2 Hits	1e3 x hits	RAM Hits	1e3 x hits	Total read+write	1e6 x reads/writes
ci_bench::ci::reader read:setup_video("big_buck_bunny_1080p_24fps_h264.h264"...	📈 view plot 🚷 view threshold	16.73 (+0.52%)	18.31 (91.38%)	📈 view plot ⚠️ NO THRESHOLD	8.27	📈 view plot ⚠️ NO THRESHOLD	10.86	📈 view plot ⚠️ NO THRESHOLD	15.37	📈 view plot ⚠️ NO THRESHOLD	165.59	📈 view plot ⚠️ NO THRESHOLD	11.04

🐰 View full continuous benchmarking report in Bencher

jerzywilczek · 2025-01-27T16:47:10Z

@dholroyd No problem! Thank you for the library.

dholroyd · 2025-01-27T17:47:57Z

I am happy with the state of this - any further comments @scottlamb ?

jerzywilczek · 2025-01-27T17:52:09Z

One more thing - can we have a new release after merging this?

scottlamb · 2025-01-27T18:52:49Z

src/nal/sps.rs

+        } else {
+            next_scale
+        };
+        // unwrap is ok here, because:


I agree with the argument in this comment, but I think it's possible to avoid the .unwrap() altogether:

keep last_scale as a NonZeroU8, starting from const { NonZeroU8::new(8).unwrap() } (msrp 1.83, the alternative of match { ..., None => panic!() } works a few versions before)

update via new_value = NonZeroU8::new(next_scale as u8).unwrap_or(last_value)

dholroyd

Looking good

jerzywilczek mentioned this pull request Nov 13, 2024

Investigate parsing of scaling lists in the hardware decoder software-mansion/smelter#806

Closed

jerzywilczek force-pushed the scaling-lists-optional branch from 706891f to ccfc26e Compare November 14, 2024 09:55

jerzywilczek force-pushed the scaling-lists-optional branch from ccfc26e to 54d98a7 Compare December 12, 2024 10:10

scottlamb reviewed Jan 25, 2025

View reviewed changes

jerzywilczek force-pushed the scaling-lists-optional branch 2 times, most recently from 41010bb to f988174 Compare January 27, 2025 11:37

jerzywilczek force-pushed the scaling-lists-optional branch from f988174 to 0e25ac8 Compare January 27, 2025 16:37

jerzywilczek requested review from scottlamb and dholroyd January 27, 2025 16:45

scottlamb reviewed Jan 27, 2025

View reviewed changes

dholroyd mentioned this pull request Jan 27, 2025

0.7.1 release on crates.io #91

Closed

Parse scaling lists properly.

e964a4a

jerzywilczek force-pushed the scaling-lists-optional branch from 0e25ac8 to e964a4a Compare January 28, 2025 10:12

jerzywilczek requested a review from scottlamb January 28, 2025 10:13

dholroyd approved these changes Jan 28, 2025

View reviewed changes

scottlamb approved these changes Jan 28, 2025

View reviewed changes

scottlamb merged commit 98048f0 into dholroyd:master Jan 28, 2025
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse scaling lists properly. #90

Parse scaling lists properly. #90

jerzywilczek commented Nov 13, 2024

github-actions bot commented Nov 13, 2024 •

edited

Loading

⚠️ WARNING: No Threshold found!

jerzywilczek commented Dec 10, 2024 •

edited

Loading

scottlamb Jan 25, 2025

dholroyd Jan 27, 2025

scottlamb Jan 25, 2025

jerzywilczek Jan 27, 2025

scottlamb Jan 25, 2025

jerzywilczek Jan 27, 2025

scottlamb Jan 25, 2025

jerzywilczek Jan 27, 2025

scottlamb Jan 25, 2025

dholroyd Jan 26, 2025

scottlamb Jan 26, 2025

dholroyd commented Jan 27, 2025

github-actions bot commented Jan 27, 2025 •

edited

Loading

⚠️ WARNING: No Threshold found!

jerzywilczek commented Jan 27, 2025

dholroyd commented Jan 27, 2025

jerzywilczek commented Jan 27, 2025

scottlamb Jan 27, 2025

dholroyd left a comment

Parse scaling lists properly. #90

Parse scaling lists properly. #90

Conversation

jerzywilczek commented Nov 13, 2024

github-actions bot commented Nov 13, 2024 • edited Loading

⚠️ WARNING: No Threshold found!

jerzywilczek commented Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dholroyd commented Jan 27, 2025

github-actions bot commented Jan 27, 2025 • edited Loading

⚠️ WARNING: No Threshold found!

jerzywilczek commented Jan 27, 2025

dholroyd commented Jan 27, 2025

jerzywilczek commented Jan 27, 2025

Choose a reason for hiding this comment

dholroyd left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 13, 2024 •

edited

Loading

jerzywilczek commented Dec 10, 2024 •

edited

Loading

github-actions bot commented Jan 27, 2025 •

edited

Loading