Handle decoding of vorbis files better on Android

Handle decoding of vorbis files better on Android

BUG=227144
Send the data size to MediaCodec so we can get the duration length for ogg/vorbis files correctly instead of returning a huge number.  With this information, we no longer need the is_vorbis flag since the duration is now correct, so we remove that code as well.

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

[Raymond Toy (Google), 2013-04-26 22:37:50]

Adding reviewers

Please review. 

palmer@: security review
bulach@: MediaCodec changes
dalecurtis@: C++ side of media/base/android and webkit/media 
brettw@: webkit/glue changes
aelias@: content/browser/renderer_host

[aelias_OOO_until_Jul13, 2013-04-26 22:47:38]

content/browser/renderer_host lgtm

[palmer, 2013-04-26 22:53:31]

IPC security LGTM, with request for sanity check in the ultimate callee. (Part
of the fun of C's and Java's not-quite-compatible integer types!)

https://codereview.chromium.org/14522002/diff/23/media/base/android/java/src/...
File
media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java
(right):

https://codereview.chromium.org/14522002/diff/23/media/base/android/java/src/...
media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:32:
long dataSize) {
A Java long is guaranteed to be a signed 64-bit integer; the size_t that you
receive from the C++ world is guaranteed to be unsigned, and may be 32 or 64
bits. Thus there is a chance that some maniac could feed this function a value
that, to Java, would be negative. (It would be an absurdly/impossibly huge value
in C++ land, too.)

In the ultimate callee, which seems to be MediaExtractor.setDataSource, it might
make sense to do a sanity check on the dataSize.

[brettw, 2013-04-27 05:13:47]

webkit lgtm

[bulach, 2013-04-29 13:16:25]

lgtm (with palmer's insightful comments, thanks for pointing that out!) and tiny
nits:

https://codereview.chromium.org/14522002/diff/23/media/base/android/java/src/...
File
media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java
(right):

https://codereview.chromium.org/14522002/diff/23/media/base/android/java/src/...
media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:82:
duration_microseconds);
nit: durationMicroseconds

https://codereview.chromium.org/14522002/diff/23/media/base/android/java/src/...
media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:166:
long duration_microseconds);
nit: durationMicroseconds

https://codereview.chromium.org/14522002/diff/23/media/base/android/webaudio_...
File media/base/android/webaudio_media_codec_bridge.h (right):

https://codereview.chromium.org/14522002/diff/23/media/base/android/webaudio_...
media/base/android/webaudio_media_codec_bridge.h:66: // The length of the
encoded data
nit: end with a .

[DaleCurtis, 2013-04-30 17:44:16]

https://codereview.chromium.org/14522002/diff/12001/media/base/android/java/s...
File
media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java
(right):

https://codereview.chromium.org/14522002/diff/12001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:78:
// (multi-year) duration value even for short files.  Tell
Comment looks out of date now?

https://codereview.chromium.org/14522002/diff/12001/media/base/android/webaud...
File media/base/android/webaudio_media_codec_bridge.cc (right):

https://codereview.chromium.org/14522002/diff/12001/media/base/android/webaud...
media/base/android/webaudio_media_codec_bridge.cc:84: unsigned long info[3] =
{static_cast<unsigned long>(channel_count),
Can you just drop the 3 and let it auto size?  All the static casts are
unnecessary, no?

https://codereview.chromium.org/14522002/diff/12001/webkit/media/android/audi...
File webkit/media/android/audio_decoder_android.cc (right):

https://codereview.chromium.org/14522002/diff/12001/webkit/media/android/audi...
webkit/media/android/audio_decoder_android.cc:134: unsigned long info[3];
Since you're reading / writing this structure across files, can you declare the
size somewhere both pieces of code can read it from?  Better yet, convert this
to a struct? :)

[Raymond Toy (Google), 2013-04-30 17:57:49]

https://codereview.chromium.org/14522002/diff/12001/media/base/android/webaud...
File media/base/android/webaudio_media_codec_bridge.cc (right):

https://codereview.chromium.org/14522002/diff/12001/media/base/android/webaud...
media/base/android/webaudio_media_codec_bridge.cc:84: unsigned long info[3] =
{static_cast<unsigned long>(channel_count),
On 2013/04/30 17:44:17, DaleCurtis wrote:
> Can you just drop the 3 and let it auto size?  All the static casts are
> unnecessary, no?

The static casts were added by benm@ to fix a WebView build to avoid
-Werror=narrowing.  This is probably only needed for the duration which is a
double being converted to a long.

[Raymond Toy (Google), 2013-04-30 19:53:15]

https://codereview.chromium.org/14522002/diff/12001/webkit/media/android/audi...
File webkit/media/android/audio_decoder_android.cc (right):

https://codereview.chromium.org/14522002/diff/12001/webkit/media/android/audi...
webkit/media/android/audio_decoder_android.cc:134: unsigned long info[3];
On 2013/04/30 17:44:17, DaleCurtis wrote:
> Since you're reading / writing this structure across files, can you declare
the
> size somewhere both pieces of code can read it from?  Better yet, convert this
> to a struct? :)

Added the struct and a new header file to define it so it can be shared.

[DaleCurtis, 2013-04-30 23:26:14]

lgtm % nits.

https://codereview.chromium.org/14522002/diff/26001/media/base/android/webaud...
File media/base/android/webaudio_media_codec_bridge.cc (right):

https://codereview.chromium.org/14522002/diff/26001/media/base/android/webaud...
media/base/android/webaudio_media_codec_bridge.cc:82: struct
WebAudioMediaCodecInfo info =
Formatting should be similar to a function declaration, i.e:

struct WebAudioMediaCodecInfo info = {
  static_cast<unsigned long>(channel_count),
  static_cast<unsigned long>(sample_rate),
  static_cast<unsigned long>(0.5 + duration_microsec * 0.000001 * sample_rate)
};

https://codereview.chromium.org/14522002/diff/26001/media/base/android/webaud...
File media/base/android/webaudio_media_codec_info.h (right):

https://codereview.chromium.org/14522002/diff/26001/media/base/android/webaud...
media/base/android/webaudio_media_codec_info.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
2013 license is w/o the (c).

https://codereview.chromium.org/14522002/diff/26001/media/base/android/webaud...
media/base/android/webaudio_media_codec_info.h:5: #ifndef
MEDIA_BASE_ANDROID_MEDIA_CODEC_INFO_H_
...WEBAUDIO_MEDIA_CODEC_INFO ?

https://codereview.chromium.org/14522002/diff/26001/media/base/android/webaud...
media/base/android/webaudio_media_codec_info.h:20: #endif //
MEDIA_BASE_ANDROID_MEDIA_CODEC_INFO_H_
ditto.

[Raymond Toy (Google), 2013-05-01 17:06:49]

Thanks, everyone, for the review.

[commit-bot: I haz the power, 2013-05-01 17:08:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtoy@google.com/14522002/35001

[commit-bot: I haz the power, 2013-05-01 17:29:44]

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

[Raymond Toy (Google), 2013-05-01 17:39:43]

+jamesr: Please review webkit/glue/webkitplatform_impl.cc

[jamesr, 2013-05-02 18:53:12]

lgtm

[commit-bot: I haz the power, 2013-05-02 18:54:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtoy@google.com/14522002/35001

[commit-bot: I haz the power, 2013-05-02 21:54:14]

Retried try job too often on win_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-05-02 22:06:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtoy@google.com/14522002/35001

[commit-bot: I haz the power, 2013-05-03 01:58:13]

Change committed as 198029