From aa7edc3114fa3b7868088a9f8f22b5ee195a8668 Mon Sep 17 00:00:00 2001 From: Carl Hetherington Date: Thu, 29 Oct 2015 16:57:33 +0000 Subject: [PATCH] Fix incorrect colourspace conversion of XYZ content when it is not being passed through as untouched JPEG2000 (#730). --- ChangeLog | 3 ++ src/lib/dcp_video.cc | 2 +- src/lib/image.cc | 2 ++ src/lib/image_proxy.h | 4 +++ src/lib/j2k_image_proxy.cc | 59 +++++++++++++++++++++++------------ src/lib/j2k_image_proxy.h | 6 +++- src/lib/magick_image_proxy.cc | 6 ++++ src/lib/magick_image_proxy.h | 1 + src/lib/player_video.cc | 10 ++++-- src/lib/player_video.h | 2 +- src/lib/raw_image_proxy.cc | 6 ++++ src/lib/raw_image_proxy.h | 1 + src/wx/film_viewer.cc | 9 +++++- 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 + * 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 frame, dcp::NoteHandler { shared_ptr xyz; - shared_ptr image = frame->image (AV_PIX_FMT_RGB48LE, note); + shared_ptr 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 +} #include #include #include @@ -62,6 +65,7 @@ public: virtual void send_binary (boost::shared_ptr) 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 = 0; + virtual AVPixelFormat pixel_format () const = 0; }; boost::shared_ptr image_proxy_factory (boost::shared_ptr xml, boost::shared_ptr 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 xml, shared_ptr soc socket->read (_data.data().get (), _data.size ()); } +void +J2KImageProxy::ensure_j2k () const +{ + if (!_j2k) { + _j2k = dcp::decompress_j2k (const_cast (_data.data().get()), _data.size (), 0); + } +} + shared_ptr J2KImageProxy::image (optional note) const { - shared_ptr image (new Image (AV_PIX_FMT_RGB48LE, _size, true)); - - shared_ptr oj = dcp::decompress_j2k (const_cast (_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 (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 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) 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; + bool same (boost::shared_ptr) 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 _eye; + mutable boost::shared_ptr _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 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) const; bool same (boost::shared_ptr 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 +} #include #include @@ -91,7 +94,7 @@ PlayerVideo::set_subtitle (PositionImage image) } shared_ptr -PlayerVideo::image (AVPixelFormat pixel_format, dcp::NoteHandler note) const +PlayerVideo::image (dcp::NoteHandler note) const { shared_ptr im = _in->image (optional (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 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 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 (AVPixelFormat pix_fmt, dcp::NoteHandler note) const; + boost::shared_ptr image (dcp::NoteHandler note) const; void add_metadata (xmlpp::Node* node) const; void send_binary (boost::shared_ptr 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 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) const; bool same (boost::shared_ptr) const; + AVPixelFormat pixel_format () const; private: boost::shared_ptr _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 +} #include #include #include @@ -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; -- 2.30.2