From 62f9b78a2eb5f0fc6b9028264bac6ad501d83309 Mon Sep 17 00:00:00 2001 From: Carl Hetherington Date: Tue, 25 May 2021 00:57:16 +0200 Subject: [PATCH] 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). --- src/lib/ffmpeg_image_proxy.cc | 23 +++++----------------- src/lib/ffmpeg_image_proxy.h | 7 +++---- src/lib/image.cc | 9 +++++++++ src/lib/image.h | 2 +- src/lib/image_decoder.cc | 2 +- src/lib/image_examiner.cc | 2 +- src/lib/image_proxy.cc | 2 +- src/lib/util.cc | 2 +- test/image_proxy_test.cc | 8 ++++---- test/image_test.cc | 20 +++++++++---------- test/test.cc | 4 ++-- test/video_level_test.cc | 36 +++++++++++++++++++++++++---------- 12 files changed, 64 insertions(+), 53 deletions(-) diff --git a/src/lib/ffmpeg_image_proxy.cc b/src/lib/ffmpeg_image_proxy.cc index ab9b94b14..0b40b8f83 100644 --- 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 node, shared_ptr socket) - : _video_range (string_to_video_range(node->string_child("VideoRange"))) - , _pos (0) +FFmpegImageProxy::FFmpegImageProxy (shared_ptr socket) + : _pos (0) { uint32_t const size = socket->read_uint32 (); _data = dcp::ArrayData (size); @@ -208,16 +205,7 @@ FFmpegImageProxy::image (optional) const throw DecodeError (N_("avcodec_receive_frame"), name_for_errors, r); } - auto const pix_fmt = static_cast(frame->format); - _image = make_shared(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) const bool FFmpegImageProxy::same (shared_ptr other) const { - shared_ptr mp = dynamic_pointer_cast (other); + auto mp = dynamic_pointer_cast(other); if (!mp) { return false; } diff --git a/src/lib/ffmpeg_image_proxy.h b/src/lib/ffmpeg_image_proxy.h index 92689abe8..21109c9d6 100644 --- 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 xml, std::shared_ptr socket); + explicit FFmpegImageProxy (boost::filesystem::path); + explicit FFmpegImageProxy (dcp::ArrayData); + FFmpegImageProxy (std::shared_ptr socket); Result image ( boost::optional size = boost::optional () @@ -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 5b926d77c..8e6c5717b 100644 --- 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 2ef7d0797..cb8f11ffc 100644 --- a/src/lib/image.h +++ b/src/lib/image.h @@ -80,7 +80,6 @@ public: void alpha_blend (std::shared_ptr image, Position pos); void copy (std::shared_ptr image, Position pos); void fade (float); - void video_range_to_full_range (); void read_from_socket (std::shared_ptr); void write_to_socket (std::shared_ptr) 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 2e0a98092..e1106f86d 100644 --- a/src/lib/image_decoder.cc +++ b/src/lib/image_decoder.cc @@ -74,7 +74,7 @@ ImageDecoder::pass () */ _image = make_shared(path, _image_content->video->size(), pf); } else { - _image = make_shared(path, _image_content->video->range()); + _image = make_shared(path); } } diff --git a/src/lib/image_examiner.cc b/src/lib/image_examiner.cc index acbf55696..89d1517ce 100644 --- a/src/lib/image_examiner.cc +++ b/src/lib/image_examiner.cc @@ -66,7 +66,7 @@ ImageExaminer::ImageExaminer (shared_ptr film, shared_ptrpath(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 d81a3ffef..9e456c941 100644 --- a/src/lib/image_proxy.cc +++ b/src/lib/image_proxy.cc @@ -45,7 +45,7 @@ image_proxy_factory (shared_ptr xml, shared_ptr socket) if (xml->string_child("Type") == N_("Raw")) { return make_shared(xml, socket); } else if (xml->string_child("Type") == N_("FFmpeg")) { - return shared_ptr (new FFmpegImageProxy(xml, socket)); + return make_shared(socket); } else if (xml->string_child("Type") == N_("J2K")) { return make_shared(xml, socket); } diff --git a/src/lib/util.cc b/src/lib/util.cc index 6286d1a65..a3c8c5082 100644 --- 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 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 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 a9872b958..210f32d40 100644 --- 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(data_file0, VideoRange::FULL); - auto proxy2 = make_shared(data_file0, VideoRange::FULL); + auto proxy1 = make_shared(data_file0); + auto proxy2 = make_shared(data_file0); BOOST_CHECK (proxy1->same(proxy2)); } { - auto proxy1 = make_shared(data_file0, VideoRange::FULL); - auto proxy2 = make_shared(data_file1, VideoRange::FULL); + auto proxy1 = make_shared(data_file0); + auto proxy2 = make_shared(data_file1); BOOST_CHECK (!proxy1->same(proxy2)); } } diff --git a/test/image_test.cc b/test/image_test.cc index 0993be6cf..3993b3efb 100644 --- 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(TestPaths::private_data() / "prophet_frame.tiff", VideoRange::FULL); + auto proxy = make_shared(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("test/data/flat_red.png", VideoRange::FULL); + auto proxy = make_shared("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(TestPaths::private_data() / "player_seek_test_0.png", VideoRange::FULL); + auto proxy = make_shared(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(TestPaths::private_data() / "player_seek_test_0.png", VideoRange::FULL); + auto proxy = make_shared(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(TestPaths::private_data() / "player_seek_test_0.png", VideoRange::FULL); + auto proxy = make_shared(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(TestPaths::private_data() / "player_seek_test_0.png", VideoRange::FULL); + auto proxy = make_shared(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("test/data/rgb_grey_testcard.png", VideoRange::FULL); + auto proxy = make_shared("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("test/data/3d_test/000001.png", VideoRange::FULL); + auto proxy = make_shared("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("test/data/flat_red.png", VideoRange::FULL); + auto proxy = make_shared("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("test/data/flat_red.png", VideoRange::FULL); + auto proxy = make_shared("test/data/flat_red.png"); auto original = proxy->image().image; list pix_fmts = { diff --git a/test/test.cc b/test/test.cc index 3cd5688e7..4dab0cff1 100644 --- 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 c974c938d..be54cd3f9 100644 --- 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((grey_pixel - 16) * 256.0 / 219); + uint8_t const expanded_grey_pixel = static_cast(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(film, film->playlist()); + + shared_ptr player_video; + player->Video.connect([&player_video](shared_ptr 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 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 pixel_range (shared_ptr film, shared_ptr 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); } -- 2.30.2