Cleanup OPUS decoder. Remove extraneous transforms and copies.

Cleanup OPUS decoder.  Remove extraneous transforms and copies.

Various cleanups:
- Fixes start trimming after seeks.
- Patches FFmpegDemuxer to workaround FFmpeg pre-stripping codec delay.
- Switches decoding to float on all platforms.
- Decodes directly into the AudioBuffer instead of making a copy.
- Switches various error logs to actually be DLOG(ERROR).
- Various tiny code cleanups.
- Rolls DEPS for a couple more OPUS FFmpeg fixes.

BUG=104241, 166752, 168524, 315165, 328207
TEST=opus decoding still works.

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

[acolwell GONE FROM CHROMIUM, 2013-12-11 23:30:02]

https://codereview.chromium.org/100503006/diff/110001/media/ffmpeg/ffmpeg_com...
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/100503006/diff/110001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:291: sample_format = kSampleFormatS16;
This is probably a separate CL, but... It seems like sample format only matters
for PCM "codecs". Other ones appear to be dependent on the actual decoder. I
wonder if we should just remove this code all together and modify
AudioDecoderConfig to specify/allow kUnknownSampleFormat for non-PCM codecs.

https://codereview.chromium.org/100503006/diff/110001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:316: if (codec != kCodecOpus) {
nit: Shouldn't this be checking for sample_format != kUnknownSampleFormat
instead?

https://codereview.chromium.org/100503006/diff/110001/media/filters/opus_audi...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/100503006/diff/110001/media/filters/opus_audi...
media/filters/opus_audio_decoder.cc:273: DLOG(ERROR) << "Initialize has already
been called.";
This looks like stale code. Please remove.

https://codereview.chromium.org/100503006/diff/110001/media/filters/opus_audi...
media/filters/opus_audio_decoder.cc:474: uint8
channel_mapping[kMaxVorbisChannels] = {0};
nit: Why is this change needed? Doesn't it immediately get overwritten by the
next few lines?

https://codereview.chromium.org/100503006/diff/110001/media/filters/opus_audi...
media/filters/opus_audio_decoder.cc:500: #if defined(OS_ANDROID)
nit: Any reason not to move these changes to the constructor since they don't
change during the life of the object?

https://codereview.chromium.org/100503006/diff/110001/media/filters/opus_audi...
media/filters/opus_audio_decoder.cc:542: #if defined(OS_ANDROID)
I don't believe we actually even use this code on Android. I'd prefer to defer
the #ifdefs until the code is actually used on that platform and we've verified
the perf difference is worth the #ifdef.

https://codereview.chromium.org/100503006/diff/110001/media/filters/opus_audi...
media/filters/opus_audio_decoder.cc:588: if (frames_to_discard_ > 0) {
This doesn't look right to me. It assumes that frames_decoded is >
frame_to_discard_. frames_to_discard_ can span multiple input buffers.

https://codereview.chromium.org/100503006/diff/110001/media/filters/opus_audi...
media/filters/opus_audio_decoder.cc:616: if
(output_timestamp_helper_->base_timestamp() == kNoTimestamp() &&
This also does not look right to me. This assumes that the first samples to be
rendered are actually at the beginning of the compressed frame. I don't believe
Opus requires that. I think the timestamp helper should be set to the first
input buffer like the old code did. I also think that AddFrames() should be
called for all frames output from the decoder, even the dropped ones. That way
the "output_timestamp_helper_->GetTimestamp() - timestamp_offset_" computation
will be correct even if the codec_delay doesn't happen to land on an input
buffer boundary.

For example say I have the following situation: (Assuming 1Hz so frames and
timestamps are the same units)
coded_delay = 100 frames -> timestamp_offset = 100, frames_to_discard_ = 100
input buffer 1 : timestamp 0, decoded_frames = 50 -> 
  * 0 frames output
  * output_timestamp_helper_->GetTimestamp() = 50
  * frames_to_discard_ = 50
input buffer 2 : timestamp 50, decoded_frames = 75 ->
  * 25 frames output
  * output_timestamp_helper_->GetTimestamp() = 100 (since it is only counting
dropped frames at this point)
  * frames_to_discard_ = 0 
  * output_timestamp_helper_->GetTimestamp() - timestamp_offset_ = 0 (which is
correct for the beginning of the output samples).

[DaleCurtis, 2013-12-12 00:03:24]

https://codereview.chromium.org/100503006/diff/110001/media/ffmpeg/ffmpeg_com...
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/100503006/diff/110001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:291: sample_format = kSampleFormatS16;
On 2013/12/11 23:30:02, acolwell wrote:
> This is probably a separate CL, but... It seems like sample format only
matters
> for PCM "codecs". Other ones appear to be dependent on the actual decoder. I
> wonder if we should just remove this code all together and modify
> AudioDecoderConfig to specify/allow kUnknownSampleFormat for non-PCM codecs.

Sounds reasonable, but for another CL.

https://codereview.chromium.org/100503006/diff/110001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:316: if (codec != kCodecOpus) {
On 2013/12/11 23:30:02, acolwell wrote:
> nit: Shouldn't this be checking for sample_format != kUnknownSampleFormat
> instead?

Not unless we make the change you talk about above. No config should have
kUnknownSampleFormat by this point.  Did you mean we should DCHECK() that as
well?

https://codereview.chromium.org/100503006/diff/110001/media/filters/opus_audi...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/100503006/diff/110001/media/filters/opus_audi...
media/filters/opus_audio_decoder.cc:273: DLOG(ERROR) << "Initialize has already
been called.";
On 2013/12/11 23:30:02, acolwell wrote:
> This looks like stale code. Please remove.

The whole block? Or just the DLOG?

https://codereview.chromium.org/100503006/diff/110001/media/filters/opus_audi...
media/filters/opus_audio_decoder.cc:474: uint8
channel_mapping[kMaxVorbisChannels] = {0};
On 2013/12/11 23:30:02, acolwell wrote:
> nit: Why is this change needed? Doesn't it immediately get overwritten by the
> next few lines?

Not necessary, but good for sanity. The memcpy below only fills 2 out of the 8
possible channels, it's not immediately obvious that RemapOpusChanneLayout() may
initialize all spots.

https://codereview.chromium.org/100503006/diff/110001/media/filters/opus_audi...
media/filters/opus_audio_decoder.cc:500: #if defined(OS_ANDROID)
On 2013/12/11 23:30:02, acolwell wrote:
> nit: Any reason not to move these changes to the constructor since they don't
> change during the life of the object?

Done.

https://codereview.chromium.org/100503006/diff/110001/media/filters/opus_audi...
media/filters/opus_audio_decoder.cc:542: #if defined(OS_ANDROID)
On 2013/12/11 23:30:02, acolwell wrote:
> I don't believe we actually even use this code on Android. I'd prefer to defer
> the #ifdefs until the code is actually used on that platform and we've
verified
> the perf difference is worth the #ifdef.

Hmm, vignesh said it works on Android? Vignesh?

https://codereview.chromium.org/100503006/diff/110001/media/filters/opus_audi...
media/filters/opus_audio_decoder.cc:588: if (frames_to_discard_ > 0) {
On 2013/12/11 23:30:02, acolwell wrote:
> This doesn't look right to me. It assumes that frames_decoded is >
> frame_to_discard_. frames_to_discard_ can span multiple input buffers.

Good eye, during testing I realized it was broken. Fixed.

https://codereview.chromium.org/100503006/diff/110001/media/filters/opus_audi...
media/filters/opus_audio_decoder.cc:616: if
(output_timestamp_helper_->base_timestamp() == kNoTimestamp() &&
On 2013/12/11 23:30:02, acolwell wrote:
> This also does not look right to me. This assumes that the first samples to be
> rendered are actually at the beginning of the compressed frame. I don't
believe
> Opus requires that. I think the timestamp helper should be set to the first
> input buffer like the old code did. I also think that AddFrames() should be
> called for all frames output from the decoder, even the dropped ones. That way
> the "output_timestamp_helper_->GetTimestamp() - timestamp_offset_" computation
> will be correct even if the codec_delay doesn't happen to land on an input
> buffer boundary.
> 
> For example say I have the following situation: (Assuming 1Hz so frames and
> timestamps are the same units)
> coded_delay = 100 frames -> timestamp_offset = 100, frames_to_discard_ = 100
> input buffer 1 : timestamp 0, decoded_frames = 50 -> 
>   * 0 frames output
>   * output_timestamp_helper_->GetTimestamp() = 50
>   * frames_to_discard_ = 50
> input buffer 2 : timestamp 50, decoded_frames = 75 ->
>   * 25 frames output
>   * output_timestamp_helper_->GetTimestamp() = 100 (since it is only counting
> dropped frames at this point)
>   * frames_to_discard_ = 0 
>   * output_timestamp_helper_->GetTimestamp() - timestamp_offset_ = 0 (which is
> correct for the beginning of the output samples).

Done.  Note: I still have some hackish detection for setting the startup
preskip. :-/

https://codereview.chromium.org/100503006/diff/150001/media/filters/opus_audi...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/100503006/diff/150001/media/filters/opus_audi...
media/filters/opus_audio_decoder.cc:398: if (last_input_timestamp_ ==
kNoTimestamp() &&
Hackish detection I'm talking about... suggestions welcome.  Works on all the
test clips I have.

[acolwell GONE FROM CHROMIUM, 2013-12-12 00:53:13]

https://codereview.chromium.org/100503006/diff/110001/media/ffmpeg/ffmpeg_com...
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/100503006/diff/110001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:316: if (codec != kCodecOpus) {
On 2013/12/12 00:03:24, DaleCurtis wrote:
> On 2013/12/11 23:30:02, acolwell wrote:
> > nit: Shouldn't this be checking for sample_format != kUnknownSampleFormat
> > instead?
> 
> Not unless we make the change you talk about above. No config should have
> kUnknownSampleFormat by this point.  Did you mean we should DCHECK() that as
> well?
ok. This was meant to go with the comment above.

https://codereview.chromium.org/100503006/diff/110001/media/filters/opus_audi...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/100503006/diff/110001/media/filters/opus_audi...
media/filters/opus_audio_decoder.cc:273: DLOG(ERROR) << "Initialize has already
been called.";
On 2013/12/12 00:03:24, DaleCurtis wrote:
> On 2013/12/11 23:30:02, acolwell wrote:
> > This looks like stale code. Please remove.
> 
> The whole block? Or just the DLOG?
The whole block.

https://codereview.chromium.org/100503006/diff/150001/media/filters/opus_audi...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/100503006/diff/150001/media/filters/opus_audi...
media/filters/opus_audio_decoder.cc:398: if (last_input_timestamp_ ==
kNoTimestamp() &&
On 2013/12/12 00:03:25, DaleCurtis wrote:
> Hackish detection I'm talking about... suggestions welcome.  Works on all the
> test clips I have.

:( This is needed because the decoder doesn't get any indication about where we
are seeking to? Perhaps add a TODO that indicates we need a way to be able to
differentiate between a seek to the start and a seek to any other place in the
file.

Why are there negative timestamps? Does ffmpeg automatically subtract the delay
from the packets in the container? Does it do this consistantly for Ogg & WebM?

https://codereview.chromium.org/100503006/diff/150001/media/filters/opus_audi...
media/filters/opus_audio_decoder.cc:618:
output_timestamp_helper_->GetTimestamp() - timestamp_offset_);
We may be double subtracting off the offset here depending on what the answers
to my FFmpeg questions are above. If you
DCHECK(output_buffer->get()->timestamp() >= base::TimeDelta()) will it ever fail
with your tests? I don't believe it should ever be negative.

[DaleCurtis, 2013-12-12 03:07:08]

https://codereview.chromium.org/100503006/diff/150001/media/filters/opus_audi...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/100503006/diff/150001/media/filters/opus_audi...
media/filters/opus_audio_decoder.cc:398: if (last_input_timestamp_ ==
kNoTimestamp() &&
On 2013/12/12 00:53:14, acolwell wrote:
> On 2013/12/12 00:03:25, DaleCurtis wrote:
> > Hackish detection I'm talking about... suggestions welcome.  Works on all
the
> > test clips I have.
> 
> :( This is needed because the decoder doesn't get any indication about where
we
> are seeking to? Perhaps add a TODO that indicates we need a way to be able to
> differentiate between a seek to the start and a seek to any other place in the
> file.
> 
> Why are there negative timestamps? Does ffmpeg automatically subtract the
delay
> from the packets in the container? Does it do this consistantly for Ogg &
WebM?

Correct, and this actually isn't enough to fix the problem. It appears Opus can
have valid output when timestamps are negative, so when we check if we're before
Preroll @ ts=0 we skip that data :(

I've uploaded a very hacky patch set that works around this. We should discuss a
cleaner way tomorrow.

[DaleCurtis, 2013-12-12 04:12:12]

https://codereview.chromium.org/100503006/diff/150001/media/filters/opus_audi...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/100503006/diff/150001/media/filters/opus_audi...
media/filters/opus_audio_decoder.cc:398: if (last_input_timestamp_ ==
kNoTimestamp() &&
On 2013/12/12 03:07:08, DaleCurtis wrote:
> On 2013/12/12 00:53:14, acolwell wrote:
> > On 2013/12/12 00:03:25, DaleCurtis wrote:
> > > Hackish detection I'm talking about... suggestions welcome.  Works on all
> the
> > > test clips I have.
> > 
> > :( This is needed because the decoder doesn't get any indication about where
> we
> > are seeking to? Perhaps add a TODO that indicates we need a way to be able
to
> > differentiate between a seek to the start and a seek to any other place in
the
> > file.
> > 
> > Why are there negative timestamps? Does ffmpeg automatically subtract the
> delay
> > from the packets in the container? Does it do this consistantly for Ogg &
> WebM?
> 
> Correct, and this actually isn't enough to fix the problem. It appears Opus
can
> have valid output when timestamps are negative, so when we check if we're
before
> Preroll @ ts=0 we skip that data :(
> 
> I've uploaded a very hacky patch set that works around this. We should discuss
a
> cleaner way tomorrow. 

(Or if it's an uncommon case we can probably avoid dealing with it for now --
the test case is abnormal in that it has such a 64k of pre-skip)

[DaleCurtis, 2013-12-12 21:51:38]

PTAL.  This now includes the demuxer patch we discussed.  All the test cases I
have sound correct at least.

[vigneshv, 2013-12-12 22:22:41]

lgtm

Thanks for all the cleanup and perf fixes!

LGTM % nits.

https://codereview.chromium.org/100503006/diff/210001/media/filters/opus_audi...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/100503006/diff/210001/media/filters/opus_audi...
media/filters/opus_audio_decoder.cc:475: if (timestamp_offset_ <=
base::TimeDelta() || frame_delay_at_start_ < 0) {
nit: is this necessary? doesn't one imply the other? also shouldn't the first
condition be strictly less than?

[acolwell GONE FROM CHROMIUM, 2013-12-12 22:40:45]

https://codereview.chromium.org/100503006/diff/190001/media/filters/ffmpeg_de...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/100503006/diff/190001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer.cc:851: if
(strcmp(glue_->format_context()->iformat->name, "ogg") == 0) {
Add a TODO & crbug to get this difference between Opus in ogg vs Opus in WebM
fixed upstream?

https://codereview.chromium.org/100503006/diff/190001/media/filters/opus_audi...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/100503006/diff/190001/media/filters/opus_audi...
media/filters/opus_audio_decoder.cc:400: if (last_input_timestamp_ ==
kNoTimestamp() &&
This doesn't seem quite right. Shouldn't this be something like

if (start_input_timestamp_ == kNoTimestamp())
  start_input_timestamp_ = input->timestamp();

if (last_input_timestamp_ == kNoTimestamp() &&
    input->timestamp() == start_input_timestamp_) {
  frames_to_discard_ = frame_delay_at_start_;
}

I think if you do this you won't need  partial_frame_delay_at_start_  or the
(first_output_frame_ != kNoTimestamp()) and (first_output_frame_ ==
kNoTimestamp()) blocks in OpusAudioDecoder::Decode()

[DaleCurtis, 2013-12-12 23:36:04]

https://codereview.chromium.org/100503006/diff/190001/media/filters/ffmpeg_de...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/100503006/diff/190001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer.cc:851: if
(strcmp(glue_->format_context()->iformat->name, "ogg") == 0) {
On 2013/12/12 22:40:46, acolwell wrote:
> Add a TODO & crbug to get this difference between Opus in ogg vs Opus in WebM
> fixed upstream?

Done.

https://codereview.chromium.org/100503006/diff/190001/media/filters/opus_audi...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/100503006/diff/190001/media/filters/opus_audi...
media/filters/opus_audio_decoder.cc:400: if (last_input_timestamp_ ==
kNoTimestamp() &&
On 2013/12/12 22:40:46, acolwell wrote:
> This doesn't seem quite right. Shouldn't this be something like
> 
> if (start_input_timestamp_ == kNoTimestamp())
>   start_input_timestamp_ = input->timestamp();
> 
> if (last_input_timestamp_ == kNoTimestamp() &&
>     input->timestamp() == start_input_timestamp_) {
>   frames_to_discard_ = frame_delay_at_start_;
> }
> 
> I think if you do this you won't need  partial_frame_delay_at_start_  or the
> (first_output_frame_ != kNoTimestamp()) and (first_output_frame_ ==
> kNoTimestamp()) blocks in OpusAudioDecoder::Decode()

Done.  Required fixing AudioRendererImpl preroll!

https://codereview.chromium.org/100503006/diff/210001/media/filters/opus_audi...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/100503006/diff/210001/media/filters/opus_audi...
media/filters/opus_audio_decoder.cc:475: if (timestamp_offset_ <=
base::TimeDelta() || frame_delay_at_start_ < 0) {
On 2013/12/12 22:22:42, vigneshv wrote:
> nit: is this necessary? doesn't one imply the other? also shouldn't the first
> condition be strictly less than?

I just collapsed the conditional from 471 which is checking codec_delay != 0.

[acolwell GONE FROM CHROMIUM, 2013-12-12 23:46:19]

lgtm

https://codereview.chromium.org/100503006/diff/220001/media/filters/audio_ren...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/100503006/diff/220001/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:406: if (state_ == kPrerolling) {
nit: move this into the kPrerolling block above?

if (state_ == kPrerolling) {
  if (IsBeforePrerollTime(buffer))
    return true;

  ....
}

[DaleCurtis, 2013-12-13 03:19:11]

https://codereview.chromium.org/100503006/diff/220001/media/filters/audio_ren...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/100503006/diff/220001/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:406: if (state_ == kPrerolling) {
On 2013/12/12 23:46:19, acolwell wrote:
> nit: move this into the kPrerolling block above?
> 
> if (state_ == kPrerolling) {
>   if (IsBeforePrerollTime(buffer))
>     return true;
> 
>   ....
> }

Done.

[commit-bot: I haz the power, 2013-12-13 03:23:35]

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

[commit-bot: I haz the power, 2013-12-13 05:21:42]

Retried try job too often on linux_chromeos for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-12-13 18:01:34]

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

[commit-bot: I haz the power, 2013-12-13 19:23:10]

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

[commit-bot: I haz the power, 2013-12-13 22:07:55]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2013-12-13 22:20:46]

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

[commit-bot: I haz the power, 2013-12-13 22:24:26]

Change committed as 240775