Move video level conversion for RGB from FFmpegImageProxy to Image.

Since FFmpeg does not do video level conversion for RGB sources
when we (sort of) ask it to in Image::crop_scale_window() it seems
to make more sense to compensate for that by calling
full_to_video_range() in the same place (rather than in
FFmpegImageProxy).

diff --git a/src/lib/ffmpeg_image_proxy.cc b/src/lib/ffmpeg_image_proxy.cc
index ab9b94b149e10e0ffdeafae194742d4e7ccef930..0b40b8f830cdc92a0f7ec0aa4a9b503a58a06faf 100644 (file)
--- a/src/lib/ffmpeg_image_proxy.cc
+++ b/src/lib/ffmpeg_image_proxy.cc
@@ -53,26 +53,23 @@ using std::dynamic_pointer_cast;
 using dcp::raw_convert;
 
 
-FFmpegImageProxy::FFmpegImageProxy (boost::filesystem::path path, VideoRange video_range)
+FFmpegImageProxy::FFmpegImageProxy (boost::filesystem::path path)
        : _data (path)
-       , _video_range (video_range)
        , _pos (0)
        , _path (path)
 {
 
 }
 
-FFmpegImageProxy::FFmpegImageProxy (dcp::ArrayData data, VideoRange video_range)
+FFmpegImageProxy::FFmpegImageProxy (dcp::ArrayData data)
        : _data (data)
-       , _video_range (video_range)
        , _pos (0)
 {
 
 }
 
-FFmpegImageProxy::FFmpegImageProxy (shared_ptr<cxml::Node> node, shared_ptr<Socket> socket)
-       : _video_range (string_to_video_range(node->string_child("VideoRange")))
-       , _pos (0)
+FFmpegImageProxy::FFmpegImageProxy (shared_ptr<Socket> socket)
+       : _pos (0)
 {
        uint32_t const size = socket->read_uint32 ();
        _data = dcp::ArrayData (size);
@@ -208,16 +205,7 @@ FFmpegImageProxy::image (optional<dcp::Size>) const
                throw DecodeError (N_("avcodec_receive_frame"), name_for_errors, r);
        }
 
-       auto const pix_fmt = static_cast<AVPixelFormat>(frame->format);
-
        _image = make_shared<Image>(frame);
-       if (_video_range == VideoRange::VIDEO && av_pix_fmt_desc_get(pix_fmt)->flags & AV_PIX_FMT_FLAG_RGB) {
-               /* Asking for the video range to be converted by libswscale (in Image) will not work for
-                * RGB sources since that method only processes video range in YUV and greyscale.  So we have
-                * to do it ourselves here.
-                */
-               _image->video_range_to_full_range();
-       }
 
        av_packet_unref (&packet);
        av_frame_free (&frame);
@@ -234,7 +222,6 @@ void
 FFmpegImageProxy::add_metadata (xmlpp::Node* node) const
 {
        node->add_child("Type")->add_child_text (N_("FFmpeg"));
-       node->add_child("VideoRange")->add_child_text(video_range_to_string(_video_range));
 }
 
 void
@@ -247,7 +234,7 @@ FFmpegImageProxy::write_to_socket (shared_ptr<Socket> socket) const
 bool
 FFmpegImageProxy::same (shared_ptr<const ImageProxy> other) const
 {
-       shared_ptr<const FFmpegImageProxy> mp = dynamic_pointer_cast<const FFmpegImageProxy> (other);
+       auto mp = dynamic_pointer_cast<const FFmpegImageProxy>(other);
        if (!mp) {
                return false;
        }

diff --git a/src/lib/ffmpeg_image_proxy.h b/src/lib/ffmpeg_image_proxy.h
index 92689abe89245df278c97cb1b463023d84e681b1..21109c9d6f2dccf9baf64f11ebbc3b2ac24618bc 100644 (file)
--- a/src/lib/ffmpeg_image_proxy.h
+++ b/src/lib/ffmpeg_image_proxy.h
@@ -27,9 +27,9 @@
 class FFmpegImageProxy : public ImageProxy
 {
 public:
-       explicit FFmpegImageProxy (boost::filesystem::path, VideoRange video_range);
-       explicit FFmpegImageProxy (dcp::ArrayData, VideoRange video_range);
-       FFmpegImageProxy (std::shared_ptr<cxml::Node> xml, std::shared_ptr<Socket> socket);
+       explicit FFmpegImageProxy (boost::filesystem::path);
+       explicit FFmpegImageProxy (dcp::ArrayData);
+       FFmpegImageProxy (std::shared_ptr<Socket> socket);
 
        Result image (
                boost::optional<dcp::Size> size = boost::optional<dcp::Size> ()
@@ -45,7 +45,6 @@ public:
 
 private:
        dcp::ArrayData _data;
-       VideoRange _video_range;
        mutable int64_t _pos;
        /** Path of a file that this image came from, if applicable; stored so that
            failed-decode errors can give more detail.

diff --git a/src/lib/image.cc b/src/lib/image.cc
index 5b926d77c07feb69e52daeeb44c1655f8b51d2b1..8e6c5717b9064025a1adb03a619731a13835b347 100644 (file)
--- a/src/lib/image.cc
+++ b/src/lib/image.cc
@@ -297,6 +297,15 @@ Image::crop_scale_window (
                out->make_part_black (corner.x + cropped_size.width, out_size.width - cropped_size.width);
        }
 
+       if (
+               video_range == VideoRange::VIDEO &&
+               out_video_range == VideoRange::FULL &&
+               av_pix_fmt_desc_get(_pixel_format)->flags & AV_PIX_FMT_FLAG_RGB
+          ) {
+               /* libswscale will not convert video range for RGB sources, so we have to do it ourselves */
+               out->video_range_to_full_range ();
+       }
+
        return out;
 }
 

diff --git a/src/lib/image.h b/src/lib/image.h
index 2ef7d07974d21432979e82cea840a35ab5a9967b..cb8f11ffc27d0eab3ab4ba86d4646b4498bc7b7a 100644 (file)
--- a/src/lib/image.h
+++ b/src/lib/image.h
@@ -80,7 +80,6 @@ public:
        void alpha_blend (std::shared_ptr<const Image> image, Position<int> pos);
        void copy (std::shared_ptr<const Image> image, Position<int> pos);
        void fade (float);
-       void video_range_to_full_range ();
 
        void read_from_socket (std::shared_ptr<Socket>);
        void write_to_socket (std::shared_ptr<Socket>) const;
@@ -106,6 +105,7 @@ private:
        void make_part_black (int x, int w);
        void yuv_16_black (uint16_t, bool);
        static uint16_t swap_16 (uint16_t);
+       void video_range_to_full_range ();
 
        dcp::Size _size;
        AVPixelFormat _pixel_format; ///< FFmpeg's way of describing the pixel format of this Image

diff --git a/src/lib/image_decoder.cc b/src/lib/image_decoder.cc
index 2e0a980928ea99213e29d94255aa6a520e14de40..e1106f86d08f616455aa2323180d80212c4f6b3c 100644 (file)
--- a/src/lib/image_decoder.cc
+++ b/src/lib/image_decoder.cc
@@ -74,7 +74,7 @@ ImageDecoder::pass ()
                        */
                        _image = make_shared<J2KImageProxy>(path, _image_content->video->size(), pf);
                } else {
-                       _image = make_shared<FFmpegImageProxy>(path, _image_content->video->range());
+                       _image = make_shared<FFmpegImageProxy>(path);
                }
        }
 

diff --git a/src/lib/image_examiner.cc b/src/lib/image_examiner.cc
index acbf55696271daa84588b179debdb60bcae0e439..89d1517ce5abf380d9e30c3a09777bd17a5a0293 100644 (file)
--- a/src/lib/image_examiner.cc
+++ b/src/lib/image_examiner.cc
@@ -66,7 +66,7 @@ ImageExaminer::ImageExaminer (shared_ptr<const Film> film, shared_ptr<const Imag
                }
                delete[] buffer;
        } else {
-               FFmpegImageProxy proxy(content->path(0), content->video->range());
+               FFmpegImageProxy proxy(content->path(0));
                _video_size = proxy.image().image->size();
        }
 

diff --git a/src/lib/image_proxy.cc b/src/lib/image_proxy.cc
index d81a3ffef0f824600c5b6eecb59710040e72d285..9e456c941b9d768dc711ca8625d0edb6be1de02b 100644 (file)
--- a/src/lib/image_proxy.cc
+++ b/src/lib/image_proxy.cc
@@ -45,7 +45,7 @@ image_proxy_factory (shared_ptr<cxml::Node> xml, shared_ptr<Socket> socket)
        if (xml->string_child("Type") == N_("Raw")) {
                return make_shared<RawImageProxy>(xml, socket);
        } else if (xml->string_child("Type") == N_("FFmpeg")) {
-               return shared_ptr<FFmpegImageProxy> (new FFmpegImageProxy(xml, socket));
+               return make_shared<FFmpegImageProxy>(socket);
        } else if (xml->string_child("Type") == N_("J2K")) {
                return make_shared<J2KImageProxy>(xml, socket);
        }

diff --git a/src/lib/util.cc b/src/lib/util.cc
index 6286d1a658dd9fde4bb1320bbd140586f49b05b4..a3c8c50820d3c5bca94f13e0fb52935eb933167d 100644 (file)
--- a/src/lib/util.cc
+++ b/src/lib/util.cc
@@ -951,7 +951,7 @@ void
 emit_subtitle_image (ContentTimePeriod period, dcp::SubtitleImage sub, dcp::Size size, shared_ptr<TextDecoder> decoder)
 {
        /* XXX: this is rather inefficient; decoding the image just to get its size */
-       FFmpegImageProxy proxy (sub.png_image(), VideoRange::FULL);
+       FFmpegImageProxy proxy (sub.png_image());
        auto image = proxy.image().image;
        /* set up rect with height and width */
        dcpomatic::Rect<double> rect(0, 0, image->size().width / double(size.width), image->size().height / double(size.height));

diff --git a/test/image_proxy_test.cc b/test/image_proxy_test.cc
index a9872b958047c30f162f7f414e8dd836530d655d..210f32d408eddb17239e37bef9dc311645a8c555 100644 (file)
--- a/test/image_proxy_test.cc
+++ b/test/image_proxy_test.cc
@@ -54,14 +54,14 @@ BOOST_AUTO_TEST_CASE (j2k_image_proxy_same_test)
 BOOST_AUTO_TEST_CASE (ffmpeg_image_proxy_same_test)
 {
        {
-               auto proxy1 = make_shared<FFmpegImageProxy>(data_file0, VideoRange::FULL);
-               auto proxy2 = make_shared<FFmpegImageProxy>(data_file0, VideoRange::FULL);
+               auto proxy1 = make_shared<FFmpegImageProxy>(data_file0);
+               auto proxy2 = make_shared<FFmpegImageProxy>(data_file0);
                BOOST_CHECK (proxy1->same(proxy2));
        }
 
        {
-               auto proxy1 = make_shared<FFmpegImageProxy>(data_file0, VideoRange::FULL);
-               auto proxy2 = make_shared<FFmpegImageProxy>(data_file1, VideoRange::FULL);
+               auto proxy1 = make_shared<FFmpegImageProxy>(data_file0);
+               auto proxy2 = make_shared<FFmpegImageProxy>(data_file1);
                BOOST_CHECK (!proxy1->same(proxy2));
        }
 }

diff --git a/test/image_test.cc b/test/image_test.cc
index 0993be6cf49d706bad9d447d42f92f4a9be919de..3993b3efbdeaf7f99e4cff9f4b1f04caf4d7c4dd 100644 (file)
--- a/test/image_test.cc
+++ b/test/image_test.cc
@@ -147,7 +147,7 @@ BOOST_AUTO_TEST_CASE (compact_image_test)
 void
 alpha_blend_test_one (AVPixelFormat format, string suffix)
 {
-       auto proxy = make_shared<FFmpegImageProxy>(TestPaths::private_data() / "prophet_frame.tiff", VideoRange::FULL);
+       auto proxy = make_shared<FFmpegImageProxy>(TestPaths::private_data() / "prophet_frame.tiff");
        auto raw = proxy->image().image;
        auto background = raw->convert_pixel_format (dcp::YUVToRGB::REC709, format, true, false);
 
@@ -273,7 +273,7 @@ BOOST_AUTO_TEST_CASE (merge_test2)
 /** Test Image::crop_scale_window with YUV420P and some windowing */
 BOOST_AUTO_TEST_CASE (crop_scale_window_test)
 {
-       auto proxy = make_shared<FFmpegImageProxy>("test/data/flat_red.png", VideoRange::FULL);
+       auto proxy = make_shared<FFmpegImageProxy>("test/data/flat_red.png");
        auto raw = proxy->image().image;
        auto out = raw->crop_scale_window(
                Crop(), dcp::Size(1998, 836), dcp::Size(1998, 1080), dcp::YUVToRGB::REC709, VideoRange::FULL, AV_PIX_FMT_YUV420P, VideoRange::FULL, true, false
@@ -299,7 +299,7 @@ BOOST_AUTO_TEST_CASE (crop_scale_window_test2)
 
 BOOST_AUTO_TEST_CASE (crop_scale_window_test3)
 {
-       auto proxy = make_shared<FFmpegImageProxy>(TestPaths::private_data() / "player_seek_test_0.png", VideoRange::FULL);
+       auto proxy = make_shared<FFmpegImageProxy>(TestPaths::private_data() / "player_seek_test_0.png");
        auto xyz = proxy->image().image->convert_pixel_format(dcp::YUVToRGB::REC709, AV_PIX_FMT_RGB24, true, false);
        auto cropped = xyz->crop_scale_window(
                Crop(512, 0, 0, 0), dcp::Size(1486, 1080), dcp::Size(1998, 1080), dcp::YUVToRGB::REC709, VideoRange::FULL, AV_PIX_FMT_RGB24, VideoRange::FULL, false, false
@@ -311,7 +311,7 @@ BOOST_AUTO_TEST_CASE (crop_scale_window_test3)
 
 BOOST_AUTO_TEST_CASE (crop_scale_window_test4)
 {
-       auto proxy = make_shared<FFmpegImageProxy>(TestPaths::private_data() / "player_seek_test_0.png", VideoRange::FULL);
+       auto proxy = make_shared<FFmpegImageProxy>(TestPaths::private_data() / "player_seek_test_0.png");
        auto xyz = proxy->image().image->convert_pixel_format(dcp::YUVToRGB::REC709, AV_PIX_FMT_RGB24, true, false);
        auto cropped = xyz->crop_scale_window(
                Crop(512, 0, 0, 0), dcp::Size(1486, 1080), dcp::Size(1998, 1080), dcp::YUVToRGB::REC709, VideoRange::FULL, AV_PIX_FMT_XYZ12LE, VideoRange::FULL, false, false
@@ -323,7 +323,7 @@ BOOST_AUTO_TEST_CASE (crop_scale_window_test4)
 
 BOOST_AUTO_TEST_CASE (crop_scale_window_test5)
 {
-       auto proxy = make_shared<FFmpegImageProxy>(TestPaths::private_data() / "player_seek_test_0.png", VideoRange::FULL);
+       auto proxy = make_shared<FFmpegImageProxy>(TestPaths::private_data() / "player_seek_test_0.png");
        auto xyz = proxy->image().image->convert_pixel_format(dcp::YUVToRGB::REC709, AV_PIX_FMT_XYZ12LE, true, false);
        auto cropped = xyz->crop_scale_window(
                Crop(512, 0, 0, 0), dcp::Size(1486, 1080), dcp::Size(1998, 1080), dcp::YUVToRGB::REC709, VideoRange::FULL, AV_PIX_FMT_RGB24, VideoRange::FULL, false, false
@@ -335,7 +335,7 @@ BOOST_AUTO_TEST_CASE (crop_scale_window_test5)
 
 BOOST_AUTO_TEST_CASE (crop_scale_window_test6)
 {
-       auto proxy = make_shared<FFmpegImageProxy>(TestPaths::private_data() / "player_seek_test_0.png", VideoRange::FULL);
+       auto proxy = make_shared<FFmpegImageProxy>(TestPaths::private_data() / "player_seek_test_0.png");
        auto xyz = proxy->image().image->convert_pixel_format(dcp::YUVToRGB::REC709, AV_PIX_FMT_XYZ12LE, true, false);
        auto cropped = xyz->crop_scale_window(
                Crop(512, 0, 0, 0), dcp::Size(1486, 1080), dcp::Size(1998, 1080), dcp::YUVToRGB::REC709, VideoRange::FULL, AV_PIX_FMT_XYZ12LE, VideoRange::FULL, false, false
@@ -350,7 +350,7 @@ BOOST_AUTO_TEST_CASE (crop_scale_window_test7)
 {
        using namespace boost::filesystem;
        for (int left_crop = 0; left_crop < 8; ++left_crop) {
-               auto proxy = make_shared<FFmpegImageProxy>("test/data/rgb_grey_testcard.png", VideoRange::FULL);
+               auto proxy = make_shared<FFmpegImageProxy>("test/data/rgb_grey_testcard.png");
                auto yuv = proxy->image().image->convert_pixel_format(dcp::YUVToRGB::REC709, AV_PIX_FMT_YUV420P, true, false);
                int rounded = left_crop - (left_crop % 2);
                auto cropped = yuv->crop_scale_window(
@@ -373,7 +373,7 @@ BOOST_AUTO_TEST_CASE (crop_scale_window_test7)
 
 BOOST_AUTO_TEST_CASE (as_png_test)
 {
-       auto proxy = make_shared<FFmpegImageProxy>("test/data/3d_test/000001.png", VideoRange::FULL);
+       auto proxy = make_shared<FFmpegImageProxy>("test/data/3d_test/000001.png");
        auto image_rgb = proxy->image().image;
        auto image_bgr = image_rgb->convert_pixel_format(dcp::YUVToRGB::REC709, AV_PIX_FMT_BGRA, true, false);
        image_rgb->as_png().write ("build/test/as_png_rgb.png");
@@ -401,7 +401,7 @@ fade_test_format_black (AVPixelFormat f, string name)
 static void
 fade_test_format_red (AVPixelFormat f, float amount, string name)
 {
-       auto proxy = make_shared<FFmpegImageProxy>("test/data/flat_red.png", VideoRange::FULL);
+       auto proxy = make_shared<FFmpegImageProxy>("test/data/flat_red.png");
        auto red = proxy->image().image->convert_pixel_format(dcp::YUVToRGB::REC709, f, true, false);
        red->fade (amount);
        string const filename = "fade_test_red_" + name + ".png";
@@ -505,7 +505,7 @@ BOOST_AUTO_TEST_CASE (make_black_test)
 
 BOOST_AUTO_TEST_CASE (make_part_black_test)
 {
-       auto proxy = make_shared<FFmpegImageProxy>("test/data/flat_red.png", VideoRange::FULL);
+       auto proxy = make_shared<FFmpegImageProxy>("test/data/flat_red.png");
        auto original = proxy->image().image;
 
        list<AVPixelFormat> pix_fmts = {

diff --git a/test/test.cc b/test/test.cc
index 3cd5688e7fd2bd69f1b87b16376c58bd6790d497..4dab0cff1b5895c0e197e19c0db140092662757e 100644 (file)
--- a/test/test.cc
+++ b/test/test.cc
@@ -363,9 +363,9 @@ static
 double
 rms_error (boost::filesystem::path ref, boost::filesystem::path check)
 {
-       FFmpegImageProxy ref_proxy (ref, VideoRange::FULL);
+       FFmpegImageProxy ref_proxy (ref);
        auto ref_image = ref_proxy.image().image;
-       FFmpegImageProxy check_proxy (check, VideoRange::FULL);
+       FFmpegImageProxy check_proxy (check);
        auto check_image = check_proxy.image().image;
 
        BOOST_REQUIRE_EQUAL (ref_image->pixel_format(), check_image->pixel_format());

diff --git a/test/video_level_test.cc b/test/video_level_test.cc
index c974c938d3002f71a4fc86b9f1518ab8597bbd9b..be54cd3f90075f18837f75c059919fbd38336eac 100644 (file)
--- a/test/video_level_test.cc
+++ b/test/video_level_test.cc
@@ -38,6 +38,8 @@
 #include "lib/image_decoder.h"
 #include "lib/ffmpeg_encoder.h"
 #include "lib/job_manager.h"
+#include "lib/player.h"
+#include "lib/player_video.h"
 #include "lib/transcode_job.h"
 #include "lib/video_decoder.h"
 #include "test.h"
@@ -91,7 +93,7 @@ BOOST_AUTO_TEST_CASE (ffmpeg_image_full_range_not_changed)
 
        write_image (grey_image(size, grey_pixel), file);
 
-       FFmpegImageProxy proxy (file, VideoRange::FULL);
+       FFmpegImageProxy proxy (file);
        ImageProxy::Result result = proxy.image ();
        BOOST_REQUIRE (!result.error);
 
@@ -106,19 +108,30 @@ BOOST_AUTO_TEST_CASE (ffmpeg_image_full_range_not_changed)
 
 BOOST_AUTO_TEST_CASE (ffmpeg_image_video_range_expanded)
 {
-       dcp::Size size(640, 480);
+       dcp::Size size(1998, 1080);
        uint8_t const grey_pixel = 128;
-       uint8_t const expanded_grey_pixel = static_cast<uint8_t>((grey_pixel - 16) * 256.0 / 219);
+       uint8_t const expanded_grey_pixel = static_cast<uint8_t>(lrintf((grey_pixel - 16) * 256.0 / 219));
        boost::filesystem::path const file = "build/test/ffmpeg_image_video_range_expanded.png";
 
-       write_image (grey_image(size, grey_pixel), file);
+       write_image(grey_image(size, grey_pixel), file);
 
-       FFmpegImageProxy proxy (file, VideoRange::VIDEO);
-       ImageProxy::Result result = proxy.image ();
-       BOOST_REQUIRE (!result.error);
+       auto content = content_factory(file).front();
+       auto film = new_test_film2 ("ffmpeg_image_video_range_expanded", { content });
+       content->video->set_range (VideoRange::VIDEO);
+       auto player = make_shared<Player>(film, film->playlist());
+
+       shared_ptr<PlayerVideo> player_video;
+       player->Video.connect([&player_video](shared_ptr<PlayerVideo> pv, dcpomatic::DCPTime) {
+               player_video = pv;
+       });
+       while (!player_video) {
+               BOOST_REQUIRE (!player->pass());
+       }
+
+       auto image = player_video->image ([](AVPixelFormat f) { return f; }, VideoRange::FULL, true, false);
 
        for (int y = 0; y < size.height; ++y) {
-               uint8_t* p = result.image->data()[0] + y * result.image->stride()[0];
+               uint8_t* p = image->data()[0] + y * image->stride()[0];
                for (int x = 0; x < size.width; ++x) {
                        BOOST_REQUIRE_EQUAL (*p++, expanded_grey_pixel);
                }
@@ -185,6 +198,9 @@ pixel_range (shared_ptr<const Image> image)
 }
 
 
+/** @return pixel range of the first frame in @ref content in its raw form, i.e.
+ *  straight out of the decoder with no level processing, scaling etc.
+ */
 static
 pair<int, int>
 pixel_range (shared_ptr<const Film> film, shared_ptr<const Content> content)
@@ -504,8 +520,8 @@ BOOST_AUTO_TEST_CASE (image_F_to_V_movie)
 BOOST_AUTO_TEST_CASE (image_FoV_to_V_movie)
 {
        auto range = V_movie_range (image_FoV("image_FoV_to_V_movie"));
-       BOOST_CHECK_EQUAL (range.first, 102);
-       BOOST_CHECK_EQUAL (range.second, 923);
+       BOOST_CHECK_EQUAL (range.first, 64);
+       BOOST_CHECK_EQUAL (range.second, 960);
 }