-
Notifications
You must be signed in to change notification settings - Fork 201
Add unit tests for InterpolationFilter::filterXxY #586
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
Conversation
adamjw24
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the two minor ocmments looks good to me.
| DimensionGenerator dim; | ||
|
|
||
| // Max buffer size for src is ( height + 7 ) * srcStride. | ||
| std::vector<Pel> src( ( MAX_CU_SIZE + 7 ) * MAX_CU_SIZE ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually max buffer size is 23x23 ((16+7)x(16+7)). The +7 in your case is not needed since a block of size MAX_CU_SIZExMAX_CU_SIZE is never tested here, thus 128x128 is always enough to fit the input. And if it was, it would be necessary to extend both the width and height of the input buffer (otherwise the filter uses samples from the next line at the of a line for horizontal filtering)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see for the 16xN actually takes blocks up to 16x128. Still, a 128x128 buffer is large enough to hold that (23x135 samples).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if you really want to test it with real stride, than you need to allocate space for the filter taps both horizontally and vertically, thus 135x135 (max cu + 7 x max cu + 7)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick feedback. I was going to say that in some of my debugging, I saw some height values for filter8x8 N8 that reaches up to 128. And the srcStride actually prints some value like srcStride_max = 2208 for example. But I decided to limit the srcStride in the test to 128 to keep the buffer size within (128+7) * 128.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the source is usually the reference picture - so it has the stride of a full padded frame (1920+ padding in your case I guess?). But the test is realistic enough if you make the input buffer 135x135.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I will go with 135x135 then.
Add unit tests for the chroma 4-tap filters:
- filter4x4_N4
- filter8x8_N4
- filter16x16_N4
These tests run for both the isLast = {false, true} conditions.
Also make the filter coefficients public so they can be referenced in
the unit tests.
Add unit tests for the luma 8-tap filters:
- filter4x4_N8
- filter8x8_N8
- filter16x16_N8
These tests run for both the isLast = {false, true} conditions.
76b1ba1 to
e2502c4
Compare
|
Patch amended. The following changes were applied to N8 unit test:
Note that for widths > 4, the height for N8 is up to 128, while the height for N4 is up to 32. (as observed in my debug prints) |
Add unit tests for the chroma 4-tap and luma 8-tap filters:
These tests run for both the isLast = {false, true} conditions.
Also make the filter coefficients public so they can be referenced in the unit tests.