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

[WIP] Don't require writing coeffs for txfm reconstruction #2086

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KyleSiefring
Copy link
Collaborator

No description provided.

@KyleSiefring KyleSiefring requested a review from ycho January 15, 2020 15:13
Copy link
Collaborator

@ycho ycho left a comment

Choose a reason for hiding this comment

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

Nice catch!

@KyleSiefring
Copy link
Collaborator Author

I think this is only triggered when tx_domain_rate is true. It never is. If I turn it on, I get a segfault with my patch.

Looking at the commit that added this if statement, I feel that I should remove the branch. Going to have to ask Thomas.

85e8829


let has_coeff = if need_recon_pixel || rdo_type.needs_coeff_rate() {
if rdo_type.needs_coeff_rate() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you have missed the condition "need_recon_pixel ||" here? That become true, for ex, when there are multiple tx blocks in a partition and the prediction mode of the partition is intra prediction. Since ref pixels for there should be pixel domain.

@@ -3987,7 +3987,7 @@ impl<'a> ContextWriter<'a> {

if eob == 0 {
self.bc.set_coeff_context(plane, bo, tx_size, xdec, ydec, 0);
return false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

With this change you aren't setting the coefficient context anymore. This might be OK, if you verify that the initial value of the coefficient context in bc is correct, and it's always restored to that after an RDO rollback.

@barrbrain barrbrain added the WorkInProgress Incomplete patchset label Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WorkInProgress Incomplete patchset
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants