Use ffmpeg for opus decoding, no need to maintain our decoder.

Use ffmpeg for opus decoding, no need to maintain our decoder.

As a bonus this allows WebAudio to start using opus.

BUG=482934
TEST=all opus tests pass with the previous expectations.

Committed: https://crrev.com/655b0cf987eca28d8035cb3ceb1af6c4b916b179
Cr-Commit-Position: refs/heads/master@{#433433}

[wolenetz, 2016-10-21 22:30:57]

wolenetz@chromium.org changed reviewers:
+ wolenetz@chromium.org

[wolenetz, 2016-10-21 22:30:58]

Looking pretty good. Just a few nits and will need inclusion of ffmpeg configs
such that Chromium's third_party/opus is used by ffmpeg's --enable-libopus
configure option, build_ffmpeg.py, and Chromium build.

https://codereview.chromium.org/2435603009/diff/1/media/base/audio_discard_he...
File media/base/audio_discard_helper.h (right):

https://codereview.chromium.org/2435603009/diff/1/media/base/audio_discard_he...
media/base/audio_discard_helper.h:95: // previous encoded buffer.  Enabled
automatically if an encoded buffer is
It's const, ctor initialized now, so no longer enable-able automatically by an
instance method like ProcessBuffers().

https://codereview.chromium.org/2435603009/diff/1/media/test/pipeline_integra...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/2435603009/diff/1/media/test/pipeline_integra...
media/test/pipeline_integration_test.cc:1578:
EXPECT_HASH_EQ("-0.25,0.67,0.04,0.14,-0.49,-0.41,", GetAudioHash());
Why did this change? Is this related to Opus at all or just an opportunistic fix
here?

[wolenetz, 2016-10-21 22:38:03]

Also, as we chatted, it SGTM to have the following sequence:
1) Land something like https://chromium-review.googlesource.com/#/c/402028/
(enable Chromium ffmpeg libopus decode, using Chromium's third_party/opus)
2) I roll FFmpeg DEPS (including configs resulting from #1) *and* including a
new Chromium piece which modifies FFmpegAudioDecoder to reject Opus decode
attempts (to fall back to the soon-to-be-removed OpusAudioDecoder path)
3) Then we land this CL (and undo the FFmpegAudioDecoder Opus codec rejection
from #2)

[wolenetz, 2016-10-24 21:37:53]

flim@ - will this work with the channel-mapping changes for ambisonic Opus
(expected to be included in the ffmpeg roll)?

[flim-chromium, 2016-10-26 22:44:03]

On 2016/10/24 21:37:53, wolenetz wrote:
> flim@ - will this work with the channel-mapping changes for ambisonic Opus
> (expected to be included in the ffmpeg roll)?

The changes for decoding ambisonic Opus are internal to ffmpeg so it should work
without modifying the the way we decode an opus encoded file. I wasn't able to
get media_unittests to run locally with the changes listed above but because of
the reason above, I believe these changes will also work for ambisonic Opus.
Perhaps it's better for me to wait for these changes to land and then verify
specifically for an ambisonic encoded file?

[wolenetz, 2016-11-03 00:17:03]

tl;dr: The post-ffmpeg-roll change to remove OpusAudioDecoder and use
FFmpegAudioDecoder will need updates to correctly handle front/end-trimming.

Detail discovered during M56 FFmpeg roll:
It looks like the front/end-trimming of Opus is broken if we use our current
ffmpeg_audio_decoder with ffmpeg as shim to libopus. Each of the following fails
with wrong hashes, and playback in chrome of
//src/media/tests/data/opus-trimming-test.webm skips too much initial audio and
plays part of the end-beep. (and DCHECK fails:
audio_discard_helper.cc(93)] Check failed: decoder_delay_ == 0u (65535 vs. 0)

[DaleCurtis, 2016-11-03 00:26:59]

Thanks for the heads up, I'll tinker with it after the roll.

[wolenetz, 2016-11-03 18:04:16]

On 2016/11/03 00:26:59, DaleCurtis wrote:
> Thanks for the heads up, I'll tinker with it after the roll.

It very well might *not* fail with this particular CL applied. I didn't try
that. I just noticed that I, of course as planned, needed to disable
FFmpegAudioDecoder decode of Opus as part of the roll :)

[DaleCurtis, 2016-11-03 18:06:17]

Oh I see, yeah that's expected :)

[wolenetz, 2016-11-03 20:44:59]

On 2016/11/03 18:06:17, DaleCurtis wrote:
> Oh I see, yeah that's expected :)

I've now tried locally on linux x64 post-roll that this CL (with a further patch
to undo my disabling-during-roll of Opus decode by ffmpegaudiodecoder) doesn't
appear to regress media_unittests. I'm still working on a few other issues to
get the roll landed.

[wolenetz, 2016-11-17 21:39:22]

On 2016/11/03 20:44:59, wolenetz wrote:
> On 2016/11/03 18:06:17, DaleCurtis wrote:
> > Oh I see, yeah that's expected :)
> 
> I've now tried locally on linux x64 post-roll that this CL (with a further
patch
> to undo my disabling-during-roll of Opus decode by ffmpegaudiodecoder) doesn't
> appear to regress media_unittests. I'm still working on a few other issues to
> get the roll landed.

The roll is in CQ (patch set 13 of https://codereview.chromium.org/2497603003/).
Please update this CL to rebase on top of it once it's landed, since the roll
includes disabling ffmpeg_audio_decoder decode of Opus. Please also see my
previous comments on patch set 1, here.

[DaleCurtis, 2016-11-17 23:06:02]

Description was changed from

==========
Use ffmpeg for opus decoding, no need to maintain our decoder.

As a bonus this allows WebAudio to start using opus.

BUG=
==========

to

==========
Use ffmpeg for opus decoding, no need to maintain our decoder.

As a bonus this allows WebAudio to start using opus.

BUG=482934
TEST=all opus tests pass with the previous expectations.
==========

[DaleCurtis, 2016-11-17 23:07:14]

Rebased on top of the roll. PTAL. The hash change is due to delayed discard
being incorrectly applied in some cases.

[wolenetz, 2016-11-18 00:36:42]

LGTM % two comments:

https://codereview.chromium.org/2435603009/diff/1/media/base/audio_discard_he...
File media/base/audio_discard_helper.h (right):

https://codereview.chromium.org/2435603009/diff/1/media/base/audio_discard_he...
media/base/audio_discard_helper.h:95: // previous encoded buffer.  Enabled
automatically if an encoded buffer is
On 2016/10/21 22:30:58, wolenetz wrote:
> It's const, ctor initialized now, so no longer enable-able automatically by an
> instance method like ProcessBuffers().

In patch set 2, this code comment still incorrectly mentions "Enabled
automatically if...ProcessBuffers()". Instead, it's set by the ctor.

https://codereview.chromium.org/2435603009/diff/20001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/2435603009/diff/20001/media/BUILD.gn#newcode487
media/BUILD.gn:487: "//third_party/opus",
Should this now be conditioned on media_use_ffmpeg?

[DaleCurtis, 2016-11-18 00:51:27]

https://codereview.chromium.org/2435603009/diff/1/media/base/audio_discard_he...
File media/base/audio_discard_helper.h (right):

https://codereview.chromium.org/2435603009/diff/1/media/base/audio_discard_he...
media/base/audio_discard_helper.h:95: // previous encoded buffer.  Enabled
automatically if an encoded buffer is
On 2016/11/18 at 00:36:41, wolenetz wrote:
> On 2016/10/21 22:30:58, wolenetz wrote:
> > It's const, ctor initialized now, so no longer enable-able automatically by
an
> > instance method like ProcessBuffers().
> 
> In patch set 2, this code comment still incorrectly mentions "Enabled
automatically if...ProcessBuffers()". Instead, it's set by the ctor.

Fixed.

https://codereview.chromium.org/2435603009/diff/20001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/2435603009/diff/20001/media/BUILD.gn#newcode487
media/BUILD.gn:487: "//third_party/opus",
On 2016/11/18 at 00:36:41, wolenetz wrote:
> Should this now be conditioned on media_use_ffmpeg?

Probably, I don't know why it's a public dep either, will try moving to normal
deps now.

[DaleCurtis, 2016-11-18 03:58:10]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-18 03:58:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-18 04:05:35]

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

[commit-bot: I haz the power, 2016-11-18 04:05:36]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[DaleCurtis, 2016-11-18 17:42:07]

dalecurtis@chromium.org changed reviewers:
+ avi@chromium.org

[DaleCurtis, 2016-11-18 17:42:07]

+avi for content/renderer/BUILD.gn update now that media's opus dep is private.

[DaleCurtis, 2016-11-18 17:46:31]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-18 17:47:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-18 17:52:33]

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

[commit-bot: I haz the power, 2016-11-18 17:52:34]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Avi (use Gerrit), 2016-11-18 17:54:05]

lgtm

okie dokie

[DaleCurtis, 2016-11-18 18:00:16]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2016-11-18 18:00:17]

Thanks! Had to fix one more in content/test too.

[DaleCurtis, 2016-11-18 18:00:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
wolenetz@chromium.org, avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2435603009/#ps70001
(title: "Fix unittest deps too.")

[commit-bot: I haz the power, 2016-11-18 18:00:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-18 18:27:56]

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

[commit-bot: I haz the power, 2016-11-18 18:27:58]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[DaleCurtis, 2016-11-18 18:34:57]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2016-11-18 18:34:57]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
wolenetz@chromium.org
Link to the patchset: https://codereview.chromium.org/2435603009/#ps90001
(title: "Relocate expectations.")

[commit-bot: I haz the power, 2016-11-18 18:35:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-18 19:25:27]

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

[commit-bot: I haz the power, 2016-11-18 19:25:28]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[DaleCurtis, 2016-11-18 19:31:18]

dalecurtis@chromium.org changed reviewers:
+ alokp@chromium.org

[DaleCurtis, 2016-11-18 19:31:19]

+alokp for chromecast/ removal of OpusAudioDecoder.

[DaleCurtis, 2016-11-18 19:31:42]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-18 19:33:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-18 21:36:32]

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

[commit-bot: I haz the power, 2016-11-18 21:36:33]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alokp, 2016-11-18 23:35:45]

lgtm

[DaleCurtis, 2016-11-18 23:38:07]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2016-11-18 23:38:07]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
wolenetz@chromium.org
Link to the patchset: https://codereview.chromium.org/2435603009/#ps130001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-11-18 23:39:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-19 04:42:57]

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

[commit-bot: I haz the power, 2016-11-19 04:42:59]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[DaleCurtis, 2016-11-19 05:35:22]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2016-11-19 05:35:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-19 05:59:30]

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

[commit-bot: I haz the power, 2016-11-19 05:59:31]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[DaleCurtis, 2016-11-19 19:01:33]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2016-11-19 19:01:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-20 00:00:16]

CQ is committing da patch.

Bot data: {"patchset_id": 130001, "attempt_start_ts": 1479582093432950,
"parent_rev": ["7e7a23048caace1a52c287546235c216d3920d9c", null], "commit_rev":
["3c781015aa8eb5e86eef0d34179923da50db0043", null]}

[commit-bot: I haz the power, 2016-11-20 00:00:40]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2016-11-20 00:03:35]

Description was changed from

==========
Use ffmpeg for opus decoding, no need to maintain our decoder.

As a bonus this allows WebAudio to start using opus.

BUG=482934
TEST=all opus tests pass with the previous expectations.
==========

to

==========
Use ffmpeg for opus decoding, no need to maintain our decoder.

As a bonus this allows WebAudio to start using opus.

BUG=482934
TEST=all opus tests pass with the previous expectations.

Committed: https://crrev.com/655b0cf987eca28d8035cb3ceb1af6c4b916b179
Cr-Commit-Position: refs/heads/master@{#433433}
==========

[commit-bot: I haz the power, 2016-11-20 00:03:36]

Patchset 8 (id:??) landed as
https://crrev.com/655b0cf987eca28d8035cb3ceb1af6c4b916b179
Cr-Commit-Position: refs/heads/master@{#433433}