This already helps performance as-is, but will help performance massively
once we add the ability for the memcpy backend to do a refcopy instead of
an actual copy.
Sponsored-by: Sovereign Tech Fund
Signed-off-by: Niklas Haas <git@haasn.dev>
If the filter cannot actually be optimized into the read (for whatever
reason), this code would previously loop infinitely. Bail out cleanly
instead.
The FFSWAP is there to make the error message print the remainder (the one
containing unsplittable ops), rather than the noop list.
Sponsored-by: Sovereign Tech Fund
Signed-off-by: Niklas Haas <git@haasn.dev>
Instead of a loop with fixed structure, this function now recursively calls
itself as many times as needed to satisfy all criteria.
This is absolutely needed for the upcoming refactor which will allow for
also splitting apart ops lists as needed to e.g. handle partially subsampled
ops lists, which may need a complex sequence of filtering and merge steps
to be fully satisfied.
This does modify the way in which subpasses are compiled slightly, in that
each new subpass first tried again un-split, rather than a single split
resulting in all subsequent passes being split as well. This is mostly a
benign change, though it might matter one day.
Sponsored-by: Sovereign Tech Fund
Signed-off-by: Niklas Haas <git@haasn.dev>
This will make it easier to keep passing around these parameters in helper
functions in the upcoming refactor.
Take the opportunity to also rename the plain `compile` function to
`compile_single`.
Sponsored-by: Sovereign Tech Fund
Signed-off-by: Niklas Haas <git@haasn.dev>
Otherwise, this will false negative if the redundant operations haven't
been optimized away yet, resulting in unnecessary memcpy operations.
Fixes: a534156083
Sponsored-by: Sovereign Tech Fund
Signed-off-by: Niklas Haas <git@haasn.dev>
Makes ff_sws_compile_pass() more robust; will be needed for plane splitting.
Besides, it's perfectly valid to have an operation list that starts with
e.g. SWS_OP_CLEAR.
Sponsored-by: Sovereign Tech Fund
Signed-off-by: Niklas Haas <git@haasn.dev>
This requires some tiny bit of extra setup work from the dispatch layer.
Specifically, we need to arrange for the palette data pointer to end up in
exec.in[1], and to disable the pointer advancement logic for this plane (this
can be accomplished by just setting the stride and bump to 0).
We also want to disable the tail buffer / overflow pixel copying logic for
the palette, which can be accomplished by ensuring that p->planes_in only
contains the number of *data* planes, excluding the fixed palette.
Sponsored-by: Sovereign Tech Fund
Signed-off-by: Niklas Haas <git@haasn.dev>
This allows adding passes which will be dispatched over a reduced number of
lines, without affecting the allocated buffer dimensions - e.g. for passes
which purely write to subsampled chroma planes.
A few hard-coded references to pass->width/height need to be replaced by
the corresponding output frame references, but it's not a huge deal.
Sponsored-by: Sovereign Tech Fund
Signed-off-by: Niklas Haas <git@haasn.dev>
I want to start adding more data layouts, like semiplanar formats (nv12), or
palette formats. I made an effort to distinguish existing checks for rw.packed
into "mode != PLANAR" and "mode == PACKED", based on the intent of the
surrounding code, in anticipation of these new layouts.
Sponsored-by: Sovereign Tech Fund
Signed-off-by: Niklas Haas <git@haasn.dev>
This won't change from frame to frame, so there's no reason to redundantly
re-setup these fields. Paves the way for the next change as well.
The one minor annoyance is that this relies on SwsOpList.src/dst being
populated, to gain access to the sub_x/sub_y fields. However, that's not
a big ask, given that e.g. the dispatch layer already relies on the
pixel dimensions from this field being accurate for sizing intermediate
buffers during filter splitting.
Sponsored-by: Sovereign Tech Fund
Signed-off-by: Niklas Haas <git@haasn.dev>
The block size is given in units of pixels, so this message as written
does not even make sense.
Sponsored-by: Sovereign Tech Fund
Signed-off-by: Niklas Haas <git@haasn.dev>
See previous commit for justification. I decided to split these
refactors up into several independent commits to make it easier
to review and bisect, since they are all independent atomic changes.
Sponsored-by: Sovereign Tech Fund
Signed-off-by: Niklas Haas <git@haasn.dev>
I want to introduce operations like semiplanar reads, which would
possibly require a different number of over_read bytes per plane.
That aside, this is just a general cleanliness improvement.
Sponsored-by: Sovereign Tech Fund
Signed-off-by: Niklas Haas <git@haasn.dev>
This is a minor cosmetic improvement that allows me to use more
convenient names for a filter-related metadata fields, without
confusion.
Sponsored-by: Sovereign Tech Fund
Signed-off-by: Niklas Haas <git@haasn.dev>
This allows constraining the set of available backends. This serves as a
better replacement for the "unstable" flag, which is a bit ambiguous. Allows
users to, for example, opt into the memcpy or x86 backend, while excluding
e.g. the upcoming JIT backends.
Signed-off-by: Niklas Haas <git@haasn.dev>
These loops were both assuming that `h` lines need to be copied; but this
varies. First of all, for plane subsampling; but more importantly, when
vertically scaling, the input line count may be substantially lower than the
actual line count.
This fixes an out-of-bounds read/write when vertically upscaling with a tail
buffer.
Verifiable via e.g.:
make libswscale/tests/swscale
valgrind -- libswscale/tests/swscale -s 63x63 -src yuv444p -dst rgb24 \
-flags unstable -align_src 1 -align_dst 1
(As well as the SSIM scores, which drop from ~e-5 to ~e-3 without this fix)
Sponsored-by: Sovereign Tech Fund
Signed-off-by: Niklas Haas <git@haasn.dev>
The code made the fundamental assumption that over-read into the padding
bytes is okay to do; because the most that can happen is that those pixel
values end up corrupted, which doesn't affect any adjacent pixels.
However, this is not true for SWS_OP_FILTER_H, because this operation
fundamentally mixes together horizontal pixels. Normally, this was fine,
because the filter weights for those pixels are set to 0, and 0 * x = 0.
However, that is not true for floating point inputs, which can contain
Infinity; and 0 * Infinity = NaN, thus corrupting the entire pixel.
Solve it by specifically preventing over-read when it would be unsafe.
Signed-off-by: Niklas Haas <git@haasn.dev>
Allows the uops macro generation code to not actually compile any passes.
More generally, this could be used to e.g. test if an op list is supported by
a backend without actually creating the passes.
The `bool first` change is needed because the `input == prev` check no longer
works if we don't actually compiled any passes.
Signed-off-by: Niklas Haas <git@haasn.dev>
This will be used eventually when I rewrite checkasm/sw_ops to re-use the
code in ops_dispatch.c instead of hand-rolling the execution layer.
Signed-off-by: Niklas Haas <git@haasn.dev>
Assign to `exec_base.in_offset_x` before the error handling,
to ensure the error cleanup path properly frees the already
allocated memory.
Fixes Coverity issue #1691725
Not only does this take into account extreme edge cases where the plane
padding can significantly exceed the actual width/stride, but it also
correctly takes into account the filter offsets when scaling; which the
previous code completely ignored.
Simpler, robuster, and more correct. Now valgrind passes for 100% of format
conversions for me, with and without scaling.
Signed-off-by: Niklas Haas <git@haasn.dev>
This is a mostly straightforward internal mechanical change that I wanted
to isolate from the following commit to make bisection easier in the case of
regressions.
While the number of tail blocks could theoretically be different for input
vs output memcpy, the extra complexity of handling that mismatch (and
adjusting all of the tail offsets, strides etc.) seems not worth it.
I tested this commit by manually setting `p->tail_blocks` to higher values
and seeing if that still passed the self-check under valgrind.
Signed-off-by: Niklas Haas <git@haasn.dev>
The x86 kernel e.g. assumes that at least one block is processed; so avoid
calling this with an empty width. This is currently only possible if e.g.
operating on an unpadded, very small image whose total linesize is less than
a single block.
Signed-off-by: Niklas Haas <git@haasn.dev>
This code had two issues:
1. It was over-allocating bytes for the input offset map case, and
2. It was hard-coding the assumption that there is only a single tail block
We can fix both of these issues by rewriting the way the tail size is derived.
In the non-offset case, and assuming only 1 tail block:
aligned_w - safe_width
= num_blocks * block_size - (num_blocks - 1) * block_size
= block_size
Additionally, the FFMAX(tail_size_in/out) is unnecessary, because:
tail_size = pass->width - safe_width <= aligned_w - safe_width
In the input offset case, we instead realize that the input kernel already
never over-reads the input due to the filter size adjustment/clamping, so
the only thing we need to ensure is that we allocate extra bytes for the
input over-read.
Signed-off-by: Niklas Haas <git@haasn.dev>
The over_read/write fields are not documented as depending on the subsampling
factor. Actually, they are not documented as depending on the plane at all.
If and when we do actually add support for horizontal subsampling to this
code, it will most likely be by turning all of these key variables into
arrays, which will be an upgrade we get basically for free.
Signed-off-by: Niklas Haas <git@haasn.dev>
This makes it far less likely to accidentally add or remove a +7 bias when
repeating this often-used expression.
Signed-off-by: Niklas Haas <git@haasn.dev>
This could trigger if e.g. a backend tries to operate on monow formats with
a block size that is not a multiple of 1. In this case, `block_size_in`
would previously be miscomputed (to e.g. 0), which is obviously wrong.
Signed-off-by: Niklas Haas <git@haasn.dev>
As well as weird edge cases like trying to filter `monow` and pixels landing
in the middle of a byte. Realistically, this will never happen - we'd instead
pre-process it into something byte-aligned, and then dispatch a byte-aligned
filter on it.
However, I need to add a check for overflow in any case, so we might as well
add the alignment check at the same time. It's basically free.
Signed-off-by: Niklas Haas <git@haasn.dev>
Prevents valgrind from complaining about operating on uninitialized bytes.
This should be cheap as it's only done once during setup().
Signed-off-by: Niklas Haas <git@haasn.dev>
As a consequence of the fact that the frame pool API doesn't let us directly
access the linesize, we have to "un-translate" the over_read/write back to
the nearest multiple of the pixel size.
Signed-off-by: Niklas Haas <git@haasn.dev>
Just define these directly as integer arrays; there's really no point in
having them re-use SwsSwizzleOp; the only place this was ever even remotely
relevant was in the no-op check, which any decent compiler should already
be capable of optimizing into a single 32-bit comparison.
Signed-off-by: Niklas Haas <git@haasn.dev>
First, we try compiling the filter pass as-is; in case any backends decide to
handle the filter as a single pass. (e.g. Vulkan, which will want to compile
such using internal temporary buffers and barriers)
If that fails, retry with a chained list of split passes.
Sponsored-by: Sovereign Tech Fund
Signed-off-by: Niklas Haas <git@haasn.dev>
This is cheap to precompute and can be used as-is for gather-style horizontal
filter implementations.
Sponsored-by: Sovereign Tech Fund
Signed-off-by: Niklas Haas <git@haasn.dev>
Rather than dispatching the compiled function for each line of the tail
individually, with a memcpy to a shared buffer in between, this instead copies
the entire tail region into a temporary intermediate buffer, processes it with
a single dispatch call, and then copies the entire result back to the
destination.
The main benefit of this is that it enables scaling, subsampling or other
quirky layouts to continue working, which may require accessing lines adjacent
to the main input.
It also arguably makes the code a bit simpler and easier to follow, but YMMV.
One minor consequence of the change in logic is that we also no longer handle
the last row of an unpadded input buffer separately - instead, if *any* row
needs to be padded, *all* rows in the current slice will be padded. This is
a bit less efficient but much more predictable, and as discussed, basically
required for scaling/filtering anyways.
While we could implement some sort of hybrid regime where we only use the new
logic when scaling is needed, I really don't think this would gain us anything
concrete enough to be worth the effort, especially since the performance is
basically roughly the same across the board:
16 threads:
yuv444p 1920x1080 -> ayuv 1920x1080: speedup=1.000x slower (input memcpy)
rgb24 1920x1080 -> argb 1920x1080: speedup=1.012x faster (output memcpy)
1 thread:
yuv444p 1920x1080 -> ayuv 1920x1080: speedup=1.062x faster (input memcpy)
rgb24 1920x1080 -> argb 1920x1080: speedup=0.959x slower (output memcpy)
Overall speedup is +/- 1% across the board, well within margin of error.
Sponsored-by: Sovereign Tech Fund
Signed-off-by: Niklas Haas <git@haasn.dev>
This is more useful for tight loops inside CPU backends, which can implement
this by having a shared path for incrementing to the next line (as normal),
and then a separate path for adding an extra position-dependent, stride
multiplied line offset after each completed line.
As a free upside, this encoding does not require any separate/special handling
for the exec tail.
Sponsored-by: Sovereign Tech Fund
Signed-off-by: Niklas Haas <git@haasn.dev>
And use it to look up the correct source plane line for each destination
line. Needed for vertical scaling, in which case multiple output lines can
reference the same input line.
Sponsored-by: Sovereign Tech Fund
Signed-off-by: Niklas Haas <git@haasn.dev>
