On 07.06.2015 22:39, Michael Niedermayer wrote:
> On Sun, Jun 07, 2015 at 04:27:42PM -0400, Ronald S. Bultje wrote:
>> So what happens when you change mv_max/min to be integers (instead of
>> int16_t) without further touching VP56mv, e.g. by making mv_max/min a
>> VP8intminmaxmv (a newly created type just for this purpose, using int
>> instead of int16_t)?
This fixes the problem (but I chose the shorter name VP8intmv).
A patch implementing this is attached.
It seems libvpx is actually also using int for this type of information,
although the implementation is different and instead of sv_{min,max}.{x,y}
uses mb_to_{left,right,bottom,top}_edge (at least if I understood the code
correctly). [1][2]
> would this work with
> clamp_mv() ?
> it limits based in mv_max/min but writes into a VP56mv, so if these
> out of 16bit limits can actually be reached then the output of
> the clip would not fit in the 16bit destination ...
> but maybe this doesnt occur, ive not deeply investigated
I think it doesn't happen, but for safety I clip s->mv_{min,max}.{x,y}
to int16_t range.
Best regards,
Andreas
1:
https://sources.debian.net/src/libvpx/1.4.0-3/vp8/common/reconinter.c/?hl=340#L340
2:
https://sources.debian.net/src/libvpx/1.4.0-3/vp9/common/vp9_blockd.h/?hl=203#L203
>From 0c18c05c3010eae7fc1b5e09367115e3ba0081a5 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun <[email protected]>
Date: Mon, 8 Jun 2015 22:38:29 +0200
Subject: [PATCH] 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} is clipped to in16_t range
before use.
Signed-off-by: Andreas Cadhalpun <[email protected]>
---
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));
}
/**
diff --git a/libavcodec/vp8.h b/libavcodec/vp8.h
index b650892..2135bd9 100644
--- a/libavcodec/vp8.h
+++ b/libavcodec/vp8.h
@@ -134,6 +134,11 @@ typedef struct VP8Frame {
AVBufferRef *seg_map;
} VP8Frame;
+typedef struct VP8intmv {
+ int x;
+ int y;
+} VP8intmv;
+
#define MAX_THREADS 8
typedef struct VP8Context {
VP8ThreadData *thread_data;
@@ -152,8 +157,8 @@ typedef struct VP8Context {
uint8_t deblock_filter;
uint8_t mbskip_enabled;
uint8_t profile;
- VP56mv mv_min;
- VP56mv mv_max;
+ VP8intmv mv_min;
+ VP8intmv mv_max;
int8_t sign_bias[4]; ///< one state [0, 1] per ref frame type
int ref_count[3];
--
2.1.4
_______________________________________________
ffmpeg-devel mailing list
[email protected]
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
