media: Enable Opus support in Clank <video> and MSE

media: Enable Opus support in Clank <video> and MSE

Re-landing the CL reverted here:
https://codereview.chromium.org/866573004

Opus audio codec is supported by the android platform starting from
Lollipop.  This CL enables canPlayType() support for Opus on Clank and
MSE playback of Opus in Clank. This brings Opus feature parity with
Desktop Chromium.

BUG=318436
TBR=jschuh,rsleevi

Committed: https://crrev.com/9cd3dd704879268a7291c9e668aa7bc56df6a1df
Cr-Commit-Position: refs/heads/master@{#315132}

[vignesh, 2015-02-06 18:39:29]

vigneshv@chromium.org changed reviewers:
+ dalecurtis@chromium.org, wolenetz@chromium.org

[vignesh, 2015-02-06 18:39:29]

PS1 contains the CL as it failed. PS2 is the fix.

Matt/Dale, the only change is in the content/browser/media folder (which Dale is
an owner of). Would it be acceptable to TBR other reviewers who already LGTM'ed
the original CL? Or should i add all of them here as well? Please let me know.
Thanks!

[wolenetz, 2015-02-06 18:58:25]

You'll need l*tm from net/base OWNER and content/common/media/ IPC OWNER on this
CL. Other original reviewers of the previous PS, please include at least as CC,
if not TBR (or R).

https://codereview.chromium.org/898393002/diff/20001/content/browser/media/me...
File content/browser/media/media_canplaytype_browsertest.cc (right):

https://codereview.chromium.org/898393002/diff/20001/content/browser/media/me...
content/browser/media/media_canplaytype_browsertest.cc:29: // Android.
(http://crbug.com/318436).
It looks inconsistent to have ogg-specific kOpusProbably[] defined here, yet
webm-specific OpusProbably defined within the webm test. IMHO, I think you
should move both here and include specific webm vs ogg differentiation in the
name of the variables to be consistent.

[wolenetz, 2015-02-06 18:59:35]

On 2015/02/06 18:58:25, wolenetz wrote:
> You'll need l*tm from net/base OWNER and content/common/media/ IPC OWNER on
this
> CL. Other original reviewers of the previous PS, please include at least as
CC,
> if not TBR (or R).
> 
>
https://codereview.chromium.org/898393002/diff/20001/content/browser/media/me...
> File content/browser/media/media_canplaytype_browsertest.cc (right):
> 
>
https://codereview.chromium.org/898393002/diff/20001/content/browser/media/me...
> content/browser/media/media_canplaytype_browsertest.cc:29: // Android.
> (http://crbug.com/318436).
> It looks inconsistent to have ogg-specific kOpusProbably[] defined here, yet
> webm-specific OpusProbably defined within the webm test. IMHO, I think you
> should move both here and include specific webm vs ogg differentiation in the
> name of the variables to be consistent.

Hmm. I suppose TBR is ok, on second look.

[DaleCurtis, 2015-02-06 19:06:13]

lgtm

[vignesh, 2015-02-06 19:07:17]

vigneshv@chromium.org changed reviewers:
+ jschuh@chromium.org, qinmin@chromium.org, rsleevi@chromium.org

[vignesh, 2015-02-06 19:07:18]

Adding rsleevi@ for net/base OWNER
Adding jschuh@ for content/common/media security review of IPC message file.
Adding qinmin@ for media/base/android OWNER

[vignesh, 2015-02-06 19:09:56]

On 2015/02/06 18:59:35, wolenetz wrote:
> On 2015/02/06 18:58:25, wolenetz wrote:
> > You'll need l*tm from net/base OWNER and content/common/media/ IPC OWNER on
> this
> > CL. Other original reviewers of the previous PS, please include at least as
> CC,
> > if not TBR (or R).
> > 
> >
>
https://codereview.chromium.org/898393002/diff/20001/content/browser/media/me...
> > File content/browser/media/media_canplaytype_browsertest.cc (right):
> > 
> >
>
https://codereview.chromium.org/898393002/diff/20001/content/browser/media/me...
> > content/browser/media/media_canplaytype_browsertest.cc:29: // Android.
> > (http://crbug.com/318436).
> > It looks inconsistent to have ogg-specific kOpusProbably[] defined here, yet
> > webm-specific OpusProbably defined within the webm test. IMHO, I think you
> > should move both here and include specific webm vs ogg differentiation in
the
> > name of the variables to be consistent.
> 
> Hmm. I suppose TBR is ok, on second look.

i just added them, never mind. i'll just wait for an l*tm from them as well. :)

[qinmin, 2015-02-06 19:11:09]

android changes lgtm % nit

https://codereview.chromium.org/898393002/diff/20001/media/base/android/java/...
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
(left):

https://codereview.chromium.org/898393002/diff/20001/media/base/android/java/...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:660: if
(index == 0) {
nit: I prefer a switch case here now

[vignesh, 2015-02-06 19:18:26]

https://codereview.chromium.org/898393002/diff/20001/content/browser/media/me...
File content/browser/media/media_canplaytype_browsertest.cc (right):

https://codereview.chromium.org/898393002/diff/20001/content/browser/media/me...
content/browser/media/media_canplaytype_browsertest.cc:29: // Android.
(http://crbug.com/318436).
On 2015/02/06 18:58:25, wolenetz wrote:
> It looks inconsistent to have ogg-specific kOpusProbably[] defined here, yet
> webm-specific OpusProbably defined within the webm test. IMHO, I think you
> should move both here and include specific webm vs ogg differentiation in the
> name of the variables to be consistent.

the webm specific one cannot be defined here as it depends on the version of
android (i.e.) requires an if condition. i presume that's why VP9 was added in
the WebM Specific test as well. i can rename this to kOggOpusProbably if that
will make things clearer (also note that this behavior is correct since Opus in
Ogg is not supported on android as of this writing). 

thoughts?

[vignesh, 2015-02-06 19:33:50]

New patchsets have been uploaded after l-g-t-m from
dalecurtis@chromium.org,qinmin@chromium.org

[vignesh, 2015-02-06 19:34:41]

https://codereview.chromium.org/898393002/diff/20001/media/base/android/java/...
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
(left):

https://codereview.chromium.org/898393002/diff/20001/media/base/android/java/...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:660: if
(index == 0) {
On 2015/02/06 19:11:09, qinmin wrote:
> nit: I prefer a switch case here now

Done.

[wolenetz, 2015-02-06 19:57:53]

lgtm % rename kOpusProbably to kOggOpusProbably

https://codereview.chromium.org/898393002/diff/20001/content/browser/media/me...
File content/browser/media/media_canplaytype_browsertest.cc (right):

https://codereview.chromium.org/898393002/diff/20001/content/browser/media/me...
content/browser/media/media_canplaytype_browsertest.cc:29: // Android.
(http://crbug.com/318436).
On 2015/02/06 19:18:26, vignesh wrote:
> On 2015/02/06 18:58:25, wolenetz wrote:
> > It looks inconsistent to have ogg-specific kOpusProbably[] defined here, yet
> > webm-specific OpusProbably defined within the webm test. IMHO, I think you
> > should move both here and include specific webm vs ogg differentiation in
the
> > name of the variables to be consistent.
> 
> the webm specific one cannot be defined here as it depends on the version of
> android (i.e.) requires an if condition. i presume that's why VP9 was added in
> the WebM Specific test as well. i can rename this to kOggOpusProbably if that
> will make things clearer (also note that this behavior is correct since Opus
in
> Ogg is not supported on android as of this writing). 
> 
> thoughts?

I have no strong feeling; the conditional webm opus is unfortunate - I agree the
best option for now is to keep it in the webm test method. Rename of the
ogg-specific kOpusProbably to kOggOpusProbably would help clarify exactly intent
for its usage.

[vignesh, 2015-02-06 20:13:41]

New patchsets have been uploaded after l-g-t-m from wolenetz@chromium.org

[vignesh, 2015-02-06 20:14:16]

On 2015/02/06 19:57:53, wolenetz wrote:
> lgtm % rename kOpusProbably to kOggOpusProbably
> 
>
https://codereview.chromium.org/898393002/diff/20001/content/browser/media/me...
> File content/browser/media/media_canplaytype_browsertest.cc (right):
> 
>
https://codereview.chromium.org/898393002/diff/20001/content/browser/media/me...
> content/browser/media/media_canplaytype_browsertest.cc:29: // Android.
> (http://crbug.com/318436).
> On 2015/02/06 19:18:26, vignesh wrote:
> > On 2015/02/06 18:58:25, wolenetz wrote:
> > > It looks inconsistent to have ogg-specific kOpusProbably[] defined here,
yet
> > > webm-specific OpusProbably defined within the webm test. IMHO, I think you
> > > should move both here and include specific webm vs ogg differentiation in
> the
> > > name of the variables to be consistent.
> > 
> > the webm specific one cannot be defined here as it depends on the version of
> > android (i.e.) requires an if condition. i presume that's why VP9 was added
in
> > the WebM Specific test as well. i can rename this to kOggOpusProbably if
that
> > will make things clearer (also note that this behavior is correct since Opus
> in
> > Ogg is not supported on android as of this writing). 
> > 
> > thoughts?
> 
> I have no strong feeling; the conditional webm opus is unfortunate - I agree
the
> best option for now is to keep it in the webm test method. Rename of the
> ogg-specific kOpusProbably to kOggOpusProbably would help clarify exactly
intent
> for its usage.

Done.

[vignesh, 2015-02-06 20:18:16]

TBR'ing rsleevi@ and jschuh@ since they lgtm'ed the original CL and nothing has
changed here from the code they reviewed.

[vignesh, 2015-02-06 20:18:33]

The CQ bit was checked by vigneshv@chromium.org

[Ryan Sleevi, 2015-02-06 20:18:41]

//net LGTM

[commit-bot: I haz the power, 2015-02-06 20:19:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/898393002/60001

[commit-bot: I haz the power, 2015-02-06 21:18:07]

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

[commit-bot: I haz the power, 2015-02-06 21:18:08]

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

[vignesh, 2015-02-06 21:25:14]

New patchsets have been uploaded after l-g-t-m from rsleevi@chromium.org

[vignesh, 2015-02-06 21:27:06]

The CQ bit was checked by vigneshv@chromium.org

[commit-bot: I haz the power, 2015-02-06 21:27:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/898393002/80001

[jschuh, 2015-02-06 22:25:59]

The CQ bit was unchecked by jschuh@chromium.org

[jschuh, 2015-02-06 22:27:20]

Sorry, what's the direction of the IPC here (e.g. renderer -> browser) and what
happens if negative values are passed?

[vignesh, 2015-02-06 22:32:58]

On 2015/02/06 22:27:20, jschuh wrote:
> Sorry, what's the direction of the IPC here (e.g. renderer -> browser) and
what
> happens if negative values are passed?

I'm not sure of the exact answer. But here's mine: the behavior of these two
fields is exactly the same as other fields in the structure (audio_sampling_rate
for example). Negative values are handled in the decoder where it will trigger
an error.

[jschuh, 2015-02-06 22:35:23]

On 2015/02/06 22:32:58, vignesh wrote:
> On 2015/02/06 22:27:20, jschuh wrote:
> > Sorry, what's the direction of the IPC here (e.g. renderer -> browser) and
> what
> > happens if negative values are passed?
> 
> I'm not sure of the exact answer. But here's mine: the behavior of these two
> fields is exactly the same as other fields in the structure
(audio_sampling_rate
> for example). Negative values are handled in the decoder where it will trigger
> an error.

Okay, how about a clearer answer to the direction question, because it may save
digging into this further?

[qinmin, 2015-02-06 22:48:22]

On 2015/02/06 22:35:23, jschuh wrote:
> On 2015/02/06 22:32:58, vignesh wrote:
> > On 2015/02/06 22:27:20, jschuh wrote:
> > > Sorry, what's the direction of the IPC here (e.g. renderer -> browser) and
> > what
> > > happens if negative values are passed?
> > 
> > I'm not sure of the exact answer. But here's mine: the behavior of these two
> > fields is exactly the same as other fields in the structure
> (audio_sampling_rate
> > for example). Negative values are handled in the decoder where it will
trigger
> > an error.
> 
> Okay, how about a clearer answer to the direction question, because it may
save
> digging into this further?

The IPCs should be from renderer->browser. ChunkDemuxer is responsible for
parsing the input and sending the IPC to the MediaCodecBridge in the browser
process.

[jschuh, 2015-02-06 22:55:39]

Okay, is there ever a legitimate reason to pass negative values to the browser?
As far as I can tell -1 is just a placeholder to represent the value is
uninitialized. So, I'd rather the receiving end just validate that it's not
getting negatives. That way we don't have to worry about whatever the codec
might do something crazy on an out-of-range value.

[vignesh, 2015-02-06 22:59:08]

On 2015/02/06 22:55:39, jschuh wrote:
> Okay, is there ever a legitimate reason to pass negative values to the
browser?
> As far as I can tell -1 is just a placeholder to represent the value is
> uninitialized. So, I'd rather the receiving end just validate that it's not
> getting negatives. That way we don't have to worry about whatever the codec
> might do something crazy on an out-of-range value.

Yes, the receiving end is the media_codec_bridge and there is a check for
negative values. This CL also adds a unit-test that confirms the same (please
see media_codec_bridge_unittest.cc diff in this CL).

[jschuh, 2015-02-06 23:18:15]

Sorry, I have no idea how I missed that on my previous pass. ipc security lgtm.

[jschuh, 2015-02-06 23:18:21]

The CQ bit was checked by jschuh@chromium.org

[vignesh, 2015-02-06 23:19:11]

On 2015/02/06 23:18:15, jschuh wrote:
> Sorry, I have no idea how I missed that on my previous pass. ipc security
lgtm.

no problem, thanks!

[commit-bot: I haz the power, 2015-02-06 23:19:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/898393002/80001

[commit-bot: I haz the power, 2015-02-06 23:21:25]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-06 23:22:33]

Patchset 5 (id:??) landed as
https://crrev.com/9cd3dd704879268a7291c9e668aa7bc56df6a1df
Cr-Commit-Position: refs/heads/master@{#315132}