Fix bugs with large initial audio discard padding There were two issues with our current handline of multi-packet audio discard values on the first packet: * Subsequent packets marked for full discard by AV_PKT_FLAG_DISCARD would have their timestamp set to kInfiniteDuration - pts. Which sent duration calculations to the moon. * As initially implemented we only handled the case where the audio discarded had a timestamp >= 0. The file in the bug applies multi packet discard to audio with timestamps <= 0. Unfortunately I haven't been able to generate a test file to add for this behavior. The user provided clip is too large and can't be truncated or remuxed without losing its special properties. Bug: 435684931 Change-Id: I5c12825338c8a4b36070733de5f33f5f2b3f6303 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6838830 Reviewed-by: Eugene Zemtsov <eugene@chromium.org> Commit-Queue: Dale Curtis <dalecurtis@chromium.org> Cr-Commit-Position: refs/heads/main@{#1500887}
diff --git a/media/filters/ffmpeg_demuxer.cc b/media/filters/ffmpeg_demuxer.cc index 9e5923d..71af53f 100644 --- a/media/filters/ffmpeg_demuxer.cc +++ b/media/filters/ffmpeg_demuxer.cc
@@ -463,6 +463,36 @@ base::HeapArray<uint8_t>::CopiedFrom(side_data); } + if (decrypt_config) { + buffer->set_decrypt_config(std::move(decrypt_config)); + } + + if (packet->duration >= 0) { + // Treat durations under 1ms as not having duration, later stages of the + // pipeline will then use the timestamps to estimate duration. Incorrect + // duration information can lead to stuttering effects during seeking. See + // https://crbug.com/397343886. + auto d = ConvertStreamTimestamp(stream_->time_base, packet->duration); + buffer->set_duration(d <= base::Milliseconds(1) ? kNoTimestamp : d); + } else { + // TODO(wolenetz): Remove when FFmpeg stops returning negative durations. + // https://crbug.com/394418 + DVLOG(1) << "FFmpeg returned a buffer with a negative duration! " + << packet->duration; + buffer->set_duration(kNoTimestamp); + } + + // Note: If pts is kNoFFmpegTimestamp, stream_timestamp will be kNoTimestamp. + const base::TimeDelta stream_timestamp = + ConvertStreamTimestamp(stream_->time_base, packet->pts); + + if (stream_timestamp == kNoTimestamp || + stream_timestamp == kInfiniteDuration) { + MEDIA_LOG(ERROR, media_log_) << "FFmpegDemuxer: PTS is not defined"; + demuxer_->NotifyDemuxerError(DEMUXER_ERROR_COULD_NOT_PARSE); + return; + } + size_t skip_samples_size = 0; const uint32_t* skip_samples_ptr = reinterpret_cast<const uint32_t*>(av_packet_get_side_data( @@ -497,42 +527,27 @@ DCHECK(is_audio); const int samples_per_second = audio_decoder_config().samples_per_second(); + auto front_discard = + FramesToTimeDelta(discard_front_samples, samples_per_second); buffer->set_discard_padding(std::make_pair( - FramesToTimeDelta(discard_front_samples, samples_per_second), + front_discard, FramesToTimeDelta(discard_end_samples, samples_per_second))); + + // In cases where the first buffer has a discard which spans multiple + // packets (or we just can't tell), treat negative timestamps as being + // dropped. Duration may be inaccurate, so instead of tracking a total + // amount discarded (like AudioDiscardHelper does post-decode), just flag + // that any negative timestamps should be treated as partially discarded. + if (front_discard > buffer->duration() || + buffer->duration() == kNoTimestamp) { + // We add one millisecond of padding to adjust for inaccurate durations + // and rounding errors which might occur with FramesToTimeDelta(). + fixup_negative_timestamps_until_ = + stream_timestamp + front_discard + base::Milliseconds(1); + } } } - if (decrypt_config) { - buffer->set_decrypt_config(std::move(decrypt_config)); - } - - if (packet->duration >= 0) { - // Treat durations under 1ms as not having duration, later stages of the - // pipeline will then use the timestamps to estimate duration. Incorrect - // duration information can lead to stuttering effects during seeking. See - // https://crbug.com/397343886. - auto d = ConvertStreamTimestamp(stream_->time_base, packet->duration); - buffer->set_duration(d <= base::Milliseconds(1) ? kNoTimestamp : d); - } else { - // TODO(wolenetz): Remove when FFmpeg stops returning negative durations. - // https://crbug.com/394418 - DVLOG(1) << "FFmpeg returned a buffer with a negative duration! " - << packet->duration; - buffer->set_duration(kNoTimestamp); - } - - // Note: If pts is kNoFFmpegTimestamp, stream_timestamp will be kNoTimestamp. - const base::TimeDelta stream_timestamp = - ConvertStreamTimestamp(stream_->time_base, packet->pts); - - if (stream_timestamp == kNoTimestamp || - stream_timestamp == kInfiniteDuration) { - MEDIA_LOG(ERROR, media_log_) << "FFmpegDemuxer: PTS is not defined"; - demuxer_->NotifyDemuxerError(DEMUXER_ERROR_COULD_NOT_PARSE); - return; - } - // If this file has negative timestamps don't rebase any other stream types // against the negative starting time. base::TimeDelta start_time = demuxer_->start_time(); @@ -561,7 +576,8 @@ if (is_audio) { // Fixup negative timestamps where the before-zero portion is completely // discarded after decoding. - if (buffer->timestamp().is_negative()) { + if (buffer->timestamp().is_negative() && buffer->discard_padding() && + buffer->discard_padding()->first != kInfiniteDuration) { // Discard padding may also remove samples after zero. auto discard_padding = buffer->discard_padding(); auto fixed_ts = (discard_padding.has_value() ? discard_padding->first @@ -578,6 +594,15 @@ } } + // Treat this buffer as if it was partially discarded by a front discard + // indicated by a previous packet. + if (fixup_negative_timestamps_until_ && + stream_timestamp < fixup_negative_timestamps_until_.value() && + last_packet_timestamp_ != kNoTimestamp && + buffer->timestamp().is_negative()) { + buffer->set_timestamp(last_packet_timestamp_); + } + // Only allow negative timestamps past if we know they'll be fixed up by the // code paths below; otherwise they should be treated as a parse error. if ((!fixup_chained_ogg_ || last_packet_timestamp_ == kNoTimestamp) && @@ -724,6 +749,7 @@ end_of_stream_ = false; last_packet_timestamp_ = kNoTimestamp; last_packet_duration_ = kNoTimestamp; + fixup_negative_timestamps_until_.reset(); aborted_ = false; }
diff --git a/media/filters/ffmpeg_demuxer.h b/media/filters/ffmpeg_demuxer.h index 2683b611..6c4a277e 100644 --- a/media/filters/ffmpeg_demuxer.h +++ b/media/filters/ffmpeg_demuxer.h
@@ -187,6 +187,15 @@ bool waiting_for_keyframe_ = false; bool aborted_ = false; + // Used to correct packets which end up with a negative calculated timestamp + // (adjusted for start time) in cases where the first packet had discard + // padding which extended beyond itself. + // + // If set, any packets with a raw stream timestamp before this value which + // also have a negative calcuated timestamp, will have their timestamp set to + // `last_packet_timestamp_`. + std::optional<base::TimeDelta> fixup_negative_timestamps_until_; + DecoderBufferQueue buffer_queue_; ReadCB read_cb_;