On Mon, Jun 08, 2015 at 05:37:22PM -0400, Ronald S. Bultje wrote: > On Mon, Jun 8, 2015 at 5:33 PM, Clément Bœsch <u...@pkh.me> wrote: > > > On Mon, Jun 08, 2015 at 11:31:15PM +0200, Andreas Cadhalpun wrote: > > > ffmpeg | branch: master | Andreas Cadhalpun < > > andreas.cadhal...@googlemail.com> | Mon Jun 8 22:38:29 2015 +0200| > > [6fdbaa2b7fb56623ab2163f861952bc1408c39b3] | committer: Andreas Cadhalpun > > > > > > vp8: change mv_{min,max}.{x,y} type to int > > > > > > If one of the dimensions is larger than 8176, s->mb_width or > > > s->mb_height is larger than 511, leading to an int16_t overflow of > > > s->mv_max.{x,y}. This then causes av_clip to be called with amin > amax. > > > > > > Changing the type to int avoids the overflow and has no negative > > > effect, because s->mv_max is only used in clamp_mv for clipping. > > > Since mv_max.{x,y} is positive and mv_min.{x,y} negative, av_clip can't > > > increase the absolute value. The input to av_clip is an int16_t, and > > > thus the output fits into int16_t as well. > > > > > > For additional safety, s->mv_{min,max}.{x,y} are clipped to int16_t range > > > before use. > > > > > > Reviewed-by: Ronald S. Bultje <rsbul...@gmail.com> > > > Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com> > > > > > > > > > http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=6fdbaa2b7fb56623ab2163f861952bc1408c39b3 > > > --- > > > > > > libavcodec/vp8.c | 6 ++++-- > > > libavcodec/vp8.h | 9 +++++++-- > > > 2 files changed, 11 insertions(+), 4 deletions(-) > > > > > > diff --git a/libavcodec/vp8.c b/libavcodec/vp8.c > > > index dbba568..becbb2c 100644 > > > --- a/libavcodec/vp8.c > > > +++ b/libavcodec/vp8.c > > > @@ -757,8 +757,10 @@ static int vp8_decode_frame_header(VP8Context *s, > > const uint8_t *buf, int buf_si > > > static av_always_inline > > > void clamp_mv(VP8Context *s, VP56mv *dst, const VP56mv *src) > > > { > > > - dst->x = av_clip(src->x, s->mv_min.x, s->mv_max.x); > > > - dst->y = av_clip(src->y, s->mv_min.y, s->mv_max.y); > > > + dst->x = av_clip(src->x, av_clip(s->mv_min.x, INT16_MIN, INT16_MAX), > > > + av_clip(s->mv_max.x, INT16_MIN, > > INT16_MAX)); > > > + dst->y = av_clip(src->y, av_clip(s->mv_min.y, INT16_MIN, INT16_MAX), > > > + av_clip(s->mv_max.y, INT16_MIN, > > INT16_MAX)); > > > > Sorry I might have missed something, but why not use av_clip_int16()? > > > > Hm yeah I forgot about that, sorry. You could also consider clipping (into > a second variable, since this one is used to increment in the main block > decode loop) so that you only clip once per mb (horizontal) + once per row > (vertical), instead of twice per mv, which might be 32x for a 4x4 block. >
I see some inline in this function and the av_clip* ones, so maybe the compiler is smart enough to handle that. av_clip* functions are marked with the av_const attribute to help that; if it's still not enough, maybe adding av_pure to those would help. > (But I can submit a patch for that later.) > > Ronald -- Clément B.
pgp5cggwNfDnY.pgp
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel