Fix incorrect images when cropping without stretch.
authorCarl Hetherington <cth@carlh.net>
Mon, 4 Nov 2019 15:38:14 +0000 (16:38 +0100)
committerCarl Hetherington <cth@carlh.net>
Mon, 4 Nov 2019 15:38:14 +0000 (16:38 +0100)
Always overallocate images so that Image::crop_scale_window is always
safe from over-reading buffers.  Relates to #1654 and probably #1653.

src/lib/image.cc
src/lib/image.h
src/lib/j2k_image_proxy.cc
test/image_test.cc
test/test.cc
test/test.h

index b473403e42ed50fdfe295815b2ed922aa7f5c76a..002c7df9ad11fb9e80947fcf51300096c5df27d2 100644 (file)
@@ -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<Image> out (new Image (out_format, out_size, out_aligned, (out_size.width - inter_size.width) / 2));
+       shared_ptr<Image> out (new Image(out_format, out_size, out_aligned));
        out->make_black ();
 
        /* Size of the image after any crop */
@@ -244,6 +222,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;
 }
 
@@ -340,6 +326,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 ()
 {
@@ -813,13 +829,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 ();
 }
@@ -853,13 +867,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
        }
 }
@@ -869,7 +906,6 @@ Image::Image (Image const & other)
        , _size (other._size)
        , _pixel_format (other._pixel_format)
        , _aligned (other._aligned)
-       , _extra_pixels (other._extra_pixels)
 {
        allocate ();
 
@@ -889,7 +925,6 @@ Image::Image (AVFrame* frame)
        : _size (frame->width, frame->height)
        , _pixel_format (static_cast<AVPixelFormat> (frame->format))
        , _aligned (true)
-       , _extra_pixels (0)
 {
        allocate ();
 
@@ -910,7 +945,6 @@ Image::Image (shared_ptr<const Image> other, bool aligned)
        : _size (other->_size)
        , _pixel_format (other->_pixel_format)
        , _aligned (aligned)
-       , _extra_pixels (other->_extra_pixels)
 {
        allocate ();
 
@@ -952,7 +986,6 @@ Image::swap (Image & other)
        }
 
        std::swap (_aligned, other._aligned);
-       std::swap (_extra_pixels, other._extra_pixels);
 }
 
 /** Destroy a Image */
index 4b059ff36fb8c7534d4559015409d282b5a50056..46633df679d00b9f6eceee34be126711c0fd5d85 100644 (file)
@@ -41,7 +41,7 @@ class Socket;
 class Image : public boost::enable_shared_from_this<Image>
 {
 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<const Image>, 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<PositionImage> images);
index d4c7a8716d7a6dae7876d3b07030b0a0f8a79fde..31fda2510492d9285f8987c73a41acd9551a5435 100644 (file)
@@ -137,14 +137,7 @@ J2KImageProxy::prepare (optional<dcp::Size> target_size) const
        }
 
        shared_ptr<dcp::OpenJPEGImage> decompressed = dcp::decompress_j2k (const_cast<uint8_t*> (_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);
 
index 8378207cfd42b0971de89cb60ab5a78bff13e524..e5c95ea2de75ce9f0e90273536bd19c87582948d 100644 (file)
@@ -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> image (new Image(AV_PIX_FMT_XYZ12LE, dcp::Size(2048, 858), true, 2048));
+       shared_ptr<Image> 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, VIDEO_RANGE_FULL, 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, VIDEO_RANGE_FULL, AV_PIX_FMT_RGB24, false, false);
 }
 
+BOOST_AUTO_TEST_CASE (crop_scale_window_test3)
+{
+       shared_ptr<FFmpegImageProxy> proxy(new FFmpegImageProxy("test/data/player_seek_test_0.png"));
+       shared_ptr<Image> xyz = proxy->image().first->convert_pixel_format(dcp::YUV_TO_RGB_REC709, AV_PIX_FMT_RGB24, true, false);
+       shared_ptr<Image> cropped = xyz->crop_scale_window(Crop(512, 0, 0, 0), dcp::Size(1486, 1080), dcp::Size(1998, 1080), dcp::YUV_TO_RGB_REC709, VIDEO_RANGE_FULL, 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<FFmpegImageProxy> proxy(new FFmpegImageProxy("test/data/player_seek_test_0.png"));
+       shared_ptr<Image> xyz = proxy->image().first->convert_pixel_format(dcp::YUV_TO_RGB_REC709, AV_PIX_FMT_RGB24, true, false);
+       shared_ptr<Image> cropped = xyz->crop_scale_window(Crop(512, 0, 0, 0), dcp::Size(1486, 1080), dcp::Size(1998, 1080), dcp::YUV_TO_RGB_REC709, VIDEO_RANGE_FULL, 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<FFmpegImageProxy> proxy(new FFmpegImageProxy("test/data/player_seek_test_0.png"));
+       shared_ptr<Image> xyz = proxy->image().first->convert_pixel_format(dcp::YUV_TO_RGB_REC709, AV_PIX_FMT_XYZ12LE, true, false);
+       shared_ptr<Image> cropped = xyz->crop_scale_window(Crop(512, 0, 0, 0), dcp::Size(1486, 1080), dcp::Size(1998, 1080), dcp::YUV_TO_RGB_REC709, VIDEO_RANGE_FULL, 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<FFmpegImageProxy> proxy(new FFmpegImageProxy("test/data/player_seek_test_0.png"));
+       shared_ptr<Image> xyz = proxy->image().first->convert_pixel_format(dcp::YUV_TO_RGB_REC709, AV_PIX_FMT_XYZ12LE, true, false);
+       shared_ptr<Image> cropped = xyz->crop_scale_window(Crop(512, 0, 0, 0), dcp::Size(1486, 1080), dcp::Size(1998, 1080), dcp::YUV_TO_RGB_REC709, VIDEO_RANGE_FULL, 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<FFmpegImageProxy> proxy(new FFmpegImageProxy("test/data/3d_test/000001.png"));
index c5bc417f3865df4d084b29dd01bb9ced1251149b..836e74f52753a66f525b92e291cb4eb7cd8c11a4 100644 (file)
@@ -1,5 +1,5 @@
 /*
-    Copyright (C) 2012-2018 Carl Hetherington <cth@carlh.net>
+    Copyright (C) 2012-2019 Carl Hetherington <cth@carlh.net>
 
     This file is part of DCP-o-matic.
 
@@ -417,11 +417,11 @@ wait_for_jobs ()
 }
 
 void
-write_image (shared_ptr<const Image> image, boost::filesystem::path file, string format)
+write_image (shared_ptr<const Image> 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 ());
 }
 
index 86b13cde53c97561ef0173d9a0a260d707ed897d..3a11effa101b1628efc1022012d9ff944b1bb265 100644 (file)
@@ -1,5 +1,5 @@
 /*
-    Copyright (C) 2013-2018 Carl Hetherington <cth@carlh.net>
+    Copyright (C) 2013-2019 Carl Hetherington <cth@carlh.net>
 
     This file is part of DCP-o-matic.
 
@@ -18,6 +18,7 @@
 
 */
 
+#include <Magick++.h>
 #include <boost/filesystem.hpp>
 
 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<const Image> image, boost::filesystem::path file, std::string format);
+extern void write_image (boost::shared_ptr<const Image> image, boost::filesystem::path file, std::string format, MagickCore::StorageType pixel_type = MagickCore::CharPixel);
 boost::filesystem::path dcp_file (boost::shared_ptr<const Film> 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> film);