From 7df93893dcb73dc45733ab4518ce5ca322c25680 Mon Sep 17 00:00:00 2001 From: Pascal Massimino Date: Wed, 5 Aug 2015 15:11:48 +0200 Subject: [PATCH] fix rescaling bug (uninitialized read, see bug #254). the x_add/x_sub increments were wrong for u/v in the upscaling case. They shouldn't be left to the caller's discretion, but set up by WebPRescalerInit to their exact necessary values. -> Cleaned-up WebPRescalerInit() param list. -> added safety asserts -> removed the mips32/mips_r2 variant of "ImportRow" which were buggy prior Change-Id: I347c75804d835811e7025de92a0758d7929dfc09 --- src/dec/io.c | 10 ---- src/dec/vp8l.c | 3 +- src/dsp/rescaler.c | 24 ++++++--- src/dsp/rescaler_mips32.c | 98 ---------------------------------- src/dsp/rescaler_mips_dsp_r2.c | 94 -------------------------------- src/enc/picture_rescale.c | 5 +- src/utils/rescaler.c | 19 ++++--- src/utils/rescaler.h | 2 - 8 files changed, 31 insertions(+), 224 deletions(-) diff --git a/src/dec/io.c b/src/dec/io.c index 09eee60f..86e398a3 100644 --- a/src/dec/io.c +++ b/src/dec/io.c @@ -300,24 +300,18 @@ static int InitYUVRescaler(const VP8Io* const io, WebPDecParams* const p) { work = (int32_t*)p->memory; WebPRescalerInit(&p->scaler_y, io->mb_w, io->mb_h, buf->y, out_width, out_height, buf->y_stride, 1, - io->mb_w, out_width, io->mb_h, out_height, work); WebPRescalerInit(&p->scaler_u, uv_in_width, uv_in_height, buf->u, uv_out_width, uv_out_height, buf->u_stride, 1, - uv_in_width, uv_out_width, - uv_in_height, uv_out_height, work + work_size); WebPRescalerInit(&p->scaler_v, uv_in_width, uv_in_height, buf->v, uv_out_width, uv_out_height, buf->v_stride, 1, - uv_in_width, uv_out_width, - uv_in_height, uv_out_height, work + work_size + uv_work_size); p->emit = EmitRescaledYUV; if (has_alpha) { WebPRescalerInit(&p->scaler_a, io->mb_w, io->mb_h, buf->a, out_width, out_height, buf->a_stride, 1, - io->mb_w, out_width, io->mb_h, out_height, work + work_size + 2 * uv_work_size); p->emit_alpha = EmitRescaledAlphaYUV; WebPInitAlphaProcessing(); @@ -479,15 +473,12 @@ static int InitRGBRescaler(const VP8Io* const io, WebPDecParams* const p) { tmp = (uint8_t*)(work + tmp_size1); WebPRescalerInit(&p->scaler_y, io->mb_w, io->mb_h, tmp + 0 * out_width, out_width, out_height, 0, 1, - io->mb_w, out_width, io->mb_h, out_height, work + 0 * work_size); WebPRescalerInit(&p->scaler_u, uv_in_width, uv_in_height, tmp + 1 * out_width, out_width, out_height, 0, 1, - io->mb_w, 2 * out_width, io->mb_h, 2 * out_height, work + 1 * work_size); WebPRescalerInit(&p->scaler_v, uv_in_width, uv_in_height, tmp + 2 * out_width, out_width, out_height, 0, 1, - io->mb_w, 2 * out_width, io->mb_h, 2 * out_height, work + 2 * work_size); p->emit = EmitRescaledRGB; WebPInitYUV444Converters(); @@ -495,7 +486,6 @@ static int InitRGBRescaler(const VP8Io* const io, WebPDecParams* const p) { if (has_alpha) { WebPRescalerInit(&p->scaler_a, io->mb_w, io->mb_h, tmp + 3 * out_width, out_width, out_height, 0, 1, - io->mb_w, out_width, io->mb_h, out_height, work + 3 * work_size); p->emit_alpha = EmitRescaledAlphaRGB; if (p->output->colorspace == MODE_RGBA_4444 || diff --git a/src/dec/vp8l.c b/src/dec/vp8l.c index 9c213de6..20fc6a59 100644 --- a/src/dec/vp8l.c +++ b/src/dec/vp8l.c @@ -441,8 +441,7 @@ static int AllocateAndInitRescaler(VP8LDecoder* const dec, VP8Io* const io) { scaled_data = (uint32_t*)memory; WebPRescalerInit(dec->rescaler, in_width, in_height, (uint8_t*)scaled_data, - out_width, out_height, 0, num_channels, - in_width, out_width, in_height, out_height, work); + out_width, out_height, 0, num_channels, work); return 1; } diff --git a/src/dsp/rescaler.c b/src/dsp/rescaler.c index 9e5f1b44..d987db4c 100644 --- a/src/dsp/rescaler.c +++ b/src/dsp/rescaler.c @@ -9,6 +9,8 @@ // // Rescaling functions +#include + #include "./dsp.h" #include "../utils/rescaler.h" @@ -24,14 +26,15 @@ static void RescalerImportRowC(WebPRescaler* const wrk, const int x_out_max = wrk->dst_width * wrk->num_channels; int x_in = channel; int x_out; - int accum = 0; if (!wrk->x_expand) { - int sum = 0; + uint32_t sum = 0; + int accum = 0; for (x_out = channel; x_out < x_out_max; x_out += x_stride) { uint32_t base = 0; accum += wrk->x_add; while (accum > 0) { accum -= wrk->x_sub; + assert(x_in < wrk->src_width * x_stride); base = src[x_in]; sum += base; x_in += x_stride; @@ -43,18 +46,27 @@ static void RescalerImportRowC(WebPRescaler* const wrk, sum = (int)MULT_FIX(frac, wrk->fx_scale); } } + assert(accum == 0); } else { // simple bilinear interpolation - int left = src[channel], right = src[channel]; - for (x_out = channel; x_out < x_out_max; x_out += x_stride) { + int accum = wrk->x_add; + int left = src[x_in]; + int right = (wrk->src_width > 1) ? src[x_in + x_stride] : left; + x_in += x_stride; + x_out = channel; + while (1) { + wrk->frow[x_out] = right * wrk->x_add + (left - right) * accum; + x_out += x_stride; + if (x_out >= x_out_max) break; + accum -= wrk->x_sub; if (accum < 0) { left = right; x_in += x_stride; + assert(x_in < wrk->src_width * x_stride); right = src[x_in]; accum += wrk->x_add; } - wrk->frow[x_out] = right * wrk->x_add + (left - right) * accum; - accum -= wrk->x_sub; } + assert(wrk->x_sub == 0 /* <- special case for src_width=1 */ || accum == 0); } // Accumulate the contribution of the new row. for (x_out = channel; x_out < x_out_max; x_out += x_stride) { diff --git a/src/dsp/rescaler_mips32.c b/src/dsp/rescaler_mips32.c index 330de3c5..aef9165c 100644 --- a/src/dsp/rescaler_mips32.c +++ b/src/dsp/rescaler_mips32.c @@ -17,103 +17,6 @@ #include "../utils/rescaler.h" -static void ImportRow(WebPRescaler* const wrk, - const uint8_t* const src, int channel) { - const int x_stride = wrk->num_channels; - const int x_out_max = wrk->dst_width * wrk->num_channels; - const int fx_scale = wrk->fx_scale; - const int x_add = wrk->x_add - wrk->x_sub; - const int x_sub = wrk->x_sub; - int* frow = wrk->frow + channel; - int* irow = wrk->irow + channel; - const uint8_t* src1 = src + channel; - int temp1, temp2, temp3; - int base, frac, sum; - int accum, accum1; - const int x_stride1 = x_stride << 2; - int loop_c = x_out_max - channel; - - if (!wrk->x_expand) { - __asm__ volatile ( - "li %[temp1], 0x8000 \n\t" - "li %[temp2], 0x10000 \n\t" - "li %[sum], 0 \n\t" - "li %[accum], 0 \n\t" - "1: \n\t" - "addu %[accum], %[accum], %[x_add] \n\t" - "blez %[accum], 3f \n\t" - "2: \n\t" - "lbu %[temp3], 0(%[src1]) \n\t" - "subu %[accum], %[accum], %[x_sub] \n\t" - "addu %[src1], %[src1], %[x_stride] \n\t" - "addu %[sum], %[sum], %[temp3] \n\t" - "bgtz %[accum], 2b \n\t" - "3: \n\t" - "lbu %[base], 0(%[src1]) \n\t" - "addu %[src1], %[src1], %[x_stride] \n\t" - "negu %[accum1], %[accum] \n\t" - "mul %[frac], %[base], %[accum1] \n\t" - "addu %[temp3], %[sum], %[base] \n\t" - "mul %[temp3], %[temp3], %[x_sub] \n\t" - "lw %[base], 0(%[irow]) \n\t" - "subu %[loop_c], %[loop_c], %[x_stride] \n\t" - "sll %[accum1], %[frac], 2 \n\t" - "mult %[temp1], %[temp2] \n\t" - "madd %[accum1], %[fx_scale] \n\t" - "mfhi %[sum] \n\t" - "subu %[temp3], %[temp3], %[frac] \n\t" - "sw %[temp3], 0(%[frow]) \n\t" - "add %[base], %[base], %[temp3] \n\t" - "sw %[base], 0(%[irow]) \n\t" - "addu %[irow], %[irow], %[x_stride1] \n\t" - "addu %[frow], %[frow], %[x_stride1] \n\t" - "bgtz %[loop_c], 1b \n\t" - - : [accum] "=&r" (accum), [src1] "+r" (src1), [temp3] "=&r" (temp3), - [sum] "=&r" (sum), [base] "=&r" (base), [frac] "=&r" (frac), - [frow] "+r" (frow), [irow] "+r" (irow), [accum1] "=&r" (accum1), - [temp2] "=&r" (temp2), [temp1] "=&r" (temp1) - : [x_stride] "r" (x_stride), [fx_scale] "r" (fx_scale), - [x_sub] "r" (x_sub), [x_add] "r" (x_add), - [loop_c] "r" (loop_c), [x_stride1] "r" (x_stride1) - : "memory", "hi", "lo" - ); - } else { - __asm__ volatile ( - "lbu %[temp1], 0(%[src1]) \n\t" - "move %[temp2], %[temp1] \n\t" - "li %[accum], 0 \n\t" - "1: \n\t" - "bgez %[accum], 2f \n\t" - "move %[temp2], %[temp1] \n\t" - "addu %[src1], %[x_stride] \n\t" - "lbu %[temp1], 0(%[src1]) \n\t" - "addu %[accum], %[x_add] \n\t" - "2: \n\t" - "subu %[temp3], %[temp2], %[temp1] \n\t" - "mul %[temp3], %[temp3], %[accum] \n\t" - "mul %[base], %[temp1], %[x_add] \n\t" - "subu %[accum], %[accum], %[x_sub] \n\t" - "lw %[frac], 0(%[irow]) \n\t" - "subu %[loop_c], %[loop_c], %[x_stride] \n\t" - "addu %[temp3], %[base], %[temp3] \n\t" - "sw %[temp3], 0(%[frow]) \n\t" - "addu %[frow], %[x_stride1] \n\t" - "addu %[frac], %[temp3] \n\t" - "sw %[frac], 0(%[irow]) \n\t" - "addu %[irow], %[x_stride1] \n\t" - "bgtz %[loop_c], 1b \n\t" - - : [src1] "+r" (src1), [accum] "=&r" (accum), [temp1] "=&r" (temp1), - [temp2] "=&r" (temp2), [temp3] "=&r" (temp3), [base] "=&r" (base), - [frac] "=&r" (frac), [frow] "+r" (frow), [irow] "+r" (irow) - : [x_stride] "r" (x_stride), [x_add] "r" (x_add), [x_sub] "r" (x_sub), - [x_stride1] "r" (x_stride1), [loop_c] "r" (loop_c) - : "memory", "hi", "lo" - ); - } -} - static void ExportRow(WebPRescaler* const wrk, int x_out) { if (wrk->y_accum <= 0) { uint8_t* const dst = wrk->dst; @@ -183,7 +86,6 @@ static void ExportRow(WebPRescaler* const wrk, int x_out) { extern void WebPRescalerDspInitMIPS32(void); WEBP_TSAN_IGNORE_FUNCTION void WebPRescalerDspInitMIPS32(void) { - WebPRescalerImportRow = ImportRow; WebPRescalerExportRow = ExportRow; } diff --git a/src/dsp/rescaler_mips_dsp_r2.c b/src/dsp/rescaler_mips_dsp_r2.c index 5f2a969b..e5f9c28b 100644 --- a/src/dsp/rescaler_mips_dsp_r2.c +++ b/src/dsp/rescaler_mips_dsp_r2.c @@ -17,99 +17,6 @@ #include "../utils/rescaler.h" -static void ImportRow(WebPRescaler* const wrk, - const uint8_t* const src, int channel) { - const int x_stride = wrk->num_channels; - const int x_out_max = wrk->dst_width * wrk->num_channels; - const int fx_scale = wrk->fx_scale; - const int x_add = wrk->x_add - wrk->x_sub; - const int x_sub = wrk->x_sub; - int* frow = wrk->frow + channel; - int* irow = wrk->irow + channel; - const uint8_t* src1 = src + channel; - int temp1, temp2, temp3; - int base, frac, sum; - int accum, accum1; - const int x_stride1 = x_stride << 2; - int loop_c = x_out_max - channel; - - if (!wrk->x_expand) { - __asm__ volatile ( - "li %[sum], 0 \n\t" - "li %[accum], 0 \n\t" - "1: \n\t" - "addu %[accum], %[accum], %[x_add] \n\t" - "blez %[accum], 3f \n\t" - "2: \n\t" - "lbu %[temp3], 0(%[src1]) \n\t" - "subu %[accum], %[accum], %[x_sub] \n\t" - "addu %[src1], %[src1], %[x_stride] \n\t" - "addu %[sum], %[sum], %[temp3] \n\t" - "bgtz %[accum], 2b \n\t" - "3: \n\t" - "lbu %[base], 0(%[src1]) \n\t" - "addu %[src1], %[src1], %[x_stride] \n\t" - "negu %[accum1], %[accum] \n\t" - "mul %[frac], %[base], %[accum1] \n\t" - "addu %[temp3], %[sum], %[base] \n\t" - "mul %[temp3], %[temp3], %[x_sub] \n\t" - "lw %[base], 0(%[irow]) \n\t" - "sll %[accum1], %[frac], 1 \n\t" - "subu %[loop_c], %[loop_c], %[x_stride] \n\t" - "mulq_rs.w %[sum], %[accum1], %[fx_scale] \n\t" - "subu %[temp3], %[temp3], %[frac] \n\t" - "sw %[temp3], 0(%[frow]) \n\t" - "add %[base], %[base], %[temp3] \n\t" - "sw %[base], 0(%[irow]) \n\t" - "addu %[irow], %[irow], %[x_stride1] \n\t" - "addu %[frow], %[frow], %[x_stride1] \n\t" - "bgtz %[loop_c], 1b \n\t" - - : [accum]"=&r"(accum), [src1]"+&r"(src1), [temp3]"=&r"(temp3), - [sum]"=&r"(sum), [base]"=&r"(base), [frac]"=&r"(frac), - [frow]"+&r"(frow), [irow]"+&r"(irow), [accum1]"=&r"(accum1), - [loop_c]"+&r"(loop_c) - : [x_stride]"r"(x_stride), [fx_scale]"r"(fx_scale), [x_sub]"r"(x_sub), - [x_add] "r" (x_add), [x_stride1] "r" (x_stride1) - : "memory", "hi", "lo" - ); - } else { - __asm__ volatile ( - "lbu %[temp1], 0(%[src1]) \n\t" - "move %[temp2], %[temp1] \n\t" - "li %[accum], 0 \n\t" - "1: \n\t" - "bgez %[accum], 2f \n\t" - "move %[temp2], %[temp1] \n\t" - "addu %[src1], %[x_stride] \n\t" - "lbu %[temp1], 0(%[src1]) \n\t" - "addu %[accum], %[x_add] \n\t" - "2: \n\t" - "subu %[temp3], %[temp2], %[temp1] \n\t" - "mul %[temp3], %[temp3], %[accum] \n\t" - "mul %[base], %[temp1], %[x_add] \n\t" - "subu %[accum], %[accum], %[x_sub] \n\t" - "lw %[frac], 0(%[irow]) \n\t" - "subu %[loop_c], %[loop_c], %[x_stride] \n\t" - "addu %[temp3], %[base], %[temp3] \n\t" - "sw %[temp3], 0(%[frow]) \n\t" - "addu %[frow], %[x_stride1] \n\t" - "addu %[frac], %[temp3] \n\t" - "sw %[frac], 0(%[irow]) \n\t" - "addu %[irow], %[x_stride1] \n\t" - "bgtz %[loop_c], 1b \n\t" - - : [src1]"+&r"(src1), [accum]"=&r"(accum), [temp1]"=&r"(temp1), - [temp2]"=&r"(temp2), [temp3]"=&r"(temp3), [base]"=&r"(base), - [frac]"=&r"(frac), [frow]"+&r"(frow), [irow]"+&r"(irow), - [loop_c]"+&r"(loop_c) - : [x_stride]"r"(x_stride), [x_add]"r"(x_add), [x_sub]"r"(x_sub), - [x_stride1]"r"(x_stride1) - : "memory", "hi", "lo" - ); - } -} - static void ExportRow(WebPRescaler* const wrk, int x_out) { if (wrk->y_accum <= 0) { // if wrk->fxy_scale can fit into 32 bits use optimized code, @@ -201,7 +108,6 @@ static void ExportRow(WebPRescaler* const wrk, int x_out) { extern void WebPRescalerDspInitMIPSdspR2(void); WEBP_TSAN_IGNORE_FUNCTION void WebPRescalerDspInitMIPSdspR2(void) { - WebPRescalerImportRow = ImportRow; WebPRescalerExportRow = ExportRow; } diff --git a/src/enc/picture_rescale.c b/src/enc/picture_rescale.c index de52848c..60339ac8 100644 --- a/src/enc/picture_rescale.c +++ b/src/enc/picture_rescale.c @@ -181,10 +181,7 @@ static void RescalePlane(const uint8_t* src, int y = 0; WebPRescalerInit(&rescaler, src_width, src_height, dst, dst_width, dst_height, dst_stride, - num_channels, - src_width, dst_width, - src_height, dst_height, - work); + num_channels, work); memset(work, 0, 2 * dst_width * num_channels * sizeof(*work)); while (y < src_height) { y += WebPRescalerImport(&rescaler, src_height - y, diff --git a/src/utils/rescaler.c b/src/utils/rescaler.c index db1ea6c9..ffb5f575 100644 --- a/src/utils/rescaler.c +++ b/src/utils/rescaler.c @@ -19,9 +19,11 @@ //------------------------------------------------------------------------------ void WebPRescalerInit(WebPRescaler* const wrk, int src_width, int src_height, - uint8_t* const dst, int dst_width, int dst_height, - int dst_stride, int num_channels, int x_add, int x_sub, - int y_add, int y_sub, int32_t* const work) { + uint8_t* const dst, + int dst_width, int dst_height, int dst_stride, + int num_channels, int32_t* const work) { + const int x_add = src_width, x_sub = dst_width; + const int y_add = src_height, y_sub = dst_height; wrk->x_expand = (src_width < dst_width); wrk->src_width = src_width; wrk->src_height = src_height; @@ -36,11 +38,12 @@ void WebPRescalerInit(WebPRescaler* const wrk, int src_width, int src_height, wrk->y_accum = y_add; wrk->y_add = y_add; wrk->y_sub = y_sub; - wrk->fx_scale = (1 << WEBP_RESCALER_RFIX) / x_sub; - wrk->fy_scale = (1 << WEBP_RESCALER_RFIX) / y_sub; - wrk->fxy_scale = wrk->x_expand ? - ((int64_t)dst_height << WEBP_RESCALER_RFIX) / (x_sub * src_height) : - ((int64_t)dst_height << WEBP_RESCALER_RFIX) / (x_add * src_height); + if (!wrk->x_expand) { // fx_scale is not used otherwise + wrk->fx_scale = (1 << WEBP_RESCALER_RFIX) / wrk->x_sub; + } + wrk->fy_scale = (1 << WEBP_RESCALER_RFIX) / wrk->y_sub; + wrk->fxy_scale = + ((int64_t)dst_height << WEBP_RESCALER_RFIX) / (wrk->x_add * src_height); wrk->irow = work; wrk->frow = work + num_channels * dst_width; diff --git a/src/utils/rescaler.h b/src/utils/rescaler.h index dd138b1d..42a544d0 100644 --- a/src/utils/rescaler.h +++ b/src/utils/rescaler.h @@ -46,8 +46,6 @@ void WebPRescalerInit(WebPRescaler* const rescaler, uint8_t* const dst, int dst_width, int dst_height, int dst_stride, int num_channels, - int x_add, int x_sub, - int y_add, int y_sub, int32_t* const work); // Returns the number of input lines needed next to produce one output line,