Work around deadlock when destroying J2KEncoder with a full writer queue (#2784).

[dcpomatic.git] / src / lib / j2k_encoder.cc

diff --git a/src/lib/j2k_encoder.cc b/src/lib/j2k_encoder.cc
index e6dab582801fd7177ecea144d294510261c9f41a..32d2fefc2830c86fec1f057179c70d5dd74ef7e6 100644 (file)
--- a/src/lib/j2k_encoder.cc
+++ b/src/lib/j2k_encoder.cc
@@ -57,17 +57,28 @@ using namespace dcpomatic;
 /** @param film Film that we are encoding.
  *  @param writer Writer that we are using.
  */
-J2KEncoder::J2KEncoder (shared_ptr<const Film> film, shared_ptr<Writer> writer)
+J2KEncoder::J2KEncoder(shared_ptr<const Film> film, Writer& writer)
        : _film (film)
        , _history (200)
        , _writer (writer)
 {
-       servers_list_changed ();
 }
 
 
 J2KEncoder::~J2KEncoder ()
 {
+       _server_found_connection.disconnect();
+
+       /* One of our encoder threads may be waiting on Writer::write() to return, if that method
+        * is blocked with the writer queue full waiting for _full_condition.  In that case, the
+        * attempt to terminate the encoder threads below (in terminate_threads()) will fail because
+        * the encoder thread waiting for ::write() will have interruption disabled.
+        *
+        * To work around that, make the writer into a zombie to unblock any pending write()s and
+        * not block on any future ones.
+        */
+       _writer.zombify();
+
        boost::mutex::scoped_lock lm (_threads_mutex);
        terminate_threads ();
 }
@@ -76,24 +87,10 @@ J2KEncoder::~J2KEncoder ()
 void
 J2KEncoder::begin ()
 {
-       weak_ptr<J2KEncoder> wp = shared_from_this ();
-       _server_found_connection = EncodeServerFinder::instance()->ServersListChanged.connect (
-               boost::bind (&J2KEncoder::call_servers_list_changed, wp)
+       _server_found_connection = EncodeServerFinder::instance()->ServersListChanged.connect(
+               boost::bind(&J2KEncoder::servers_list_changed, this)
                );
-}
-
-
-/* We don't want the servers-list-changed callback trying to do things
-   during destruction of J2KEncoder, and I think this is the neatest way
-   to achieve that.
-*/
-void
-J2KEncoder::call_servers_list_changed (weak_ptr<J2KEncoder> encoder)
-{
-       auto e = encoder.lock ();
-       if (e) {
-               e->servers_list_changed ();
-       }
+       servers_list_changed ();
 }
 
 
@@ -137,7 +134,7 @@ J2KEncoder::end ()
        for (auto const& i: _queue) {
                LOG_GENERAL(N_("Encode left-over frame %1"), i.index());
                try {
-                       _writer->write (
+                       _writer.write(
                                make_shared<dcp::ArrayData>(i.encode_locally()),
                                i.index(),
                                i.eyes()
@@ -210,7 +207,7 @@ J2KEncoder::encode (shared_ptr<PlayerVideo> pv, DCPTime time)
                LOG_TIMING ("decoder-wake queue=%1 threads=%2", _queue.size(), threads);
        }
 
-       _writer->rethrow ();
+       _writer.rethrow();
        /* Re-throw any exception raised by one of our threads.  If more
           than one has thrown an exception, only one will be rethrown, I think;
           but then, if that happens something has gone badly wrong.
@@ -219,19 +216,19 @@ J2KEncoder::encode (shared_ptr<PlayerVideo> pv, DCPTime time)
 
        auto const position = time.frames_floor(_film->video_frame_rate());
 
-       if (_writer->can_fake_write (position)) {
+       if (_writer.can_fake_write(position)) {
                /* We can fake-write this frame */
                LOG_DEBUG_ENCODE("Frame @ %1 FAKE", to_string(time));
-               _writer->fake_write (position, pv->eyes ());
+               _writer.fake_write(position, pv->eyes ());
                frame_done ();
        } else if (pv->has_j2k() && !_film->reencode_j2k()) {
                LOG_DEBUG_ENCODE("Frame @ %1 J2K", to_string(time));
                /* This frame already has J2K data, so just write it */
-               _writer->write (pv->j2k(), position, pv->eyes ());
+               _writer.write(pv->j2k(), position, pv->eyes ());
                frame_done ();
-       } else if (_last_player_video[static_cast<int>(pv->eyes())] && _writer->can_repeat(position) && pv->same (_last_player_video[static_cast<int>(pv->eyes())])) {
+       } else if (_last_player_video[pv->eyes()] && _writer.can_repeat(position) && pv->same(_last_player_video[pv->eyes()])) {
                LOG_DEBUG_ENCODE("Frame @ %1 REPEAT", to_string(time));
-               _writer->repeat (position, pv->eyes ());
+               _writer.repeat(position, pv->eyes());
        } else {
                LOG_DEBUG_ENCODE("Frame @ %1 ENCODE", to_string(time));
                /* Queue this new frame for encoding */
@@ -250,7 +247,7 @@ J2KEncoder::encode (shared_ptr<PlayerVideo> pv, DCPTime time)
                _empty_condition.notify_all ();
        }
 
-       _last_player_video[static_cast<int>(pv->eyes())] = pv;
+       _last_player_video[pv->eyes()] = pv;
        _last_player_video_time = time;
 }
 
@@ -357,7 +354,7 @@ try
                        }
 
                        if (encoded) {
-                               _writer->write (encoded, vf.index(), vf.eyes());
+                               _writer.write(encoded, vf.index(), vf.eyes());
                                frame_done ();
                        } else {
                                lock.lock ();
@@ -420,5 +417,5 @@ J2KEncoder::servers_list_changed ()
                }
        }
 
-       _writer->set_encoder_threads (_threads->size());
+       _writer.set_encoder_threads(_threads->size());
 }