-
Notifications
You must be signed in to change notification settings - Fork 77
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
Resampling engine - edges processing error #431
Comments
Sorry, I don't understand this duplication issue. The only difference I see is that there are more shades in AVS+. I have a few questions if you could check them: How does fmtconv behave? |
The essence of the issue: AVS+ resampler create ringed frame's edge result if the (good/perfect) anti-ring conditioned transient is located 'too close' to the frame's buffer edge. Other (avsresize and expected other correct) resamplers - make perfect result without ringing at this case. The issue is ringed resize result. The 'duplication' is possible workaround to handle edge special cases (where discontinuity for resampler engine happens). If we 'pad framebuffer' with its edge samples to the length about support_size/2 we throw this discontinuity out of the output. It is one possible solution of the issue. Possible source of the issue - incorrect handling of the 'edge special case' for resampler where there is not enough input samples for full filter's support covering. Expected solution for this special edge case - make duplication of the last sample to fill all required samples for the filter's support at the start (and end) of the framebuffer processing (it can be simulated with AddBorders(filter_support/2, r=0) (where filter_support is LanczosResize(taps=16) support of 16). I expect this special edge case must be handled in the 'resampling program' creation - but do not found correctly looking solution - starting from this https://github.com/pinterf/AviSynthPlus/blob/33fa8d6a085b37a74b61a5b298517e75b3cd18fc/avs_core/filters/resample_functions.cpp#L372 And at the resampler's processing it must be used - in C-implementation looks like here - https://github.com/pinterf/AviSynthPlus/blob/33fa8d6a085b37a74b61a5b298517e75b3cd18fc/avs_core/filters/resample.cpp#L104 . But I do not see any visible attempt to 'virtual frame expansion' at start and end of the processing loop to handle these special cases (with a way like first and last sample duplication for example). We need to look at other resampler;s implementations how it is designed to handle such edge case- like avsresize sources (or may be even 'more simple' JincResize sources - they are lower in size and 'resampling program' is somehow more simple - starting from As the convolution+resampling loops do not have special border_case handling - https://github.com/Asd-g/AviSynth-JincResize/blob/9d813f95e2aee549800e64470ddd6b2841a51c84/src/JincResize.cpp#L500 it looks everything is somehow solved at the table of coefficients generation step. May be JincResize engine uses different way of solving the issue like altering kernel function so it not rings near borders (?). Though it may be not the single possible solution. I need to found sources of resampling engine from avsresize to look how it is designed to attempt to show difference with AVS+ resampler. |
Testing with fmtconv - it shows some residual ringing (the p=10 for Gauss is not perfect but gives somehow less blurring, p=8 make less ringing in 8bit). But the total ringing is much less. I make Photoshop levels expansion to show better. Script:
If process transient with p=8 - the ringing difference is even more:
I even thinking of recommend to change default p-param for AddBorders/LetterBox default Gauss kernel from 10 to 8 as it create close to no-ringing at least for 8bit result. Though it create more blurring. |
Here, in fmtconv is an edge condition mentioned: |
Yes - it must be equal at all 4 frame edges. Top and bottom were only first simple example. |
Possible workaround without changing current kernel for resampling (and may be best quality as all 'active frame samples' being processed far from edges of the buffer with non-changed filter's kernel):
Other solutions may be compared with this method for quality. If the method of adjusting kernel near edge will provide lower quality - it may be recommended in the Notes in Resize documentation or implemented as second way of handling edge conditions (with lower performance because of more samples to process and more RAM usage if being implemented as intermediate buffer size increasing instead of edge samples duplicating at row/column reading into SIMD with edge sample broadcasting). Also tested 'negative crop' as Resize parameter (to replace additional AddBorders(pad,pad,pad,pad) before Resize) - it do not help (looks like edges samples are duplicated after resampling instaed of before). |
Testing of padded-AVS+ resize vs avsresize and fmtconv with internal edge handling:
The avsresize vs fmtconv is about equal (with about 1LSB difference at some corner samples) but all slightly different from 'padded resize'. The worst at r4246 is unpadded AVS+ (std) resize. |
The used in avsresize and fmtc workarounds for handling edge conditions are also not perfect and can not provide equal quality of resampler for center frame area and at the edges.
The padded versions of both avsresize and fmtconv also make some different outputs in comparison with unpadded resize by same engines. So different edges conditions handling workarounds (in different resize engines) gives still different results (vs padded method). It looks changing of kernel of resize filter in the 'resampling program' for handling edge conditions is not perfect way (but may give best performance because resampler process lowest data size). |
Also looks like other bug in Resize:
Return green frame after PointResize() While trying to do any content of the frame padded resize with edges samples repeating via negative-cropping padding like:
|
Yes, PointResize fails. I tried to fix it, but it's better to revamp our resizers altogether. There are also quite a few hacks that I'd like to stop using. So, thank you, I have everything needed for the reproductions and the other artifacts as well, and I'll start working on it in the next week(s). |
I really not sure what is the correct values for 'negative crop' for Resize for Are the values of May be negative should be added new border size only ? Like |
Meanwhile, there have been many achievements in fixing existing resizer issues. The green pointresize result is fixed. I understood the edge condition from fmtconv and adopted its logic. This led to the need to handle variable kernel sizes at the beginning and end, allowing the resizing process to no longer restrict actual frame sizes. Thanks to this, I successfully removed all limitations that resulted in errors like "this width/height is too small for this resizer/support/whatever." Then, significant code simplification is in progress (finally!) by dropping some SSSE3 and SSE4.1 8-bit resizer variants, which sacrificed accuracy for a slightly quicker mulhrs. I believe this was a mid-2000s hack. I retained SSE2, which is correct. Similarly, the AVX2 version was rewritten to avoid using mulhrs. I came across these while looking into the old resizer sources. Having different effective kernel sizes made the fixed-filter-size template-cunami unnecessary as well (simplifying this is in progress). Now, there is a (hidden) ability to make the resizers have an option not working in "keep-the-image-center" mode (which will probably be needed for more accurate convolution use). This is now fixed to how Avisynth worked so far, but may be extracted to parameter level. The "// TODO this look wrong, gotta check" comment is deleted, finally. We still don't know if it was wrong, anyway it's done in another logical way. Other hacks that were needed to protect the end-of-scanline access for the edge conditions can likely be removed as well. I guess, I'd need this week for doing the cleanup, depending on a possible ongoing release process, this may or may not fit into the time frame. |
It is great news on updating an old AVS resampling engine. But anyway changing kernel at some areas of the frame (line/row) will degrade quality. As you see even workarounds from avsresize and fmtconv can not provide even quality between inner part of frame and at the edges of still 'active frame area'. So for the better future it is nice to have an optimized (for performance) version of the 'padded-resize' method when we get equal quality over all 'active frame areas'. Currently in scripting form it is a 3 stages process:
Or maybe we can warp stages 1 and 2 using the 'negative crop' option ? But as I remember my tests it is applied after resampling not before and can not help in this way. |
Here is an actual build, x64 only, it may crash here and there, but in recent builds I was not able to crash it and did not see unwanted artifacts. Btw I mentioned changing kernel size, really, the kernel size does not change, but if there is no valid position on left (top) or right (bottom) side, it is using the accumulated coefficients on the first/last valid pixel. The same as if it was padded on the left and the right with the edge pixel value. |
First test with old script -
VirtualDub return grey frame and report System exception - Access violation. It looks like still memory corruption somewhere exist - complex scripts report access violation on simple lines:
Return error: Access violation at line 10 and line 23. Simple AddBorders also stop working as expected - either no borders added (frame size is as expected larger) and only grey frame returned or access violation with green frame (and some random pixels at the top) returned:
Still unstable and can not even got source to start test resizer. Setting SetMaxCPU("none") looks like start to help.
The version with AddBorders(2, 2, 2, 2, r=2, param1=8) is not totally clean - but I not sure if it is enough 'ideal' with too short transient size near edge. But it is anyway much better in comparison with old AVS resampler. For 4:4:4 format it allow to create transient with r=3 and it also looks clean (in 8bit) - very good. As was noted in that ARIB document good quality transient must be from 6 samples size and more. The 4 samples size of r=2 may be not enough. This also create equal result with 'padded resize'
Now we need SIMD-enabled version of functions to be stable. Will try to test next days more. |
Yep, C vs SIMD work in progress, minor adjustsments (like loop end limits) are needed but at many places, I just wanted you to see the results when it works fine. |
Well - what is working syntax for negative src_width=, src_height= for PointResize ? I tested different values - but all failed. Only left and top padding-duplication is working. Right and bottom either start to crop or return black frame:
With any values for src_width=, src_height= the size of the frame never expanded to 2_x_pad for width and height even. It looks PointResize(c, c.width, c.height, ...) always returns an image of size width_x_height and any src_left/right/width/height can not change image size but only shifts input image inside the same frame size. Looks this way is not applicable if we need to expand frame size by edges samples duplication and we need to use other plugins like FillBorders/FillMargins ? Though it also can not increase frame size. Sad. This makes testing more complex - I need to check edge sample value in some editor and use manual AddBorders() with the same color value. May we have a special mode for AddBorders with edge color (sample value) duplication ? Maybe as a special color-command word for parameter 'color'. After some testing: Looks the AddBorders(2,2,2,2,r=2) source processing is good enough. The difference is because r=2 makes an edge sample of about value=2 and it is not equal to the current 'padded resize' virtual border expansion with value=0 (color=black). So it is a limitation of both current test tools and too narrow length of transient. The AddBorders(1,1,1,1,r=1) will make an even larger difference. It is partially recommended to be added to documentation of AddBorders/LetterBox: The r=1 and r=2 transient half-length is not enough for best quality. It can be treated as usual digital imaging issues like 'not enough data for upsampling/decoding engine to understand the initial shape of digitally encoded transient between 2 levels' . Same applicable for unfiltered transients causing 'Gibbs-ringing'. Here we have some smaller versions of the same source. Currently users have a limitation for 4:2:0 and 4:2:2 formats - the r=3 transient length processing is not supported and looks like truncated to 2:
Though we have full-band Y plane and r=3 may be applied to this plane (and r=2 to UV planes) to make quality somehow better before going into more blurring and/or more changing frame size r=4 processing value. Making conversion to 4:4:4 and back to get r=3 quality or edges transients only will add more distortions to the all frame area and also slower. Current documentation of AddBorders : https://avisynthplus.readthedocs.io/en/latest/avisynthdoc/corefilters/addborders.html The r radius is automatically adjusted with the video format subsampling requirements. AddBorders won't give error if e.g. for a YV12 r=1 is given: due to the chroma subampling it will be automatically promoted to 2. But it not note limiting of r=3 to r=2 for both Y and UV planes for YV12 ? For better quality even with AddBorders(2, r=3) for YV12 it is better to use r=3 for Y plane and r=2 for UV (though it looks only r=1 possible for UVs ?). Current documentation note: "The exact dimensions can be a bit different, e.g. if r is larger than the actually added left border. Then the radius is reduced accordingly." It may be not best way and we can allow some asymmetrical filtering result to copy to output frame for a bit better quality (need check) with very narrow used number of samples like 1,2,3. So AddBorders(2, r=3) with r non-truncated to 2 may give some better quality in comparison with AddBorders(2, r=2). Same is with AddBorders(1,r=2) vs AddBorders(1,r=1) |
" This led to the need to handle variable kernel sizes at the beginning and end, allowing the resizing process to no longer restrict actual frame sizes. Thanks to this, I successfully removed all limitations that resulted in errors like "this width/height is too small for this resizer/support/whatever."" Now you can try to remove 'magic-numbers' of +-10 from AddBorders input image cropped parts to filtering engine and replace with +-(r+support/2). Or if it is not enough - max expected is (r+support). Where support is the 'support' value returned by current selected and configured filtering kernel - member function
With this changes user can set r-param value to AddBorders/LetterBox to high enough values (also p-param to Gauss kernel and other kernels with big enough supports possible) and not got kernel and/or total convolution result truncated and distorted output if larger r-values is required. |
Please, try this one. I don't know why I said that the previous version didn't crash, I had lucky dimensions or so, hah, almost nothing worked after that - except the C, but even the C did not work then in a scenario. I had a black vertical line in C mode, spent two hours on it then it turned out that ConvertBits 16->8 C mode was buggy ( (0xFFFF + 128 rounder) >> 8 was 256, so it gave me a nice white line in 8 bit, which I thought the 1000x tested C resizer was buggy. C, SSExx or AVX2, 8 bit, 10-16 bit a float, all must be OK now. Unless you find something. It's late here, so I'm gonna read the AddBorder stuff later. |
Yes - it become much more stable and can run with default SetMaxCPU() (all SIMD enabled). At least with some short tests with YV12 and YV24 with a few frame sizes. I found working better combination of YV24 source and Now we have new transient after filtering being 'centered' between 2 samples. But we can add some shift to the kernel and it will make shifting (up to +-0.5 in integer coordinate) of the transient to the inner or outer part of the frame. It may be the same as using src_top and src_left < 1.0 crop-shift params. Though with 'simple' implementation it will also shift part of the frame and it may be (or not) visible. In the more 'complex' implementation we can shift only transient's shape leaving original content of the frame at the filtered area non-shifted. But it require some thinking - may be first make sub-sample cropping of the frame-part of the transient to make shift to the opposite direction. And this cropping must be performed with 'interpolating' filter kernel like sinc. It require some experiments and thinking in the math. The simple implementation may be only add 'src_left/top' sub-shifting param to the standard resampler and see if it cause more or less visible shifted (and possible visible border at the edge of crop-cut-paste area of filtered into original frame). This may be some 'nice to have' feature for next versions of AddBorders/LetterBox. |
I read at forum some more complain on AVS+ core resampler like it process additional chroma-sub sample shift at downscale (or any rescale ?). https://forum.doom9.org/showthread.php?p=2016643#post2016643 Starting to check the samples values - looks like found issue with PointResize: Is it duplicates left and top sample twice (line shifting to right and bottom):
Output looks in VirtualDub 400% - |
I've prepared another build for you to try. This version includes significant code cleanup and optimization. It has been changed a lot, mainly the 8 bit horizontals. https://drive.google.com/uc?export=download&id=16erBvpSd6SynlLMtyGBwjGHTitgqhUy_ From now on, I'll focus on making the code ready for commits. Btw you mentioned the magic 10 (+/-10) which I set as a minimum for the transient filtering width, do I have to do anything about that? |
Thank you for next build. Will try it next days.
It is too small if user want to set higher gauss fliter radius/support/size (like AddBorders(20,r=10, param1=1) to get long size soft borders. You can try either set it to (r+support) or at least conditional max(10, r+support) . Where 'support' is return value from ResamplingFunction class (after constructor init its members and support for gauss kernel is calculated in auto-mode). Need testing. |
Google shows 404 error for the link. Have you delete that file ? |
Don't know why. Reuploaded a new build, (with the max(10, r, support()) addition) |
One more bug found (with previous build while testing):
return grey field. No added border and no subtitle. The BlankClip(100, 200, 20, color=$7F7F7F, pixel_type="YV24") - YV24 input format is working. |
Expected minimum required number of samples to input to filter for +-(r) required filtered output is +-(r+support). So the half-width of the cropped area to filter expected to be enough as not max (10 or r or support) as I understand max(10, r, support()) Though as returned support is double - it is nice to have clip it to nearest larger integer as we have in the 'filter_size' calculation to not miss something possibly useful.
So it is better to use r+(int(ceil(support)) .
This download link is working now. Got downloaded and will test now more. |
Thanks, I get it now. I will check your report as well in the evening session. |
For the 'very low' supports like 2.01 the usage of int(ceil(2.01)) return 3 and it make better protection from missing something useful for the input to filtering process. And as I understand (currently think) - the convolution process do not input samples at the distance longer than +-support (ceiled to nearest integer) from current sample to output so more samples in the input do not change output result. And for radius of filter r - the max distance to used input samples is r+support (to one side of processing). Addition: Tested with build from 210325 - the script of
Is working now as expected with both YV12 and YV24 formats. |
O.k, it must be the image center calculation, there is a +0.5 inside until I figured out hor it matches exactly with old avs and the others. I will check it to match exactly as expected. |
In 20250318 it was wrong, since 20250320 it's OK. |
Next build, If it really works, it was worth digging into the resizer update project. Namely taking the chroma position into account. Personally I did not meet with the problem, but I remembered it, and your link is showing one such issue. Check the readme_history inside for details. In brief: After understanding the reason for chroma shifts and the "keep center" parameters, they have been made able to be parameterized or used automatically. Until now Avisynth was resizing the chroma with the same center position like luma. (0.5, 0.5) New Parameters:
AddBorders, LetterBox The internal blurring resizers take the chroma placement into account (_ChromaLocation frame prop) The resizer area is +/- (r + ceil(filter.support()) https://drive.google.com/uc?export=download&id=1TCrWka9pI3GmAW81aoyGOFtZDZ5678QV |
And yes, there is a difference. I chose a format which has not the center as its default placement.
|
Looks like working good. Even the output of script-based AddBordersHF() with filtered transients is now equal to AddBorders() with very large r and low p-param. Looks like something more was changed also. |
Test script:
It looks resampler engine for Resize() filters in AVS+ core do not handle edge conditions as expected (last/edge sample duplication at least to match 'support' of the resize kernel at the beginning of line or column processing or some other method). Tested with r4246.
The text was updated successfully, but these errors were encountered: