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

bin: Bring --photon-noise out from under unstable feature #3017

Merged
merged 1 commit into from
Sep 13, 2022

Conversation

barrbrain
Copy link
Collaborator

Align the field name with the flag and the av1-grain API. This feature is external to librav1e and used only in binaries.

Align the field name with the flag and the av1-grain API.
This feature is external to librav1e and used only in binaries.
@codecov-commenter
Copy link

Codecov Report

Base: 86.46% // Head: 86.41% // Decreases project coverage by -0.05% ⚠️

Coverage data is based on head (88b1afd) compared to base (25f6c0f).
Patch coverage: 66.66% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3017      +/-   ##
==========================================
- Coverage   86.46%   86.41%   -0.06%     
==========================================
  Files          89       89              
  Lines       34084    34099      +15     
==========================================
- Hits        29470    29465       -5     
- Misses       4614     4634      +20     
Impacted Files Coverage Δ
src/bin/rav1e.rs 68.53% <50.00%> (-1.67%) ⬇️
src/bin/common.rs 54.37% <100.00%> (-0.03%) ⬇️
src/cpu_features/x86.rs 39.58% <0.00%> (-2.09%) ⬇️
src/asm/x86/lrf.rs 93.48% <0.00%> (-0.64%) ⬇️
v_frame/src/plane.rs 88.78% <0.00%> (-0.23%) ⬇️
src/deblock.rs 94.82% <0.00%> (-0.08%) ⬇️
src/rdo.rs 85.21% <0.00%> (-0.05%) ⬇️
src/encoder.rs 87.06% <0.00%> (-0.04%) ⬇️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@shssoichiro
Copy link
Collaborator

I actually have a WIP PR here #2931 that does this. The catch is that Lynne is dead set on not wanting this arg to go into stable unless there is denoising built in, and in the interest of being tired of arguing I attempted to implement denoising. The problem is it's currently too slow to be usable, something like 10-20x slower than the Vapoursynth implementation.

@barrbrain
Copy link
Collaborator Author

I have no objection to splitting this into multiple steps. Film grain tables are already part of our stable API and this just adds a means to generate them to our binaries. Denoising is an open problem and orthogonal to grain synthesis which is outside the RDO loop. We most certainly should factor the grain parameters into our encoding decisions in a future enhancement.

@barrbrain barrbrain added this to In progress in Release 0.6 via automation Sep 13, 2022
@barrbrain
Copy link
Collaborator Author

There's nothing stopping a user of the rav1e binary from using an external denoiser and setting film grain parameters via the CLI. When we make film grain parameter generation part of the librav1e API, it makes sense to couple it with denoising and RDO changes.

@barrbrain
Copy link
Collaborator Author

Anyhow, as a compromise I suggest completing #2931 on top of this so that both a baseline denoiser and grain parameter generation are available through the CLI.

Copy link
Collaborator

@lu-zero lu-zero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch is fine IMHO.

@barrbrain barrbrain merged commit 88b1afd into xiph:master Sep 13, 2022
Release 0.6 automation moved this from In progress to Done Sep 13, 2022
@barrbrain barrbrain deleted the stable-photon-noise branch September 13, 2022 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants