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

interpolate_target_q assumes a sample will always be available #690

Open
mqudsi opened this issue Dec 13, 2022 · 0 comments
Open

interpolate_target_q assumes a sample will always be available #690

mqudsi opened this issue Dec 13, 2022 · 0 comments

Comments

@mqudsi
Copy link
Contributor

mqudsi commented Dec 13, 2022

thread '<unnamed>' panicked at 'called `Option::unwrap()` on a `None` value', av1an-core/src/target_quality.rs:340:6

This corresponds to the last unwrap in the following code fragment:

pub fn interpolate_target_q(scores: Vec<(f64, u32)>, target: f64) -> Result<f64, Error> {
  let mut sorted = scores;
  sorted.sort_by(|a, b| a.0.partial_cmp(&b.0).unwrap());

  let keys = sorted
    .iter()
    .map(|(x, y)| Key::new(*x, f64::from(*y), Interpolation::Linear))
    .collect();

  let spline = Spline::from_vec(keys);

  Ok(spline.sample(target).unwrap())
}

The problem is that the call to spline.sample() is not guaranteed to return a value. In particular, since the requested interpolation method is Interpolation::Linear, the only way the sample could be None is if search_lower_cp() returned None before the sampling was actually attempted.

I'm not sure if it makes sense for sample_with_key() to fall back to i = 0 if search_lower_cp() returned None (for one thing, there may not be a keys[0], for another, the math might turn out wrong). It might be better to have interpolate_target_q return an Option, but its only call site is interpolated_target_q which doesn't have much room for "value not available" as it bubbles the result directly to its only caller, per_shot_target_quality() which is quite definitive and only returns an error in case of an encoder crash and not any other type of situation.

I suspect in this case scores was empty, for unknown reasons? In that case, the calculation of interpolated target might need to extend the evaluated fragment until it isn't?

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

No branches or pull requests

1 participant