Fix incorrect colourspace conversion of XYZ content

when it is not being passed through as untouched
JPEG2000 (#730).

13 files changed, 85 insertions, 26 deletions

diff --git a/ChangeLog b/ChangeLog
index a02d1016a..a9e415d4b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,8 @@
 2015-10-29  Carl Hetherington  <cth@carlh.net>
 
+	* Fix incorrect colours following re-scale of existing
+	DCP content (#730).
+
 	* Updated nl_NL translation from Rob van Nieuwkerk.
 
 	* Updated pt_PT translation from Tiago Casal Ribeiro.
diff --git a/src/lib/dcp_video.cc b/src/lib/dcp_video.cc
index 35cc282fd..b94093d38 100644
--- a/src/lib/dcp_video.cc
+++ b/src/lib/dcp_video.cc
@@ -98,7 +98,7 @@ DCPVideo::convert_to_xyz (shared_ptr<const PlayerVideo> frame, dcp::NoteHandler
 {
 	shared_ptr<dcp::OpenJPEGImage> xyz;
 
-	shared_ptr<Image> image = frame->image (AV_PIX_FMT_RGB48LE, note);
+	shared_ptr<Image> image = frame->image (note);
 	if (frame->colour_conversion()) {
 		xyz = dcp::rgb_to_xyz (
 			image->data()[0],
diff --git a/src/lib/image.cc b/src/lib/image.cc
index 134172038..48a4e16cd 100644
--- a/src/lib/image.cc
+++ b/src/lib/image.cc
@@ -360,6 +360,7 @@ Image::make_black ()
 	case AV_PIX_FMT_RGB555LE:
 	case AV_PIX_FMT_RGB48LE:
 	case AV_PIX_FMT_RGB48BE:
+	case AV_PIX_FMT_XYZ12LE:
 		memset (data()[0], 0, sample_size(0).height * stride()[0]);
 		break;
 
@@ -830,6 +831,7 @@ Image::fade (float f)
 	case AV_PIX_FMT_YUVA422P10LE:
 	case AV_PIX_FMT_YUVA444P10LE:
 	case AV_PIX_FMT_RGB48LE:
+	case AV_PIX_FMT_XYZ12LE:
 		/* 16-bit little-endian */
 		for (int c = 0; c < 3; ++c) {
 			int const stride_pixels = stride()[c] / 2;
diff --git a/src/lib/image_proxy.h b/src/lib/image_proxy.h
index d6b3f878e..def4878e3 100644
--- a/src/lib/image_proxy.h
+++ b/src/lib/image_proxy.h
@@ -24,6 +24,9 @@
  *  @brief ImageProxy and subclasses.
  */
 
+extern "C" {
+#include <libavutil/pixfmt.h>
+}
 #include <dcp/types.h>
 #include <boost/shared_ptr.hpp>
 #include <boost/optional.hpp>
@@ -62,6 +65,7 @@ public:
 	virtual void send_binary (boost::shared_ptr<Socket>) const = 0;
 	/** @return true if our image is definitely the same as another, false if it is probably not */
 	virtual bool same (boost::shared_ptr<const ImageProxy>) const = 0;
+	virtual AVPixelFormat pixel_format () const = 0;
 };
 
 boost::shared_ptr<ImageProxy> image_proxy_factory (boost::shared_ptr<cxml::Node> xml, boost::shared_ptr<Socket> socket);
diff --git a/src/lib/j2k_image_proxy.cc b/src/lib/j2k_image_proxy.cc
index b6e684815..2c96b2d21 100644
--- a/src/lib/j2k_image_proxy.cc
+++ b/src/lib/j2k_image_proxy.cc
@@ -80,39 +80,46 @@ J2KImageProxy::J2KImageProxy (shared_ptr<cxml::Node> xml, shared_ptr<Socket> soc
 	socket->read (_data.data().get (), _data.size ());
 }
 
+void
+J2KImageProxy::ensure_j2k () const
+{
+	if (!_j2k) {
+		_j2k = dcp::decompress_j2k (const_cast<uint8_t*> (_data.data().get()), _data.size (), 0);
+	}
+}
+
 shared_ptr<Image>
 J2KImageProxy::image (optional<dcp::NoteHandler> note) const
 {
-	shared_ptr<Image> image (new Image (AV_PIX_FMT_RGB48LE, _size, true));
-
-	shared_ptr<dcp::OpenJPEGImage> oj = dcp::decompress_j2k (const_cast<uint8_t*> (_data.data().get()), _data.size (), 0);
+	ensure_j2k ();
 
-	if (oj->opj_image()->comps[0].prec < 12) {
-		int const shift = 12 - oj->opj_image()->comps[0].prec;
+	if (_j2k->opj_image()->comps[0].prec < 12) {
+		int const shift = 12 - _j2k->opj_image()->comps[0].prec;
 		for (int c = 0; c < 3; ++c) {
-			int* p = oj->data (c);
-			for (int y = 0; y < oj->size().height; ++y) {
-				for (int x = 0; x < oj->size().width; ++x) {
+			int* p = _j2k->data (c);
+			for (int y = 0; y < _j2k->size().height; ++y) {
+				for (int x = 0; x < _j2k->size().width; ++x) {
 					*p++ <<= shift;
 				}
 			}
 		}
 	}
 
-	if (oj->opj_image()->color_space == CLRSPC_SRGB) {
-		/* No XYZ -> RGB conversion necessary; just copy and interleave the values */
-		int p = 0;
-		for (int y = 0; y < oj->size().height; ++y) {
-			uint16_t* q = (uint16_t *) (image->data()[0] + y * image->stride()[0]);
-			for (int x = 0; x < oj->size().width; ++x) {
-				for (int c = 0; c < 3; ++c) {
-					*q++ = oj->data(c)[p] << 4;
-				}
-				++p;
+	shared_ptr<Image> image (new Image (pixel_format(), _size, true));
+
+	/* Copy data in whatever format (sRGB or XYZ) into our Image; I'm assuming
+	   the data is 12-bit either way.
+	*/
+
+	int p = 0;
+	for (int y = 0; y < _j2k->size().height; ++y) {
+		uint16_t* q = (uint16_t *) (image->data()[0] + y * image->stride()[0]);
+		for (int x = 0; x < _j2k->size().width; ++x) {
+			for (int c = 0; c < 3; ++c) {
+				*q++ = _j2k->data(c)[p] << 4;
 			}
+			++p;
 		}
-	} else {
-		dcp::xyz_to_rgb (oj, dcp::ColourConversion::srgb_to_xyz(), image->data()[0], image->stride()[0], note);
 	}
 
 	return image;
@@ -151,6 +158,18 @@ J2KImageProxy::same (shared_ptr<const ImageProxy> other) const
 	return memcmp (_data.data().get(), jp->_data.data().get(), _data.size()) == 0;
 }
 
+AVPixelFormat
+J2KImageProxy::pixel_format () const
+{
+	ensure_j2k ();
+
+	if (_j2k->opj_image()->color_space == CLRSPC_SRGB) {
+		return AV_PIX_FMT_RGB48LE;
+	}
+
+	return AV_PIX_FMT_XYZ12LE;
+}
+
 J2KImageProxy::J2KImageProxy (Data data, dcp::Size size)
 	: _data (data)
 	, _size (size)
diff --git a/src/lib/j2k_image_proxy.h b/src/lib/j2k_image_proxy.h
index bb8c216e3..1d34e7f84 100644
--- a/src/lib/j2k_image_proxy.h
+++ b/src/lib/j2k_image_proxy.h
@@ -40,7 +40,8 @@ public:
 	void add_metadata (xmlpp::Node *) const;
 	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 */
-	virtual bool same (boost::shared_ptr<const ImageProxy>) const;
+	bool same (boost::shared_ptr<const ImageProxy>) const;
+	AVPixelFormat pixel_format () const;
 
 	Data j2k () const {
 		return _data;
@@ -53,9 +54,12 @@ public:
 private:
 	friend struct client_server_test_j2k;
 
+	/* For tests */
 	J2KImageProxy (Data data, dcp::Size size);
+	void ensure_j2k () const;
 
 	Data _data;
 	dcp::Size _size;
 	boost::optional<dcp::Eye> _eye;
+	mutable boost::shared_ptr<dcp::OpenJPEGImage> _j2k;
 };
diff --git a/src/lib/magick_image_proxy.cc b/src/lib/magick_image_proxy.cc
index ccc0878d9..cb168ce63 100644
--- a/src/lib/magick_image_proxy.cc
+++ b/src/lib/magick_image_proxy.cc
@@ -149,3 +149,9 @@ MagickImageProxy::same (shared_ptr<const ImageProxy> other) const
 
 	return memcmp (_blob.data(), mp->_blob.data(), _blob.length()) == 0;
 }
+
+AVPixelFormat
+MagickImageProxy::pixel_format () const
+{
+	return AV_PIX_FMT_RGB24;
+}
diff --git a/src/lib/magick_image_proxy.h b/src/lib/magick_image_proxy.h
index 6e94492ad..8fc00b0de 100644
--- a/src/lib/magick_image_proxy.h
+++ b/src/lib/magick_image_proxy.h
@@ -32,6 +32,7 @@ public:
 	void add_metadata (xmlpp::Node *) const;
 	void send_binary (boost::shared_ptr<Socket>) const;
 	bool same (boost::shared_ptr<const ImageProxy> other) const;
+	AVPixelFormat pixel_format () const;
 
 private:
 	Magick::Blob _blob;
diff --git a/src/lib/player_video.cc b/src/lib/player_video.cc
index c654abdde..9cdc6c564 100644
--- a/src/lib/player_video.cc
+++ b/src/lib/player_video.cc
@@ -23,6 +23,9 @@
 #include "j2k_image_proxy.h"
 #include "film.h"
 #include "raw_convert.h"
+extern "C" {
+#include <libavutil/pixfmt.h>
+}
 #include <libxml++/libxml++.h>
 #include <iostream>
 
@@ -91,7 +94,7 @@ PlayerVideo::set_subtitle (PositionImage image)
 }
 
 shared_ptr<Image>
-PlayerVideo::image (AVPixelFormat pixel_format, dcp::NoteHandler note) const
+PlayerVideo::image (dcp::NoteHandler note) const
 {
 	shared_ptr<Image> im = _in->image (optional<dcp::NoteHandler> (note));
 
@@ -118,7 +121,10 @@ PlayerVideo::image (AVPixelFormat pixel_format, dcp::NoteHandler note) const
 		yuv_to_rgb = _colour_conversion.get().yuv_to_rgb();
 	}
 
-	shared_ptr<Image> out = im->crop_scale_window (total_crop, _inter_size, _out_size, yuv_to_rgb, pixel_format, true);
+	/* If the input is XYZ, keep it otherwise convert to RGB */
+	AVPixelFormat const p = _in->pixel_format() == AV_PIX_FMT_XYZ12LE ? AV_PIX_FMT_XYZ12LE : AV_PIX_FMT_RGB48LE;
+
+	shared_ptr<Image> out = im->crop_scale_window (total_crop, _inter_size, _out_size, yuv_to_rgb, p, true);
 
 	if (_subtitle) {
 		out->alpha_blend (_subtitle->image, _subtitle->position);
diff --git a/src/lib/player_video.h b/src/lib/player_video.h
index 77e19a80e..2a471584b 100644
--- a/src/lib/player_video.h
+++ b/src/lib/player_video.h
@@ -55,7 +55,7 @@ public:
 
 	void set_subtitle (PositionImage);
 
-	boost::shared_ptr<Image> image (AVPixelFormat pix_fmt, dcp::NoteHandler note) const;
+	boost::shared_ptr<Image> image (dcp::NoteHandler note) const;
 
 	void add_metadata (xmlpp::Node* node) const;
 	void send_binary (boost::shared_ptr<Socket> socket) const;
diff --git a/src/lib/raw_image_proxy.cc b/src/lib/raw_image_proxy.cc
index a7c77ce6c..21a2b0de4 100644
--- a/src/lib/raw_image_proxy.cc
+++ b/src/lib/raw_image_proxy.cc
@@ -81,3 +81,9 @@ RawImageProxy::same (shared_ptr<const ImageProxy> other) const
 
 	return (*_image.get()) == (*rp->image().get());
 }
+
+AVPixelFormat
+RawImageProxy::pixel_format () const
+{
+	return _image->pixel_format ();
+}
diff --git a/src/lib/raw_image_proxy.h b/src/lib/raw_image_proxy.h
index 71c8df30b..49459dbcb 100644
--- a/src/lib/raw_image_proxy.h
+++ b/src/lib/raw_image_proxy.h
@@ -32,6 +32,7 @@ public:
 	void add_metadata (xmlpp::Node *) const;
 	void send_binary (boost::shared_ptr<Socket>) const;
 	bool same (boost::shared_ptr<const ImageProxy>) const;
+	AVPixelFormat pixel_format () const;
 
 private:
 	boost::shared_ptr<Image> _image;
diff --git a/src/wx/film_viewer.cc b/src/wx/film_viewer.cc
index 5cbe884ec..ead1cf9ae 100644
--- a/src/wx/film_viewer.cc
+++ b/src/wx/film_viewer.cc
@@ -37,6 +37,9 @@
 #include "lib/log.h"
 #include "film_viewer.h"
 #include "wx_util.h"
+extern "C" {
+#include <libavutil/pixfmt.h>
+}
 #include <dcp/exceptions.h>
 #include <wx/tglbtn.h>
 #include <iostream>
@@ -186,7 +189,11 @@ FilmViewer::get (DCPTime p, bool accurate)
 
 	if (!pvf.empty ()) {
 		try {
-			_frame = pvf.front()->image (AV_PIX_FMT_RGB24, boost::bind (&Log::dcp_log, _film->log().get(), _1, _2));
+			/* XXX: this could now give us a 48-bit image, which is a bit wasteful,
+			   or a XYZ image, which the code below will currently rely on FFmpeg
+			   to colourspace-convert.
+			*/
+			_frame = pvf.front()->image (boost::bind (&Log::dcp_log, _film->log().get(), _1, _2));
 			ImageChanged (pvf.front ());
 
 			dcp::YUVToRGB yuv_to_rgb = dcp::YUV_TO_RGB_REC601;