Wire up codec_delay() to MP3StreamParser and FFmpegAudioDecoder.

Wire up codec_delay() to MP3StreamParser and FFmpegAudioDecoder.

Allows us to drop silent samples from the decoder after initialization
and resets.

codec_delay() is always in frames, so I've changed all call sites to use
frames instead of a TimeDelta.

Coincidentally also fixes an issue where not enough data was dropped from
Vorbis buffers with negative timestamps.

BUG=360961
TEST=new pipeline_integration_test

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264763

[DaleCurtis, 2014-04-15 22:22:26]

https://codereview.chromium.org/239423005/diff/1/media/formats/mpeg/mpeg_audi...
File media/formats/mpeg/mpeg_audio_stream_parser_base.cc (right):

https://codereview.chromium.org/239423005/diff/1/media/formats/mpeg/mpeg_audi...
media/formats/mpeg/mpeg_audio_stream_parser_base.cc:215:
timestamp_helper_->GetFrameDuration(codec_delay_in_frames_));
I'm wondering if we should just change codec_delay() to be in frames instead of
base::TimeDelta since we're essentially converting it unnecessarily in all cases
right now.

[acolwell GONE FROM CHROMIUM, 2014-04-16 15:47:35]

https://codereview.chromium.org/239423005/diff/1/media/formats/mpeg/mpeg_audi...
File media/formats/mpeg/mpeg_audio_stream_parser_base.cc (right):

https://codereview.chromium.org/239423005/diff/1/media/formats/mpeg/mpeg_audi...
media/formats/mpeg/mpeg_audio_stream_parser_base.cc:215:
timestamp_helper_->GetFrameDuration(codec_delay_in_frames_));
On 2014/04/15 22:22:26, DaleCurtis wrote:
> I'm wondering if we should just change codec_delay() to be in frames instead
of
> base::TimeDelta since we're essentially converting it unnecessarily in all
cases
> right now.

Yes. I think the coding_delay should be in frames since that is the more common
use of this value.

[DaleCurtis, 2014-04-16 18:22:22]

https://codereview.chromium.org/239423005/diff/1/media/formats/mpeg/mpeg_audi...
File media/formats/mpeg/mpeg_audio_stream_parser_base.cc (right):

https://codereview.chromium.org/239423005/diff/1/media/formats/mpeg/mpeg_audi...
media/formats/mpeg/mpeg_audio_stream_parser_base.cc:215:
timestamp_helper_->GetFrameDuration(codec_delay_in_frames_));
On 2014/04/16 15:47:35, acolwell wrote:
> On 2014/04/15 22:22:26, DaleCurtis wrote:
> > I'm wondering if we should just change codec_delay() to be in frames instead
> of
> > base::TimeDelta since we're essentially converting it unnecessarily in all
> cases
> > right now.
> 
> Yes. I think the coding_delay should be in frames since that is the more
common
> use of this value.

Do you have a preference in naming? Keep codec_delay() or rename to something
like codec_delay_in_frames()?

[DaleCurtis, 2014-04-16 20:44:02]

PTAL. Changing codec_delay() to be in samples resulted in some simplifications
to the OPUS decoder as well.

[acolwell GONE FROM CHROMIUM, 2014-04-17 21:11:23]

lgtm % nits

https://codereview.chromium.org/239423005/diff/20001/media/base/audio_decoder...
File media/base/audio_decoder_config.h (right):

https://codereview.chromium.org/239423005/diff/20001/media/base/audio_decoder...
media/base/audio_decoder_config.h:122: // |codec_delay_| is the number of
samples the decoder should discard before
nit: s/samples/frames/ . I know there are mixed signals in the names in this
class, but I'm pretty sure we mean frames here as in bytes_per_frame_

https://codereview.chromium.org/239423005/diff/20001/media/filters/ffmpeg_aud...
File media/filters/ffmpeg_audio_decoder.cc (left):

https://codereview.chromium.org/239423005/diff/20001/media/filters/ffmpeg_aud...
media/filters/ffmpeg_audio_decoder.cc:270: output_frames_to_drop_ = floor(0.5 +
-buffer->timestamp().InSecondsF() *
Why is this change needed?

https://codereview.chromium.org/239423005/diff/20001/media/filters/ffmpeg_aud...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/239423005/diff/20001/media/filters/ffmpeg_aud...
media/filters/ffmpeg_audio_decoder.cc:365:
output_timestamp_helper_->SetBaseTimestamp(base::TimeDelta());
Can we move this up into DecodeBuffer() next to the code that sets
output_frames_to_drop_? It seems like that would be cleaner by setting all the
state up in one place.

https://codereview.chromium.org/239423005/diff/20001/media/filters/opus_audio...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/239423005/diff/20001/media/filters/opus_audio...
media/filters/opus_audio_decoder.cc:495: // droped due to codec delay.
nit: s/droped/dropped/

https://codereview.chromium.org/239423005/diff/20001/media/formats/webm/webm_...
File media/formats/webm/webm_audio_client.cc (right):

https://codereview.chromium.org/239423005/diff/20001/media/formats/webm/webm_...
media/formats/webm/webm_audio_client.cc:71: codec_delay_in_samples =
nit: s/samples/frames/ ?

[DaleCurtis, 2014-04-17 21:42:48]

Thanks for review!

https://codereview.chromium.org/239423005/diff/20001/media/base/audio_decoder...
File media/base/audio_decoder_config.h (right):

https://codereview.chromium.org/239423005/diff/20001/media/base/audio_decoder...
media/base/audio_decoder_config.h:122: // |codec_delay_| is the number of
samples the decoder should discard before
On 2014/04/17 21:11:24, acolwell wrote:
> nit: s/samples/frames/ . I know there are mixed signals in the names in this
> class, but I'm pretty sure we mean frames here as in bytes_per_frame_

Done.

https://codereview.chromium.org/239423005/diff/20001/media/filters/ffmpeg_aud...
File media/filters/ffmpeg_audio_decoder.cc (left):

https://codereview.chromium.org/239423005/diff/20001/media/filters/ffmpeg_aud...
media/filters/ffmpeg_audio_decoder.cc:270: output_frames_to_drop_ = floor(0.5 +
-buffer->timestamp().InSecondsF() *
On 2014/04/17 21:11:24, acolwell wrote:
> Why is this change needed?

It's a cleanup from an earlier patch set.  I kept it since the floor() is
unnecessary for n >= 0.

https://codereview.chromium.org/239423005/diff/20001/media/filters/ffmpeg_aud...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/239423005/diff/20001/media/filters/ffmpeg_aud...
media/filters/ffmpeg_audio_decoder.cc:365:
output_timestamp_helper_->SetBaseTimestamp(base::TimeDelta());
On 2014/04/17 21:11:24, acolwell wrote:
> Can we move this up into DecodeBuffer() next to the code that sets
> output_frames_to_drop_? It seems like that would be cleaner by setting all the
> state up in one place.

Done.

https://codereview.chromium.org/239423005/diff/20001/media/filters/opus_audio...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/239423005/diff/20001/media/filters/opus_audio...
media/filters/opus_audio_decoder.cc:495: // droped due to codec delay.
On 2014/04/17 21:11:24, acolwell wrote:
> nit: s/droped/dropped/

Done.

https://codereview.chromium.org/239423005/diff/20001/media/formats/webm/webm_...
File media/formats/webm/webm_audio_client.cc (right):

https://codereview.chromium.org/239423005/diff/20001/media/formats/webm/webm_...
media/formats/webm/webm_audio_client.cc:71: codec_delay_in_samples =
On 2014/04/17 21:11:24, acolwell wrote:
> nit: s/samples/frames/ ?

Done.

[DaleCurtis, 2014-04-17 21:43:22]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-04-17 21:44:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/239423005/40001

[commit-bot: I haz the power, 2014-04-17 22:56:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-17 22:56:22]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[DaleCurtis, 2014-04-17 23:42:39]

Turns out a DCHECK() I added after reflowing the AudioTimestampHelper usage
exposed a vorbis frame dropping bug.  We were previously dropping too little
when multiple negative timestamp buffers came in. I've fixed this and added a
new check to catch regressions in the future.

[DaleCurtis, 2014-04-17 23:52:51]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-04-17 23:54:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/239423005/60001

[commit-bot: I haz the power, 2014-04-18 01:03:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-18 01:03:41]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[DaleCurtis, 2014-04-18 01:34:33]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-04-18 01:35:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/239423005/100001

[commit-bot: I haz the power, 2014-04-18 10:25:05]

Change committed as 264763