media: Fix Opus support, and handle bad timestamps correctly in the Opus wrapper.

media: Fix Opus support, and handle bad timestamps correctly in the Opus wrapper.

- Recent changes to ffmpeg_common broke support for Opus. The check fails
  because Opus support is not enabled in FFmpeg, which causes the FFmpeg
  codec context to contain an invalid sample format value. Avoid the problem
  by skipping a DCHECK for Opus input.
- Add missing return when non-monotonically increasing timestamps are
  detected by the OpusAudioDecoder. This fixes a crash when attempting to
  seek the Ogg file referenced in crbug.com/168524.

 BUG=168524
TEST=Opus audio in WebM containers plays back with the flag
 --enable-opus-playback


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

[Tom Finegan, 2013-01-14 20:29:51]

PTAL: Don't let the commit message scare you; it's a tiny CL.

Thanks!

[fbarchard1, 2013-01-14 20:42:30]

with a public ffmpeg build from today, I see opus?

ffmpeg -i tulip2.opus.webm
Input #0, matroska,webm, from 'tulip2.opus.webm':
  Duration: 00:00:16.68, start: 0.000000, bitrate: 2123 kb/s
    Stream #0:0(eng): Video: vp8, yuv420p, 1280x720, SAR 1:1 DAR 16:9, 1k fps,
1k tbr, 1k tbn, 1k tbc (default)
    Stream #0:1: Audio: opus, 48000 Hz, stereo, s16 (default)
At least one output file must be specified

If its supported in public build, and roll would fix this?

https://codereview.chromium.org/11888011/diff/1/media/ffmpeg/ffmpeg_common.cc
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/11888011/diff/1/media/ffmpeg/ffmpeg_common.cc...
media/ffmpeg/ffmpeg_common.cc:292: if (codec != kCodecOpus) {
add a comment or todo to remove this.

[Tom Finegan, 2013-01-14 20:50:54]

On 2013/01/14 20:42:30, fbarchard1 wrote:
> with a public ffmpeg build from today, I see opus?
> 
> ffmpeg -i tulip2.opus.webm
> Input #0, matroska,webm, from 'tulip2.opus.webm':
>   Duration: 00:00:16.68, start: 0.000000, bitrate: 2123 kb/s
>     Stream #0:0(eng): Video: vp8, yuv420p, 1280x720, SAR 1:1 DAR 16:9, 1k fps,
> 1k tbr, 1k tbn, 1k tbc (default)
>     Stream #0:1: Audio: opus, 48000 Hz, stereo, s16 (default)
> At least one output file must be specified
> 
> If its supported in public build, and roll would fix this?
> 

No. We don't enable opus in chrome/chromium FFmpeg builds. This is for multiple
reasons, but the simplest one is size: enabling Opus in FFmpeg results in an
extra copy of the Opus code.

> https://codereview.chromium.org/11888011/diff/1/media/ffmpeg/ffmpeg_common.cc
> File media/ffmpeg/ffmpeg_common.cc (right):
> 
>
https://codereview.chromium.org/11888011/diff/1/media/ffmpeg/ffmpeg_common.cc...
> media/ffmpeg/ffmpeg_common.cc:292: if (codec != kCodecOpus) {
> add a comment or todo to remove this.

Not sure if a TODO is appropriate-- What would it say? "Remove this when we
enable Opus support in FFmpeg"? I'm not sure if that will ever be the case.

[DaleCurtis, 2013-01-14 21:09:49]

lgtm

[fbarchard1, 2013-01-15 00:48:36]

https://codereview.chromium.org/11888011/diff/1/media/filters/opus_audio_deco...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/11888011/diff/1/media/filters/opus_audio_deco...
media/filters/opus_audio_decoder.cc:361: return;
I've seen this message before, and it sounds bad but isnt fatal.  This change
makes it fatal.  Should allow the file to be decoded?

[Tom Finegan, 2013-01-15 00:52:07]

On 2013/01/15 00:48:36, fbarchard1 wrote:
>
https://codereview.chromium.org/11888011/diff/1/media/filters/opus_audio_deco...
> File media/filters/opus_audio_decoder.cc (right):
> 
>
https://codereview.chromium.org/11888011/diff/1/media/filters/opus_audio_deco...
> media/filters/opus_audio_decoder.cc:361: return;
> I've seen this message before, and it sounds bad but isnt fatal.  This change
> makes it fatal.  Should allow the file to be decoded?

I'm intentionally duplicating behavior from FFmpegAudioDecoder, and from
FFmpegAudioDecoder's perspective this is fatal. 

Under what circumstances would we ever want to decode and render out of order
audio?

[DaleCurtis, 2013-01-15 01:00:28]

https://codereview.chromium.org/11888011/diff/1/media/filters/opus_audio_deco...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/11888011/diff/1/media/filters/opus_audio_deco...
media/filters/opus_audio_decoder.cc:361: return;
On 2013/01/15 00:48:36, fbarchard1 wrote:
> I've seen this message before, and it sounds bad but isnt fatal.  This change
> makes it fatal.  Should allow the file to be decoded?

We explicitly forbid this behavior since it has lead to weird duration and
playback problems in the past.

[fbarchard1, 2013-01-15 02:26:50]

I'm okay with the Opus hack, but would prefer a comment about why.  Its the
truncation of files based on time stamp that concerns me, as this actually
happens in production.

https://codereview.chromium.org/11888011/diff/1/media/ffmpeg/ffmpeg_common.cc
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/11888011/diff/1/media/ffmpeg/ffmpeg_common.cc...
media/ffmpeg/ffmpeg_common.cc:292: if (codec != kCodecOpus) {
On 2013/01/14 20:42:30, fbarchard1 wrote:
> add a comment or todo to remove this.

Isn't Opus already in current ffmpeg?  I ran some tests on ffmpeg with opus on
webm and it seemed ok?  Or do you mean opus is disabled in chromium's version of
ffmpeg?

https://codereview.chromium.org/11888011/diff/1/media/filters/opus_audio_deco...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/11888011/diff/1/media/filters/opus_audio_deco...
media/filters/opus_audio_decoder.cc:361: return;
This behavior still occurs on current/future codecs.  Theres no known work
around?  Stopping playback will cause a larger aberration.

[Tom Finegan, 2013-01-15 02:44:51]

https://codereview.chromium.org/11888011/diff/1/media/ffmpeg/ffmpeg_common.cc
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/11888011/diff/1/media/ffmpeg/ffmpeg_common.cc...
media/ffmpeg/ffmpeg_common.cc:292: if (codec != kCodecOpus) {
On 2013/01/15 02:26:50, fbarchard1 wrote:
> On 2013/01/14 20:42:30, fbarchard1 wrote:
> > add a comment or todo to remove this.
> 
> Isn't Opus already in current ffmpeg?  I ran some tests on ffmpeg with opus on
> webm and it seemed ok?  Or do you mean opus is disabled in chromium's version
of
> ffmpeg?

Disabled in chromium's version.

https://codereview.chromium.org/11888011/diff/1/media/filters/opus_audio_deco...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/11888011/diff/1/media/filters/opus_audio_deco...
media/filters/opus_audio_decoder.cc:361: return;
On 2013/01/15 02:26:50, fbarchard1 wrote:
> This behavior still occurs on current/future codecs.  Theres no known work
> around?  Stopping playback will cause a larger aberration. 

What do you mean by "a larger aberration"? If this happens in FFmpegAudioDecoder
it does the same thing that OpusAudioDecoder does. I'm just fixing a bug related
to the omission of a single line of code that was missed in the initial review
of this class.

As Dale said, this behavior is explicitly forbidden.

[fbarchard1, 2013-01-15 23:10:17]

LGTM
Re Input timestamps are not monotonically increasing! 
Discussed offline.  This is already in audio decoder, and this does it for opus
as well.
Just be aware that these do occur and a solution is not known for the bug.  This
causes playback to stop prematurely.

https://codereview.chromium.org/11888011/diff/1/media/ffmpeg/ffmpeg_common.cc
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/11888011/diff/1/media/ffmpeg/ffmpeg_common.cc...
media/ffmpeg/ffmpeg_common.cc:275: // TODO(tomfinegan): |sample_fmt| in
|codec_context| is -1... because
Should be // TODO(finegan): ?
I didnt see this comment before.  Looks ok.

[Tom Finegan, 2013-01-15 23:22:28]

https://codereview.chromium.org/11888011/diff/1/media/ffmpeg/ffmpeg_common.cc
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/11888011/diff/1/media/ffmpeg/ffmpeg_common.cc...
media/ffmpeg/ffmpeg_common.cc:275: // TODO(tomfinegan): |sample_fmt| in
|codec_context| is -1... because
On 2013/01/15 23:10:17, fbarchard1 wrote:
> Should be // TODO(finegan): ?
> I didnt see this comment before.  Looks ok.

I'm tomfinegan@ for chromium and google-- this is correct. :)

[commit-bot: I haz the power, 2013-01-15 23:25:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tomfinegan@chromium.org/11888011/1

[commit-bot: I haz the power, 2013-01-16 02:42:51]

Change committed as 177061