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 b473403..002c7df 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 4b059ff..46633df 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 d4c7a87..31fda25 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 8378207..e5c95ea 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 c5bc417..836e74f 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 86b13cd..3a11eff 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);