From: Carl Hetherington Date: Wed, 2 Apr 2014 16:04:47 +0000 (+0100) Subject: Various fixes to FFmpeg decoder, including a couple of tests. X-Git-Tag: v2.0.48~879 X-Git-Url: https://main.carlh.net/gitweb/?p=dcpomatic.git;a=commitdiff_plain;h=c86ed0c80b762d31eb68386662a7c37ae4e21b6b Various fixes to FFmpeg decoder, including a couple of tests. --- diff --git a/.gitignore b/.gitignore index 1c848fbe5..6be4e4c89 100644 --- a/.gitignore +++ b/.gitignore @@ -30,3 +30,4 @@ GRTAGS GSYMS GTAGS TAGS +.*.swp diff --git a/src/lib/decoder.h b/src/lib/decoder.h index df3ac4f39..38556c818 100644 --- a/src/lib/decoder.h +++ b/src/lib/decoder.h @@ -42,10 +42,12 @@ public: virtual ~Decoder () {} protected: - /** Seek so that the next peek() will yield the next thing + /** Seek so that the next pass() will yield the next thing * (video/sound frame, subtitle etc.) at or after the requested * time. Pass accurate = true to try harder to get close to - * the request. + * the request. Note that seeking to time t may mean that + * the next pass() yields, for example, audio at time t and then + * video before it. */ virtual void seek (ContentTime time, bool accurate) = 0; virtual bool pass () = 0; diff --git a/src/lib/ffmpeg_decoder.cc b/src/lib/ffmpeg_decoder.cc index c9efd80fc..0a4624569 100644 --- a/src/lib/ffmpeg_decoder.cc +++ b/src/lib/ffmpeg_decoder.cc @@ -1,5 +1,5 @@ /* - Copyright (C) 2012 Carl Hetherington + Copyright (C) 2012-2014 Carl Hetherington 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 @@ -313,9 +313,9 @@ FFmpegDecoder::minimal_run (boost::function, optiona avcodec_get_frame_defaults (_frame); - int finished = 0; - r = avcodec_decode_video2 (video_codec_context(), _frame, &finished, &_packet); - if (r >= 0 && finished) { + 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; } @@ -323,9 +323,9 @@ FFmpegDecoder::minimal_run (boost::function, optiona AVPacket copy_packet = _packet; while (copy_packet.size > 0) { - int finished; - r = avcodec_decode_audio4 (audio_codec_context(), _frame, &finished, &_packet); - if (r >= 0 && finished) { + 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; } @@ -387,12 +387,12 @@ FFmpegDecoder::seek (ContentTime time, bool accurate) VideoDecoder::seek (time, accurate); AudioDecoder::seek (time, accurate); - /* If we are doing an accurate seek, our initial shot will be 200ms (200 being + /* 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. */ - ContentTime pre_roll = accurate ? ContentTime::from_seconds (0.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); @@ -551,7 +551,9 @@ FFmpegDecoder::decode_subtitle_packet () AVSubtitleRect const * rect = sub.rects[0]; if (rect->type != SUBTITLE_BITMAP) { - throw DecodeError (_("non-bitmap subtitles not yet supported")); + /* XXX */ + // throw DecodeError (_("non-bitmap subtitles not yet supported")); + return; } /* Note RGBA is expressed little-endian, so the first byte in the word is R, second diff --git a/src/lib/ffmpeg_examiner.cc b/src/lib/ffmpeg_examiner.cc index 6daba4b40..72db9bce1 100644 --- a/src/lib/ffmpeg_examiner.cc +++ b/src/lib/ffmpeg_examiner.cc @@ -109,7 +109,7 @@ FFmpegExaminer::frame_time (AVStream* s) const int64_t const bet = av_frame_get_best_effort_timestamp (_frame); if (bet != AV_NOPTS_VALUE) { - t = ContentTime (bet * av_q2d (s->time_base)); + t = ContentTime::from_seconds (bet * av_q2d (s->time_base)); } return t; @@ -118,13 +118,11 @@ FFmpegExaminer::frame_time (AVStream* s) const float FFmpegExaminer::video_frame_rate () const { - AVStream* s = _format_context->streams[_video_stream]; - - if (s->avg_frame_rate.num && s->avg_frame_rate.den) { - return av_q2d (s->avg_frame_rate); - } - - return av_q2d (s->r_frame_rate); + /* This use of r_frame_rate is debateable; there's a few different + * frame rates in the format context, but this one seems to be the most + * reliable. + */ + return av_q2d (av_stream_get_r_frame_rate (_format_context->streams[_video_stream])); } dcp::Size diff --git a/src/lib/player.cc b/src/lib/player.cc index 02ae4e5c9..368d4c2ff 100644 --- a/src/lib/player.cc +++ b/src/lib/player.cc @@ -287,6 +287,23 @@ Player::set_approximate_size () _approximate_size = true; } +shared_ptr +Player::black_dcp_video (DCPTime time) const +{ + return shared_ptr ( + new DCPVideo ( + _black_image, + EYES_BOTH, + Crop (), + _video_container_size, + _video_container_size, + Scaler::from_id ("bicubic"), + Config::instance()->colour_conversions().front().conversion, + time + ) + ); +} + shared_ptr Player::get_video (DCPTime time, bool accurate) { @@ -296,19 +313,8 @@ Player::get_video (DCPTime time, bool accurate) list > ov = overlaps (time); if (ov.empty ()) { - /* No video content at this time: return a black frame */ - return shared_ptr ( - new DCPVideo ( - _black_image, - EYES_BOTH, - Crop (), - _video_container_size, - _video_container_size, - Scaler::from_id ("bicubic"), - Config::instance()->colour_conversions().front().conversion, - time - ) - ); + /* No video content at this time */ + return black_dcp_video (time); } /* Create a DCPVideo from the content's video at this time */ @@ -320,7 +326,9 @@ Player::get_video (DCPTime time, bool accurate) assert (content); optional dec = decoder->get_video (dcp_to_content_video (piece, time), accurate); - assert (dec); + if (!dec) { + return black_dcp_video (time); + } dcp::Size image_size = content->scale().size (content, _video_container_size, _film->frame_size ()); if (_approximate_size) { diff --git a/src/lib/player.h b/src/lib/player.h index 852e7243f..d83045ab1 100644 --- a/src/lib/player.h +++ b/src/lib/player.h @@ -131,6 +131,7 @@ private: VideoFrame dcp_to_content_video (boost::shared_ptr piece, DCPTime t) const; AudioFrame dcp_to_content_audio (boost::shared_ptr piece, DCPTime t) const; ContentTime dcp_to_content_subtitle (boost::shared_ptr piece, DCPTime t) const; + boost::shared_ptr black_dcp_video (DCPTime) const; template std::list > diff --git a/src/lib/video_decoder.cc b/src/lib/video_decoder.cc index 6a7a62b74..bd609d168 100644 --- a/src/lib/video_decoder.cc +++ b/src/lib/video_decoder.cc @@ -29,7 +29,12 @@ using boost::shared_ptr; using boost::optional; VideoDecoder::VideoDecoder (shared_ptr c) +#ifdef DCPOMATIC_DEBUG + : test_gaps (0) + , _video_content (c) +#else : _video_content (c) +#endif { } @@ -56,13 +61,34 @@ VideoDecoder::get_video (VideoFrame frame, bool accurate) optional dec; - /* Now enough pass() calls will either: + /* Now enough pass() calls should either: * (a) give us what we want, or * (b) hit the end of the decoder. */ if (accurate) { - /* We are being accurate, so we want the right frame */ - while (!decoded_video (frame) && !pass ()) {} + /* We are being accurate, so we want the right frame. + * This could all be one statement but it's split up for clarity. + */ + while (true) { + if (decoded_video (frame)) { + /* We got what we want */ + break; + } + + if (pass ()) { + /* The decoder has nothing more for us */ + break; + } + + if (!_decoded_video.empty() && _decoded_video.front().frame > frame) { + /* We're never going to get the frame we want. Perhaps the caller is asking + * for a video frame before the content's video starts (if its audio + * begins before its video, for example). + */ + break; + } + } + dec = decoded_video (frame); } else { /* Any frame will do: use the first one that comes out of pass() */ @@ -85,9 +111,16 @@ VideoDecoder::get_video (VideoFrame frame, bool accurate) void VideoDecoder::video (shared_ptr image, VideoFrame frame) { + /* We should not receive the same thing twice */ + assert (_decoded_video.empty() || frame != _decoded_video.back().frame); + /* Fill in gaps */ /* XXX: 3D */ + while (!_decoded_video.empty () && (_decoded_video.back().frame + 1) < frame) { +#ifdef DCPOMATIC_DEBUG + test_gaps++; +#endif _decoded_video.push_back ( ContentVideo ( _decoded_video.back().image, diff --git a/src/lib/video_decoder.h b/src/lib/video_decoder.h index 86a9489b1..8715b9714 100644 --- a/src/lib/video_decoder.h +++ b/src/lib/video_decoder.h @@ -41,6 +41,10 @@ public: return _video_content; } +#ifdef DCPOMATIC_DEBUG + int test_gaps; +#endif + protected: void seek (ContentTime time, bool accurate); diff --git a/test/ffmpeg_decoder_seek_test.cc b/test/ffmpeg_decoder_seek_test.cc new file mode 100644 index 000000000..76124786e --- /dev/null +++ b/test/ffmpeg_decoder_seek_test.cc @@ -0,0 +1,85 @@ +/* + Copyright (C) 2014 Carl Hetherington + + 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 + the Free Software Foundation; either version 2 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. + +*/ + +#include +#include +#include +#include "lib/ffmpeg_content.h" +#include "lib/ffmpeg_decoder.h" +#include "lib/log.h" +#include "lib/film.h" +#include "test.h" + +using std::cerr; +using std::vector; +using boost::shared_ptr; +using boost::optional; + +static void +check (FFmpegDecoder& decoder, int frame) +{ + optional v; + v = decoder.get_video (frame, true); + BOOST_CHECK (v); + BOOST_CHECK_EQUAL (v->frame, frame); +} + +static void +test (boost::filesystem::path file, vector frames) +{ + boost::filesystem::path path = private_data / file; + if (!boost::filesystem::exists (path)) { + cerr << "Skipping test: " << path.string() << " not found.\n"; + return; + } + + shared_ptr film = new_test_film ("ffmpeg_decoder_seek_test_" + file.string()); + shared_ptr content (new FFmpegContent (film, path)); + film->examine_and_add_content (content); + wait_for_jobs (); + shared_ptr log (new NullLog); + FFmpegDecoder decoder (content, log); + + for (vector::const_iterator i = frames.begin(); i != frames.end(); ++i) { + check (decoder, *i); + } +} + +BOOST_AUTO_TEST_CASE (ffmpeg_decoder_seek_test) +{ + vector frames; + + frames.clear (); + frames.push_back (0); + frames.push_back (42); + frames.push_back (999); + frames.push_back (0); + + test ("boon_telly.mkv", frames); + test ("Sintel_Trailer1.480p.DivX_Plus_HD.mkv", frames); + + frames.clear (); + frames.push_back (15); + frames.push_back (42); + frames.push_back (999); + frames.push_back (15); + + test ("prophet_clip.mkv", frames); +} + diff --git a/test/ffmpeg_decoder_sequential_test.cc b/test/ffmpeg_decoder_sequential_test.cc new file mode 100644 index 000000000..96de9be27 --- /dev/null +++ b/test/ffmpeg_decoder_sequential_test.cc @@ -0,0 +1,76 @@ +/* + Copyright (C) 2014 Carl Hetherington + + 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 + the Free Software Foundation; either version 2 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. + +*/ + +#include +#include +#include "lib/ffmpeg_content.h" +#include "lib/ffmpeg_decoder.h" +#include "lib/log.h" +#include "lib/film.h" +#include "test.h" + +using std::cout; +using std::cerr; +using boost::shared_ptr; +using boost::optional; + +/** @param black Frame index of first frame in the video */ +static void +test (boost::filesystem::path file, float fps, int first) +{ + boost::filesystem::path path = private_data / file; + if (!boost::filesystem::exists (path)) { + cerr << "Skipping test: " << path.string() << " not found.\n"; + return; + } + + shared_ptr film = new_test_film ("ffmpeg_decoder_seek_test_" + file.string()); + shared_ptr content (new FFmpegContent (film, path)); + film->examine_and_add_content (content); + wait_for_jobs (); + shared_ptr log (new NullLog); + FFmpegDecoder decoder (content, log); + + BOOST_CHECK_CLOSE (decoder.video_content()->video_frame_rate(), fps, 0.01); + + VideoFrame const N = decoder.video_content()->video_length().frames (decoder.video_content()->video_frame_rate ()); + decoder.test_gaps = 0; + for (VideoFrame i = 0; i < N; ++i) { + optional v; + v = decoder.get_video (i, true); + if (i < first) { + BOOST_CHECK (!v); + } else { + BOOST_CHECK (v); + BOOST_CHECK_EQUAL (v->frame, i); + } + } + BOOST_CHECK_EQUAL (decoder.test_gaps, 0); +} + +/** Check that the FFmpeg decoder produces sequential frames without gaps or dropped frames; + * (dropped frames being checked by assert() in VideoDecoder). Also that the decoder picks up frame rates correctly. + */ +BOOST_AUTO_TEST_CASE (ffmpeg_decoder_sequential_test) +{ + //test ("boon_telly.mkv", 29.97, 0); + //test ("Sintel_Trailer1.480p.DivX_Plus_HD.mkv", 24, 0); + test ("prophet_clip.mkv", 23.976, 12); +} + diff --git a/test/test.cc b/test/test.cc index 241032099..f393e80c3 100644 --- a/test/test.cc +++ b/test/test.cc @@ -1,5 +1,5 @@ /* - Copyright (C) 2012 Carl Hetherington + Copyright (C) 2012-2014 Carl Hetherington 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 @@ -43,6 +43,8 @@ using std::cerr; using std::list; using boost::shared_ptr; +boost::filesystem::path private_data = boost::filesystem::path ("test") / boost::filesystem::path ("private"); + class TestUISignaller : public UISignaller { public: diff --git a/test/test.h b/test/test.h index a42b41577..57ea32c57 100644 --- a/test/test.h +++ b/test/test.h @@ -22,6 +22,8 @@ class Film; class Image; +extern boost::filesystem::path private_data; + extern void wait_for_jobs (); extern boost::shared_ptr new_test_film (std::string); extern void check_dcp (boost::filesystem::path, boost::filesystem::path); diff --git a/test/util_test.cc b/test/util_test.cc index 25b814004..b2085afe1 100644 --- a/test/util_test.cc +++ b/test/util_test.cc @@ -66,6 +66,9 @@ BOOST_AUTO_TEST_CASE (dcptime_round_up_test) BOOST_CHECK_EQUAL (DCPTime (1).round_up (DCPTime::HZ / 42), DCPTime (42)); BOOST_CHECK_EQUAL (DCPTime (42).round_up (DCPTime::HZ / 42), DCPTime (42)); BOOST_CHECK_EQUAL (DCPTime (43).round_up (DCPTime::HZ / 42), DCPTime (84)); + + /* Check that rounding up to non-integer frame rates works */ + BOOST_CHECK_EQUAL (DCPTime (45312).round_up (29.976), DCPTime (48045)); } diff --git a/test/wscript b/test/wscript index ba5aabb7c..2fbe74150 100644 --- a/test/wscript +++ b/test/wscript @@ -24,6 +24,8 @@ def build(bld): colour_conversion_test.cc ffmpeg_audio_test.cc ffmpeg_dcp_test.cc + ffmpeg_decoder_seek_test.cc + ffmpeg_decoder_sequential_test.cc ffmpeg_examiner_test.cc ffmpeg_pts_offset.cc file_group_test.cc diff --git a/wscript b/wscript index 88e928f12..64026191e 100644 --- a/wscript +++ b/wscript @@ -404,4 +404,4 @@ def pot_merge(bld): bld.recurse('src') def tags(bld): - os.system('etags src/lib/*.cc src/lib/*.h src/wx/*.cc src/wx/*.h src/tools/*.cc src/tools/*.h') + os.system('etags src/lib/*.cc src/lib/*.h src/wx/*.cc src/wx/*.h src/tools/*.cc src/tools/*.h test/*.cc')