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.

Sign up for GitHub

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

Jump to bottom

Signal interp_filter in the bitstream. #1518

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

Signal interp_filter in the bitstream. #1518

wants to merge 1 commit into from

Conversation

tdaede
Copy link
Collaborator

@tdaede tdaede commented Aug 1, 2019

No description provided.

@coveralls
Copy link
Collaborator

coveralls commented Aug 1, 2019 •
edited
Loading

Coverage Status

Coverage increased (+0.05%) to 75.472% when pulling e1c3f48 on tdaede:interpfilter into edbc586 on xiph:master.

@tdaede tdaede changed the title WIP: Signal interp_filter in the bitstream. Signal interp_filter in the bitstream. Aug 10, 2019
@tdaede tdaede requested a review from YaLTeR August 10, 2019 01:01
@tdaede
Copy link
Collaborator Author

tdaede commented Aug 10, 2019

about a 0.04% loss https://beta.arewecompressedyet.com/?job=interpfilter-2019-08-09_175924-f3c43a4&job=master-ec0c7787dd1598f4cecaedda2a120ff4ab0eb6d0

@tdaede
Copy link
Collaborator Author

tdaede commented Aug 23, 2019

going to actually stack all of the interpfilter commits here before landing

@ycho
Copy link
Collaborator

ycho commented Oct 8, 2019

Can we land this?

@barrbrain
Copy link
Collaborator

A first step toward this was landed in #1866.

@ycho
Copy link
Collaborator

ycho commented Nov 25, 2019

I might have seen this PR, but was not able to recall this much progress is already there. I hope we want not to waste this PR and land of most of it please.

ycho
ycho reviewed Nov 25, 2019
View reviewed changes
src/encoder.rs
if fi.is_filter_switchable
&& ContextWriter::needs_interp_filter(bsize, luma_mode)
{
cw.write_interp_filter(w, tile_bo, bsize, 0, FilterMode::REGULAR);
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 the reason why there was desync in your test is that the decoder does not think it needs to decode the filter for each inter mode block if a frame header does not have is_filter_switchable = true, i.e. instead decoder already has decoded default filter. In spec, https://aomediacodec.github.io/av1-spec/#inter-block-mode-info-syntax, there is if block by " if ( interpolation_filter == SWITCHABLE ) {", if true it decodes the filter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's the fi.is_filter_switchable flag here I think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, I mean it should sync with 'is_filter_switchable' written in frame header. Otherwise, I think the decoder does not decode the info by cw.write_interp_filter(w, tile_bo, bsize, 0, FilterMode::REGULAR); ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right. I think it is synced, unless I'm missing something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I landed 485d179#diff-e59bfbbede3bbaad0b099696e5ec5716L635 , and if fi.is_filter_switchable is true, encoder does not write defalut filter. Then, decoder knows it needs to decode filter from each inter mode block.

@ycho
Copy link
Collaborator

ycho commented Nov 25, 2019 •
edited
Loading

When you rebased on master today, was there still desync? I think it should not be there.
Since 485d179
Before 485d179, frame header still have default filter written when your PR set is_switchable_filter = true, that I believe can cause desync.

@tdaede
Copy link
Collaborator Author

tdaede commented Nov 25, 2019

No, there is no desync.

@ycho
Copy link
Collaborator

ycho commented Nov 25, 2019

No, there is no desync.

Great!

@tdaede
Signal interp_filter in the bitstream.
8e47ac1 
A small loss due to additional symbols coded.

master-ec0c7787dd1598f4cecaedda2a120ff4ab0eb6d0 -> interpfilter-2019-08-09_175924-f3c43a4

  PSNR | PSNR Cb | PSNR Cr | PSNR HVS |   SSIM | MS SSIM | CIEDE 2000
0.0426 |  0.1195 |  0.2747 |   0.0629 | 0.0270 |  0.0501 |     0.1175

https://beta.arewecompressedyet.com/?job=interpfilter-2019-08-09_175924-f3c43a4&job=master-ec0c7787dd1598f4cecaedda2a120ff4ab0eb6d0
ycho
ycho approved these changes Nov 25, 2019
View reviewed changes
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.

Please land, once it passes all travis tests.

@ycho
Copy link
Collaborator

ycho commented Nov 27, 2019

@tdaede Would you have plan to follow up the build errors (mismatch) soon, or might be later after your current other work?

@tdaede
Copy link
Collaborator Author

tdaede commented Nov 27, 2019

@ycho I will look at it a bit today, but if I don't find a fix I'd appreciate it if you could also look.

@ycho
Copy link
Collaborator

ycho commented Dec 2, 2019 •
edited
Loading

@ycho I will look at it a bit today, but if I don't find a fix I'd appreciate it if you could also look.

Sure, I wanted to help on this. Just I got completely taken by my son during thx giving holidays. Also, I think I plan move on this after trying fixing desync one #1858.

@ycho
Copy link
Collaborator

ycho commented Dec 9, 2019 •
edited
Loading

With local testing, this causes corruption from 2nd frame, either speed 0, 6, or 10. I expect speed 10 does not encode filter type per blocks, since its block size is 64x64 >= 32x32 checked by needs_interp_filter(), other than right or bottom frame borders.

Testing with:
./target/release/rav1e nyan.y4m -o test.ivf -r test_rec.y4m --quantizer 50 --speed=0 --limit=30
./target/release/rav1e nyan.y4m -o test.ivf -r test_rec.y4m --quantizer 50 --speed=6 --limit=30
./target/release/rav1e nyan.y4m -o test.ivf -r test_rec.y4m --quantizer 50 --speed=10 --limit=30
only 1st frame is decodable.

@ycho
Copy link
Collaborator

ycho commented Dec 10, 2019

No, there is no desync.

Which input and speed have you tested with?

@ycho ycho added the WorkInProgress Incomplete patchset label Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YaLTeR YaLTeR YaLTeR approved these changes

@ycho ycho ycho approved these changes

Assignees
No one assigned
Labels
WorkInProgress Incomplete patchset
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tdaede @coveralls @ycho @barrbrain @YaLTeR