Fix handling of incorrectly-recognised JPEG2000 files.

Previously we asked libdcp whether an imported J2K file was
RGB or XYZ.  The answer it gives is sometimes wrong, for reasons
that are not clear (either the files are not marked correctly,
or openjpeg is not parsing whatever metadata correctly).

However it seems that, in general, we use the user's specified
colour conversion to decide what to do with an image, rather than
asking the image what should be done to it.

Hence it makes more sense to assume that if a user specifies no
colour conversion for a J2K file then the file is XYZ.

With preview, the colour conversion from XYZ back to RGB is done
by FFmpeg, so we have to set the pixel format correctly on the
Image that comes back from J2KImageProxy.  Now we get that pixel
format from the configured colourspace conversion rather than
from openjpeg's guess as to the file's colourspace.

It's a bit ugly that the only thing we ask the file about is whether
or not it is in YUV (which governs whether or not FFmpeg applies
the user's configured YUV-to-RGB conversion).  Everything else is
decided by the configured conversion.

I think there's still some uglyness in here that I can't put my
finger on.

diff --git a/ChangeLog b/ChangeLog
index 12fccd704f933da2ac9b3a0fdb3b5d02867b700c..6442b5633ee75361fd86612eebcf8cddfe75800a 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2016-06-29  Carl Hetherington  <cth@carlh.net>
+
+       * Obey specified colour conversion when previewing
+       RGB and XYZ JPEG2000 files.
+
 2016-06-24  Carl Hetherington  <cth@carlh.net>
 
        * Version 2.8.14 released.

diff --git a/src/lib/dcp_decoder.cc b/src/lib/dcp_decoder.cc
index 35fe375e9b97c925c02d25c9d240e152b1712b53..283cb29054ab900c0abc9881abccce032f6a05a9 100644 (file)
--- a/src/lib/dcp_decoder.cc
+++ b/src/lib/dcp_decoder.cc
@@ -95,15 +95,22 @@ DCPDecoder::pass (PassReason reason, bool)
                shared_ptr<dcp::PictureAsset> asset = (*_reel)->main_picture()->asset ();
                int64_t const entry_point = (*_reel)->main_picture()->entry_point ();
                if (_mono_reader) {
-                       video->give (shared_ptr<ImageProxy> (new J2KImageProxy (_mono_reader->get_frame (entry_point + frame), asset->size())), _offset + frame);
+                       video->give (
+                               shared_ptr<ImageProxy> (
+                                       new J2KImageProxy (_mono_reader->get_frame (entry_point + frame), asset->size(), AV_PIX_FMT_XYZ12LE)
+                                       ),
+                               _offset + frame
+                               );
                } else {
                        video->give (
-                               shared_ptr<ImageProxy> (new J2KImageProxy (_stereo_reader->get_frame (entry_point + frame), asset->size(), dcp::EYE_LEFT)),
+                               shared_ptr<ImageProxy> (
+                                       new J2KImageProxy (_stereo_reader->get_frame (entry_point + frame), asset->size(), dcp::EYE_LEFT, AV_PIX_FMT_XYZ12LE)),
                                _offset + frame
                                );
 
                        video->give (
-                               shared_ptr<ImageProxy> (new J2KImageProxy (_stereo_reader->get_frame (entry_point + frame), asset->size(), dcp::EYE_RIGHT)),
+                               shared_ptr<ImageProxy> (
+                                       new J2KImageProxy (_stereo_reader->get_frame (entry_point + frame), asset->size(), dcp::EYE_RIGHT, AV_PIX_FMT_XYZ12LE)),
                                _offset + frame
                                );
                }

diff --git a/src/lib/image_decoder.cc b/src/lib/image_decoder.cc
index d0973f1227afdf51dbbc495f0d55651337b3a639..f7afbc0a153b86cbabc77d88ba0c3f4d4bf1684d 100644 (file)
--- a/src/lib/image_decoder.cc
+++ b/src/lib/image_decoder.cc
@@ -55,10 +55,18 @@ ImageDecoder::pass (PassReason, bool)
                /* Either we need an image or we are using moving images, so load one */
                boost::filesystem::path path = _image_content->path (_image_content->still() ? 0 : _video_position);
                if (valid_j2k_file (path)) {
+                       AVPixelFormat pf;
+                       if (_image_content->video->colour_conversion()) {
+                               /* We have a specified colour conversion: assume the image is RGB */
+                               pf = AV_PIX_FMT_RGB48LE;
+                       } else {
+                               /* No specified colour conversion: assume the image is XYZ */
+                               pf = AV_PIX_FMT_XYZ12LE;
+                       }
                        /* We can't extract image size from a JPEG2000 codestream without decoding it,
                           so pass in the image content's size here.
                        */
-                       _image.reset (new J2KImageProxy (path, _image_content->video->size ()));
+                       _image.reset (new J2KImageProxy (path, _image_content->video->size(), pf));
                } else {
                        _image.reset (new MagickImageProxy (path));
                }

diff --git a/src/lib/j2k_image_proxy.cc b/src/lib/j2k_image_proxy.cc
index 44b5ebea7f6d67cce56f367e7d917f5173e4fcc1..a2685bb49251eba41bd03a7f30a9c170dde2145f 100644 (file)
--- a/src/lib/j2k_image_proxy.cc
+++ b/src/lib/j2k_image_proxy.cc
@@ -42,23 +42,26 @@ using boost::dynamic_pointer_cast;
 using dcp::Data;
 
 /** Construct a J2KImageProxy from a JPEG2000 file */
-J2KImageProxy::J2KImageProxy (boost::filesystem::path path, dcp::Size size)
+J2KImageProxy::J2KImageProxy (boost::filesystem::path path, dcp::Size size, AVPixelFormat pixel_format)
        : _data (path)
        , _size (size)
+       , _pixel_format (pixel_format)
 {
 
 }
 
-J2KImageProxy::J2KImageProxy (shared_ptr<const dcp::MonoPictureFrame> frame, dcp::Size size)
+J2KImageProxy::J2KImageProxy (shared_ptr<const dcp::MonoPictureFrame> frame, dcp::Size size, AVPixelFormat pixel_format)
        : _data (frame->j2k_size ())
        , _size (size)
+       , _pixel_format (pixel_format)
 {
        memcpy (_data.data().get(), frame->j2k_data(), _data.size ());
 }
 
-J2KImageProxy::J2KImageProxy (shared_ptr<const dcp::StereoPictureFrame> frame, dcp::Size size, dcp::Eye eye)
+J2KImageProxy::J2KImageProxy (shared_ptr<const dcp::StereoPictureFrame> frame, dcp::Size size, dcp::Eye eye, AVPixelFormat pixel_format)
        : _size (size)
        , _eye (eye)
+       , _pixel_format (pixel_format)
 {
        switch (eye) {
        case dcp::EYE_LEFT:
@@ -79,6 +82,11 @@ J2KImageProxy::J2KImageProxy (shared_ptr<cxml::Node> xml, shared_ptr<Socket> soc
                _eye = static_cast<dcp::Eye> (xml->number_child<int> ("Eye"));
        }
        _data = Data (xml->number_child<int> ("Size"));
+       /* This only matters when we are using J2KImageProxy for the preview, which
+          will never use this constructor (which is only used for passing data to
+          encode servers).  So we can put anything in here.  It's a bit of a hack.
+       */
+       _pixel_format = AV_PIX_FMT_XYZ12LE;
        socket->read (_data.data().get (), _data.size ());
 }
 
@@ -107,7 +115,7 @@ J2KImageProxy::image (optional<dcp::NoteHandler>) const
                }
        }
 
-       shared_ptr<Image> image (new Image (pixel_format(), _size, true));
+       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.
@@ -160,21 +168,10 @@ 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->srgb ()) {
-               return AV_PIX_FMT_RGB48LE;
-       }
-
-       return AV_PIX_FMT_XYZ12LE;
-}
-
-J2KImageProxy::J2KImageProxy (Data data, dcp::Size size)
+J2KImageProxy::J2KImageProxy (Data data, dcp::Size size, AVPixelFormat pixel_format)
        : _data (data)
        , _size (size)
+       , _pixel_format (pixel_format)
 {
 
 }

diff --git a/src/lib/j2k_image_proxy.h b/src/lib/j2k_image_proxy.h
index 72815a0f6c57d8d3b1575c124816304e58b1d47d..96a776f2a5333bc6100a41629a4cf2919f24f7c6 100644 (file)
--- a/src/lib/j2k_image_proxy.h
+++ b/src/lib/j2k_image_proxy.h
@@ -30,9 +30,9 @@ namespace dcp {
 class J2KImageProxy : public ImageProxy
 {
 public:
-       J2KImageProxy (boost::filesystem::path path, dcp::Size);
-       J2KImageProxy (boost::shared_ptr<const dcp::MonoPictureFrame> frame, dcp::Size);
-       J2KImageProxy (boost::shared_ptr<const dcp::StereoPictureFrame> frame, dcp::Size, dcp::Eye);
+       J2KImageProxy (boost::filesystem::path path, dcp::Size, AVPixelFormat pixel_format);
+       J2KImageProxy (boost::shared_ptr<const dcp::MonoPictureFrame> frame, dcp::Size, AVPixelFormat pixel_format);
+       J2KImageProxy (boost::shared_ptr<const dcp::StereoPictureFrame> frame, dcp::Size, dcp::Eye, AVPixelFormat pixel_format);
        J2KImageProxy (boost::shared_ptr<cxml::Node> xml, boost::shared_ptr<Socket> socket);
 
        boost::shared_ptr<Image> image (boost::optional<dcp::NoteHandler> note = boost::optional<dcp::NoteHandler> ()) const;
@@ -40,7 +40,9 @@ 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;
-       AVPixelFormat pixel_format () const;
+       AVPixelFormat pixel_format () const {
+               return _pixel_format;
+       }
 
        dcp::Data j2k () const {
                return _data;
@@ -54,11 +56,12 @@ private:
        friend struct client_server_test_j2k;
 
        /* For tests */
-       J2KImageProxy (dcp::Data data, dcp::Size size);
+       J2KImageProxy (dcp::Data data, dcp::Size size, AVPixelFormat pixel_format);
        void ensure_j2k () const;
 
        dcp::Data _data;
        dcp::Size _size;
        boost::optional<dcp::Eye> _eye;
        mutable boost::shared_ptr<dcp::OpenJPEGImage> _j2k;
+       AVPixelFormat _pixel_format;
 };

diff --git a/src/lib/player.cc b/src/lib/player.cc
index 20a3e14533557997117cfd9fb0c9a91ae017a700..0360858cb2b19a2030b7c4d514a718c14cf3ebf8 100644 (file)
--- a/src/lib/player.cc
+++ b/src/lib/player.cc
@@ -173,7 +173,8 @@ Player::playlist_content_changed (weak_ptr<Content> w, int property, bool freque
                property == SubtitleContentProperty::COLOUR ||
                property == SubtitleContentProperty::OUTLINE ||
                property == SubtitleContentProperty::OUTLINE_COLOUR ||
-               property == FFmpegContentProperty::SUBTITLE_STREAM
+               property == FFmpegContentProperty::SUBTITLE_STREAM ||
+               property == VideoContentProperty::COLOUR_CONVERSION
                ) {
 
                _have_valid_pieces = false;
@@ -190,8 +191,7 @@ Player::playlist_content_changed (weak_ptr<Content> w, int property, bool freque
                property == VideoContentProperty::CROP ||
                property == VideoContentProperty::SCALE ||
                property == VideoContentProperty::FADE_IN ||
-               property == VideoContentProperty::FADE_OUT ||
-               property == VideoContentProperty::COLOUR_CONVERSION
+               property == VideoContentProperty::FADE_OUT
                ) {
 
                Changed (frequent);

diff --git a/src/lib/video_mxf_decoder.cc b/src/lib/video_mxf_decoder.cc
index 938d7deaf5c540668e0316b3a45cb99bf5b31501..dc4f8d60b6882b23b255240cc4e23359bd2dcfbc 100644 (file)
--- a/src/lib/video_mxf_decoder.cc
+++ b/src/lib/video_mxf_decoder.cc
@@ -77,10 +77,16 @@ VideoMXFDecoder::pass (PassReason, bool)
        }
 
        if (_mono_reader) {
-               video->give (shared_ptr<ImageProxy> (new J2KImageProxy (_mono_reader->get_frame(frame), _size)), frame);
+               video->give (
+                       shared_ptr<ImageProxy> (new J2KImageProxy (_mono_reader->get_frame(frame), _size, AV_PIX_FMT_XYZ12LE)), frame
+                       );
        } else {
-               video->give (shared_ptr<ImageProxy> (new J2KImageProxy (_stereo_reader->get_frame(frame), _size, dcp::EYE_LEFT)), frame);
-               video->give (shared_ptr<ImageProxy> (new J2KImageProxy (_stereo_reader->get_frame(frame), _size, dcp::EYE_RIGHT)), frame);
+               video->give (
+                       shared_ptr<ImageProxy> (new J2KImageProxy (_stereo_reader->get_frame(frame), _size, dcp::EYE_LEFT, AV_PIX_FMT_XYZ12LE)), frame
+                       );
+               video->give (
+                       shared_ptr<ImageProxy> (new J2KImageProxy (_stereo_reader->get_frame(frame), _size, dcp::EYE_RIGHT, AV_PIX_FMT_XYZ12LE)), frame
+                       );
        }
 
        _next += ContentTime::from_frames (1, vfr);

diff --git a/src/wx/film_viewer.cc b/src/wx/film_viewer.cc
index e0244ce83370f0561177db30b8d21496a4e844cd..42ce7cd1841fe27b5440e7324e4936b533c74c61 100644 (file)
--- a/src/wx/film_viewer.cc
+++ b/src/wx/film_viewer.cc
@@ -222,8 +222,28 @@ FilmViewer::get (DCPTime p, bool accurate)
                                pv = all_pv.front ();
                        }
 
+                       /* In an ideal world, what we would do here is:
+                        *
+                        * 1. convert to XYZ exactly as we do in the DCP creation path.
+                        * 2. convert back to RGB for the preview display, compensating
+                        *    for the monitor etc. etc.
+                        *
+                        * but this is inefficient if the source is RGB.  Since we don't
+                        * (currently) care too much about the precise accuracy of the preview's
+                        * colour mapping (and we care more about its speed) we try to short-
+                        * circuit this "ideal" situation in some cases.
+                        *
+                        * The content's specified colour conversion indicates the colourspace
+                        * which the content is in (according to the user).
+                        *
+                        * PlayerVideo::image (bound to PlayerVideo::always_rgb) will take the source
+                        * image and convert it (from whatever the user has said it is) to RGB.
+                        */
+
                        _frame = pv->image (
-                               bind (&Log::dcp_log, _film->log().get(), _1, _2), bind (&PlayerVideo::always_rgb, _1), false, true
+                               bind (&Log::dcp_log, _film->log().get(), _1, _2),
+                               bind (&PlayerVideo::always_rgb, _1),
+                               false, true
                                );
 
                        ImageChanged (pv);

diff --git a/test/client_server_test.cc b/test/client_server_test.cc
index 1f77f7f907cc718a0bcf61dd32b41881c9dd34c3..f12c5335d42f3480ffddfe4fc828bd767cc6c016 100644 (file)
--- a/test/client_server_test.cc
+++ b/test/client_server_test.cc
@@ -260,7 +260,7 @@ BOOST_AUTO_TEST_CASE (client_server_test_j2k)
 
        shared_ptr<PlayerVideo> j2k_pvf (
                new PlayerVideo (
-                       shared_ptr<ImageProxy> (new J2KImageProxy (raw_locally_encoded, dcp::Size (1998, 1080))),
+                       shared_ptr<ImageProxy> (new J2KImageProxy (raw_locally_encoded, dcp::Size (1998, 1080), AV_PIX_FMT_XYZ12LE)),
                        DCPTime (),
                        Crop (),
                        optional<double> (),