From ffae0655938f4ebfc99893b2de907abe0465bece Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Wed, 31 May 2023 09:54:29 -0400 Subject: [PATCH] WebP/Lossy: Clamp right after summing IDCT output, instead of later https://datatracker.ietf.org/doc/html/rfc6386#section-14.5 says: """ The summing procedure is fairly straightforward, having only a couple of details. The prediction and residue buffers are both arrays of 16-bit signed integers. Each individual (Y, U, and V pixel) result is calculated first as a 32-bit sum of the prediction and residue, and is then saturated to 8-bit unsigned range (using, say, the clamp255 function defined above) before being stored as an 8-bit unsigned pixel value. """ It's IMHO not 100% clear if the clamping is supposed to happen immediately (so that it affects prediction inputs for the next macroblock) or later. But vp8_dixie_idct_add() on page 173 in https://datatracker.ietf.org/doc/html/rfc6386#section-20.8 does: recon[0] = CLAMP_255(predict[0] + ((a1 + d1 + 4) >> 3)); So it does look like it should happen immediately. (I'm a bit confused why the spec then says "The prediction and residue buffers are both arrays of 16-bit signed integers", since the prediction buffer can just be an u8 buffer now, without changing behavior. --- .../Libraries/LibGfx/ImageFormats/WebPLoaderLossy.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Userland/Libraries/LibGfx/ImageFormats/WebPLoaderLossy.cpp b/Userland/Libraries/LibGfx/ImageFormats/WebPLoaderLossy.cpp index 37b1b20665..c5aa35f46a 100644 --- a/Userland/Libraries/LibGfx/ImageFormats/WebPLoaderLossy.cpp +++ b/Userland/Libraries/LibGfx/ImageFormats/WebPLoaderLossy.cpp @@ -1051,7 +1051,7 @@ void add_idct_to_prediction(Span prediction, Coefficients coefficients, int for (int py = 0; py < 4; ++py) { for (int px = 0; px < 4; ++px) { i16& p = prediction[(4 * y + py) * N + (4 * x + px)]; - p += idct_output[py * 4 + px]; + p = clamp(p + idct_output[py * 4 + px], 0, 255); } } } @@ -1123,11 +1123,11 @@ void convert_yuv_to_rgb(Bitmap& bitmap, int mb_x, int mb_y, ReadonlySpan y_ // "is then saturated to 8-bit unsigned range (using, say, the // clamp255 function defined above) before being stored as an 8-bit // unsigned pixel value." - u8 Y = clamp(y_data[y * 16 + x], 0, 255); + u8 Y = y_data[y * 16 + x]; // FIXME: Could do nicer upsampling than just nearest neighbor - u8 U = clamp(u_data[(y / 2) * 8 + x / 2], 0, 255); - u8 V = clamp(v_data[(y / 2) * 8 + x / 2], 0, 255); + u8 U = u_data[(y / 2) * 8 + x / 2]; + u8 V = v_data[(y / 2) * 8 + x / 2]; // XXX: These numbers are from the fixed-point values in libwebp's yuv.h. There's probably a better reference somewhere. int r = 1.1655 * Y + 1.596 * V - 222.4;