Use ffmpeg for opus decoding, no need to maintain our decoder. (issue 2435603009 by dalecurtis@chromium.org)

[wole...@chromium.org]

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_helper.h
File media/base/audio_discard_helper.h (right):

https://codereview.chromium.org/2435603009/diff/1/media/base/audio_discard_helper.h#newcode95
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_integration_test.cc
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/2435603009/diff/1/media/test/pipeline_integration_test.cc#newcode1578
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?

https://codereview.chromium.org/2435603009/

[wole...@chromium.org]

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)

https://codereview.chromium.org/2435603009/

[wole...@chromium.org]

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

https://codereview.chromium.org/2435603009/

[fl...@chromium.org]

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?

https://codereview.chromium.org/2435603009/

[wole...@chromium.org]

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)


https://codereview.chromium.org/2435603009/

[dalec...@chromium.org]

Reviewers: wolenetz
CL: https://codereview.chromium.org/2435603009/

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

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

As a bonus this allows WebAudio to start using opus.

BUG=

Affected files (+62, -586 lines):
M media/BUILD.gn
M media/base/audio_discard_helper.h
M media/base/audio_discard_helper.cc
M media/base/audio_discard_helper_unittest.cc
M media/ffmpeg/ffmpeg_common.cc
M media/ffmpeg/ffmpeg_common_unittest.cc
M media/filters/audio_decoder_unittest.cc
M media/filters/ffmpeg_audio_decoder.cc
D media/filters/opus_audio_decoder.h
D media/filters/opus_audio_decoder.cc
M media/renderers/default_renderer_factory.cc
M media/test/pipeline_integration_test.cc
M media/test/pipeline_integration_test_base.cc

[wole...@chromium.org]

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 :)

https://codereview.chromium.org/2435603009/

[dalec...@chromium.org]

Oh I see, yeah that's expected :)

https://codereview.chromium.org/2435603009/

[wole...@chromium.org]

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.

https://codereview.chromium.org/2435603009/