Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
311373004:
Consolidate and improve audio decoding test for all decoders. (Closed)
|
Created:
6 years, 6 months ago by DaleCurtis Modified:
6 years, 6 months ago Reviewers:
wolenetz CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionConsolidate and improve audio decoding test for all decoders.
- Consolidates FFmpegAudioDecoder and OpusAudioDecoder unittests
since they were identical anyways.
- Extends the AudioFileReader unittests to perform packet consistency
checks between seeks.
- Extends the new consolidated tests for WAV, FLAC, MP3, and AAC.
- Adds decoded output consistency checks using MD5 for all files.
- Removes old tests which end up duplicating efforts.
- Expands tests for bad decoder configs and buffers w/o timestamps.
- Expands tests to include AudioDiscardHelper usage.
BUG=381356
TEST=shiny new tests!
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278667
Patch Set 1 : Rename only. #
Total comments: 2
Patch Set 2 : Megapatch! #
Total comments: 77
Patch Set 3 : Comments. DEPS roll. #
Total comments: 20
Patch Set 4 : Rebase. Expand tests. #
Total comments: 8
Patch Set 5 : Comments. #
Messages
Total messages: 16 (0 generated)
And I quote, "I'd like to see associated test for this in our downstream" ... which I guess means you want to review this too :)
As discussed, PS#1 is the rename only, PS#2 contains the new work and changes. Thanks!
looking pretty good. Comments from my first pass through all of PS1-2: https://codereview.chromium.org/311373004/diff/20001/media/filters/ffmpeg_aud... File media/filters/ffmpeg_audio_decoder_unittest.cc (left): https://codereview.chromium.org/311373004/diff/20001/media/filters/ffmpeg_aud... media/filters/ffmpeg_audio_decoder_unittest.cc:157: AudioDecoderConfig config(kCodecVorbis, aside: why did the previous ffmpeg test need this config? test constructor initialized the decoder... https://codereview.chromium.org/311373004/diff/20001/media/filters/opus_audio... File media/filters/opus_audio_decoder_unittest.cc (left): https://codereview.chromium.org/311373004/diff/20001/media/filters/opus_audio... media/filters/opus_audio_decoder_unittest.cc:65: pending_decode_ = true; nit: EXPECT_FALSE(pending_decode_) first? https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... File media/filters/audio_decoder_unittest.cc (right): https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:24: nit: add using testing::ValuesIn; ? https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:35: struct AudioSample { nit: "Sample" seems strange term to me for what appears to me to be a summary of a group of decoded output frames. IIUC in context, 'sample' would be a discrete frame. Maybe s/AudioSample/AudioDecodeExpectation/ ? https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:49: return os << data.filename; nit: include decoder type or any summary of the samples? https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:62: new OpusAudioDecoder(message_loop_.message_loop_proxy())); nit: wrap previous line like Decoder(\n ? https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:76: pending_decode_ = true; First, ASSERT/EXPECT_FALSE(pending_decode_); and ASSERT/EXPECT_FALSE(pending_reset_); ? Or if decodes can be pending while sending EOS (or reset be pending while sending EOS), test? https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:89: reader_->Open(); nit: We only decode/verify the first enumerated audio stream (done in reader_). Do we need to verify more than one audio stream per file? (or TODO bug?) https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:94: start_timestamp_ = ConvertFromTimeBase( nit: Verify expected start timestamp? https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:103: AVCodecContextToAudioDecoderConfig( nit: Verify expected config? https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:104: reader_->codec_context_for_testing(), false, &config, false); Can the config change during any test? If not, verify same or just cache it as config_? https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:114: pending_decode_ = true; ditto: (expect nothing pending, or add tests if things could be pending when Decode() is called) https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:122: reader_->codec_context_for_testing()->time_base, packet.pts)); Do decoders care at all about decode timestamp? Do we need to buffer->SetDecodeTimestamp(....)? https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:129: av_free_packet(&packet); nit: Why free after RunUntilIdle() and not before? https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:133: pending_reset_ = true; ditto: (expect nothing pending, or add tests if things could be pending when Decode() is called) https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:140: decoder_->Stop(); nit: does setting some flag like |stopped_| true make sense, along with EXPECT_FALSE(stopped_) here and in Decode(), Reset(), and SendEndOfStream()? https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:147: const base::TimeDelta converted_time = ditto of audio_file_reader.cc comment: this looks really strange instead of just using int64 for converted_time. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:158: if (status == AudioDecoder::kNotEnoughData) { Verify versus expectation of not enough data? https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:169: EXPECT_EQ(status, AudioDecoder::kAborted); nit: here and multiple other places in this file: make like EXPECT_EQ(expectation, actual). See https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl... https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:185: // across platforms as audio varies slightly across platforms. Is cross-platform audio variance a bug? https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:194: // Generate an exact MD5 hash for comparison of multiple runs on the same nit: truncated comment https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:209: EXPECT_LT(i, decoded_audio_.size()); nit: s/EXPECT/ASSERT or CHECK/ https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:223: audio_hash.Update(output.get(), output->frames()); nit: I'm not sure the claim of "Value chosen by dice roll" for fuzzy hash is observed (https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_h...). Really, it's just a round-robin, right? https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:227: EXPECT_EQ(exact_hash, GetDecodedAudioMD5(i)); nit: have a test show that this portion of the method is working as intended (detects some error using exact hash)? https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:231: EXPECT_LT(i, decoded_audio_.size()); nit: s/EXPECT/ASSERT or CHECK/ https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:260: if (GetParam().decoder_type != OPUS_DECODER) nit: Can this test can be done explicitly (and simply) only for Opus by instantiation? https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:319: Stop(); We had some further verification in previous FFmpegAudioDecoderTest.DecodeAbort() like the following. Can we encode some (possibly per-decoder) expectation and verify it here? EXPECT_EQ(decoded_audio_.size(), 1u); EXPECT_TRUE(decoded_audio_[0].get() == NULL); https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:326: SatisfyPendingDecode(); Is there some deterministic [partial/empty] decode output we could verify here? https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:334: Stop(); ditto https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:342: SatisfyPendingDecode(); ditto https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_file... File media/filters/audio_file_reader.cc (right): https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_file... media/filters/audio_file_reader.cc:260: bool AudioFileReader::SeekForTesting(base::TimeDelta seek_time) { How ingrained is embedding alternate timebase times in a base::TimeDelta? It seems strange / confusing versus just int64. Does it give us any advantage like non-finite or invalid value representation that is meaningful for wherever we put a non-microsecond time value as a microseconds in a TimeDelta? Especially strange to me if this is ingrained in our non-test code. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_file... File media/filters/audio_file_reader_unittest.cc (right): https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_file... media/filters/audio_file_reader_unittest.cc:51: // the actual first packet, but it should correspond to the first decoded If not the actual first packet, why not? (elaborate in comment, please) https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_file... media/filters/audio_file_reader_unittest.cc:53: ASSERT_TRUE(reader_->SeekForTesting(base::TimeDelta())); nit: is base::TimeDelta() the first timestamp in all the test data? Perhaps I'm confused about av_seek_frame(), but if 0 isn't the lowest/first pts, then would seek fail?
Just comments. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... File media/filters/audio_decoder_unittest.cc (right): https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:24: On 2014/06/06 21:00:03, wolenetz wrote: > nit: add using testing::ValuesIn; ? No, I dislike those. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:49: return os << data.filename; On 2014/06/06 21:00:04, wolenetz wrote: > nit: include decoder type or any summary of the samples? No, not relevant for figuring out what's wrong. Those type of values are already printed by the EXPECT/ASSERT checks. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:62: new OpusAudioDecoder(message_loop_.message_loop_proxy())); On 2014/06/06 21:00:04, wolenetz wrote: > nit: wrap previous line like Decoder(\n ? Are you arguing with our deity, clang-format?! :) https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:89: reader_->Open(); On 2014/06/06 21:00:04, wolenetz wrote: > nit: We only decode/verify the first enumerated audio stream (done in reader_). > Do we need to verify more than one audio stream per file? (or TODO bug?) No, we don't support multiple streams of the same type per file. We always choose the first audio and first video streams. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:94: start_timestamp_ = ConvertFromTimeBase( On 2014/06/06 21:00:04, wolenetz wrote: > nit: Verify expected start timestamp? Maybe. I'll see about adding it. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:103: AVCodecContextToAudioDecoderConfig( On 2014/06/06 21:00:04, wolenetz wrote: > nit: Verify expected config? Maybe. I could verify codec, channels, and sample rate, but not the extradata sections. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:104: reader_->codec_context_for_testing(), false, &config, false); On 2014/06/06 21:00:05, wolenetz wrote: > Can the config change during any test? If not, verify same or just cache it as > config_? This is already checked within the decoder. Only explicit config changes are allowed. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:122: reader_->codec_context_for_testing()->time_base, packet.pts)); On 2014/06/06 21:00:03, wolenetz wrote: > Do decoders care at all about decode timestamp? Do we need to > buffer->SetDecodeTimestamp(....)? No. DecodeTimestamp is a StreamParserBuffer only construct. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:129: av_free_packet(&packet); On 2014/06/06 21:00:03, wolenetz wrote: > nit: Why free after RunUntilIdle() and not before? To ensure all operations which might be using the packet have completed. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:147: const base::TimeDelta converted_time = On 2014/06/06 21:00:04, wolenetz wrote: > ditto of audio_file_reader.cc comment: this looks really strange instead of just > using int64 for converted_time. Reading the docs, it looks like this is wrong. The time given is expected to be in microseconds and will be converted into the appropriate timebase automatically. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:158: if (status == AudioDecoder::kNotEnoughData) { On 2014/06/06 21:00:04, wolenetz wrote: > Verify versus expectation of not enough data? I don't understand what you're saying. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:185: // across platforms as audio varies slightly across platforms. On 2014/06/06 21:00:04, wolenetz wrote: > Is cross-platform audio variance a bug? No, it's expected due to differing implementations of the standard libraries with regards to things like sin(), cos(), etc. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:223: audio_hash.Update(output.get(), output->frames()); On 2014/06/06 21:00:05, wolenetz wrote: > nit: I'm not sure the claim of "Value chosen by dice roll" for fuzzy hash is > observed > (https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_h...). > Really, it's just a round-robin, right? Again, I don't understand what you're asking. The hash uses a bucketing system to ensure positional correctness. See the unit tests for AudioHash. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:260: if (GetParam().decoder_type != OPUS_DECODER) On 2014/06/06 21:00:03, wolenetz wrote: > nit: Can this test can be done explicitly (and simply) only for Opus by > instantiation? Possibly, but it's dirty since I'll need to make AudioDecoderTest a base class which can be extended by the new Opus test. It'll have to avoid extending testing::WithParamInterface, which makes some of the GetParam() calls tricky (they'll need to be virtual methods). https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:326: SatisfyPendingDecode(); On 2014/06/06 21:00:04, wolenetz wrote: > Is there some deterministic [partial/empty] decode output we could verify here? I don't understand what you're asking. ProduceAudioSamples is sufficient testing of the actual decoded data. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_file... File media/filters/audio_file_reader.cc (right): https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_file... media/filters/audio_file_reader.cc:260: bool AudioFileReader::SeekForTesting(base::TimeDelta seek_time) { On 2014/06/06 21:00:05, wolenetz wrote: > How ingrained is embedding alternate timebase times in a base::TimeDelta? It > seems strange / confusing versus just int64. Does it give us any advantage like > non-finite or invalid value representation that is meaningful for wherever we > put a non-microsecond time value as a microseconds in a TimeDelta? Especially > strange to me if this is ingrained in our non-test code. See my comment about skipping timebase conversion when calling this function. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_file... File media/filters/audio_file_reader_unittest.cc (right): https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_file... media/filters/audio_file_reader_unittest.cc:51: // the actual first packet, but it should correspond to the first decoded On 2014/06/06 21:00:05, wolenetz wrote: > If not the actual first packet, why not? (elaborate in comment, please) I think this is sufficient. I don't need to explain the details of why ogg in a pita here :) https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_file... media/filters/audio_file_reader_unittest.cc:53: ASSERT_TRUE(reader_->SeekForTesting(base::TimeDelta())); On 2014/06/06 21:00:05, wolenetz wrote: > nit: is base::TimeDelta() the first timestamp in all the test data? Perhaps I'm > confused about av_seek_frame(), but if 0 isn't the lowest/first pts, then would > seek fail? Setting AV_SEEK_FLAG_BACKWARD chooses the first PTS <= 0
Just responses. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... File media/filters/audio_decoder_unittest.cc (right): https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:62: new OpusAudioDecoder(message_loop_.message_loop_proxy())); On 2014/06/06 21:17:00, DaleCurtis wrote: > On 2014/06/06 21:00:04, wolenetz wrote: > > nit: wrap previous line like Decoder(\n ? > > Are you arguing with our deity, clang-format?! :) I figured as much. No worries. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:147: const base::TimeDelta converted_time = On 2014/06/06 21:16:59, DaleCurtis wrote: > On 2014/06/06 21:00:04, wolenetz wrote: > > ditto of audio_file_reader.cc comment: this looks really strange instead of > just > > using int64 for converted_time. > > Reading the docs, it looks like this is wrong. The time given is expected to be > in microseconds and will be converted into the appropriate timebase > automatically. I'm not sure about the automatic conversion, unless AVStream.time_base always corresponds to microseconds. See https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg.... It appears per this comment that av_seek_frame is expecting an int64 already converted to AVStream.time_base. Which docs are you reading, or am I misreading? https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:158: if (status == AudioDecoder::kNotEnoughData) { On 2014/06/06 21:16:59, DaleCurtis wrote: > On 2014/06/06 21:00:04, wolenetz wrote: > > Verify versus expectation of not enough data? > > I don't understand what you're saying. Sorry. Can we deterministically expect and verify kNotEnoughData status in DecodeFinished? Or is this also platform-specific? https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:185: // across platforms as audio varies slightly across platforms. On 2014/06/06 21:17:00, DaleCurtis wrote: > On 2014/06/06 21:00:04, wolenetz wrote: > > Is cross-platform audio variance a bug? > > No, it's expected due to differing implementations of the standard libraries > with regards to things like sin(), cos(), etc. Much learning for me today. Many wow :) https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:223: audio_hash.Update(output.get(), output->frames()); On 2014/06/06 21:17:00, DaleCurtis wrote: > On 2014/06/06 21:00:05, wolenetz wrote: > > nit: I'm not sure the claim of "Value chosen by dice roll" for fuzzy hash is > > observed > > > (https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_h...). > > Really, it's just a round-robin, right? > > Again, I don't understand what you're asking. The hash uses a bucketing system > to ensure positional correctness. See the unit tests for AudioHash. I was commenting that the apparent 'randomness' of a 'dice roll' isn't what actually is used to bucket. If it were, then even the fuzzy hashes wouldn't match across multiple runs. So my nit was really with the AudioHash comment https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_h..., and not with this change. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:260: if (GetParam().decoder_type != OPUS_DECODER) On 2014/06/06 21:17:00, DaleCurtis wrote: > On 2014/06/06 21:00:03, wolenetz wrote: > > nit: Can this test can be done explicitly (and simply) only for Opus by > > instantiation? > > Possibly, but it's dirty since I'll need to make AudioDecoderTest a base class > which can be extended by the new Opus test. It'll have to avoid extending > testing::WithParamInterface, which makes some of the GetParam() calls tricky > (they'll need to be virtual methods). That's icky. Please forget I asked :). https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:326: SatisfyPendingDecode(); On 2014/06/06 21:17:00, DaleCurtis wrote: > On 2014/06/06 21:00:04, wolenetz wrote: > > Is there some deterministic [partial/empty] decode output we could verify > here? > > I don't understand what you're asking. ProduceAudioSamples is sufficient testing > of the actual decoded data. Sorry. I'm confused about the difference between this test and DecodeAbort, since the last thing Stop() does is RunUntilIdle(), and all SatisfyPendingDecode() does is RunUntilIdle(). Are you expecting some different decoder output versus DecodeAbort, and if so, can it be verified? https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_file... File media/filters/audio_file_reader_unittest.cc (right): https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_file... media/filters/audio_file_reader_unittest.cc:53: ASSERT_TRUE(reader_->SeekForTesting(base::TimeDelta())); On 2014/06/06 21:17:00, DaleCurtis wrote: > On 2014/06/06 21:00:05, wolenetz wrote: > > nit: is base::TimeDelta() the first timestamp in all the test data? Perhaps > I'm > > confused about av_seek_frame(), but if 0 isn't the lowest/first pts, then > would > > seek fail? > > Setting AV_SEEK_FLAG_BACKWARD chooses the first PTS <= 0 Ok. I was confusing with MSE where lowest PTS in a media segment after an init segment can be > 0.
PTAL. All comments addressed. Bugs squashed. FFmpeg roll included to pick up necessary fixes for these tests to pass. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... File media/filters/audio_decoder_unittest.cc (right): https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:35: struct AudioSample { On 2014/06/06 21:00:03, wolenetz wrote: > nit: "Sample" seems strange term to me for what appears to me to be a summary of > a group of decoded output frames. IIUC in context, 'sample' would be a discrete > frame. Maybe s/AudioSample/AudioDecodeExpectation/ ? Done. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:76: pending_decode_ = true; On 2014/06/06 21:00:04, wolenetz wrote: > First, ASSERT/EXPECT_FALSE(pending_decode_); and > ASSERT/EXPECT_FALSE(pending_reset_); ? Or if decodes can be pending while > sending EOS (or reset be pending while sending EOS), test? Done. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:94: start_timestamp_ = ConvertFromTimeBase( On 2014/06/06 21:16:59, DaleCurtis wrote: > On 2014/06/06 21:00:04, wolenetz wrote: > > nit: Verify expected start timestamp? > > Maybe. I'll see about adding it. Done. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:103: AVCodecContextToAudioDecoderConfig( On 2014/06/06 21:17:00, DaleCurtis wrote: > On 2014/06/06 21:00:04, wolenetz wrote: > > nit: Verify expected config? > > Maybe. I could verify codec, channels, and sample rate, but not the extradata > sections. Done. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:114: pending_decode_ = true; On 2014/06/06 21:00:04, wolenetz wrote: > ditto: (expect nothing pending, or add tests if things could be pending when > Decode() is called) Done. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:133: pending_reset_ = true; On 2014/06/06 21:00:04, wolenetz wrote: > ditto: (expect nothing pending, or add tests if things could be pending when > Decode() is called) Done. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:140: decoder_->Stop(); On 2014/06/06 21:00:05, wolenetz wrote: > nit: does setting some flag like |stopped_| true make sense, along with > EXPECT_FALSE(stopped_) here and in Decode(), Reset(), and SendEndOfStream()? Seems unnecessary, that'll blow up in all sorts of other ways. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:147: const base::TimeDelta converted_time = On 2014/06/06 21:52:31, wolenetz wrote: > On 2014/06/06 21:16:59, DaleCurtis wrote: > > On 2014/06/06 21:00:04, wolenetz wrote: > > > ditto of audio_file_reader.cc comment: this looks really strange instead of > > just > > > using int64 for converted_time. > > > > Reading the docs, it looks like this is wrong. The time given is expected to > be > > in microseconds and will be converted into the appropriate timebase > > automatically. > > I'm not sure about the automatic conversion, unless AVStream.time_base always > corresponds to microseconds. See > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg.... > It appears per this comment that av_seek_frame is expecting an int64 already > converted to AVStream.time_base. Which docs are you reading, or am I misreading? Ah yeah, you're correct. I need to convert to the stream timebase when specifying a seek index. I was looking at when the index is set to -1. Fixed internally by SeekForTesting(), all other clients don't need to care. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:158: if (status == AudioDecoder::kNotEnoughData) { On 2014/06/06 21:52:31, wolenetz wrote: > On 2014/06/06 21:16:59, DaleCurtis wrote: > > On 2014/06/06 21:00:04, wolenetz wrote: > > > Verify versus expectation of not enough data? > > > > I don't understand what you're saying. > > Sorry. Can we deterministically expect and verify kNotEnoughData status in > DecodeFinished? Or is this also platform-specific? Not easily and I don't think it adds any value that the tests aren't already verifying. It would depend on the codec and what the test is doing. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:169: EXPECT_EQ(status, AudioDecoder::kAborted); On 2014/06/06 21:00:04, wolenetz wrote: > nit: here and multiple other places in this file: make like > EXPECT_EQ(expectation, actual). See > https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl... Done. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:194: // Generate an exact MD5 hash for comparison of multiple runs on the same On 2014/06/06 21:00:05, wolenetz wrote: > nit: truncated comment Removed. Function comment says more. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:209: EXPECT_LT(i, decoded_audio_.size()); On 2014/06/06 21:00:03, wolenetz wrote: > nit: s/EXPECT/ASSERT or CHECK/ Done. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:223: audio_hash.Update(output.get(), output->frames()); On 2014/06/06 21:52:31, wolenetz wrote: > On 2014/06/06 21:17:00, DaleCurtis wrote: > > On 2014/06/06 21:00:05, wolenetz wrote: > > > nit: I'm not sure the claim of "Value chosen by dice roll" for fuzzy hash > is > > > observed > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_h...). > > > Really, it's just a round-robin, right? > > > > Again, I don't understand what you're asking. The hash uses a bucketing > system > > to ensure positional correctness. See the unit tests for AudioHash. > > I was commenting that the apparent 'randomness' of a 'dice roll' isn't what > actually is used to bucket. If it were, then even the fuzzy hashes wouldn't > match across multiple runs. So my nit was really with the AudioHash comment > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_h..., > and not with this change. Still don't understand :) That value is used for the modulus of the index which determines what bucket the data is hashed into: https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_h... https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:227: EXPECT_EQ(exact_hash, GetDecodedAudioMD5(i)); On 2014/06/06 21:00:04, wolenetz wrote: > nit: have a test show that this portion of the method is working as intended > (detects some error using exact hash)? Done. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:231: EXPECT_LT(i, decoded_audio_.size()); On 2014/06/06 21:00:03, wolenetz wrote: > nit: s/EXPECT/ASSERT or CHECK/ Done. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:319: Stop(); On 2014/06/06 21:00:04, wolenetz wrote: > We had some further verification in previous > FFmpegAudioDecoderTest.DecodeAbort() like the following. Can we encode some > (possibly per-decoder) expectation and verify it here? > EXPECT_EQ(decoded_audio_.size(), 1u); > EXPECT_TRUE(decoded_audio_[0].get() == NULL); Fixed. Exposed a bug in the opus decoder! https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:326: SatisfyPendingDecode(); On 2014/06/06 21:52:31, wolenetz wrote: > On 2014/06/06 21:17:00, DaleCurtis wrote: > > On 2014/06/06 21:00:04, wolenetz wrote: > > > Is there some deterministic [partial/empty] decode output we could verify > > here? > > > > I don't understand what you're asking. ProduceAudioSamples is sufficient > testing > > of the actual decoded data. > > Sorry. I'm confused about the difference between this test and DecodeAbort, > since the last thing Stop() does is RunUntilIdle(), and all > SatisfyPendingDecode() does is RunUntilIdle(). Are you expecting some different > decoder output versus DecodeAbort, and if so, can it be verified? Reworked since this covers the Decode() -> Stop() w/o a Reset() sequence. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:334: Stop(); On 2014/06/06 21:00:03, wolenetz wrote: > ditto Nuked. ProduceAudioSamples covers this now. The satisfy part has been incorrect since we switched to decoder stream. https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:342: SatisfyPendingDecode(); On 2014/06/06 21:00:05, wolenetz wrote: > ditto Nuked, ProduceAudioSamples covers the Reset() -> Stop() test sequence now.
DEPS roll now landing with https://codereview.chromium.org/325503003/
Looking pretty good. I'm not clear why sometimes ASSERT_NO_FATAL_FAILURE(...) is used and other times it is not. What is the guidance for when/when not to use this? https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... File media/filters/audio_decoder_unittest.cc (right): https://codereview.chromium.org/311373004/diff/70001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:223: audio_hash.Update(output.get(), output->frames()); On 2014/06/12 22:00:48, DaleCurtis wrote: > On 2014/06/06 21:52:31, wolenetz wrote: > > On 2014/06/06 21:17:00, DaleCurtis wrote: > > > On 2014/06/06 21:00:05, wolenetz wrote: > > > > nit: I'm not sure the claim of "Value chosen by dice roll" for fuzzy hash > > is > > > > observed > > > > > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_h...). > > > > Really, it's just a round-robin, right? > > > > > > Again, I don't understand what you're asking. The hash uses a bucketing > > system > > > to ensure positional correctness. See the unit tests for AudioHash. > > > > I was commenting that the apparent 'randomness' of a 'dice roll' isn't what > > actually is used to bucket. If it were, then even the fuzzy hashes wouldn't > > match across multiple runs. So my nit was really with the AudioHash comment > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_h..., > > and not with this change. > > Still don't understand :) That value is used for the modulus of the index which > determines what bucket the data is hashed into: > > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_h... We're missing each other :) I think AudioHash's claim in source comment "Value chosen by dice roll" should instead be something like "Bucket selected by position index modulo kHashBuckets". "Dice roll" implies random bucket selection versus we're using deterministic bucket selection based on a function of sample position. https://codereview.chromium.org/311373004/diff/90001/DEPS File DEPS (right): https://codereview.chromium.org/311373004/diff/90001/DEPS#newcode211 DEPS:211: "/chromium/third_party/ffmpeg.git@1e661a63bbd0b686d33ba7c875185a044a7be9ce", DEPS roll that was (temporarily) landed in https://codereview.chromium.org/325503003/ was reverted. Note that there are also at least three more recent commits than 1e661a63. https://codereview.chromium.org/311373004/diff/90001/media/filters/audio_deco... File media/filters/audio_decoder_unittest.cc (right): https://codereview.chromium.org/311373004/diff/90001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:108: EXPECT_EQ(GetParam().first_packet_pts_us, packet.pts); Explicitly compare a converted-to-microseconds packet.pts, or rename first_packet_pts_us to just first_packet_pts. I prefer the former. https://codereview.chromium.org/311373004/diff/90001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:128: decoder_->Initialize(config, NewExpectedStatusCB(PIPELINE_OK)); ToT has recent change around decoder init + decode output CB that appears to not be included here. See 49cea86805ad5d7cf5163a480f864b61d079889e https://codereview.chromium.org/311373004/diff/90001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:262: return decoded_audio_[i]; nit: bounds-check |i| https://codereview.chromium.org/311373004/diff/90001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:403: {46439, 23219, "-1.43,-1.25,0.11,1.29,1.86,0.14,"}, Did bear.ogv change, are these changes vs PS2 due to ffmpeg roll, or something else? https://codereview.chromium.org/311373004/diff/90001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:425: {FFMPEG, kCodecVorbis, "bear.ogv", kBearOgvExpectations, -704, 44100, Negative start timestamp?! Is this normal? https://codereview.chromium.org/311373004/diff/90001/media/filters/audio_file... File media/filters/audio_file_reader_unittest.cc (right): https://codereview.chromium.org/311373004/diff/90001/media/filters/audio_file... media/filters/audio_file_reader_unittest.cc:54: void VerifyPackets() { Verify packet pts/dts/duration and any other important packet metadata also is the same across each of kTestPasses? (perhaps for a subset of types, since ogg metadata is not correctly demuxed the second time around?) https://codereview.chromium.org/311373004/diff/90001/media/filters/audio_file... media/filters/audio_file_reader_unittest.cc:70: // demuxing. Currently our test files only have metadata in the first aside: Is this a bug upstream? Do we use the metadata downstream of demuxer such that its absence on re-demux causes problems?? What kind of metadata is this? https://codereview.chromium.org/311373004/diff/90001/media/filters/audio_file... media/filters/audio_file_reader_unittest.cc:71: // packet, so limit the fix to those packets. nit: s/limit the fix to those packets/explicitly drop metadata from the first demuxed ogg packet/ https://codereview.chromium.org/311373004/diff/90001/media/filters/opus_audio... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/311373004/diff/90001/media/filters/opus_audio... media/filters/opus_audio_decoder.cc:293: OpusAudioDecoder::~OpusAudioDecoder() {} Enforce that ::Stop() is called before this if Initialize() is pending or has successfully completed. See https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_d... and DCHECKs in https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/ffmp... This is orthogonal enough to be in a different CL if you like.
PTAL. Tests expanded further! :) https://codereview.chromium.org/311373004/diff/90001/DEPS File DEPS (right): https://codereview.chromium.org/311373004/diff/90001/DEPS#newcode211 DEPS:211: "/chromium/third_party/ffmpeg.git@1e661a63bbd0b686d33ba7c875185a044a7be9ce", On 2014/06/16 23:36:08, wolenetz wrote: > DEPS roll that was (temporarily) landed in > https://codereview.chromium.org/325503003/ was reverted. > > Note that there are also at least three more recent commits than 1e661a63. Still planning on landing this within the other patch. Removed. https://codereview.chromium.org/311373004/diff/90001/media/filters/audio_deco... File media/filters/audio_decoder_unittest.cc (right): https://codereview.chromium.org/311373004/diff/90001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:108: EXPECT_EQ(GetParam().first_packet_pts_us, packet.pts); On 2014/06/16 23:36:08, wolenetz wrote: > Explicitly compare a converted-to-microseconds packet.pts, or rename > first_packet_pts_us to just first_packet_pts. I prefer the former. Prefer the latter. https://codereview.chromium.org/311373004/diff/90001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:128: decoder_->Initialize(config, NewExpectedStatusCB(PIPELINE_OK)); On 2014/06/16 23:36:08, wolenetz wrote: > ToT has recent change around decoder init + decode output CB that appears to not > be included here. See 49cea86805ad5d7cf5163a480f864b61d079889e Merged. https://codereview.chromium.org/311373004/diff/90001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:262: return decoded_audio_[i]; On 2014/06/16 23:36:08, wolenetz wrote: > nit: bounds-check |i| Decline. This is a hacker style method meant to mirror vector [] access. All callers should have checks in place for this already. https://codereview.chromium.org/311373004/diff/90001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:403: {46439, 23219, "-1.43,-1.25,0.11,1.29,1.86,0.14,"}, On 2014/06/16 23:36:08, wolenetz wrote: > Did bear.ogv change, are these changes vs PS2 due to ffmpeg roll, or something > else? FFmpeg roll. https://codereview.chromium.org/311373004/diff/90001/media/filters/audio_deco... media/filters/audio_decoder_unittest.cc:425: {FFMPEG, kCodecVorbis, "bear.ogv", kBearOgvExpectations, -704, 44100, On 2014/06/16 23:36:08, wolenetz wrote: > Negative start timestamp?! Is this normal? Yup, I've added a comment with some more info. https://codereview.chromium.org/311373004/diff/90001/media/filters/audio_file... File media/filters/audio_file_reader_unittest.cc (right): https://codereview.chromium.org/311373004/diff/90001/media/filters/audio_file... media/filters/audio_file_reader_unittest.cc:54: void VerifyPackets() { On 2014/06/16 23:36:08, wolenetz wrote: > Verify packet pts/dts/duration and any other important packet metadata also is > the same across each of kTestPasses? (perhaps for a subset of types, since ogg > metadata is not correctly demuxed the second time around?) Not here, AudioFileReader doesn't care about timestamps. AudioDecoderTest handles all that. https://codereview.chromium.org/311373004/diff/90001/media/filters/audio_file... media/filters/audio_file_reader_unittest.cc:70: // demuxing. Currently our test files only have metadata in the first On 2014/06/16 23:36:08, wolenetz wrote: > aside: Is this a bug upstream? Do we use the metadata downstream of demuxer such > that its absence on re-demux causes problems?? What kind of metadata is this? No, I figured out the problem. I hadn't removed side data. New patch set removes all this code. https://codereview.chromium.org/311373004/diff/90001/media/filters/audio_file... media/filters/audio_file_reader_unittest.cc:71: // packet, so limit the fix to those packets. On 2014/06/16 23:36:08, wolenetz wrote: > nit: s/limit the fix to those packets/explicitly drop metadata from the first > demuxed ogg packet/ Removed. https://codereview.chromium.org/311373004/diff/90001/media/filters/opus_audio... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/311373004/diff/90001/media/filters/opus_audio... media/filters/opus_audio_decoder.cc:293: OpusAudioDecoder::~OpusAudioDecoder() {} On 2014/06/16 23:36:08, wolenetz wrote: > Enforce that ::Stop() is called before this if Initialize() is pending or has > successfully completed. See > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_d... > and DCHECKs in > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/ffmp... > > This is orthogonal enough to be in a different CL if you like. Added DCHECK() as best-effort since decoder doesn't keep state.
lgtm % nits https://codereview.chromium.org/311373004/diff/130001/media/filters/audio_dec... File media/filters/audio_decoder_unittest.cc (right): https://codereview.chromium.org/311373004/diff/130001/media/filters/audio_dec... media/filters/audio_decoder_unittest.cc:62: static void SetDiscardPadding(AVPacket* packet, nit: as mentioned offline, this looks similar to what ffmpegdemuxer does now. Consider lifting to common routine if code maintenance benefit outweighs cost. I'm fine if left to separate (or never) CL at your discretion. https://codereview.chromium.org/311373004/diff/130001/media/filters/audio_dec... media/filters/audio_decoder_unittest.cc:82: if (skip_samples_size < 4) nit: why just 4 here, versus 10 in ffmpegdemuxer? (https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/ffmp...) https://codereview.chromium.org/311373004/diff/130001/media/filters/audio_dec... media/filters/audio_decoder_unittest.cc:405: const uint8_t kOpusExtraData[] = { nit: Lift this duplicate to common definition (same is used in InitializeWithNoCodecDelay) https://codereview.chromium.org/311373004/diff/130001/media/filters/audio_fil... File media/filters/audio_file_reader.cc (right): https://codereview.chromium.org/311373004/diff/130001/media/filters/audio_fil... media/filters/audio_file_reader.cc:75: if (result < 0) { nit: consistency : s/if.../DLOG_IF like you did above?
https://codereview.chromium.org/311373004/diff/130001/media/filters/audio_fil... File media/filters/audio_file_reader.cc (right): https://codereview.chromium.org/311373004/diff/130001/media/filters/audio_fil... media/filters/audio_file_reader.cc:75: if (result < 0) { On 2014/06/19 20:56:47, wolenetz_OOO_June20-June29 wrote: > nit: consistency : s/if.../DLOG_IF like you did above? actually, forget this nit... there's a case where we don't yet want to return, so it's not worth using DLOG_IF here.
Thanks for review! https://codereview.chromium.org/311373004/diff/130001/media/filters/audio_dec... File media/filters/audio_decoder_unittest.cc (right): https://codereview.chromium.org/311373004/diff/130001/media/filters/audio_dec... media/filters/audio_decoder_unittest.cc:62: static void SetDiscardPadding(AVPacket* packet, On 2014/06/19 20:56:47, wolenetz_OOO_June20-June29 wrote: > nit: as mentioned offline, this looks similar to what ffmpegdemuxer does now. > Consider lifting to common routine if code maintenance benefit outweighs cost. > I'm fine if left to separate (or never) CL at your discretion. Leaving here as its different enough that lifting it out would just make things more confusing than not. https://codereview.chromium.org/311373004/diff/130001/media/filters/audio_dec... media/filters/audio_decoder_unittest.cc:82: if (skip_samples_size < 4) On 2014/06/19 20:56:47, wolenetz_OOO_June20-June29 wrote: > nit: why just 4 here, versus 10 in ffmpegdemuxer? > (https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/ffmp...) We only use the skip_samples and not the discard end padding. https://codereview.chromium.org/311373004/diff/130001/media/filters/audio_dec... media/filters/audio_decoder_unittest.cc:405: const uint8_t kOpusExtraData[] = { On 2014/06/19 20:56:47, wolenetz_OOO_June20-June29 wrote: > nit: Lift this duplicate to common definition (same is used in > InitializeWithNoCodecDelay) Done.
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/311373004/150001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
Message was sent while issue was closed.
Change committed as 278667 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
