Disable use of stream IDs if there are duplicates, rather than rewriting.

It turns out that FFmpeg decoders (e.g. flv, see FFmpeg
25faaa311a74efdfdc4fed56996d7338ed807488) check stream IDs and sometimes
create new streams if they see one that they didn't see before.  If we
change stream IDs we break this.

Here we try to use stream indices in cases where the IDs are duplicated.
We also account for the case where a new stream appears during
examination. This wasn't covered by tests until the FFmpeg commit
mentioned above, were the flv decoder creates a new stream during
examination of boon_telly.mkv.

12 files changed, 168 insertions, 97 deletions

diff --git a/src/lib/content_factory.cc b/src/lib/content_factory.cc
index 5d0fe8c3c..25577953e 100644
--- a/src/lib/content_factory.cc
+++ b/src/lib/content_factory.cc
@@ -79,7 +79,7 @@ content_factory(cxml::ConstNodePtr node, boost::optional<boost::filesystem::path
 
 		content->audio->set_stream (
 			std::make_shared<FFmpegAudioStream>(
-				"Stream", 0,
+				"Stream", 0, 0,
 				node->number_child<int> ("AudioFrameRate"),
 				node->number_child<Frame> ("AudioLength"),
 				AudioMapping(node->node_child("AudioMapping"), version),
diff --git a/src/lib/ffmpeg.cc b/src/lib/ffmpeg.cc
index 30b725839..d7c741d09 100644
--- a/src/lib/ffmpeg.cc
+++ b/src/lib/ffmpeg.cc
@@ -147,28 +147,6 @@ FFmpeg::setup_general ()
 		_video_stream = video_stream_undefined_frame_rate.get();
 	}
 
-	/* Hack: if the AVStreams have duplicate IDs, replace them with our
-	   own.  We use the IDs so that we can cope with VOBs, in which streams
-	   move about in index but remain with the same ID in different
-	   VOBs.  However, some files have duplicate IDs, hence this hack.
-	*/
-
-	bool duplicates = false;
-	for (uint32_t i = 0; i < _format_context->nb_streams; ++i) {
-		for (uint32_t j = i + 1; j < _format_context->nb_streams; ++j) {
-			if (_format_context->streams[i]->id == _format_context->streams[j]->id) {
-				duplicates = true;
-			}
-		}
-	}
-
-	if (duplicates) {
-		/* Put in our own IDs */
-		for (uint32_t i = 0; i < _format_context->nb_streams; ++i) {
-			_format_context->streams[i]->id = i;
-		}
-	}
-
 	_video_frame = av_frame_alloc ();
 	if (_video_frame == nullptr) {
 		throw std::bad_alloc ();
@@ -179,46 +157,55 @@ FFmpeg::setup_general ()
 void
 FFmpeg::setup_decoders ()
 {
+	for (uint32_t i = 0; i < _format_context->nb_streams; ++i) {
+		setup_decoder(i);
+	}
+}
+
+
+void
+FFmpeg::setup_decoder(int stream_index)
+{
 	boost::mutex::scoped_lock lm (_mutex);
 
-	_codec_context.resize (_format_context->nb_streams);
-	for (uint32_t i = 0; i < _format_context->nb_streams; ++i) {
-		auto codec = avcodec_find_decoder (_format_context->streams[i]->codecpar->codec_id);
-		if (codec) {
-			auto context = avcodec_alloc_context3 (codec);
-			if (!context) {
-				throw std::bad_alloc ();
-			}
-			_codec_context[i] = context;
+	if (stream_index >= static_cast<int>(_codec_context.size())) {
+		_codec_context.resize(stream_index + 1);
+	}
 
-			int r = avcodec_parameters_to_context (context, _format_context->streams[i]->codecpar);
-			if (r < 0) {
-				throw DecodeError ("avcodec_parameters_to_context", "FFmpeg::setup_decoders", r);
-			}
+	if (auto codec = avcodec_find_decoder(_format_context->streams[stream_index]->codecpar->codec_id)) {
+		auto context = avcodec_alloc_context3(codec);
+		if (!context) {
+			throw std::bad_alloc();
+		}
+		_codec_context[stream_index] = context;
 
-			context->thread_count = 8;
-			context->thread_type = FF_THREAD_FRAME | FF_THREAD_SLICE;
-
-			AVDictionary* options = nullptr;
-			/* This option disables decoding of DCA frame footers in our patched version
-			   of FFmpeg.  I believe these footers are of no use to us, and they can cause
-			   problems when FFmpeg fails to decode them (mantis #352).
-			*/
-			av_dict_set (&options, "disable_footer", "1", 0);
-			/* This allows decoding of some DNxHR 444 and HQX files; see
-			   https://trac.ffmpeg.org/ticket/5681
-			*/
-			av_dict_set_int (&options, "strict", FF_COMPLIANCE_EXPERIMENTAL, 0);
-			/* Enable following of links in files */
-			av_dict_set_int (&options, "enable_drefs", 1, 0);
-
-			r = avcodec_open2 (context, codec, &options);
-			if (r < 0) {
-				throw DecodeError (N_("avcodec_open2"), N_("FFmpeg::setup_decoders"), r);
-			}
-		} else {
-			dcpomatic_log->log (fmt::format("No codec found for stream {}", i), LogEntry::TYPE_WARNING);
+		int r = avcodec_parameters_to_context(context, _format_context->streams[stream_index]->codecpar);
+		if (r < 0) {
+			throw DecodeError("avcodec_parameters_to_context", "FFmpeg::setup_decoders", r);
+		}
+
+		context->thread_count = 8;
+		context->thread_type = FF_THREAD_FRAME | FF_THREAD_SLICE;
+
+		AVDictionary* options = nullptr;
+		/* This option disables decoding of DCA frame footers in our patched version
+		   of FFmpeg.  I believe these footers are of no use to us, and they can cause
+		   problems when FFmpeg fails to decode them (mantis #352).
+		*/
+		av_dict_set(&options, "disable_footer", "1", 0);
+		/* This allows decoding of some DNxHR 444 and HQX files; see
+		   https://trac.ffmpeg.org/ticket/5681
+		*/
+		av_dict_set_int(&options, "strict", FF_COMPLIANCE_EXPERIMENTAL, 0);
+		/* Enable following of links in files */
+		av_dict_set_int(&options, "enable_drefs", 1, 0);
+
+		r = avcodec_open2(context, codec, &options);
+		if (r < 0) {
+			throw DecodeError(N_("avcodec_open2"), N_("FFmpeg::setup_decoders"), r);
 		}
+	} else {
+		dcpomatic_log->log(fmt::format("No codec found for stream {}", stream_index), LogEntry::TYPE_WARNING);
 	}
 }
 
diff --git a/src/lib/ffmpeg.h b/src/lib/ffmpeg.h
index 834c8946c..63446e5cd 100644
--- a/src/lib/ffmpeg.h
+++ b/src/lib/ffmpeg.h
@@ -63,6 +63,7 @@ protected:
 	dcpomatic::ContentTime pts_offset (
 		std::vector<std::shared_ptr<FFmpegAudioStream>> audio_streams, boost::optional<dcpomatic::ContentTime> first_video, double video_frame_rate
 		) const;
+	void setup_decoder(int stream_index);
 
 	static FFmpegSubtitlePeriod subtitle_period (AVPacket const* packet, AVStream const* stream, AVSubtitle const & sub);
 
diff --git a/src/lib/ffmpeg_audio_stream.h b/src/lib/ffmpeg_audio_stream.h
index 6c591e2fe..32138e50a 100644
--- a/src/lib/ffmpeg_audio_stream.h
+++ b/src/lib/ffmpeg_audio_stream.h
@@ -30,19 +30,19 @@ struct ffmpeg_pts_offset_test;
 class FFmpegAudioStream : public FFmpegStream, public AudioStream
 {
 public:
-	FFmpegAudioStream(std::string name, int id, int frame_rate, Frame length, int channels, int bit_depth)
-		: FFmpegStream(name, id)
+	FFmpegAudioStream(std::string name, int id, int index, int frame_rate, Frame length, int channels, int bit_depth)
+		: FFmpegStream(name, id, index)
 		, AudioStream(frame_rate, length, channels, bit_depth)
 	{}
 
-	FFmpegAudioStream(std::string name, std::string codec_name_, int id, int frame_rate, Frame length, int channels, int bit_depth)
-		: FFmpegStream(name, id)
+	FFmpegAudioStream(std::string name, std::string codec_name_, int id, int index, int frame_rate, Frame length, int channels, int bit_depth)
+		: FFmpegStream(name, id, index)
 		, AudioStream(frame_rate, length, channels, bit_depth)
 		, codec_name(codec_name_)
 	{}
 
-	FFmpegAudioStream(std::string name, int id, int frame_rate, Frame length, AudioMapping mapping, int bit_depth)
-		: FFmpegStream(name, id)
+	FFmpegAudioStream(std::string name, int id, int index, int frame_rate, Frame length, AudioMapping mapping, int bit_depth)
+		: FFmpegStream(name, id, index)
 		, AudioStream(frame_rate, length, mapping, bit_depth)
 	{}
 
@@ -60,7 +60,7 @@ private:
 
 	/* Constructor for tests */
 	FFmpegAudioStream()
-		: FFmpegStream("", 0)
+		: FFmpegStream("", 0, 0)
 		, AudioStream(0, 0, 0, 0)
 	{}
 };
diff --git a/src/lib/ffmpeg_content.cc b/src/lib/ffmpeg_content.cc
index 6261c4003..3c5db11f4 100644
--- a/src/lib/ffmpeg_content.cc
+++ b/src/lib/ffmpeg_content.cc
@@ -400,14 +400,14 @@ FFmpegContent::set_subtitle_stream (shared_ptr<FFmpegSubtitleStream> s)
 bool
 operator== (FFmpegStream const & a, FFmpegStream const & b)
 {
-	return a._id == b._id;
+	return a._id == b._id && b._index == b._index;
 }
 
 
 bool
 operator!= (FFmpegStream const & a, FFmpegStream const & b)
 {
-	return a._id != b._id;
+	return a._id != b._id || a._index != b._index;
 }
 
 
diff --git a/src/lib/ffmpeg_decoder.cc b/src/lib/ffmpeg_decoder.cc
index 74836c1a8..024b87391 100644
--- a/src/lib/ffmpeg_decoder.cc
+++ b/src/lib/ffmpeg_decoder.cc
@@ -528,7 +528,7 @@ FFmpegDecoder::process_audio_frame (shared_ptr<FFmpegAudioStream> stream)
 			"Crazy timestamp {} for {} samples in stream {} (ts={} tb={}, off={})",
 			to_string(ct),
 			data->frames(),
-			stream->id(),
+			stream->identifier(),
 			frame->best_effort_timestamp,
 			av_q2d(time_base),
 			to_string(_pts_offset)
diff --git a/src/lib/ffmpeg_examiner.cc b/src/lib/ffmpeg_examiner.cc
index ee91970a7..c50f13ba5 100644
--- a/src/lib/ffmpeg_examiner.cc
+++ b/src/lib/ffmpeg_examiner.cc
@@ -78,6 +78,7 @@ FFmpegExaminer::FFmpegExaminer(shared_ptr<const FFmpegContent> c, shared_ptr<Job
 					stream_name(s),
 					codec->name,
 					s->id,
+					i,
 					s->codecpar->sample_rate,
 					_need_length ? 0 : rint((double(_format_context->duration) / AV_TIME_BASE) * s->codecpar->sample_rate),
 					s->codecpar->ch_layout.nb_channels,
@@ -86,7 +87,7 @@ FFmpegExaminer::FFmpegExaminer(shared_ptr<const FFmpegContent> c, shared_ptr<Job
 				);
 
 		} else if (s->codecpar->codec_type == AVMEDIA_TYPE_SUBTITLE) {
-			_subtitle_streams.push_back(make_shared<FFmpegSubtitleStream>(subtitle_stream_name(s), s->id));
+			_subtitle_streams.push_back(make_shared<FFmpegSubtitleStream>(subtitle_stream_name(s), s->id, i));
 		}
 	}
 
@@ -130,6 +131,11 @@ FFmpegExaminer::FFmpegExaminer(shared_ptr<const FFmpegContent> c, shared_ptr<Job
 			}
 		}
 
+		if (packet->stream_index >= static_cast<int>(_codec_context.size())) {
+			setup_decoder(packet->stream_index);
+			check_for_duplicate_ids();
+		}
+
 		auto context = _codec_context[packet->stream_index];
 
 		boost::optional<size_t> audio_stream_index;
@@ -202,6 +208,42 @@ FFmpegExaminer::FFmpegExaminer(shared_ptr<const FFmpegContent> c, shared_ptr<Job
 		_pulldown = true;
 		LOG_GENERAL_NC("Suggest that this may be 2:3 pull-down (soft telecine)");
 	}
+
+	check_for_duplicate_ids();
+}
+
+void
+FFmpegExaminer::check_for_duplicate_ids()
+{
+	bool duplicates = false;
+	std::set<int> stream_ids;
+
+	if (_video_stream) {
+		stream_ids.insert(*_video_stream);
+	}
+
+	for (auto stream: _audio_streams) {
+		if (stream->id() && !stream_ids.insert(*stream->id()).second) {
+			duplicates = true;
+			break;
+		}
+	}
+
+	for (auto stream: _subtitle_streams) {
+		if (stream->id() && !stream_ids.insert(*stream->id()).second) {
+			duplicates = true;
+			break;
+		}
+	}
+
+	if (duplicates) {
+		for (auto stream: _audio_streams) {
+			stream->unset_id();
+		}
+		for (auto stream: _subtitle_streams) {
+			stream->unset_id();
+		}
+	}
 }
 
 
diff --git a/src/lib/ffmpeg_examiner.h b/src/lib/ffmpeg_examiner.h
index 3dd79b2b5..65fa9cb5c 100644
--- a/src/lib/ffmpeg_examiner.h
+++ b/src/lib/ffmpeg_examiner.h
@@ -91,6 +91,7 @@ public:
 private:
 	bool video_packet(AVCodecContext* context, std::string& temporal_reference, AVPacket* packet);
 	bool audio_packet(AVCodecContext* context, std::shared_ptr<FFmpegAudioStream>, AVPacket* packet);
+	void check_for_duplicate_ids();
 
 	std::string stream_name(AVStream* s) const;
 	std::string subtitle_stream_name(AVStream* s) const;
diff --git a/src/lib/ffmpeg_stream.cc b/src/lib/ffmpeg_stream.cc
index f5f12bde6..af43df261 100644
--- a/src/lib/ffmpeg_stream.cc
+++ b/src/lib/ffmpeg_stream.cc
@@ -36,7 +36,8 @@ using std::string;
 
 FFmpegStream::FFmpegStream(cxml::ConstNodePtr node)
 	: name(node->string_child("Name"))
-	, _id(node->number_child<int>("Id"))
+	, _id(node->optional_number_child<int>("Id"))
+	, _index(node->optional_number_child<int>("Index"))
 {
 
 }
@@ -45,56 +46,89 @@ void
 FFmpegStream::as_xml(xmlpp::Element* root) const
 {
 	cxml::add_text_child(root, "Name", name);
-	cxml::add_text_child(root, "Id", fmt::to_string(_id));
+	if (_id) {
+		cxml::add_text_child(root, "Id", fmt::to_string(*_id));
+	}
+	if (_index) {
+		cxml::add_text_child(root, "Index", fmt::to_string(*_index));
+	}
 }
 
 bool
 FFmpegStream::uses_index(AVFormatContext const * fc, int index) const
 {
-	return fc->streams[index]->id == _id;
+	DCPOMATIC_ASSERT(static_cast<bool>(_id) || static_cast<bool>(_index));
+
+	if (_id) {
+		return fc->streams[index]->id == *_id;
+	} else {
+		return *_index == index;
+	}
 }
 
 AVStream *
 FFmpegStream::stream(AVFormatContext const * fc) const
 {
-	size_t i = 0;
-	while (i < fc->nb_streams) {
-		if (fc->streams[i]->id == _id) {
-			return fc->streams[i];
+	DCPOMATIC_ASSERT(static_cast<bool>(_id) || static_cast<bool>(_index));
+
+	if (_id) {
+		size_t i = 0;
+		while (i < fc->nb_streams) {
+			if (fc->streams[i]->id == *_id) {
+				return fc->streams[i];
+			}
+			++i;
 		}
-		++i;
-	}
 
-	DCPOMATIC_ASSERT(false);
-	return 0;
+		DCPOMATIC_ASSERT(false);
+		return 0;
+	} else {
+		return fc->streams[*_index];
+	}
 }
 
 int
 FFmpegStream::index(AVFormatContext const * fc) const
 {
-	size_t i = 0;
-	while (i < fc->nb_streams) {
-		if (fc->streams[i]->id == _id) {
-			return i;
+	DCPOMATIC_ASSERT(static_cast<bool>(_id) || static_cast<bool>(_index));
+
+	if (_id) {
+		size_t i = 0;
+		while (i < fc->nb_streams) {
+			if (fc->streams[i]->id == *_id) {
+				return i;
+			}
+			++i;
 		}
-		++i;
-	}
 
-	DCPOMATIC_ASSERT(false);
-	return 0;
+		DCPOMATIC_ASSERT(false);
+		return 0;
+	} else {
+		return *_index;
+	}
 }
 
 
 string
 FFmpegStream::technical_summary() const
 {
-	return "id " + fmt::to_string(_id);
+	DCPOMATIC_ASSERT(static_cast<bool>(_id) || static_cast<bool>(_index));
+
+	if (_id) {
+		return fmt::format("id {}", *_id);
+	} else {
+		return fmt::format("index {}", *_index);
+	}
 }
 
 
 string
 FFmpegStream::identifier() const
 {
-	return fmt::to_string(_id);
+	if (_id) {
+		return fmt::to_string(*_id);
+	} else {
+		return fmt::to_string(*_index);
+	}
 }
 
diff --git a/src/lib/ffmpeg_stream.h b/src/lib/ffmpeg_stream.h
index 0ed0b3be2..1583e5221 100644
--- a/src/lib/ffmpeg_stream.h
+++ b/src/lib/ffmpeg_stream.h
@@ -30,9 +30,10 @@ struct AVStream;
 class FFmpegStream
 {
 public:
-	FFmpegStream(std::string n, int i)
+	FFmpegStream(std::string n, int id, int index)
 		: name(n)
-		, _id(i)
+		, _id(id)
+		, _index(index)
 	{}
 
 	explicit FFmpegStream(cxml::ConstNodePtr);
@@ -49,10 +50,14 @@ public:
 	std::string technical_summary() const;
 	std::string identifier() const;
 
-	int id() const {
+	boost::optional<int> id() const {
 		return _id;
 	}
 
+	void unset_id() {
+		_id = boost::none;
+	}
+
 	int index(AVFormatContext const * c) const;
 
 	std::string name;
@@ -61,7 +66,8 @@ public:
 	friend bool operator!=(FFmpegStream const & a, FFmpegStream const & b);
 
 private:
-	int _id;
+	boost::optional<int> _id;
+	boost::optional<int> _index;
 };
 
 #endif
diff --git a/src/lib/ffmpeg_subtitle_stream.h b/src/lib/ffmpeg_subtitle_stream.h
index 614a014e6..2ecabdea6 100644
--- a/src/lib/ffmpeg_subtitle_stream.h
+++ b/src/lib/ffmpeg_subtitle_stream.h
@@ -27,8 +27,8 @@
 class FFmpegSubtitleStream : public FFmpegStream
 {
 public:
-	FFmpegSubtitleStream(std::string n, int i)
-		: FFmpegStream(n, i)
+	FFmpegSubtitleStream(std::string n, int id, int index)
+		: FFmpegStream(n, id, index)
 	{}
 
 	FFmpegSubtitleStream(cxml::ConstNodePtr node, int version);
diff --git a/test/frame_rate_test.cc b/test/frame_rate_test.cc
index f38b65450..8d085478f 100644
--- a/test/frame_rate_test.cc
+++ b/test/frame_rate_test.cc
@@ -249,7 +249,7 @@ BOOST_AUTO_TEST_CASE (audio_sampling_rate_test)
 	std::list<int> afr = { 24, 25, 30 };
 	Config::instance()->set_allowed_dcp_frame_rates (afr);
 
-	auto stream = std::make_shared<FFmpegAudioStream>("foo", 0, 0, 0, 0, 0);
+	auto stream = std::make_shared<FFmpegAudioStream>("foo", 0, 0, 0, 0, 0, 0);
 	content->audio = std::make_shared<AudioContent>(content.get());
 	content->audio->add_stream (stream);
 	content->_video_frame_rate = 24;