[FFmpeg-devel] [FFmpeg-cvslog] vp8: change mv_{min, max}.{x, y} type to int

[Andreas Cadhalpun]

On 08.06.2015 23:37, Ronald S. Bultje wrote:
> On Mon, Jun 8, 2015 at 5:33 PM, Clément Bœsch <u at pkh.me> wrote:
> 
>> On Mon, Jun 08, 2015 at 11:31:15PM +0200, Andreas Cadhalpun wrote:
>>> ffmpeg | branch: master | Andreas Cadhalpun <
>> Andreas.Cadhalpun at 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 <rsbultje at gmail.com>
>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at 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()?

I didn't know about that function. ;)

> 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.
> 
> (But I can submit a patch for that later.)

If you think these av_clip make it noticeably slower, I think it would be
better to just remove them, because they shouldn't be needed.

Best regards,
Andreas