From f9535bd6d61d7e0b2cb452e6976a914d4ee62a2b Mon Sep 17 00:00:00 2001 From: Dale Curtis Date: Thu, 17 May 2018 23:44:41 +0000 Subject: [PATCH] Remove dependency on ffmpeg internals for start time calculations. Some theora clips still have issues, but using first_dts where we know it's the same as the first pts resolves the lingering issues. It also looks like we don't need to use pts_buffer now. This does the following small fixes: - MEDIA_LOG(DEBUG) -> MEDIA_LOG(ERROR) for negative ts error. - Enables previous disabled test in more limited state. - Adds kNoFFmpegTimestamp so static_cast(AV_NOPTS_VALUE) is no longer necessary everywhere. A followup patch set will use this is more places. - Removes pts_buffer and packet_buffer inspection. BUG=731766 TEST=all tests pass. Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Change-Id: I5aadf67a3b5ea2d2a8dd19bbddd7b107208094c5 Reviewed-on: https://chromium-review.googlesource.com/1064538 Commit-Queue: Dale Curtis Reviewed-by: Frank Liberato Cr-Commit-Position: refs/heads/master@{#559737} --- media/ffmpeg/ffmpeg_common.h | 5 +- media/filters/ffmpeg_demuxer.cc | 88 +++++++----------------- media/filters/ffmpeg_demuxer_unittest.cc | 24 +++---- 3 files changed, 37 insertions(+), 80 deletions(-) diff --git a/media/ffmpeg/ffmpeg_common.h b/media/ffmpeg/ffmpeg_common.h index ec33068fb84f..f641d6bcf92e 100644 --- a/media/ffmpeg/ffmpeg_common.h +++ b/media/ffmpeg/ffmpeg_common.h @@ -28,9 +28,6 @@ extern "C" { MSVC_PUSH_DISABLE_WARNING(4244); #include #include -#if !BUILDFLAG(USE_SYSTEM_FFMPEG) -#include -#endif // !BUILDFLAG(USE_SYSTEM_FFMPEG) #include #include #include @@ -42,6 +39,8 @@ MSVC_POP_WARNING(); namespace media { +constexpr int64_t kNoFFmpegTimestamp = static_cast(AV_NOPTS_VALUE); + class AudioDecoderConfig; class EncryptionScheme; class VideoDecoderConfig; diff --git a/media/filters/ffmpeg_demuxer.cc b/media/filters/ffmpeg_demuxer.cc index 49ca01c4dc0b..7402ce16ab5e 100644 --- a/media/filters/ffmpeg_demuxer.cc +++ b/media/filters/ffmpeg_demuxer.cc @@ -85,29 +85,26 @@ static base::TimeDelta FramesToTimeDelta(int frames, double sample_rate) { frames * base::Time::kMicrosecondsPerSecond / sample_rate); } -static base::TimeDelta ExtractStartTime(AVStream* stream, - base::TimeDelta start_time_estimate) { - DCHECK(start_time_estimate != kNoTimestamp); - if (stream->start_time == static_cast(AV_NOPTS_VALUE)) { - return start_time_estimate == kInfiniteDuration ? base::TimeDelta() - : start_time_estimate; +static base::TimeDelta ExtractStartTime(AVStream* stream) { + // The default start time is zero. + base::TimeDelta start_time; + + // First try to use the |start_time| value as is. + if (stream->start_time != kNoFFmpegTimestamp) + start_time = ConvertFromTimeBase(stream->time_base, stream->start_time); + + // Next try to use the first DTS value, for codecs where we know PTS == DTS + // (excludes all H26x codecs). The start time must be returned in PTS. + if (stream->first_dts != kNoFFmpegTimestamp && + stream->codecpar->codec_id != AV_CODEC_ID_HEVC && + stream->codecpar->codec_id != AV_CODEC_ID_H264 && + stream->codecpar->codec_id != AV_CODEC_ID_MPEG4) { + const base::TimeDelta first_pts = + ConvertFromTimeBase(stream->time_base, stream->first_dts); + if (first_pts < start_time) + start_time = first_pts; } - // First try the lower of the estimate and the |start_time| value. - base::TimeDelta start_time = - std::min(ConvertFromTimeBase(stream->time_base, stream->start_time), - start_time_estimate); - - // Next see if the first buffered pts value is usable. - if (stream->pts_buffer[0] != static_cast(AV_NOPTS_VALUE)) { - const base::TimeDelta buffered_pts = - ConvertFromTimeBase(stream->time_base, stream->pts_buffer[0]); - if (buffered_pts < start_time) - start_time = buffered_pts; - } - - // NOTE: Do not use AVStream->first_dts since |start_time| should be a - // presentation timestamp. return start_time; } @@ -513,7 +510,7 @@ void FFmpegDemuxerStream::EnqueuePacket(ScopedAVPacket packet) { buffer->set_duration(kNoTimestamp); } - // Note: If pts is AV_NOPTS_VALUE, stream_timestamp will be kNoTimestamp. + // Note: If pts is kNoFFmpegTimestamp, stream_timestamp will be kNoTimestamp. const base::TimeDelta stream_timestamp = ConvertStreamTimestamp(stream_->time_base, packet->pts); @@ -556,8 +553,8 @@ void FFmpegDemuxerStream::EnqueuePacket(ScopedAVPacket packet) { // code paths below; otherwise they should be treated as a parse error. if ((!fixup_chained_ogg_ || last_packet_timestamp_ == kNoTimestamp) && buffer->timestamp() < base::TimeDelta()) { - MEDIA_LOG(DEBUG, media_log_) - << "FFmpegDemuxer: unfixable negative timestamp"; + MEDIA_LOG(ERROR, media_log_) + << "FFmpegDemuxer: unfixable negative timestamp."; demuxer_->NotifyDemuxerError(DEMUXER_ERROR_COULD_NOT_PARSE); return; } @@ -863,7 +860,7 @@ std::string FFmpegDemuxerStream::GetMetadata(const char* key) const { base::TimeDelta FFmpegDemuxerStream::ConvertStreamTimestamp( const AVRational& time_base, int64_t timestamp) { - if (timestamp == static_cast(AV_NOPTS_VALUE)) + if (timestamp == kNoFFmpegTimestamp) return kNoTimestamp; return ConvertFromTimeBase(time_base, timestamp); @@ -1256,42 +1253,6 @@ void FFmpegDemuxer::OnFindStreamInfoDone(const PipelineStatusCB& status_cb, AVFormatContext* format_context = glue_->format_context(); streams_.resize(format_context->nb_streams); - // Estimate the start time for each stream by looking through the packets - // buffered during avformat_find_stream_info(). These values will be - // considered later when determining the actual stream start time. - // - // These packets haven't been completely processed yet, so only look through - // these values if the AVFormatContext has a valid start time. - // - // If no estimate is found, the stream entry will be kInfiniteDuration. - std::vector start_time_estimates(format_context->nb_streams, - kInfiniteDuration); -#if !BUILDFLAG(USE_SYSTEM_FFMPEG) - const AVFormatInternal* internal = format_context->internal; - if (internal && internal->packet_buffer && - format_context->start_time != static_cast(AV_NOPTS_VALUE)) { - struct AVPacketList* packet_buffer = internal->packet_buffer; - while (packet_buffer != internal->packet_buffer_end) { - DCHECK_LT(static_cast(packet_buffer->pkt.stream_index), - start_time_estimates.size()); - const AVStream* stream = - format_context->streams[packet_buffer->pkt.stream_index]; - if (packet_buffer->pkt.pts != static_cast(AV_NOPTS_VALUE)) { - const base::TimeDelta packet_pts = - ConvertFromTimeBase(stream->time_base, packet_buffer->pkt.pts); - // We ignore kNoTimestamp here since -int64_t::min() is possible; see - // https://crbug.com/700501. Technically this is a valid value, but in - // practice shouldn't occur, so just ignore it when estimating. - if (packet_pts != kNoTimestamp && packet_pts != kInfiniteDuration && - packet_pts < start_time_estimates[stream->index]) { - start_time_estimates[stream->index] = packet_pts; - } - } - packet_buffer = packet_buffer->next; - } - } -#endif // !BUILDFLAG(USE_SYSTEM_FFMPEG) - std::unique_ptr media_tracks(new MediaTracks()); DCHECK(track_id_to_demux_stream_map_.empty()); @@ -1440,8 +1401,7 @@ void FFmpegDemuxer::OnFindStreamInfoDone(const PipelineStatusCB& status_cb, max_duration = std::max(max_duration, streams_[i]->duration()); - base::TimeDelta start_time = - ExtractStartTime(stream, start_time_estimates[i]); + base::TimeDelta start_time = ExtractStartTime(stream); // Note: This value is used for seeking, so we must take the true value and // not the one possibly clamped to zero below. @@ -1479,7 +1439,7 @@ void FFmpegDemuxer::OnFindStreamInfoDone(const PipelineStatusCB& status_cb, if (text_enabled_) AddTextStreams(); - if (format_context->duration != static_cast(AV_NOPTS_VALUE)) { + if (format_context->duration != kNoFFmpegTimestamp) { // If there is a duration value in the container use that to find the // maximum between it and the duration from A/V streams. const AVRational av_time_base = {1, AV_TIME_BASE}; diff --git a/media/filters/ffmpeg_demuxer_unittest.cc b/media/filters/ffmpeg_demuxer_unittest.cc index 71dbed07b376..b09816a3ba3a 100644 --- a/media/filters/ffmpeg_demuxer_unittest.cc +++ b/media/filters/ffmpeg_demuxer_unittest.cc @@ -696,12 +696,9 @@ TEST_F(FFmpegDemuxerTest, Read_InvalidNegativeTimestamp) { ReadUntilEndOfStream(GetStream(DemuxerStream::AUDIO)); } -// TODO(dalecurtis): Test is disabled since FFmpeg does not currently guarantee -// the order of demuxed packets in OGG containers. Re-enable and fix key frame -// expectations once we decide to either workaround it or attempt a fix -// upstream. See http://crbug.com/387996. -TEST_F(FFmpegDemuxerTest, - DISABLED_Read_AudioNegativeStartTimeAndOggDiscard_Bear) { +// Android has no Theora support, so these tests doesn't work. +#if !defined(OS_ANDROID) +TEST_F(FFmpegDemuxerTest, Read_AudioNegativeStartTimeAndOggDiscard_Bear) { // Many ogg files have negative starting timestamps, so ensure demuxing and // seeking work correctly with a negative start time. CreateDemuxer("bear.ogv"); @@ -711,8 +708,12 @@ TEST_F(FFmpegDemuxerTest, DemuxerStream* video = GetStream(DemuxerStream::VIDEO); DemuxerStream* audio = GetStream(DemuxerStream::AUDIO); - // Run the test twice with a seek in between. - for (int i = 0; i < 2; ++i) { + // Run the test once (should be twice..., see note) with a seek in between. + // + // TODO(dalecurtis): We only run the test once since FFmpeg does not currently + // guarantee the order of demuxed packets in OGG containers. See + // http://crbug.com/387996. + for (int i = 0; i < 1; ++i) { audio->Read( NewReadCBWithCheckedDiscard(FROM_HERE, 40, 0, kInfiniteDuration, true)); base::RunLoop().Run(); @@ -731,10 +732,10 @@ TEST_F(FFmpegDemuxerTest, video->Read(NewReadCB(FROM_HERE, 5751, 0, true)); base::RunLoop().Run(); - video->Read(NewReadCB(FROM_HERE, 846, 33367, true)); + video->Read(NewReadCB(FROM_HERE, 846, 33367, false)); base::RunLoop().Run(); - video->Read(NewReadCB(FROM_HERE, 1255, 66733, true)); + video->Read(NewReadCB(FROM_HERE, 1255, 66733, false)); base::RunLoop().Run(); // Seek back to the beginning and repeat the test. @@ -747,9 +748,6 @@ TEST_F(FFmpegDemuxerTest, // Same test above, but using sync2.ogv which has video stream muxed before the // audio stream, so seeking based only on start time will fail since ffmpeg is // essentially just seeking based on file position. -// -// Android has no Theora support, so this test doesn't work. -#if !defined(OS_ANDROID) TEST_F(FFmpegDemuxerTest, Read_AudioNegativeStartTimeAndOggDiscard_Sync) { // Many ogg files have negative starting timestamps, so ensure demuxing and // seeking work correctly with a negative start time. -- 2.17.0