From 67a404fff364c6e1fa02eab270755895ba0e1fe8 Mon Sep 17 00:00:00 2001 From: Carl Hetherington Date: Fri, 12 Jan 2018 13:24:52 +0000 Subject: [PATCH] Use an enum for the effect in SubtitleContent. --- src/lib/player.cc | 3 +- src/lib/subtitle_content.cc | 63 ++++++++++++++++++---------- src/lib/subtitle_content.h | 18 +++----- src/lib/subtitle_decoder.cc | 6 +-- src/wx/subtitle_appearance_dialog.cc | 25 ++++++++--- test/ffmpeg_encoder_test.cc | 8 ++-- 6 files changed, 70 insertions(+), 53 deletions(-) diff --git a/src/lib/player.cc b/src/lib/player.cc index 499aa2a38..8e56991f8 100644 --- a/src/lib/player.cc +++ b/src/lib/player.cc @@ -186,8 +186,7 @@ Player::playlist_content_changed (weak_ptr w, int property, bool freque property == DCPContentProperty::NEEDS_ASSETS || property == DCPContentProperty::NEEDS_KDM || property == SubtitleContentProperty::COLOUR || - property == SubtitleContentProperty::OUTLINE || - property == SubtitleContentProperty::SHADOW || + property == SubtitleContentProperty::EFFECT || property == SubtitleContentProperty::EFFECT_COLOUR || property == FFmpegContentProperty::SUBTITLE_STREAM || property == FFmpegContentProperty::FILTERS || diff --git a/src/lib/subtitle_content.cc b/src/lib/subtitle_content.cc index 0083ff8a2..df05b356a 100644 --- a/src/lib/subtitle_content.cc +++ b/src/lib/subtitle_content.cc @@ -49,13 +49,12 @@ int const SubtitleContentProperty::BURN = 505; int const SubtitleContentProperty::LANGUAGE = 506; int const SubtitleContentProperty::FONTS = 507; int const SubtitleContentProperty::COLOUR = 508; -int const SubtitleContentProperty::OUTLINE = 509; -int const SubtitleContentProperty::SHADOW = 510; -int const SubtitleContentProperty::EFFECT_COLOUR = 511; -int const SubtitleContentProperty::LINE_SPACING = 512; -int const SubtitleContentProperty::FADE_IN = 513; -int const SubtitleContentProperty::FADE_OUT = 514; -int const SubtitleContentProperty::OUTLINE_WIDTH = 515; +int const SubtitleContentProperty::EFFECT = 509; +int const SubtitleContentProperty::EFFECT_COLOUR = 510; +int const SubtitleContentProperty::LINE_SPACING = 511; +int const SubtitleContentProperty::FADE_IN = 512; +int const SubtitleContentProperty::FADE_OUT = 513; +int const SubtitleContentProperty::OUTLINE_WIDTH = 514; SubtitleContent::SubtitleContent (Content* parent) : ContentPart (parent) @@ -65,8 +64,7 @@ SubtitleContent::SubtitleContent (Content* parent) , _y_offset (0) , _x_scale (1) , _y_scale (1) - , _outline (false) - , _shadow (false) + , _effect (dcp::NONE) , _line_spacing (1) , _outline_width (2) { @@ -102,8 +100,6 @@ SubtitleContent::SubtitleContent (Content* parent, cxml::ConstNodePtr node, int , _y_offset (0) , _x_scale (1) , _y_scale (1) - , _outline (node->optional_bool_child("Outline").get_value_or(false)) - , _shadow (node->optional_bool_child("Shadow").get_value_or(false)) , _line_spacing (node->optional_number_child("LineSpacing").get_value_or (1)) , _fade_in (node->optional_number_child("SubtitleFadeIn").get_value_or (0)) , _fade_out (node->optional_number_child("SubtitleFadeOut").get_value_or (0)) @@ -121,6 +117,25 @@ SubtitleContent::SubtitleContent (Content* parent, cxml::ConstNodePtr node, int _y_offset = node->number_child ("SubtitleOffset"); } + if (node->optional_bool_child("Outline").get_value_or(false)) { + _effect = dcp::BORDER; + } else if (node->optional_bool_child("Shadow").get_value_or(false)) { + _effect = dcp::SHADOW; + } else { + _effect = dcp::NONE; + } + + optional effect = node->optional_string_child("Effect"); + if (effect) { + if (*effect == "none") { + _effect = dcp::NONE; + } else if (*effect == "outline") { + _effect = dcp::BORDER; + } else if (*effect == "shadow") { + _effect = dcp::SHADOW; + } + } + if (version >= 10) { _x_scale = node->number_child ("SubtitleXScale"); _y_scale = node->number_child ("SubtitleYScale"); @@ -256,8 +271,17 @@ SubtitleContent::as_xml (xmlpp::Node* root) const root->add_child("Green")->add_child_text (raw_convert (_colour->g)); root->add_child("Blue")->add_child_text (raw_convert (_colour->b)); } - root->add_child("Outline")->add_child_text (_outline ? "1" : "0"); - root->add_child("Shadow")->add_child_text (_shadow ? "1" : "0"); + switch (_effect) { + case dcp::NONE: + root->add_child("none"); + break; + case dcp::BORDER: + root->add_child("outline"); + break; + case dcp::SHADOW: + root->add_child("shadow"); + break; + } if (_effect_colour) { root->add_child("EffectRed")->add_child_text (raw_convert (_effect_colour->r)); root->add_child("EffectGreen")->add_child_text (raw_convert (_effect_colour->g)); @@ -341,15 +365,9 @@ SubtitleContent::unset_colour () } void -SubtitleContent::set_outline (bool o) -{ - maybe_set (_outline, o, SubtitleContentProperty::OUTLINE); -} - -void -SubtitleContent::set_shadow (bool s) +SubtitleContent::set_effect (dcp::Effect e) { - maybe_set (_shadow, s, SubtitleContentProperty::SHADOW); + maybe_set (_effect, e, SubtitleContentProperty::EFFECT); } void @@ -445,8 +463,7 @@ SubtitleContent::take_settings_from (shared_ptr c) } else { unset_colour (); } - set_outline (c->_outline); - set_shadow (c->_shadow); + set_effect (c->_effect); if (c->_effect_colour) { set_effect_colour (*c->_effect_colour); } else { diff --git a/src/lib/subtitle_content.h b/src/lib/subtitle_content.h index baf412bb6..73e53e446 100644 --- a/src/lib/subtitle_content.h +++ b/src/lib/subtitle_content.h @@ -40,8 +40,7 @@ public: static int const LANGUAGE; static int const FONTS; static int const COLOUR; - static int const OUTLINE; - static int const SHADOW; + static int const EFFECT; static int const EFFECT_COLOUR; static int const LINE_SPACING; static int const FADE_IN; @@ -76,8 +75,7 @@ public: void set_language (std::string language); void set_colour (dcp::Colour); void unset_colour (); - void set_outline (bool); - void set_shadow (bool); + void set_effect (dcp::Effect); void set_effect_colour (dcp::Colour); void unset_effect_colour (); void set_line_spacing (double s); @@ -130,14 +128,9 @@ public: return _colour; } - bool outline () const { + dcp::Effect effect () const { boost::mutex::scoped_lock lm (_mutex); - return _outline; - } - - bool shadow () const { - boost::mutex::scoped_lock lm (_mutex); - return _shadow; + return _effect; } boost::optional effect_colour () const { @@ -196,8 +189,7 @@ private: double _y_scale; std::list > _fonts; boost::optional _colour; - bool _outline; - bool _shadow; + dcp::Effect _effect; boost::optional _effect_colour; /** scaling factor for line spacing; 1 is "standard", < 1 is closer together, > 1 is further apart */ double _line_spacing; diff --git a/src/lib/subtitle_decoder.cc b/src/lib/subtitle_decoder.cc index 8badf5183..90bedf0ce 100644 --- a/src/lib/subtitle_decoder.cc +++ b/src/lib/subtitle_decoder.cc @@ -82,11 +82,7 @@ SubtitleDecoder::emit_text_start (ContentTime from, list s) if (content()->effect_colour()) { i.set_effect_colour (*content()->effect_colour()); } - if (content()->outline()) { - i.set_effect (dcp::BORDER); - } else if (content()->shadow()) { - i.set_effect (dcp::SHADOW); - } + i.set_effect (content()->effect()); i.set_fade_up_time (dcp::Time(content()->fade_in().seconds(), 1000)); i.set_fade_down_time (dcp::Time(content()->fade_out().seconds(), 1000)); } diff --git a/src/wx/subtitle_appearance_dialog.cc b/src/wx/subtitle_appearance_dialog.cc index 5cb668e7b..929bc6006 100644 --- a/src/wx/subtitle_appearance_dialog.cc +++ b/src/wx/subtitle_appearance_dialog.cc @@ -157,12 +157,16 @@ SubtitleAppearanceDialog::SubtitleAppearanceDialog (wxWindow* parent, shared_ptr _colour->SetColour (wxColour (255, 255, 255)); } - if (_content->subtitle->outline()) { + switch (_content->subtitle->effect()) { + case dcp::NONE: + _effect->SetSelection (NONE); + break; + case dcp::BORDER: _effect->SetSelection (OUTLINE); - } else if (_content->subtitle->shadow()) { + break; + case dcp::SHADOW: _effect->SetSelection (SHADOW); - } else { - _effect->SetSelection (NONE); + break; } optional effect_colour = _content->subtitle->effect_colour(); @@ -195,8 +199,17 @@ SubtitleAppearanceDialog::apply () } else { _content->subtitle->unset_colour (); } - _content->subtitle->set_outline (_effect->GetSelection() == OUTLINE); - _content->subtitle->set_shadow (_effect->GetSelection() == SHADOW); + switch (_effect->GetSelection()) { + case NONE: + _content->subtitle->set_effect (dcp::NONE); + break; + case OUTLINE: + _content->subtitle->set_effect (dcp::BORDER); + break; + case SHADOW: + _content->subtitle->set_effect (dcp::SHADOW); + break; + } if (_force_effect_colour->GetValue ()) { wxColour const ec = _effect_colour->GetColour (); _content->subtitle->set_effect_colour (dcp::Colour (ec.Red(), ec.Green(), ec.Blue())); diff --git a/test/ffmpeg_encoder_test.cc b/test/ffmpeg_encoder_test.cc index e6cff9b8f..df64a2c48 100644 --- a/test/ffmpeg_encoder_test.cc +++ b/test/ffmpeg_encoder_test.cc @@ -76,7 +76,7 @@ BOOST_AUTO_TEST_CASE (ffmpeg_encoder_test_subs_h264_1) film->examine_and_add_content (s); BOOST_REQUIRE (!wait_for_jobs ()); s->subtitle->set_colour (dcp::Colour (255, 255, 0)); - s->subtitle->set_shadow (true); + s->subtitle->set_effect (dcp::SHADOW); s->subtitle->set_effect_colour (dcp::Colour (0, 255, 255)); film->write_metadata(); @@ -101,7 +101,7 @@ BOOST_AUTO_TEST_CASE (ffmpeg_encoder_test_subs_h264_2) film->examine_and_add_content (s); BOOST_REQUIRE (!wait_for_jobs ()); s->subtitle->set_colour (dcp::Colour (255, 255, 0)); - s->subtitle->set_shadow (true); + s->subtitle->set_effect (dcp::SHADOW); s->subtitle->set_effect_colour (dcp::Colour (0, 255, 255)); film->write_metadata(); @@ -122,7 +122,7 @@ BOOST_AUTO_TEST_CASE (ffmpeg_encoder_test_subs_prores_1) film->examine_and_add_content (s); BOOST_REQUIRE (!wait_for_jobs ()); s->subtitle->set_colour (dcp::Colour (255, 255, 0)); - s->subtitle->set_shadow (true); + s->subtitle->set_effect (dcp::SHADOW); s->subtitle->set_effect_colour (dcp::Colour (0, 255, 255)); film->write_metadata(); @@ -147,7 +147,7 @@ BOOST_AUTO_TEST_CASE (ffmpeg_encoder_test_subs_prores_2) film->examine_and_add_content (s); BOOST_REQUIRE (!wait_for_jobs ()); s->subtitle->set_colour (dcp::Colour (255, 255, 0)); - s->subtitle->set_shadow (true); + s->subtitle->set_effect (dcp::SHADOW); s->subtitle->set_effect_colour (dcp::Colour (0, 255, 255)); shared_ptr job (new TranscodeJob (film)); -- 2.30.2