On Thu, Sep 29, 2016 at 02:21:17PM +0300, Martin Storsjö wrote:
> On Tue, 20 Sep 2016, Diego Biurrun wrote:
>
> > This avoids SIMD-optimized functions having to sign-extend their
> > stride argument manually to be able to do pointer arithmetic.
> > ---
> > libavcodec/arm/rv34dsp_neon.S | 4 +--
> > libavcodec/arm/rv40dsp_neon.S | 4 +--
> > libavcodec/rv30.c | 4 +--
> > libavcodec/rv30dsp.c | 64
> > +++++++++++++++++++++++++++++++++++--------
> > libavcodec/rv34.c | 10 ++++---
> > libavcodec/rv34.h | 2 +-
> > libavcodec/rv40.c | 2 +-
> > libavcodec/rv40dsp.c | 18 +++++++-----
> > libavcodec/x86/rv34dsp.asm | 4 +--
> > libavcodec/x86/rv40dsp.asm | 11 +++-----
> > libavcodec/x86/rv40dsp_init.c | 4 +--
> > 11 files changed, 85 insertions(+), 42 deletions(-)
> > --- a/libavcodec/rv40dsp.c
> > +++ b/libavcodec/rv40dsp.c
> > @@ -34,7 +34,8 @@
> >
> > #define RV40_LOWPASS(OPNAME, OP) \
> > -static void OPNAME ## rv40_qpel8_h_lowpass(uint8_t *dst, const uint8_t
> > *src, int dstStride, int srcStride,\
> > +static void OPNAME ## rv40_qpel8_h_lowpass(uint8_t *dst, const uint8_t
> > *src, \
> > + ptrdiff_t dstStride, ptrdiff_t
> > srcStride, \
> > const int h, const int
> > C1, const int C2, const int SHIFT){\
>
> Please fix the alignment of this line as well - I don't mind if you do it
> in the same patch.
Changed locally.
> > --- a/libavcodec/x86/rv40dsp.asm
> > +++ b/libavcodec/x86/rv40dsp.asm
> > @@ -77,14 +77,11 @@ SECTION .text
> > ;-----------------------------------------------------------------------------
> > ; subpel MC functions:
> > ;
> > -; void ff_[put|rv40]_rv40_qpel_[h|v]_<opt>(uint8_t *dst, int deststride,
> > -; uint8_t *src, int srcstride,
> > -; int len, int m);
> > +; void ff_[put|avg]_rv40_qpel_[h|v]_<opt>(uint8_t *dst, ptrdiff_t
> > deststride,
> > +; uint8_t *src, ptrdiff_t
> > srcstride,
> > +; int len, ptrdiff_t m);
> > ;----------------------------------------------------------------------
> > %macro LOAD 2
> > -%if WIN64
> > - movsxd %1q, %1d
> > -%endif
> > %ifdef PIC
> > add %1q, picregq
> > %else
> > @@ -438,7 +435,7 @@ FILTER_SSSE3 avg
> >
> > %endmacro
> >
> > -; void ff_rv40_weight_func_%1(uint8_t *dst, uint8_t *src1, uint8_t *src2,
> > int w1, int w2, int stride)
> > +; void ff_rv40_weight_func_%1(uint8_t *dst, uint8_t *src1, uint8_t *src2,
> > int w1, int w2, ptrdiff_t stride)
> > ; %1=size %2=num of xmm regs
> > ; The weights are FP0.14 notation of fractions depending on pts.
> > ; For timebases without rounding error (i.e. PAL), the fractions
>
> So, the only existing sign extension this actually changes is for the "int
> m" parameter. I don't see where the existing code does sign extension for
> srcstride nor dststride anywhere. Isn't that a latent bug, that you're
> fixing silently?
All of the int strides are latent bugs. This file only does sign extension
for WIN64, for one function parameter. Possibly that was the only
environment that triggered an actual, visible bug, I don't know.
> In that case, please first explicitly fix the bug by introducing the right
> sign extensions (which is cherrypickable to release branches), then remove
> them in this patch.
This sounds like overkill to me as we don't know if any real-world
samples are affected. If any such cases creep up I'll gladly add the
necessary sign extension for release branches.
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
