From: Carl Hetherington Date: Mon, 4 Nov 2019 15:38:14 +0000 (+0100) Subject: Fix incorrect images when cropping without stretch. X-Git-Tag: v2.14.14~2 X-Git-Url: https://main.carlh.net/gitweb/?p=dcpomatic.git;a=commitdiff_plain;h=ed4fc06db6957b2b63b2400a737f47c18a1003be Fix incorrect images when cropping without stretch. Always overallocate images so that Image::crop_scale_window is always safe from over-reading buffers. Relates to #1654 and probably #1653. Backported from 7b0372776ac4da6a8e4ff29f41a4f08b9b4de506 in v2.15.x. --- diff --git a/src/lib/image.cc b/src/lib/image.cc index d3b193e7b..1439d9f7f 100644 --- a/src/lib/image.cc +++ b/src/lib/image.cc @@ -140,29 +140,7 @@ Image::crop_scale_window ( DCPOMATIC_ASSERT (out_size.width >= inter_size.width); DCPOMATIC_ASSERT (out_size.height >= inter_size.height); - /* Here's an image of out_size. Below we may write to it starting at an offset so we get some padding. - Hence we want to write in the following pattern: - - block start write start line end - |..(padding)..|<------line-size------------->|..(padding)..| - |..(padding)..|<------line-size------------->|..(padding)..| - |..(padding)..|<------line-size------------->|..(padding)..| - - where line-size is of the smaller (inter_size) image and the full padded line length is that of - out_size. To get things to work we have to tell FFmpeg that the stride is that of out_size. - However some parts of FFmpeg (notably rgb48Toxyz12 in swscale.c) process data for the full - specified *stride*. This does not matter until we get to the last line: - - block start write start line end - |..(padding)..|<------line-size------------->|XXXwrittenXXX| - |XXXwrittenXXX|<------line-size------------->|XXXwrittenXXX| - |XXXwrittenXXX|<------line-size------------->|XXXwrittenXXXXXXwrittenXXX - ^^^^ out of bounds - - To get around this, we ask Image to overallocate its buffers by the overrun. - */ - - shared_ptr out (new Image (out_format, out_size, out_aligned, (out_size.width - inter_size.width) / 2)); + shared_ptr out (new Image(out_format, out_size, out_aligned)); out->make_black (); /* Size of the image after any crop */ @@ -245,6 +223,14 @@ Image::crop_scale_window ( sws_freeContext (scale_context); + if (crop != Crop() && cropped_size == inter_size && _pixel_format == out_format) { + /* We are cropping without any scaling or pixel format conversion, so FFmpeg may have left some + data behind in our image. Clear it out. It may get to the point where we should just stop + trying to be clever with cropping. + */ + out->make_part_black (corner.x + cropped_size.width, out_size.width - cropped_size.width); + } + return out; } @@ -342,6 +328,36 @@ Image::swap_16 (uint16_t v) return ((v >> 8) & 0xff) | ((v & 0xff) << 8); } +void +Image::make_part_black (int x, int w) +{ + switch (_pixel_format) { + case AV_PIX_FMT_RGB24: + case AV_PIX_FMT_ARGB: + case AV_PIX_FMT_RGBA: + case AV_PIX_FMT_ABGR: + case AV_PIX_FMT_BGRA: + case AV_PIX_FMT_RGB555LE: + case AV_PIX_FMT_RGB48LE: + case AV_PIX_FMT_RGB48BE: + case AV_PIX_FMT_XYZ12LE: + { + int const h = sample_size(0).height; + int const bpp = bytes_per_pixel(0); + int const s = stride()[0]; + uint8_t* p = data()[0]; + for (int y = 0; y < h; y++) { + memset (p + x * bpp, 0, w * bpp); + p += s; + } + break; + } + + default: + throw PixelFormatError ("make_part_black()", _pixel_format); + } +} + void Image::make_black () { @@ -815,13 +831,11 @@ Image::bytes_per_pixel (int c) const * @param p Pixel format. * @param s Size in pixels. * @param aligned true to make each row of this image aligned to a 32-byte boundary. - * @param extra_pixels Amount of extra "run-off" memory to allocate at the end of each plane in pixels. */ -Image::Image (AVPixelFormat p, dcp::Size s, bool aligned, int extra_pixels) +Image::Image (AVPixelFormat p, dcp::Size s, bool aligned) : _size (s) , _pixel_format (p) , _aligned (aligned) - , _extra_pixels (extra_pixels) { allocate (); } @@ -855,13 +869,36 @@ Image::allocate () over-reads by more then _avx. I can't follow the code to work out how much, so I'll just over-allocate by 32 bytes and have done with it. Empirical testing suggests that it works. + + In addition to these concerns, we may read/write as much as a whole extra line + at the end of each plane in cases where we are messing with offsets in order to + do pad or crop. To solve this we over-allocate by an extra _stride[i] bytes. + + As an example: we may write to images starting at an offset so we get some padding. + Hence we want to write in the following pattern: + + block start write start line end + |..(padding)..|<------line-size------------->|..(padding)..| + |..(padding)..|<------line-size------------->|..(padding)..| + |..(padding)..|<------line-size------------->|..(padding)..| + + where line-size is of the smaller (inter_size) image and the full padded line length is that of + out_size. To get things to work we have to tell FFmpeg that the stride is that of out_size. + However some parts of FFmpeg (notably rgb48Toxyz12 in swscale.c) process data for the full + specified *stride*. This does not matter until we get to the last line: + + block start write start line end + |..(padding)..|<------line-size------------->|XXXwrittenXXX| + |XXXwrittenXXX|<------line-size------------->|XXXwrittenXXX| + |XXXwrittenXXX|<------line-size------------->|XXXwrittenXXXXXXwrittenXXX + ^^^^ out of bounds */ - _data[i] = (uint8_t *) wrapped_av_malloc (_stride[i] * sample_size(i).height + _extra_pixels * bytes_per_pixel(i) + 32); + _data[i] = (uint8_t *) wrapped_av_malloc (_stride[i] * (sample_size(i).height + 1) + 32); #if HAVE_VALGRIND_MEMCHECK_H /* The data between the end of the line size and the stride is undefined but processed by libswscale, causing lots of valgrind errors. Mark it all defined to quell these errors. */ - VALGRIND_MAKE_MEM_DEFINED (_data[i], _stride[i] * sample_size(i).height + _extra_pixels * bytes_per_pixel(i) + 32); + VALGRIND_MAKE_MEM_DEFINED (_data[i], _stride[i] * (sample_size(i).height + 1) + 32); #endif } } @@ -871,7 +908,6 @@ Image::Image (Image const & other) , _size (other._size) , _pixel_format (other._pixel_format) , _aligned (other._aligned) - , _extra_pixels (other._extra_pixels) { allocate (); @@ -891,7 +927,6 @@ Image::Image (AVFrame* frame) : _size (frame->width, frame->height) , _pixel_format (static_cast (frame->format)) , _aligned (true) - , _extra_pixels (0) { allocate (); @@ -912,7 +947,6 @@ Image::Image (shared_ptr other, bool aligned) : _size (other->_size) , _pixel_format (other->_pixel_format) , _aligned (aligned) - , _extra_pixels (other->_extra_pixels) { allocate (); @@ -954,7 +988,6 @@ Image::swap (Image & other) } std::swap (_aligned, other._aligned); - std::swap (_extra_pixels, other._extra_pixels); } /** Destroy a Image */ diff --git a/src/lib/image.h b/src/lib/image.h index dbcb38cc7..51d768a13 100644 --- a/src/lib/image.h +++ b/src/lib/image.h @@ -41,7 +41,7 @@ class Socket; class Image : public boost::enable_shared_from_this { public: - Image (AVPixelFormat p, dcp::Size s, bool aligned, int extra_pixels = 0); + Image (AVPixelFormat p, dcp::Size s, bool aligned); explicit Image (AVFrame *); explicit Image (Image const &); Image (boost::shared_ptr, bool); @@ -92,6 +92,7 @@ private: void allocate (); void swap (Image &); + void make_part_black (int x, int w); void yuv_16_black (uint16_t, bool); static uint16_t swap_16 (uint16_t); @@ -101,7 +102,6 @@ private: int* _line_size; ///< array of sizes of the data in each line, in bytes (without any alignment padding bytes) int* _stride; ///< array of strides for each line, in bytes (including any alignment padding bytes) bool _aligned; - int _extra_pixels; }; extern PositionImage merge (std::list images); diff --git a/src/lib/j2k_image_proxy.cc b/src/lib/j2k_image_proxy.cc index d4c7a8716..31fda2510 100644 --- a/src/lib/j2k_image_proxy.cc +++ b/src/lib/j2k_image_proxy.cc @@ -137,14 +137,7 @@ J2KImageProxy::prepare (optional target_size) const } shared_ptr decompressed = dcp::decompress_j2k (const_cast (_data.data().get()), _data.size (), reduce); - - /* When scaling JPEG2000 images (using AV_PIX_FMT_XYZ12LE) ffmpeg will call xyz12ToRgb48 which reads data - from the whole of the image stride. If we are cropping, Image::crop_scale_window munges the - start addresses of each image row (to do the crop) but keeps the stride the same. This means - that under crop we will read over the end of the image by the amount of the crop. To allow this - to happen without invalid memory access we need to overallocate by one whole stride's worth of pixels. - */ - _image.reset (new Image (_pixel_format, decompressed->size(), true, decompressed->size().width)); + _image.reset (new Image (_pixel_format, decompressed->size(), true)); int const shift = 16 - decompressed->precision (0); diff --git a/test/image_test.cc b/test/image_test.cc index 8b809c591..8579785a7 100644 --- a/test/image_test.cc +++ b/test/image_test.cc @@ -270,12 +270,43 @@ BOOST_AUTO_TEST_CASE (crop_scale_window_test) /** Special cases of Image::crop_scale_window which triggered some valgrind warnings */ BOOST_AUTO_TEST_CASE (crop_scale_window_test2) { - /* This 2048 does the same as J2KImageProxy does when it makes an image */ - shared_ptr image (new Image(AV_PIX_FMT_XYZ12LE, dcp::Size(2048, 858), true, 2048)); + shared_ptr image (new Image(AV_PIX_FMT_XYZ12LE, dcp::Size(2048, 858), true)); image->crop_scale_window (Crop(279, 0, 0, 0), dcp::Size(1069, 448), dcp::Size(1069, 578), dcp::YUV_TO_RGB_REC709, AV_PIX_FMT_RGB24, false, false); image->crop_scale_window (Crop(2048, 0, 0, 0), dcp::Size(1069, 448), dcp::Size(1069, 578), dcp::YUV_TO_RGB_REC709, AV_PIX_FMT_RGB24, false, false); } +BOOST_AUTO_TEST_CASE (crop_scale_window_test3) +{ + shared_ptr proxy(new FFmpegImageProxy("test/data/player_seek_test_0.png")); + shared_ptr xyz = proxy->image().first->convert_pixel_format(dcp::YUV_TO_RGB_REC709, AV_PIX_FMT_RGB24, true, false); + shared_ptr cropped = xyz->crop_scale_window(Crop(512, 0, 0, 0), dcp::Size(1486, 1080), dcp::Size(1998, 1080), dcp::YUV_TO_RGB_REC709, AV_PIX_FMT_RGB24, false, false); + write_image(cropped, "build/test/crop_scale_window_test3.png", "RGB", MagickCore::CharPixel); +} + +BOOST_AUTO_TEST_CASE (crop_scale_window_test4) +{ + shared_ptr proxy(new FFmpegImageProxy("test/data/player_seek_test_0.png")); + shared_ptr xyz = proxy->image().first->convert_pixel_format(dcp::YUV_TO_RGB_REC709, AV_PIX_FMT_RGB24, true, false); + shared_ptr cropped = xyz->crop_scale_window(Crop(512, 0, 0, 0), dcp::Size(1486, 1080), dcp::Size(1998, 1080), dcp::YUV_TO_RGB_REC709, AV_PIX_FMT_XYZ12LE, false, false); + write_image(cropped, "build/test/crop_scale_window_test4.png", "RGB", MagickCore::ShortPixel); +} + +BOOST_AUTO_TEST_CASE (crop_scale_window_test5) +{ + shared_ptr proxy(new FFmpegImageProxy("test/data/player_seek_test_0.png")); + shared_ptr xyz = proxy->image().first->convert_pixel_format(dcp::YUV_TO_RGB_REC709, AV_PIX_FMT_XYZ12LE, true, false); + shared_ptr cropped = xyz->crop_scale_window(Crop(512, 0, 0, 0), dcp::Size(1486, 1080), dcp::Size(1998, 1080), dcp::YUV_TO_RGB_REC709, AV_PIX_FMT_RGB24, false, false); + write_image(cropped, "build/test/crop_scale_window_test5.png", "RGB", MagickCore::CharPixel); +} + +BOOST_AUTO_TEST_CASE (crop_scale_window_test6) +{ + shared_ptr proxy(new FFmpegImageProxy("test/data/player_seek_test_0.png")); + shared_ptr xyz = proxy->image().first->convert_pixel_format(dcp::YUV_TO_RGB_REC709, AV_PIX_FMT_XYZ12LE, true, false); + shared_ptr cropped = xyz->crop_scale_window(Crop(512, 0, 0, 0), dcp::Size(1486, 1080), dcp::Size(1998, 1080), dcp::YUV_TO_RGB_REC709, AV_PIX_FMT_XYZ12LE, false, false); + write_image(cropped, "build/test/crop_scale_window_test6.png", "RGB", MagickCore::ShortPixel); +} + BOOST_AUTO_TEST_CASE (as_png_test) { shared_ptr proxy(new FFmpegImageProxy("test/data/3d_test/000001.png")); diff --git a/test/test.cc b/test/test.cc index 50770a687..a9d3ed1c7 100644 --- a/test/test.cc +++ b/test/test.cc @@ -1,5 +1,5 @@ /* - Copyright (C) 2012-2018 Carl Hetherington + Copyright (C) 2012-2019 Carl Hetherington This file is part of DCP-o-matic. @@ -417,11 +417,11 @@ wait_for_jobs () } void -write_image (shared_ptr image, boost::filesystem::path file, string format) +write_image (shared_ptr image, boost::filesystem::path file, string format, MagickCore::StorageType pixel_type) { using namespace MagickCore; - Magick::Image m (image->size().width, image->size().height, format.c_str(), CharPixel, (void *) image->data()[0]); + Magick::Image m (image->size().width, image->size().height, format.c_str(), pixel_type, (void *) image->data()[0]); m.write (file.string ()); } diff --git a/test/test.h b/test/test.h index 86b13cde5..3a11effa1 100644 --- a/test/test.h +++ b/test/test.h @@ -1,5 +1,5 @@ /* - Copyright (C) 2013-2018 Carl Hetherington + Copyright (C) 2013-2019 Carl Hetherington This file is part of DCP-o-matic. @@ -18,6 +18,7 @@ */ +#include #include class Film; @@ -38,7 +39,7 @@ extern void check_file (boost::filesystem::path, boost::filesystem::path); extern void check_ffmpeg (boost::filesystem::path, boost::filesystem::path, int audio_tolerance); extern void check_image (boost::filesystem::path, boost::filesystem::path, double threshold = 0.01); extern boost::filesystem::path test_film_dir (std::string); -extern void write_image (boost::shared_ptr image, boost::filesystem::path file, std::string format); +extern void write_image (boost::shared_ptr image, boost::filesystem::path file, std::string format, MagickCore::StorageType pixel_type = MagickCore::CharPixel); boost::filesystem::path dcp_file (boost::shared_ptr film, std::string prefix); void check_one_frame (boost::filesystem::path dcp, int64_t index, boost::filesystem::path ref); extern boost::filesystem::path subtitle_file (boost::shared_ptr film);