Account for J2K decoding at lower-than-maximum resolution when cropping

the resulting images; fixes #1274.

10 files changed, 54 insertions, 22 deletions

diff --git a/ChangeLog b/ChangeLog
index 02029d714..27614d708 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2018-04-13  Carl Hetherington  <cth@carlh.net>
+
+	* Fix incorrect preview crop with DCP sources when the preview is smaller than half
+	of the DCP's resolution (#1274).
+
 2018-04-12  Carl Hetherington  <cth@carlh.net>
 
 	* Prevent error when starting export without specifying a filename (#1260).
diff --git a/src/lib/image_proxy.h b/src/lib/image_proxy.h
index 01cb79552..846aec136 100644
--- a/src/lib/image_proxy.h
+++ b/src/lib/image_proxy.h
@@ -63,9 +63,11 @@ public:
 	/** @param note Handler for any notes that occur.
 	 *  @param size Size that the returned image will be scaled to, in case this
 	 *  can be used as an optimisation.
-	 *  @return Image (which must be aligned)
+	 *  @return Image (which must be aligned) and log2 of any scaling down that has
+	 *  already been applied to the image; e.g. if the the image is already half the size
+	 *  of the original, the second part of the return value will be 1.
 	 */
-	virtual boost::shared_ptr<Image> image (
+	virtual std::pair<boost::shared_ptr<Image>, int> image (
 		boost::optional<dcp::NoteHandler> note = boost::optional<dcp::NoteHandler> (),
 		boost::optional<dcp::Size> size = boost::optional<dcp::Size> ()
 		) const = 0;
@@ -76,8 +78,9 @@ public:
 	virtual bool same (boost::shared_ptr<const ImageProxy>) const = 0;
 	/** Do any useful work that would speed up a subsequent call to ::image().
 	 *  This method may be called in a different thread to image().
+	 *  @return log2 of any scaling down that will be applied to the image.
 	 */
-	virtual void prepare (boost::optional<dcp::Size> = boost::optional<dcp::Size>()) const {}
+	int prepare (boost::optional<dcp::Size> = boost::optional<dcp::Size>()) const { return 0; }
 	virtual AVPixelFormat pixel_format () const = 0;
 	virtual size_t memory_used () const = 0;
 };
diff --git a/src/lib/j2k_image_proxy.cc b/src/lib/j2k_image_proxy.cc
index 2489a9499..9100414e4 100644
--- a/src/lib/j2k_image_proxy.cc
+++ b/src/lib/j2k_image_proxy.cc
@@ -21,6 +21,7 @@
 #include "j2k_image_proxy.h"
 #include "dcpomatic_socket.h"
 #include "image.h"
+#include "dcpomatic_assert.h"
 #include <dcp/raw_convert.h>
 #include <dcp/openjpeg_image.h>
 #include <dcp/mono_picture_frame.h>
@@ -38,6 +39,8 @@
 using std::string;
 using std::cout;
 using std::max;
+using std::pair;
+using std::make_pair;
 using boost::shared_ptr;
 using boost::optional;
 using boost::dynamic_pointer_cast;
@@ -106,13 +109,14 @@ J2KImageProxy::J2KImageProxy (shared_ptr<cxml::Node> xml, shared_ptr<Socket> soc
 	socket->read (_data.data().get (), _data.size ());
 }
 
-void
+int
 J2KImageProxy::prepare (optional<dcp::Size> target_size) const
 {
 	boost::mutex::scoped_lock lm (_mutex);
 
 	if (_decompressed && target_size == _target_size) {
-		return;
+		DCPOMATIC_ASSERT (_reduce);
+		return *_reduce;
 	}
 
 	int reduce = 0;
@@ -143,12 +147,15 @@ J2KImageProxy::prepare (optional<dcp::Size> target_size) const
 	}
 
 	_target_size = target_size;
+	_reduce = reduce;
+
+	return reduce;
 }
 
-shared_ptr<Image>
+pair<shared_ptr<Image>, int>
 J2KImageProxy::image (optional<dcp::NoteHandler>, optional<dcp::Size> target_size) const
 {
-	prepare (target_size);
+	int const reduce = prepare (target_size);
 
 	shared_ptr<Image> image (new Image (_pixel_format, _decompressed->size(), true));
 
@@ -169,7 +176,7 @@ J2KImageProxy::image (optional<dcp::NoteHandler>, optional<dcp::Size> target_siz
 		}
 	}
 
-	return image;
+	return make_pair (image, reduce);
 }
 
 void
diff --git a/src/lib/j2k_image_proxy.h b/src/lib/j2k_image_proxy.h
index 3680de111..77d53a9e5 100644
--- a/src/lib/j2k_image_proxy.h
+++ b/src/lib/j2k_image_proxy.h
@@ -50,7 +50,7 @@ public:
 
 	J2KImageProxy (boost::shared_ptr<cxml::Node> xml, boost::shared_ptr<Socket> socket);
 
-	boost::shared_ptr<Image> image (
+	std::pair<boost::shared_ptr<Image>, int> image (
 		boost::optional<dcp::NoteHandler> note = boost::optional<dcp::NoteHandler> (),
 		boost::optional<dcp::Size> size = boost::optional<dcp::Size> ()
 		) const;
@@ -59,7 +59,7 @@ public:
 	void send_binary (boost::shared_ptr<Socket>) const;
 	/** @return true if our image is definitely the same as another, false if it is probably not */
 	bool same (boost::shared_ptr<const ImageProxy>) const;
-	void prepare (boost::optional<dcp::Size> = boost::optional<dcp::Size>()) const;
+	int prepare (boost::optional<dcp::Size> = boost::optional<dcp::Size>()) const;
 	AVPixelFormat pixel_format () const {
 		return _pixel_format;
 	}
@@ -85,6 +85,7 @@ private:
 	boost::optional<dcp::Eye> _eye;
 	mutable boost::shared_ptr<dcp::OpenJPEGImage> _decompressed;
 	mutable boost::optional<dcp::Size> _target_size;
+	mutable boost::optional<int> _reduce;
 	AVPixelFormat _pixel_format;
 	mutable boost::mutex _mutex;
 	boost::optional<int> _forced_reduction;
diff --git a/src/lib/magick_image_proxy.cc b/src/lib/magick_image_proxy.cc
index 0fd4a5edb..b12a81db5 100644
--- a/src/lib/magick_image_proxy.cc
+++ b/src/lib/magick_image_proxy.cc
@@ -32,6 +32,8 @@
 
 using std::string;
 using std::cout;
+using std::pair;
+using std::make_pair;
 using boost::shared_ptr;
 using boost::optional;
 using boost::dynamic_pointer_cast;
@@ -66,13 +68,13 @@ MagickImageProxy::MagickImageProxy (shared_ptr<cxml::Node>, shared_ptr<Socket> s
 	delete[] data;
 }
 
-shared_ptr<Image>
+pair<shared_ptr<Image>, int>
 MagickImageProxy::image (optional<dcp::NoteHandler>, optional<dcp::Size>) const
 {
 	boost::mutex::scoped_lock lm (_mutex);
 
 	if (_image) {
-		return _image;
+		return make_pair (_image, 0);
 	}
 
 	Magick::Image* magick_image = 0;
@@ -137,7 +139,7 @@ MagickImageProxy::image (optional<dcp::NoteHandler>, optional<dcp::Size>) const
 
 	delete magick_image;
 
-	return _image;
+	return make_pair (_image, 0);
 }
 
 void
diff --git a/src/lib/magick_image_proxy.h b/src/lib/magick_image_proxy.h
index 1db45d73b..df90c7723 100644
--- a/src/lib/magick_image_proxy.h
+++ b/src/lib/magick_image_proxy.h
@@ -29,7 +29,7 @@ public:
 	MagickImageProxy (boost::filesystem::path);
 	MagickImageProxy (boost::shared_ptr<cxml::Node> xml, boost::shared_ptr<Socket> socket);
 
-	boost::shared_ptr<Image> image (
+	std::pair<boost::shared_ptr<Image>, int> image (
 		boost::optional<dcp::NoteHandler> note = boost::optional<dcp::NoteHandler> (),
 		boost::optional<dcp::Size> size = boost::optional<dcp::Size> ()
 		) const;
diff --git a/src/lib/player_video.cc b/src/lib/player_video.cc
index 8eb39efed..a50b196a2 100644
--- a/src/lib/player_video.cc
+++ b/src/lib/player_video.cc
@@ -34,6 +34,7 @@ extern "C" {
 
 using std::string;
 using std::cout;
+using std::pair;
 using boost::shared_ptr;
 using boost::weak_ptr;
 using boost::dynamic_pointer_cast;
@@ -112,7 +113,9 @@ PlayerVideo::set_subtitle (PositionImage image)
 shared_ptr<Image>
 PlayerVideo::image (dcp::NoteHandler note, function<AVPixelFormat (AVPixelFormat)> pixel_format, bool aligned, bool fast) const
 {
-	shared_ptr<Image> im = _in->image (optional<dcp::NoteHandler> (note), _inter_size);
+	pair<shared_ptr<Image>, int> prox = _in->image (optional<dcp::NoteHandler> (note), _inter_size);
+	shared_ptr<Image> im = prox.first;
+	int const reduce = prox.second;
 
 	Crop total_crop = _crop;
 	switch (_part) {
@@ -132,6 +135,15 @@ PlayerVideo::image (dcp::NoteHandler note, function<AVPixelFormat (AVPixelFormat
 		break;
 	}
 
+	if (reduce > 0) {
+		/* Scale the crop down to account for the scaling that has already happened in ImageProxy::image */
+		int const r = pow(2, reduce);
+		total_crop.left /= r;
+		total_crop.right /= r;
+		total_crop.top /= r;
+		total_crop.bottom /= r;
+	}
+
 	dcp::YUVToRGB yuv_to_rgb = dcp::YUV_TO_RGB_REC601;
 	if (_colour_conversion) {
 		yuv_to_rgb = _colour_conversion.get().yuv_to_rgb();
diff --git a/src/lib/raw_image_proxy.cc b/src/lib/raw_image_proxy.cc
index 094b50d05..c3b565f3c 100644
--- a/src/lib/raw_image_proxy.cc
+++ b/src/lib/raw_image_proxy.cc
@@ -1,5 +1,5 @@
 /*
-    Copyright (C) 2014 Carl Hetherington <cth@carlh.net>
+    Copyright (C) 2014-2018 Carl Hetherington <cth@carlh.net>
 
     This file is part of DCP-o-matic.
 
@@ -31,6 +31,8 @@ extern "C" {
 #include "i18n.h"
 
 using std::string;
+using std::pair;
+using std::make_pair;
 using boost::shared_ptr;
 using boost::dynamic_pointer_cast;
 using boost::optional;
@@ -52,10 +54,10 @@ RawImageProxy::RawImageProxy (shared_ptr<cxml::Node> xml, shared_ptr<Socket> soc
 	_image->read_from_socket (socket);
 }
 
-shared_ptr<Image>
+pair<shared_ptr<Image>, int>
 RawImageProxy::image (optional<dcp::NoteHandler>, optional<dcp::Size>) const
 {
-	return _image;
+	return make_pair (_image, 0);
 }
 
 void
@@ -81,7 +83,7 @@ RawImageProxy::same (shared_ptr<const ImageProxy> other) const
 		return false;
 	}
 
-	return (*_image.get()) == (*rp->image().get());
+	return (*_image.get()) == (*rp->image().first.get());
 }
 
 AVPixelFormat
diff --git a/src/lib/raw_image_proxy.h b/src/lib/raw_image_proxy.h
index 2b64cbd9b..99af10f99 100644
--- a/src/lib/raw_image_proxy.h
+++ b/src/lib/raw_image_proxy.h
@@ -29,7 +29,7 @@ public:
 	RawImageProxy (boost::shared_ptr<Image>);
 	RawImageProxy (boost::shared_ptr<cxml::Node> xml, boost::shared_ptr<Socket> socket);
 
-	boost::shared_ptr<Image> image (
+	std::pair<boost::shared_ptr<Image>, int> image (
 		boost::optional<dcp::NoteHandler> note = boost::optional<dcp::NoteHandler> (),
 		boost::optional<dcp::Size> size = boost::optional<dcp::Size> ()
 		) const;
diff --git a/test/image_test.cc b/test/image_test.cc
index 7780c94cc..5adc35603 100644
--- a/test/image_test.cc
+++ b/test/image_test.cc
@@ -139,7 +139,7 @@ void
 alpha_blend_test_one (AVPixelFormat format, string suffix)
 {
 	shared_ptr<MagickImageProxy> proxy (new MagickImageProxy (private_data / "prophet_frame.tiff"));
-	shared_ptr<Image> raw = proxy->image();
+	shared_ptr<Image> raw = proxy->image().first;
 	shared_ptr<Image> background = raw->convert_pixel_format (dcp::YUV_TO_RGB_REC709, format, true, false);
 
 	shared_ptr<Image> overlay (new Image (AV_PIX_FMT_BGRA, dcp::Size(431, 891), true));
@@ -261,7 +261,7 @@ BOOST_AUTO_TEST_CASE (merge_test2)
 BOOST_AUTO_TEST_CASE (crop_scale_window_test)
 {
 	shared_ptr<MagickImageProxy> proxy(new MagickImageProxy("test/data/flat_red.png"));
-	shared_ptr<Image> raw = proxy->image();
+	shared_ptr<Image> raw = proxy->image().first;
 	shared_ptr<Image> out = raw->crop_scale_window(Crop(), dcp::Size(1998, 836), dcp::Size(1998, 1080), dcp::YUV_TO_RGB_REC709, AV_PIX_FMT_YUV420P, true, false);
 	shared_ptr<Image> save = out->scale(dcp::Size(1998, 1080), dcp::YUV_TO_RGB_REC709, AV_PIX_FMT_RGB24, false, false);
 	write_image(save, "build/test/crop_scale_window_test.png", "RGB");