| Summary: | simplify the scaling functions | ||
|---|---|---|---|
| Product: | SDL | Reporter: | Sylvain <sylvain.becker> |
| Component: | render | Assignee: | Sylvain <sylvain.becker> |
| Status: | RESOLVED FIXED | QA Contact: | Sam Lantinga <slouken> |
| Severity: | normal | ||
| Priority: | P2 | ||
| Version: | HG 2.0 | ||
| Hardware: | All | ||
| OS: | All | ||
| Attachments: |
test-case
image test_scaler_nearest.c test_blit.c test_blit.c test_scaler_nearest.c test_scaler_nearest.c test_scaler_nearest.c |
||
|
Description
Sylvain
2021-01-28 10:49:16 UTC
Simplifying the function https://hg.libsdl.org/SDL/rev/6618a817d8d2 https://hg.libsdl.org/SDL/rev/1bc8f3acfcce little improvement: eg: blit slow (activating it manually) with a basic copy: SDL_BlitSlow: < INFO: Time Avg: 4.61 ms Avg Perf: 4608957 (min=4532672) --- > INFO: Time Avg: 4.174 ms Avg Perf: 4174557 (min=4126734) One scale function in SDL_blit_auto: < INFO: Time Avg: 7.856 ms Avg Perf: 7857160 (min=7702752) --- > INFO: Time Avg: 6.998 ms Avg Perf: 6995194 (min=6864477) Created attachment 4725 [details]
test-case
test case
Created attachment 4726 [details]
image
Created attachment 4727 [details]
test_scaler_nearest.c
other test-case:
From the diffs, it's not clear that both revisions are identical.
But scaling with nearest, is only about mapping a 'src' index for each 'dst' index.
So proving the diff revision, can be done by comparing both old and new mappings. Hence this test case.
- compute previous way to scale nearest
- and new way.
both give the same result (tested up to 'from 1..10000 to 1..10000')
Created attachment 4728 [details]
test_blit.c
Update first test-case
Created attachment 4729 [details]
test_blit.c
mark older test-case as obsolete
Looks good, go ahead and make this change. It looks like this code doesn't support sizes > 65535. Can you verify? Created attachment 4733 [details]
test_scaler_nearest.c
Update test-case
Yes, there are issues with large sizes ... but issues are also present in the old code: for instance, if you scale: [0 .. 32768] -> [0..32767] it pulls wrong indexes, dst[0] = src[0] (fine) dst[32767] = src[0] (not ok!) so the old code, works up to 32767, eg: src_width == 32767 is working but fails with src_width == 32768 the new code seems to works up to 65535, eg: src_width == 65535 is working but fails with src_width == 65536 I have updated the test-case to be more flexible. (use CASE==5, set PROCESS_OLD/NEW, tweaks the value val of CASE==5) I think the old code could also be fixed to work up to 65535 (should use some unsigned to the steps). no But I think the best to now to add a check against scaling with w/h > 65535 Adding checks: https://hg.libsdl.org/SDL/rev/79cb445a0635 Maybe the restrictions only apply for the source surfaces. or can be less restrictive for the destination size. but 65535 seems fair enough Yes, we should only apply size restrictions where they are actually necessary. Created attachment 4734 [details] test_scaler_nearest.c update test-case Ok, so it seems there is not need to add restriction on destination size, because scaling remains coherent https://hg.libsdl.org/SDL/rev/af7a34284f0a Created attachment 4739 [details] test_scaler_nearest.c I thought again about it and I am mistaken about the last check removed. If the destination size w or h is too big, then the value are actually wrongs. (increment is truncated and so the scale ratio isn't respected) ( https://hg.libsdl.org/SDL/rev/ce3e0c74b49a ) If somebody need it, it can probably improved by using some int64 at some place but it needs more check about the various implements nearest and linear marked as fixed |