Fix incorrect colourspace conversion of XYZ content

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

diff --git a/ChangeLog b/ChangeLog
index a02d1016a4a54fae966dbf680354281d0836e9d3..a9e415d4b1f3113a5693255f7f115e5464a2301e 100644 (file)
--- 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 35cc282fd6f6780ad3730bbf282d01e1e5858415..b94093d3860a08fb62c4f9a6c73014c631282ccb 100644 (file)
--- 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 134172038fc04892615cd02af5593de2b4b626e8..48a4e16cd9e8f797cf5d0409359ec569d1d05210 100644 (file)
--- 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 d6b3f878e74a5179483a67d843d9f69a47e9693d..def4878e3e1e5d6f52d16d29ec0f101e3c8c22f2 100644 (file)
--- 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 b6e684815b3e8cfcd346de91b60ad6b08ab28afd..2c96b2d2128c20fb43a33345ea2fc281cf091d47 100644 (file)
--- 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 bb8c216e3d76bf5b2770705a7fa75f1ce930eb9e..1d34e7f84e1a218ebc40b052e78a1b6b1cb1feb6 100644 (file)
--- 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 ccc0878d9f490d360f3b95d44fbe6932d29ba85e..cb168ce63a7797e6cf17aa8f8c42ca3cd81c2eca 100644 (file)
--- 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 6e94492ad5f13b33c919a04576dc42f4881070cf..8fc00b0de31131dfd84a7f166f1c0da680112747 100644 (file)
--- 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 c654abddeee91816294edd541cdabaf4d4d30cfa..9cdc6c56403cac517a06233843a1ae37f0136359 100644 (file)
--- 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 77e19a80e7c911057aa1bd29897a763fc99b93e1..2a471584bdae8c8b5e2d1465f2f6f2b9ede67748 100644 (file)
--- 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 a7c77ce6c6406e9168503384b92535fafd4c305b..21a2b0de42a0f9232fc8f59a9bba387eaaa23649 100644 (file)
--- 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 71c8df30bab4ce5a1ad029944e7d0b3cc93a2205..49459dbcb789c753174fd1fa501c15cf1808cf00 100644 (file)
--- 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 5cbe884ec8bd43f42daf28da1724a6df0eb185a9..ead1cf9aee227717d26413ada1071c4e4ff16182 100644 (file)
--- 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;