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 d3b193e7bc99a7d17c5c103489118809a3875d33..1439d9f7f964228385ba20e28c0bbb8ba85082c5 100644 (file)
--- 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);
 
        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 */
        out->make_black ();
 
        /* Size of the image after any crop */


@@ -245,6 +223,14 @@ Image::crop_scale_window (
 
        sws_freeContext (scale_context);
 
 
        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;
 }
 
        return out;
 }
 


@@ -342,6 +328,36 @@ Image::swap_16 (uint16_t v)
        return ((v >> 8) & 0xff) | ((v & 0xff) << 8);
 }
 
        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 ()
 {
 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 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)
        : _size (s)
        , _pixel_format (p)
        , _aligned (aligned)

-       , _extra_pixels (extra_pixels)

 {
        allocate ();
 }
 {
        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.
                   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.
                */
 #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
        }
 }
 #endif
        }
 }


@@ -871,7 +908,6 @@ Image::Image (Image const & other)
        , _size (other._size)
        , _pixel_format (other._pixel_format)
        , _aligned (other._aligned)
        , _size (other._size)
        , _pixel_format (other._pixel_format)
        , _aligned (other._aligned)

-       , _extra_pixels (other._extra_pixels)

 {
        allocate ();
 
 {
        allocate ();
 


@@ -891,7 +927,6 @@ Image::Image (AVFrame* frame)
        : _size (frame->width, frame->height)
        , _pixel_format (static_cast<AVPixelFormat> (frame->format))
        , _aligned (true)
        : _size (frame->width, frame->height)
        , _pixel_format (static_cast<AVPixelFormat> (frame->format))
        , _aligned (true)

-       , _extra_pixels (0)

 {
        allocate ();
 
 {
        allocate ();
 


@@ -912,7 +947,6 @@ Image::Image (shared_ptr<const Image> other, bool aligned)
        : _size (other->_size)
        , _pixel_format (other->_pixel_format)
        , _aligned (aligned)
        : _size (other->_size)
        , _pixel_format (other->_pixel_format)
        , _aligned (aligned)

-       , _extra_pixels (other->_extra_pixels)

 {
        allocate ();
 
 {
        allocate ();
 


@@ -954,7 +988,6 @@ Image::swap (Image & other)
        }
 
        std::swap (_aligned, other._aligned);
        }
 
        std::swap (_aligned, other._aligned);

-       std::swap (_extra_pixels, other._extra_pixels);

 }
 
 /** Destroy a Image */
 }
 
 /** Destroy a Image */

diff --git a/src/lib/image.h b/src/lib/image.h
index dbcb38cc7a5c41123e7ff3aec60fcb04b0bea8bf..51d768a13971a9868aaab2e655edd41f58f066f6 100644 (file)
--- a/src/lib/image.h
+++ b/src/lib/image.h
@@ -41,7 +41,7 @@ class Socket;
 class Image : public boost::enable_shared_from_this<Image>
 {
 public:
 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);
        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 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);
 
        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* _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);
 };
 
 extern PositionImage merge (std::list<PositionImage> images);

diff --git a/src/lib/j2k_image_proxy.cc b/src/lib/j2k_image_proxy.cc
index d4c7a8716d7a6dae7876d3b07030b0a0f8a79fde..31fda2510492d9285f8987c73a41acd9551a5435 100644 (file)
--- a/src/lib/j2k_image_proxy.cc
+++ b/src/lib/j2k_image_proxy.cc
@@ -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);
        }
 
        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);
 
 
        int const shift = 16 - decompressed->precision (0);
 

diff --git a/test/image_test.cc b/test/image_test.cc
index 8b809c591f490a895d94430f0f57e5165c892a08..8579785a74aeebe264c63978280d2136d1b66ba9 100644 (file)
--- 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)
 {
 /** 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, 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);
 }
 
        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<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, 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, 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, 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, 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"));
 BOOST_AUTO_TEST_CASE (as_png_test)
 {
        shared_ptr<FFmpegImageProxy> proxy(new FFmpegImageProxy("test/data/3d_test/000001.png"));

diff --git a/test/test.cc b/test/test.cc
index 50770a6875ce2e2f05f9685f2319eb2fdc651d40..a9d3ed1c797e132dc317ea47dddae8038123045b 100644 (file)
--- a/test/test.cc
+++ b/test/test.cc
@@ -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.
 
 
     This file is part of DCP-o-matic.
 


@@ -417,11 +417,11 @@ wait_for_jobs ()
 }
 
 void
 }
 
 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;
 
 {
        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 ());
 }
 
        m.write (file.string ());
 }
 

diff --git a/test/test.h b/test/test.h
index 86b13cde53c97561ef0173d9a0a260d707ed897d..3a11effa101b1628efc1022012d9ff944b1bb265 100644 (file)
--- a/test/test.h
+++ b/test/test.h
@@ -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.
 
 
     This file is part of DCP-o-matic.
 


@@ -18,6 +18,7 @@
 
 */
 
 
 */
 

+#include <Magick++.h>

 #include <boost/filesystem.hpp>
 
 class Film;
 #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 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);
 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);