Handle multiple audio streams in a single piece of content
[dcpomatic.git] / src / lib / ffmpeg_decoder.cc
index 7d152e4904a32682a8c64f0e84ee4d9470efd280..b7516f6d2d7a7cb005e08dd617e4b0a7f370146f 100644 (file)
@@ -1,5 +1,5 @@
 /*
 /*
-    Copyright (C) 2012-2014 Carl Hetherington <cth@carlh.net>
+    Copyright (C) 2012-2015 Carl Hetherington <cth@carlh.net>
 
     This program is free software; you can redistribute it and/or modify
     it under the terms of the GNU General Public License as published by
 
     This program is free software; you can redistribute it and/or modify
     it under the terms of the GNU General Public License as published by
  *  @brief A decoder using FFmpeg to decode content.
  */
 
  *  @brief A decoder using FFmpeg to decode content.
  */
 
-#include <stdexcept>
-#include <vector>
-#include <sstream>
-#include <iomanip>
-#include <iostream>
-#include <stdint.h>
-#include <sndfile.h>
-extern "C" {
-#include <libavcodec/avcodec.h>
-#include <libavformat/avformat.h>
-}
 #include "filter.h"
 #include "exceptions.h"
 #include "image.h"
 #include "filter.h"
 #include "exceptions.h"
 #include "image.h"
@@ -43,17 +32,36 @@ extern "C" {
 #include "filter_graph.h"
 #include "audio_buffers.h"
 #include "ffmpeg_content.h"
 #include "filter_graph.h"
 #include "audio_buffers.h"
 #include "ffmpeg_content.h"
-#include "image_proxy.h"
+#include "raw_image_proxy.h"
+#include "film.h"
+#include "timer.h"
+extern "C" {
+#include <libavcodec/avcodec.h>
+#include <libavformat/avformat.h>
+}
+#include <boost/foreach.hpp>
+#include <stdexcept>
+#include <vector>
+#include <iomanip>
+#include <iostream>
+#include <stdint.h>
+#include <sndfile.h>
 
 #include "i18n.h"
 
 
 #include "i18n.h"
 
+#define LOG_GENERAL(...) _video_content->film()->log()->log (String::compose (__VA_ARGS__), Log::TYPE_GENERAL);
+#define LOG_ERROR(...) _video_content->film()->log()->log (String::compose (__VA_ARGS__), Log::TYPE_ERROR);
+#define LOG_WARNING_NC(...) _video_content->film()->log()->log (__VA_ARGS__, Log::TYPE_WARNING);
+#define LOG_WARNING(...) _video_content->film()->log()->log (String::compose (__VA_ARGS__), Log::TYPE_WARNING);
+
 using std::cout;
 using std::string;
 using std::vector;
 using std::cout;
 using std::string;
 using std::vector;
-using std::stringstream;
 using std::list;
 using std::min;
 using std::pair;
 using std::list;
 using std::min;
 using std::pair;
+using std::make_pair;
+using std::max;
 using boost::shared_ptr;
 using boost::optional;
 using boost::dynamic_pointer_cast;
 using boost::shared_ptr;
 using boost::optional;
 using boost::dynamic_pointer_cast;
@@ -62,10 +70,9 @@ using dcp::Size;
 FFmpegDecoder::FFmpegDecoder (shared_ptr<const FFmpegContent> c, shared_ptr<Log> log)
        : VideoDecoder (c)
        , AudioDecoder (c)
 FFmpegDecoder::FFmpegDecoder (shared_ptr<const FFmpegContent> c, shared_ptr<Log> log)
        : VideoDecoder (c)
        , AudioDecoder (c)
+       , SubtitleDecoder (c)
        , FFmpeg (c)
        , _log (log)
        , FFmpeg (c)
        , _log (log)
-       , _subtitle_codec_context (0)
-       , _subtitle_codec (0)
 {
        /* Audio and video frame PTS values may not start with 0.  We want
           to fiddle them so that:
 {
        /* Audio and video frame PTS values may not start with 0.  We want
           to fiddle them so that:
@@ -76,36 +83,40 @@ FFmpegDecoder::FFmpegDecoder (shared_ptr<const FFmpegContent> c, shared_ptr<Log>
           Then we remove big initial gaps in PTS and we allow our
           insertion of black frames to work.
 
           Then we remove big initial gaps in PTS and we allow our
           insertion of black frames to work.
 
-          We will do pts_to_use = pts_from_ffmpeg + pts_offset;
+          We will do:
+            audio_pts_to_use = audio_pts_from_ffmpeg + pts_offset;
+            video_pts_to_use = video_pts_from_ffmpeg + pts_offset;
        */
 
        */
 
-       bool const have_video = c->first_video();
-       bool const have_audio = c->audio_stream () && c->audio_stream()->first_audio;
-
        /* First, make one of them start at 0 */
 
        /* First, make one of them start at 0 */
 
-       if (have_audio && have_video) {
-               _pts_offset = - min (c->first_video().get(), c->audio_stream()->first_audio.get());
-       } else if (have_video) {
-               _pts_offset = - c->first_video().get();
-       } else if (have_audio) {
-               _pts_offset = - c->audio_stream()->first_audio.get();
+       vector<shared_ptr<FFmpegAudioStream> > streams = c->ffmpeg_audio_streams ();
+
+       _pts_offset = ContentTime::min ();
+
+       if (c->first_video ()) {
+               _pts_offset = - c->first_video().get ();
        }
 
        }
 
-       /* Now adjust both so that the video pts starts on a frame */
-       if (have_video && have_audio) {
-               ContentTime first_video = c->first_video().get() + _pts_offset;
-               ContentTime const old_first_video = first_video;
-               _pts_offset += first_video.round_up (c->video_frame_rate ()) - old_first_video;
+       BOOST_FOREACH (shared_ptr<FFmpegAudioStream> i, streams) {
+               if (i->first_audio) {
+                       _pts_offset = max (_pts_offset, - i->first_audio.get ());
+               }
        }
        }
-}
 
 
-FFmpegDecoder::~FFmpegDecoder ()
-{
-       boost::mutex::scoped_lock lm (_mutex);
+       /* If _pts_offset is positive we would be pushing things from a -ve PTS to be played.
+          I don't think we ever want to do that, as it seems things at -ve PTS are not meant
+          to be seen (use for alignment bars etc.); see mantis #418.
+       */
+       if (_pts_offset > ContentTime ()) {
+               _pts_offset = ContentTime ();
+       }
 
 
-       if (_subtitle_codec_context) {
-               avcodec_close (_subtitle_codec_context);
+       /* Now adjust so that the video pts starts on a frame */
+       if (c->first_video ()) {
+               ContentTime first_video = c->first_video().get() + _pts_offset;
+               ContentTime const old_first_video = first_video;
+               _pts_offset += first_video.round_up (c->video_frame_rate ()) - old_first_video;
        }
 }
 
        }
 }
 
@@ -121,36 +132,39 @@ FFmpegDecoder::flush ()
        
        while (decode_video_packet ()) {}
        
        
        while (decode_video_packet ()) {}
        
-       if (_ffmpeg_content->audio_stream()) {
-               decode_audio_packet ();
-               AudioDecoder::flush ();
-       }
+       decode_audio_packet ();
+       AudioDecoder::flush ();
 }
 
 bool
 }
 
 bool
-FFmpegDecoder::pass ()
+FFmpegDecoder::pass (PassReason reason)
 {
        int r = av_read_frame (_format_context, &_packet);
 
 {
        int r = av_read_frame (_format_context, &_packet);
 
-       if (r < 0) {
+       /* AVERROR_INVALIDDATA can apparently be returned sometimes even when av_read_frame
+          has pretty-much succeeded (and hence generated data which should be processed).
+          Hence it makes sense to continue here in that case.
+       */
+       if (r < 0 && r != AVERROR_INVALIDDATA) {
                if (r != AVERROR_EOF) {
                        /* Maybe we should fail here, but for now we'll just finish off instead */
                        char buf[256];
                        av_strerror (r, buf, sizeof(buf));
                if (r != AVERROR_EOF) {
                        /* Maybe we should fail here, but for now we'll just finish off instead */
                        char buf[256];
                        av_strerror (r, buf, sizeof(buf));
-                       _log->log (String::compose (N_("error on av_read_frame (%1) (%2)"), buf, r));
+                       LOG_ERROR (N_("error on av_read_frame (%1) (%2)"), buf, r);
                }
                }
-
+               
                flush ();
                return true;
        }
 
        int const si = _packet.stream_index;
                flush ();
                return true;
        }
 
        int const si = _packet.stream_index;
-       
-       if (si == _video_stream) {
+       shared_ptr<const FFmpegContent> fc = _ffmpeg_content;
+
+       if (si == _video_stream && !_ignore_video && reason != PASS_REASON_SUBTITLE) {
                decode_video_packet ();
                decode_video_packet ();
-       } else if (_ffmpeg_content->audio_stream() && _ffmpeg_content->audio_stream()->uses_index (_format_context, si)) {
+       } else if (reason != PASS_REASON_SUBTITLE) {
                decode_audio_packet ();
                decode_audio_packet ();
-       } else if (_ffmpeg_content->subtitle_stream() && _ffmpeg_content->subtitle_stream()->uses_index (_format_context, si)) {
+       } else if (fc->subtitle_stream() && fc->subtitle_stream()->uses_index (_format_context, si)) {
                decode_subtitle_packet ();
        }
 
                decode_subtitle_packet ();
        }
 
@@ -162,20 +176,20 @@ FFmpegDecoder::pass ()
  *  Only the first buffer will be used for non-planar data, otherwise there will be one per channel.
  */
 shared_ptr<AudioBuffers>
  *  Only the first buffer will be used for non-planar data, otherwise there will be one per channel.
  */
 shared_ptr<AudioBuffers>
-FFmpegDecoder::deinterleave_audio (uint8_t** data, int size)
+FFmpegDecoder::deinterleave_audio (shared_ptr<FFmpegAudioStream> stream, uint8_t** data, int size)
 {
 {
-       assert (_ffmpeg_content->audio_channels());
-       assert (bytes_per_audio_sample());
+       DCPOMATIC_ASSERT (bytes_per_audio_sample (stream));
 
        /* Deinterleave and convert to float */
 
 
        /* Deinterleave and convert to float */
 
-       assert ((size % (bytes_per_audio_sample() * _ffmpeg_content->audio_channels())) == 0);
-
-       int const total_samples = size / bytes_per_audio_sample();
-       int const frames = total_samples / _ffmpeg_content->audio_channels();
-       shared_ptr<AudioBuffers> audio (new AudioBuffers (_ffmpeg_content->audio_channels(), frames));
+       /* total_samples and frames will be rounded down here, so if there are stray samples at the end
+          of the block that do not form a complete sample or frame they will be dropped.
+       */
+       int const total_samples = size / bytes_per_audio_sample (stream);
+       int const frames = total_samples / stream->channels();
+       shared_ptr<AudioBuffers> audio (new AudioBuffers (stream->channels(), frames));
 
 
-       switch (audio_sample_format()) {
+       switch (audio_sample_format (stream)) {
        case AV_SAMPLE_FMT_U8:
        {
                uint8_t* p = reinterpret_cast<uint8_t *> (data[0]);
        case AV_SAMPLE_FMT_U8:
        {
                uint8_t* p = reinterpret_cast<uint8_t *> (data[0]);
@@ -185,7 +199,7 @@ FFmpegDecoder::deinterleave_audio (uint8_t** data, int size)
                        audio->data(channel)[sample] = float(*p++) / (1 << 23);
 
                        ++channel;
                        audio->data(channel)[sample] = float(*p++) / (1 << 23);
 
                        ++channel;
-                       if (channel == _ffmpeg_content->audio_channels()) {
+                       if (channel == stream->channels()) {
                                channel = 0;
                                ++sample;
                        }
                                channel = 0;
                                ++sample;
                        }
@@ -202,7 +216,7 @@ FFmpegDecoder::deinterleave_audio (uint8_t** data, int size)
                        audio->data(channel)[sample] = float(*p++) / (1 << 15);
 
                        ++channel;
                        audio->data(channel)[sample] = float(*p++) / (1 << 15);
 
                        ++channel;
-                       if (channel == _ffmpeg_content->audio_channels()) {
+                       if (channel == stream->channels()) {
                                channel = 0;
                                ++sample;
                        }
                                channel = 0;
                                ++sample;
                        }
@@ -213,7 +227,7 @@ FFmpegDecoder::deinterleave_audio (uint8_t** data, int size)
        case AV_SAMPLE_FMT_S16P:
        {
                int16_t** p = reinterpret_cast<int16_t **> (data);
        case AV_SAMPLE_FMT_S16P:
        {
                int16_t** p = reinterpret_cast<int16_t **> (data);
-               for (int i = 0; i < _ffmpeg_content->audio_channels(); ++i) {
+               for (int i = 0; i < stream->channels(); ++i) {
                        for (int j = 0; j < frames; ++j) {
                                audio->data(i)[j] = static_cast<float>(p[i][j]) / (1 << 15);
                        }
                        for (int j = 0; j < frames; ++j) {
                                audio->data(i)[j] = static_cast<float>(p[i][j]) / (1 << 15);
                        }
@@ -230,7 +244,7 @@ FFmpegDecoder::deinterleave_audio (uint8_t** data, int size)
                        audio->data(channel)[sample] = static_cast<float>(*p++) / (1 << 31);
 
                        ++channel;
                        audio->data(channel)[sample] = static_cast<float>(*p++) / (1 << 31);
 
                        ++channel;
-                       if (channel == _ffmpeg_content->audio_channels()) {
+                       if (channel == stream->channels()) {
                                channel = 0;
                                ++sample;
                        }
                                channel = 0;
                                ++sample;
                        }
@@ -247,7 +261,7 @@ FFmpegDecoder::deinterleave_audio (uint8_t** data, int size)
                        audio->data(channel)[sample] = *p++;
 
                        ++channel;
                        audio->data(channel)[sample] = *p++;
 
                        ++channel;
-                       if (channel == _ffmpeg_content->audio_channels()) {
+                       if (channel == stream->channels()) {
                                channel = 0;
                                ++sample;
                        }
                                channel = 0;
                                ++sample;
                        }
@@ -258,127 +272,29 @@ FFmpegDecoder::deinterleave_audio (uint8_t** data, int size)
        case AV_SAMPLE_FMT_FLTP:
        {
                float** p = reinterpret_cast<float**> (data);
        case AV_SAMPLE_FMT_FLTP:
        {
                float** p = reinterpret_cast<float**> (data);
-               for (int i = 0; i < _ffmpeg_content->audio_channels(); ++i) {
+               for (int i = 0; i < stream->channels(); ++i) {
                        memcpy (audio->data(i), p[i], frames * sizeof(float));
                }
        }
        break;
 
        default:
                        memcpy (audio->data(i), p[i], frames * sizeof(float));
                }
        }
        break;
 
        default:
-               throw DecodeError (String::compose (_("Unrecognised audio sample format (%1)"), static_cast<int> (audio_sample_format())));
+               throw DecodeError (String::compose (_("Unrecognised audio sample format (%1)"), static_cast<int> (audio_sample_format (stream))));
        }
 
        return audio;
 }
 
 AVSampleFormat
        }
 
        return audio;
 }
 
 AVSampleFormat
-FFmpegDecoder::audio_sample_format () const
+FFmpegDecoder::audio_sample_format (shared_ptr<FFmpegAudioStream> stream) const
 {
 {
-       if (!_ffmpeg_content->audio_stream()) {
-               return (AVSampleFormat) 0;
-       }
-       
-       return audio_codec_context()->sample_fmt;
+       return stream->stream (_format_context)->codec->sample_fmt;
 }
 
 int
 }
 
 int
-FFmpegDecoder::bytes_per_audio_sample () const
+FFmpegDecoder::bytes_per_audio_sample (shared_ptr<FFmpegAudioStream> stream) const
 {
 {
-       return av_get_bytes_per_sample (audio_sample_format ());
-}
-
-int
-FFmpegDecoder::minimal_run (boost::function<bool (optional<ContentTime>, optional<ContentTime>, int)> finished)
-{
-       int frames_read = 0;
-       optional<ContentTime> last_video;
-       optional<ContentTime> last_audio;
-
-       while (!finished (last_video, last_audio, frames_read)) {
-               int r = av_read_frame (_format_context, &_packet);
-               if (r < 0) {
-                       /* We should flush our decoders here, possibly yielding a few more frames,
-                          but the consequence of having to do that is too hideous to contemplate.
-                          Instead we give up and say that you can't seek too close to the end
-                          of a file.
-                       */
-                       return frames_read;
-               }
-
-               ++frames_read;
-
-               double const time_base = av_q2d (_format_context->streams[_packet.stream_index]->time_base);
-
-               if (_packet.stream_index == _video_stream) {
-
-                       avcodec_get_frame_defaults (_frame);
-                       
-                       int got_picture = 0;
-                       r = avcodec_decode_video2 (video_codec_context(), _frame, &got_picture, &_packet);
-                       if (r >= 0 && got_picture) {
-                               last_video = ContentTime::from_seconds (av_frame_get_best_effort_timestamp (_frame) * time_base) + _pts_offset;
-                       }
-
-               } else if (_ffmpeg_content->audio_stream() && _ffmpeg_content->audio_stream()->uses_index (_format_context, _packet.stream_index)) {
-                       AVPacket copy_packet = _packet;
-                       while (copy_packet.size > 0) {
-
-                               int got_frame;
-                               r = avcodec_decode_audio4 (audio_codec_context(), _frame, &got_frame, &_packet);
-                               if (r >= 0 && got_frame) {
-                                       last_audio = ContentTime::from_seconds (av_frame_get_best_effort_timestamp (_frame) * time_base) + _pts_offset;
-                               }
-                                       
-                               copy_packet.data += r;
-                               copy_packet.size -= r;
-                       }
-               }
-               
-               av_free_packet (&_packet);
-       }
-
-       return frames_read;
-}
-
-bool
-FFmpegDecoder::seek_overrun_finished (ContentTime seek, optional<ContentTime> last_video, optional<ContentTime> last_audio) const
-{
-       return (last_video && last_video.get() >= seek) || (last_audio && last_audio.get() >= seek);
-}
-
-bool
-FFmpegDecoder::seek_final_finished (int n, int done) const
-{
-       return n == done;
-}
-
-void
-FFmpegDecoder::seek_and_flush (ContentTime t)
-{
-       ContentTime const u = t - _pts_offset;
-       int64_t s = u.seconds() / av_q2d (_format_context->streams[_video_stream]->time_base);
-
-       if (_ffmpeg_content->audio_stream ()) {
-               s = min (
-                       s, int64_t (u.seconds() / av_q2d (_ffmpeg_content->audio_stream()->stream(_format_context)->time_base))
-                       );
-       }
-
-       /* Ridiculous empirical hack */
-       s--;
-       if (s < 0) {
-               s = 0;
-       }
-
-       av_seek_frame (_format_context, _video_stream, s, 0);
-
-       avcodec_flush_buffers (video_codec_context());
-       if (audio_codec_context ()) {
-               avcodec_flush_buffers (audio_codec_context ());
-       }
-       if (_subtitle_codec_context) {
-               avcodec_flush_buffers (_subtitle_codec_context);
-       }
+       return av_get_bytes_per_sample (audio_sample_format (stream));
 }
 
 void
 }
 
 void
@@ -386,32 +302,28 @@ FFmpegDecoder::seek (ContentTime time, bool accurate)
 {
        VideoDecoder::seek (time, accurate);
        AudioDecoder::seek (time, accurate);
 {
        VideoDecoder::seek (time, accurate);
        AudioDecoder::seek (time, accurate);
-       
-       /* If we are doing an accurate seek, our initial shot will be 2s (2 being
-          a number plucked from the air) earlier than we want to end up.  The loop below
-          will hopefully then step through to where we want to be.
+       SubtitleDecoder::seek (time, accurate);
+
+       /* If we are doing an `accurate' seek, we need to use pre-roll, as
+          we don't really know what the seek will give us.
        */
 
        ContentTime pre_roll = accurate ? ContentTime::from_seconds (2) : ContentTime (0);
        */
 
        ContentTime pre_roll = accurate ? ContentTime::from_seconds (2) : ContentTime (0);
-       ContentTime initial_seek = time - pre_roll;
-       if (initial_seek < ContentTime (0)) {
-               initial_seek = ContentTime (0);
-       }
+       time -= pre_roll;
 
 
-       /* Initial seek time in the video stream's timebase */
-
-       seek_and_flush (initial_seek);
-
-       if (!accurate) {
-               /* That'll do */
-               return;
-       }
+       /* XXX: it seems debatable whether PTS should be used here...
+          http://www.mjbshaw.com/2012/04/seeking-in-ffmpeg-know-your-timestamp.html
+       */
+       
+       ContentTime const u = time - _pts_offset;
+       av_seek_frame (_format_context, _video_stream, u.seconds() / av_q2d (_format_context->streams[_video_stream]->time_base), 0);
 
 
-       int const N = minimal_run (boost::bind (&FFmpegDecoder::seek_overrun_finished, this, time, _1, _2));
+       avcodec_flush_buffers (video_codec_context());
 
 
-       seek_and_flush (initial_seek);
-       if (N > 0) {
-               minimal_run (boost::bind (&FFmpegDecoder::seek_final_finished, this, N - 1, _3));
+       /* XXX: should be flushing audio buffers? */
+       
+       if (subtitle_codec_context ()) {
+               avcodec_flush_buffers (subtitle_codec_context ());
        }
 }
 
        }
 }
 
@@ -423,28 +335,50 @@ FFmpegDecoder::decode_audio_packet ()
        */
        
        AVPacket copy_packet = _packet;
        */
        
        AVPacket copy_packet = _packet;
+
+       /* XXX: inefficient */
+       vector<shared_ptr<FFmpegAudioStream> > streams = ffmpeg_content()->ffmpeg_audio_streams ();
+       vector<shared_ptr<FFmpegAudioStream> >::const_iterator stream = streams.begin ();
+       while (stream != streams.end () && !(*stream)->uses_index (_format_context, copy_packet.stream_index)) {
+               ++stream;
+       }
+
+       if (stream == streams.end ()) {
+               /* The packet's stream may not be an audio one; just ignore it in this method if so */
+               return;
+       }
        
        while (copy_packet.size > 0) {
 
                int frame_finished;
        
        while (copy_packet.size > 0) {
 
                int frame_finished;
-               int const decode_result = avcodec_decode_audio4 (audio_codec_context(), _frame, &frame_finished, &copy_packet);
-
+               int decode_result = avcodec_decode_audio4 ((*stream)->stream (_format_context)->codec, _frame, &frame_finished, &copy_packet);
                if (decode_result < 0) {
                if (decode_result < 0) {
-                       _log->log (String::compose ("avcodec_decode_audio4 failed (%1)", decode_result));
-                       return;
+                       /* avcodec_decode_audio4 can sometimes return an error even though it has decoded
+                          some valid data; for example dca_subframe_footer can return AVERROR_INVALIDDATA
+                          if it overreads the auxiliary data.  ffplay carries on if frame_finished is true,
+                          even in the face of such an error, so I think we should too.
+
+                          Returning from the method here caused mantis #352.
+                       */
+                       LOG_WARNING ("avcodec_decode_audio4 failed (%1)", decode_result);
+
+                       /* Fudge decode_result so that we come out of the while loop when
+                          we've processed this data.
+                       */
+                       decode_result = copy_packet.size;
                }
 
                if (frame_finished) {
                        ContentTime const ct = ContentTime::from_seconds (
                                av_frame_get_best_effort_timestamp (_frame) *
                }
 
                if (frame_finished) {
                        ContentTime const ct = ContentTime::from_seconds (
                                av_frame_get_best_effort_timestamp (_frame) *
-                               av_q2d (_ffmpeg_content->audio_stream()->stream (_format_context)->time_base))
+                               av_q2d ((*stream)->stream (_format_context)->time_base))
                                + _pts_offset;
                        
                        int const data_size = av_samples_get_buffer_size (
                                + _pts_offset;
                        
                        int const data_size = av_samples_get_buffer_size (
-                               0, audio_codec_context()->channels, _frame->nb_samples, audio_sample_format (), 1
+                               0, (*stream)->stream(_format_context)->codec->channels, _frame->nb_samples, audio_sample_format (*stream), 1
                                );
 
                                );
 
-                       audio (deinterleave_audio (_frame->data, data_size), ct);
+                       audio (*stream, deinterleave_audio (*stream, _frame->data, data_size), ct);
                }
                        
                copy_packet.data += decode_result;
                }
                        
                copy_packet.data += decode_result;
@@ -472,7 +406,7 @@ FFmpegDecoder::decode_video_packet ()
        if (i == _filter_graphs.end ()) {
                graph.reset (new FilterGraph (_ffmpeg_content, dcp::Size (_frame->width, _frame->height), (AVPixelFormat) _frame->format));
                _filter_graphs.push_back (graph);
        if (i == _filter_graphs.end ()) {
                graph.reset (new FilterGraph (_ffmpeg_content, dcp::Size (_frame->width, _frame->height), (AVPixelFormat) _frame->format));
                _filter_graphs.push_back (graph);
-               _log->log (String::compose (N_("New graph for %1x%2, pixel format %3"), _frame->width, _frame->height, _frame->format));
+               LOG_GENERAL (N_("New graph for %1x%2, pixel format %3"), _frame->width, _frame->height, _frame->format);
        } else {
                graph = *i;
        }
        } else {
                graph = *i;
        }
@@ -485,9 +419,12 @@ FFmpegDecoder::decode_video_packet ()
                
                if (i->second != AV_NOPTS_VALUE) {
                        double const pts = i->second * av_q2d (_format_context->streams[_video_stream]->time_base) + _pts_offset.seconds ();
                
                if (i->second != AV_NOPTS_VALUE) {
                        double const pts = i->second * av_q2d (_format_context->streams[_video_stream]->time_base) + _pts_offset.seconds ();
-                       video (shared_ptr<ImageProxy> (new RawImageProxy (image)), rint (pts * _ffmpeg_content->video_frame_rate ()));
+                       video (
+                               shared_ptr<ImageProxy> (new RawImageProxy (image)),
+                               rint (pts * _ffmpeg_content->video_frame_rate ())
+                               );
                } else {
                } else {
-                       _log->log ("Dropping frame without PTS");
+                       LOG_WARNING_NC ("Dropping frame without PTS");
                }
        }
 
                }
        }
 
@@ -499,43 +436,72 @@ FFmpegDecoder::decode_subtitle_packet ()
 {
        int got_subtitle;
        AVSubtitle sub;
 {
        int got_subtitle;
        AVSubtitle sub;
-       if (avcodec_decode_subtitle2 (_subtitle_codec_context, &sub, &got_subtitle, &_packet) < 0 || !got_subtitle) {
+       if (avcodec_decode_subtitle2 (subtitle_codec_context(), &sub, &got_subtitle, &_packet) < 0 || !got_subtitle) {
                return;
        }
                return;
        }
-
-       /* Sometimes we get an empty AVSubtitle, which is used by some codecs to
-          indicate that the previous subtitle should stop.
-       */
+       
        if (sub.num_rects <= 0) {
        if (sub.num_rects <= 0) {
-               image_subtitle (ContentTimePeriod (), shared_ptr<Image> (), dcpomatic::Rect<double> ());
+               /* Sometimes we get an empty AVSubtitle, which is used by some codecs to
+                  indicate that the previous subtitle should stop.  We can ignore it here.
+               */
                return;
        } else if (sub.num_rects > 1) {
                throw DecodeError (_("multi-part subtitles not yet supported"));
        }
                return;
        } else if (sub.num_rects > 1) {
                throw DecodeError (_("multi-part subtitles not yet supported"));
        }
-               
+
        /* Subtitle PTS (within the source, not taking into account any of the
        /* Subtitle PTS (within the source, not taking into account any of the
-          source that we may have chopped off for the DCP)
+          source that we may have chopped off for the DCP).
        */
        */
-       ContentTime packet_time = ContentTime::from_seconds (static_cast<double> (sub.pts) / AV_TIME_BASE) + _pts_offset;
-
-       ContentTimePeriod period (
-               packet_time + ContentTime::from_seconds (sub.start_display_time / 1e3),
-               packet_time + ContentTime::from_seconds (sub.end_display_time / 1e3)
-               );
-
+       FFmpegSubtitlePeriod sub_period = subtitle_period (sub);
+       ContentTimePeriod period;
+       period.from = sub_period.from + _pts_offset;
+       if (sub_period.to) {
+               /* We already know the subtitle period `to' time */
+               period.to = sub_period.to.get() + _pts_offset;
+       } else {
+               /* We have to look up the `to' time in the stream's records */
+               period.to = ffmpeg_content()->subtitle_stream()->find_subtitle_to (sub_period.from);
+       }
+       
        AVSubtitleRect const * rect = sub.rects[0];
 
        AVSubtitleRect const * rect = sub.rects[0];
 
-       if (rect->type != SUBTITLE_BITMAP) {
-               /* XXX */
-               // throw DecodeError (_("non-bitmap subtitles not yet supported"));
-               return;
+       switch (rect->type) {
+       case SUBTITLE_NONE:
+               break;
+       case SUBTITLE_BITMAP:
+               decode_bitmap_subtitle (rect, period);
+               break;
+       case SUBTITLE_TEXT:
+               cout << "XXX: SUBTITLE_TEXT " << rect->text << "\n";
+               break;
+       case SUBTITLE_ASS:
+               cout << "XXX: SUBTITLE_ASS " << rect->ass << "\n";
+               break;
        }
        }
+       
+       avsubtitle_free (&sub);
+}
 
 
+list<ContentTimePeriod>
+FFmpegDecoder::image_subtitles_during (ContentTimePeriod p, bool starting) const
+{
+       return _ffmpeg_content->subtitles_during (p, starting);
+}
+
+list<ContentTimePeriod>
+FFmpegDecoder::text_subtitles_during (ContentTimePeriod, bool) const
+{
+       return list<ContentTimePeriod> ();
+}
+
+void
+FFmpegDecoder::decode_bitmap_subtitle (AVSubtitleRect const * rect, ContentTimePeriod period)
+{
        /* Note RGBA is expressed little-endian, so the first byte in the word is R, second
           G, third B, fourth A.
        */
        shared_ptr<Image> image (new Image (PIX_FMT_RGBA, dcp::Size (rect->w, rect->h), true));
        /* Note RGBA is expressed little-endian, so the first byte in the word is R, second
           G, third B, fourth A.
        */
        shared_ptr<Image> image (new Image (PIX_FMT_RGBA, dcp::Size (rect->w, rect->h), true));
-
+       
        /* Start of the first line in the subtitle */
        uint8_t* sub_p = rect->pict.data[0];
        /* sub_p looks up into a BGRA palette which is here
        /* Start of the first line in the subtitle */
        uint8_t* sub_p = rect->pict.data[0];
        /* sub_p looks up into a BGRA palette which is here
@@ -544,7 +510,7 @@ FFmpegDecoder::decode_subtitle_packet ()
        uint32_t const * palette = (uint32_t *) rect->pict.data[1];
        /* Start of the output data */
        uint32_t* out_p = (uint32_t *) image->data()[0];
        uint32_t const * palette = (uint32_t *) rect->pict.data[1];
        /* Start of the output data */
        uint32_t* out_p = (uint32_t *) image->data()[0];
-
+       
        for (int y = 0; y < rect->h; ++y) {
                uint8_t* sub_line_p = sub_p;
                uint32_t* out_line_p = out_p;
        for (int y = 0; y < rect->h; ++y) {
                uint8_t* sub_line_p = sub_p;
                uint32_t* out_line_p = out_p;
@@ -555,19 +521,15 @@ FFmpegDecoder::decode_subtitle_packet ()
                sub_p += rect->pict.linesize[0];
                out_p += image->stride()[0] / sizeof (uint32_t);
        }
                sub_p += rect->pict.linesize[0];
                out_p += image->stride()[0] / sizeof (uint32_t);
        }
-
+       
        dcp::Size const vs = _ffmpeg_content->video_size ();
        dcp::Size const vs = _ffmpeg_content->video_size ();
-
-       image_subtitle (
-               period,
-               image,
-               dcpomatic::Rect<double> (
-                       static_cast<double> (rect->x) / vs.width,
-                       static_cast<double> (rect->y) / vs.height,
-                       static_cast<double> (rect->w) / vs.width,
-                       static_cast<double> (rect->h) / vs.height
-                       )
+       dcpomatic::Rect<double> const scaled_rect (
+               static_cast<double> (rect->x) / vs.width,
+               static_cast<double> (rect->y) / vs.height,
+               static_cast<double> (rect->w) / vs.width,
+               static_cast<double> (rect->h) / vs.height
                );
        
                );
        
-       avsubtitle_free (&sub);
+       image_subtitle (period, image, scaled_rect);
 }
 }
+