MediaSourcePlayer implements IsTypeSupported().

MediaSourcePlayer implements IsTypeSupported().

BUG=224793
TEST=Added unittests.

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

[xhwang, 2013-08-27 01:42:40]

I am not if I should introduce the name CanPlayType into implementation. Let me
know what you think.

ddorwin: Please review MediaSourcePlayer::CanPlayType() interface.

qinmin: Please review everything.

[ddorwin, 2013-08-27 18:14:19]

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_code...
File media/base/android/media_codec_bridge.h (right):

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_code...
media/base/android/media_codec_bridge.h:42: // Returns if MediaCodecBridge can
play |mime| type in |secure| mode.
1) Is |mime| the correct name?

2) Who is converting from web codec strings to Android-specific "mime types"? I
would think this would happen inside the bridge.

Assuming we address #2, #1 could be |codec| or something like that.

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_code...
media/base/android/media_codec_bridge.h:43: static bool CanPlayType(const char*
mime, bool secure);
Why not use "const std::string&" ?

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_code...
media/base/android/media_codec_bridge.h:106: explicit MediaCodecBridge(const
char* mime);
same x2
And the Audio/Video subclasses.

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_sour...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_sour...
media/base/android/media_source_player.cc:35: bool secure =
MediaDrmBridge::IsSecureDecoderRequired(security_level);
nit: is_secure

Really, is_secure_required, but that's long and would cause two more lines of
code.

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_sour...
File media/base/android/media_source_player.h (right):

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_sour...
media/base/android/media_source_player.h:44: static bool CanPlayType(const
std::vector<uint8>& scheme_uuid,
Use IsTypeSupported, since it is the permanent name for this functionality.

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_sour...
media/base/android/media_source_player.h:45: const std::string& security_level,
should we just call this "properties" to support other options? Going further,
maybe this should be a vector of pairs of strings. Of course, YAGNI, this is
simpler, and we can always change it.

[qinmin, 2013-08-27 21:41:12]

https://codereview.chromium.org/23517002/diff/1/media/base/android/java/src/o...
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
(right):

https://codereview.chromium.org/23517002/diff/1/media/base/android/java/src/o...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:113:
MediaCodec media_codec = null;
java coding style use camelCase

https://codereview.chromium.org/23517002/diff/1/media/base/android/java/src/o...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:120: }
catch (Exception e) {
use specific exception type

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_drm_...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_drm_...
media/base/android/media_drm_bridge.cc:141: static MediaDrmBridge::SecurityLevel
GetSecurityLevelFromString(
this doesn't needs to be a class member function, make it a helper function

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_sour...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_sour...
media/base/android/media_source_player.cc:37: if
(!MediaCodecBridge::CanPlayType(codecs[i].c_str(), secure))
how about putting the for loop inside MediaCodecBridge and pass a vector to
MediaDrmBridge::CanPlayType()

[xhwang, 2013-08-28 01:21:24]

PTAL again.

https://codereview.chromium.org/23517002/diff/1/media/base/android/java/src/o...
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
(right):

https://codereview.chromium.org/23517002/diff/1/media/base/android/java/src/o...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:113:
MediaCodec media_codec = null;
On 2013/08/27 21:41:12, qinmin wrote:
> java coding style use camelCase

Done.

https://codereview.chromium.org/23517002/diff/1/media/base/android/java/src/o...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:120: }
catch (Exception e) {
On 2013/08/27 21:41:12, qinmin wrote:
> use specific exception type

Well this is complicated. The old code assumes MediaCodec.createDecoderByType
could throw IOException (due to failure to load some library). But if I catch
IOException here I got a compiling error that "exception java.io.IOException is
never thrown in body of corresponding try statement". That's why I catch a more
general Exception. Do you know what's the best way to deal with this?

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_code...
File media/base/android/media_codec_bridge.h (right):

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_code...
media/base/android/media_codec_bridge.h:42: // Returns if MediaCodecBridge can
play |mime| type in |secure| mode.
On 2013/08/27 18:14:19, ddorwin wrote:
> 1) Is |mime| the correct name?
> 
> 2) Who is converting from web codec strings to Android-specific "mime types"?
I
> would think this would happen inside the bridge.

Current code uses AudioCodecToMimeType and VideoCodecToMimeType to do the
translation.
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...

What will the browser side have, a string for the codec, or a
media::AudioCodec/media::VideoCodec?
> 
> Assuming we address #2, #1 could be |codec| or something like that.

I am using char* and |mime| only to be consistent with the existing code :)

|mime| is really a bad name. But since Android API is using that all over the
place, I am not gonna fix it in this CL.

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_code...
media/base/android/media_codec_bridge.h:43: static bool CanPlayType(const char*
mime, bool secure);
On 2013/08/27 18:14:19, ddorwin wrote:
> Why not use "const std::string&" ?

Done.

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_code...
media/base/android/media_codec_bridge.h:106: explicit MediaCodecBridge(const
char* mime);
On 2013/08/27 18:14:19, ddorwin wrote:
> same x2
> And the Audio/Video subclasses.

ditto

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_drm_...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_drm_...
media/base/android/media_drm_bridge.cc:141: static MediaDrmBridge::SecurityLevel
GetSecurityLevelFromString(
On 2013/08/27 21:41:12, qinmin wrote:
> this doesn't needs to be a class member function, make it a helper function

This is already a helper function and not a class member function.

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_sour...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_sour...
media/base/android/media_source_player.cc:35: bool secure =
MediaDrmBridge::IsSecureDecoderRequired(security_level);
On 2013/08/27 18:14:19, ddorwin wrote:
> nit: is_secure
> 
> Really, is_secure_required, but that's long and would cause two more lines of
> code.

I debated about that. |is_secure| isn't accurate either.
|is_secure_decoder_required| will be too long...

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_sour...
media/base/android/media_source_player.cc:37: if
(!MediaCodecBridge::CanPlayType(codecs[i].c_str(), secure))
On 2013/08/27 21:41:12, qinmin wrote:
> how about putting the for loop inside MediaCodecBridge and pass a vector to
> MediaDrmBridge::CanPlayType()

This function is already complicated (involves MediaDrm and MediaCodec) so I
like to keep all complicated things here. This way, I can make MediaCodec
relatively simple. What do you think?

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_sour...
File media/base/android/media_source_player.h (right):

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_sour...
media/base/android/media_source_player.h:44: static bool CanPlayType(const
std::vector<uint8>& scheme_uuid,
On 2013/08/27 18:14:19, ddorwin wrote:
> Use IsTypeSupported, since it is the permanent name for this functionality.

Done.

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_sour...
media/base/android/media_source_player.h:45: const std::string& security_level,
On 2013/08/27 18:14:19, ddorwin wrote:
> should we just call this "properties" to support other options? Going further,
> maybe this should be a vector of pairs of strings. Of course, YAGNI, this is
> simpler, and we can always change it.

Yeah, added a TODO for the possible future refactoring.

[ddorwin, 2013-08-28 02:28:49]

I'll defer to qinmin now.

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_code...
File media/base/android/media_codec_bridge.h (right):

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_code...
media/base/android/media_codec_bridge.h:42: // Returns if MediaCodecBridge can
play |mime| type in |secure| mode.
On 2013/08/28 01:21:24, xhwang wrote:
> On 2013/08/27 18:14:19, ddorwin wrote:
> > 1) Is |mime| the correct name?
> > 
> > 2) Who is converting from web codec strings to Android-specific "mime
types"?
> I
> > would think this would happen inside the bridge.
> 
> Current code uses AudioCodecToMimeType and VideoCodecToMimeType to do the
> translation.
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...
> 
> What will the browser side have, a string for the codec, or a
> media::AudioCodec/media::VideoCodec?

I think we should hide these platform-specific strings as deep as possible,
which would mean hiding it within the browser. I think the browser/renderer API
will use standard strings for all platforms. (Of course, this means doing more
work in the browser process, but I think it's the right thing to do from an API
perspective.)
> > 
> > Assuming we address #2, #1 could be |codec| or something like that.
> 
> I am using char* and |mime| only to be consistent with the existing code :)
> 
> |mime| is really a bad name. But since Android API is using that all over the
> place, I am not gonna fix it in this CL.

Similarly, it would be nice to hide this inconsistency (use of "mime") in the
lowest levels. Do we use mime in the Android sense in the player, bridges, etc.
already? If not, I think it would be nice to present the normal use here. That
could be a different CL, though, since this CL seems to be getting close.

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_sour...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_sour...
media/base/android/media_source_player.cc:35: bool secure =
MediaDrmBridge::IsSecureDecoderRequired(security_level);
On 2013/08/28 01:21:24, xhwang wrote:
> On 2013/08/27 18:14:19, ddorwin wrote:
> > nit: is_secure
> > 
> > Really, is_secure_required, but that's long and would cause two more lines
of
> > code.
> 
> I debated about that. |is_secure| isn't accurate either.
> |is_secure_decoder_required| will be too long...

Yes, but bools should generally have an is_ (or similar) prefix. How about
use_secure?

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_sour...
File media/base/android/media_source_player.h (right):

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_sour...
media/base/android/media_source_player.h:45: const std::string& security_level,
On 2013/08/28 01:21:24, xhwang wrote:
> On 2013/08/27 18:14:19, ddorwin wrote:
> > should we just call this "properties" to support other options? Going
further,
> > maybe this should be a vector of pairs of strings. Of course, YAGNI, this is
> > simpler, and we can always change it.
> 
> Yeah, added a TODO for the possible future refactoring.

I'd just drop the TODO since we don't plan to address it unless/until needed. We
recognize and accept the current limitation, and the solution will hopefully be
obvious when the time comes.

https://codereview.chromium.org/23517002/diff/14001/media/base/android/java/s...
File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right):

https://codereview.chromium.org/23517002/diff/14001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:176:
Log.e(TAG, "generateKeyRequest is called when another call is pending.");
nit: remove "is "

https://codereview.chromium.org/23517002/diff/14001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:274:
Log.d(TAG, "provide key response.");
nit: This sounds like a command. Should we just use the function name?

https://codereview.chromium.org/23517002/diff/14001/media/base/android/media_...
File media/base/android/media_codec_bridge.cc (right):

https://codereview.chromium.org/23517002/diff/14001/media/base/android/media_...
media/base/android/media_codec_bridge.cc:40: return "";
std::string()

https://codereview.chromium.org/23517002/diff/14001/media/base/android/media_...
media/base/android/media_codec_bridge.cc:253: DCHECK(!codec_string.empty());
Who calls Start()? Are we sure invalid types won't be passed? Does it come from
a place where it might have already been converted?

https://codereview.chromium.org/23517002/diff/14001/media/base/android/media_...
media/base/android/media_codec_bridge.cc:426: return mime.empty() ? NULL: new
AudioCodecBridge(mime);
space before ':'
below too

https://codereview.chromium.org/23517002/diff/14001/media/base/android/media_...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/23517002/diff/14001/media/base/android/media_...
media/base/android/media_source_player.cc:38: if
(!MediaCodecBridge::IsDecoderSupported(codecs[i].c_str(), secure))
s/Decoder/Codec/? Otherwise, maybe HasDecoder().

https://codereview.chromium.org/23517002/diff/14001/media/base/android/media_...
File media/base/android/media_source_player_unittest.cc (right):

https://codereview.chromium.org/23517002/diff/14001/media/base/android/media_...
media/base/android/media_source_player_unittest.cc:482:
EXPECT_TRUE(MediaSourcePlayer::IsTypeSupported(
Isn't this currently false for WebM on real systems?

https://codereview.chromium.org/23517002/diff/14001/media/base/android/media_...
media/base/android/media_source_player_unittest.cc:518: TODO(xhwang): Add more
IsTypeSupported tests.
FYI, when my work is done, we'll have chrome browsertests that may give a lot of
coverage here.

[xhwang, 2013-08-28 18:19:15]

Most comments addressed. I still can't test my CL with the real container types
because the latest builds seem to be broken :( So, please review my
implementation, and I'll test if it really works later :)

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_code...
File media/base/android/media_codec_bridge.h (right):

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_code...
media/base/android/media_codec_bridge.h:42: // Returns if MediaCodecBridge can
play |mime| type in |secure| mode.
On 2013/08/28 02:28:50, ddorwin wrote:
> On 2013/08/28 01:21:24, xhwang wrote:
> > On 2013/08/27 18:14:19, ddorwin wrote:
> > > 1) Is |mime| the correct name?
> > > 
> > > 2) Who is converting from web codec strings to Android-specific "mime
> types"?
> > I
> > > would think this would happen inside the bridge.
> > 
> > Current code uses AudioCodecToMimeType and VideoCodecToMimeType to do the
> > translation.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...
> > 
> > What will the browser side have, a string for the codec, or a
> > media::AudioCodec/media::VideoCodec?
> 
> I think we should hide these platform-specific strings as deep as possible,
> which would mean hiding it within the browser. I think the browser/renderer
API
> will use standard strings for all platforms. (Of course, this means doing more
> work in the browser process, but I think it's the right thing to do from an
API
> perspective.)

I agree that we should not expose this non-standard mime to the outside world.
My question is what we should use instead? media::AudioCodec/media::VideoCodec,
or strings like "vp8", as listed in
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/mime_util...
I guess string will be easier to pass here. I'll go with it and do conversion in
this file.
> > > 
> > > Assuming we address #2, #1 could be |codec| or something like that.
> > 
> > I am using char* and |mime| only to be consistent with the existing code :)
> > 
> > |mime| is really a bad name. But since Android API is using that all over
the
> > place, I am not gonna fix it in this CL.
> 
> Similarly, it would be nice to hide this inconsistency (use of "mime") in the
> lowest levels. Do we use mime in the Android sense in the player, bridges,
etc.
> already? If not, I think it would be nice to present the normal use here. That
> could be a different CL, though, since this CL seems to be getting close.

Agreed. Will do it in a separate CL, and probably after we have feature complete
since it's lower priority.

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_sour...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_sour...
media/base/android/media_source_player.cc:35: bool secure =
MediaDrmBridge::IsSecureDecoderRequired(security_level);
On 2013/08/28 02:28:50, ddorwin wrote:
> On 2013/08/28 01:21:24, xhwang wrote:
> > On 2013/08/27 18:14:19, ddorwin wrote:
> > > nit: is_secure
> > > 
> > > Really, is_secure_required, but that's long and would cause two more lines
> of
> > > code.
> > 
> > I debated about that. |is_secure| isn't accurate either.
> > |is_secure_decoder_required| will be too long...
> 
> Yes, but bools should generally have an is_ (or similar) prefix. How about
> use_secure?

Changed to is_secure, which is short and fits well in HasDecoder(mime,
is_secure).

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_sour...
File media/base/android/media_source_player.h (right):

https://codereview.chromium.org/23517002/diff/1/media/base/android/media_sour...
media/base/android/media_source_player.h:45: const std::string& security_level,
On 2013/08/28 02:28:50, ddorwin wrote:
> On 2013/08/28 01:21:24, xhwang wrote:
> > On 2013/08/27 18:14:19, ddorwin wrote:
> > > should we just call this "properties" to support other options? Going
> further,
> > > maybe this should be a vector of pairs of strings. Of course, YAGNI, this
is
> > > simpler, and we can always change it.
> > 
> > Yeah, added a TODO for the possible future refactoring.
> 
> I'd just drop the TODO since we don't plan to address it unless/until needed.
We
> recognize and accept the current limitation, and the solution will hopefully
be
> obvious when the time comes.

Done.

https://codereview.chromium.org/23517002/diff/14001/media/base/android/java/s...
File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right):

https://codereview.chromium.org/23517002/diff/14001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:176:
Log.e(TAG, "generateKeyRequest is called when another call is pending.");
On 2013/08/28 02:28:50, ddorwin wrote:
> nit: remove "is "

Done.

https://codereview.chromium.org/23517002/diff/14001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:274:
Log.d(TAG, "provide key response.");
On 2013/08/28 02:28:50, ddorwin wrote:
> nit: This sounds like a command. Should we just use the function name?

Done.

https://codereview.chromium.org/23517002/diff/14001/media/base/android/media_...
File media/base/android/media_codec_bridge.cc (right):

https://codereview.chromium.org/23517002/diff/14001/media/base/android/media_...
media/base/android/media_codec_bridge.cc:40: return "";
On 2013/08/28 02:28:50, ddorwin wrote:
> std::string()

Done.

https://codereview.chromium.org/23517002/diff/14001/media/base/android/media_...
media/base/android/media_codec_bridge.cc:253: DCHECK(!codec_string.empty());
On 2013/08/28 02:28:50, ddorwin wrote:
> Who calls Start()? Are we sure invalid types won't be passed? Does it come
from
> a place where it might have already been converted?

Good point. It comes from renderer. So we could hit empty string here. Replaced
DCHECK with runtime check.

https://codereview.chromium.org/23517002/diff/14001/media/base/android/media_...
media/base/android/media_codec_bridge.cc:426: return mime.empty() ? NULL: new
AudioCodecBridge(mime);
On 2013/08/28 02:28:50, ddorwin wrote:
> space before ':'
> below too

Done.

https://codereview.chromium.org/23517002/diff/14001/media/base/android/media_...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/23517002/diff/14001/media/base/android/media_...
media/base/android/media_source_player.cc:38: if
(!MediaCodecBridge::IsDecoderSupported(codecs[i].c_str(), secure))
On 2013/08/28 02:28:50, ddorwin wrote:
> s/Decoder/Codec/? Otherwise, maybe HasDecoder().

Done.

https://codereview.chromium.org/23517002/diff/14001/media/base/android/media_...
File media/base/android/media_source_player_unittest.cc (right):

https://codereview.chromium.org/23517002/diff/14001/media/base/android/media_...
media/base/android/media_source_player_unittest.cc:482:
EXPECT_TRUE(MediaSourcePlayer::IsTypeSupported(
On 2013/08/28 02:28:50, ddorwin wrote:
> Isn't this currently false for WebM on real systems?

That check is commented out. I'll enable it, test it with the latest image and
update this test.

https://codereview.chromium.org/23517002/diff/14001/media/base/android/media_...
media/base/android/media_source_player_unittest.cc:518: TODO(xhwang): Add more
IsTypeSupported tests.
On 2013/08/28 02:28:50, ddorwin wrote:
> FYI, when my work is done, we'll have chrome browsertests that may give a lot
of
> coverage here.

I see. SG.

[xhwang, 2013-08-28 18:20:11]

BTW, sorry for the rebase noise...

[qinmin, 2013-08-28 18:39:58]

lgtm % nit

https://codereview.chromium.org/23517002/diff/28001/media/base/android/java/s...
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
(right):

https://codereview.chromium.org/23517002/diff/28001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:112:
private static MediaCodecBridge create(String mime, boolean secure) {
nit: s/secure/isSecure/

[ddorwin, 2013-08-28 18:41:56]

lgtm. Comments can be addressed now or later.

https://codereview.chromium.org/23517002/diff/28001/media/base/android/media_...
File media/base/android/media_codec_bridge.cc (right):

https://codereview.chromium.org/23517002/diff/28001/media/base/android/media_...
media/base/android/media_codec_bridge.cc:44: static const std::string
VideoCodecToMimeType(const VideoCodec codec) {
Who is converting to these codec enums? It seems we should have a consistent API
that always takes an enum or string.

https://codereview.chromium.org/23517002/diff/28001/media/base/android/media_...
media/base/android/media_codec_bridge.cc:58: // TODO(xhwang): Shall we handle
strings like "mp4a.40.2"?
We should*. We'll need to see whether the upper layers pass this down or pass
just the stripped version. If the latter, we should DCHECK that the value does
not start with "mp4a.".

On second thought: The reason I say we should is that it's possible that the
platform (this code) can do something with the additional information. However,
it doesn't appear Android differentiates, so maybe we should just simplify
things in the browser. And maybe we can use an enum (made common with the above
conversions).

* I know Chrome allows this for avc1, but I'm not sure about mp4a. The TODO
should probably be above line 56.

https://codereview.chromium.org/23517002/diff/28001/media/base/android/media_...
File media/base/android/media_codec_bridge.h (right):

https://codereview.chromium.org/23517002/diff/28001/media/base/android/media_...
media/base/android/media_codec_bridge.h:42: // Returns if MediaCodecBridge has a
decoder that |is_secure| and can decode
nit: "if" > "true if" or "whether"

https://codereview.chromium.org/23517002/diff/28001/media/base/android/media_...
File media/base/android/media_source_player_unittest.cc (right):

https://codereview.chromium.org/23517002/diff/28001/media/base/android/media_...
media/base/android/media_source_player_unittest.cc:188: return
MediaSourcePlayer::IsTypeSupported(
Would a using statement avoid the need for this function?

[ddorwin, 2013-08-29 21:45:52]

Please update the description.

[xhwang, 2013-08-30 05:32:38]

Comments addressed. It seems the Android bug is fixed already and I'll give this
a test tomorrow.

https://codereview.chromium.org/23517002/diff/28001/media/base/android/java/s...
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
(right):

https://codereview.chromium.org/23517002/diff/28001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:112:
private static MediaCodecBridge create(String mime, boolean secure) {
On 2013/08/28 18:39:59, qinmin wrote:
> nit: s/secure/isSecure/

Done.

https://codereview.chromium.org/23517002/diff/28001/media/base/android/media_...
File media/base/android/media_codec_bridge.cc (right):

https://codereview.chromium.org/23517002/diff/28001/media/base/android/media_...
media/base/android/media_codec_bridge.cc:44: static const std::string
VideoCodecToMimeType(const VideoCodec codec) {
On 2013/08/28 18:41:56, ddorwin wrote:
> Who is converting to these codec enums? It seems we should have a consistent
API
> that always takes an enum or string.

Enums (AudioCodec et al) comes from media code. It comes from container meta
info and converted to enum by the demuxer.

https://codereview.chromium.org/23517002/diff/28001/media/base/android/media_...
media/base/android/media_codec_bridge.cc:58: // TODO(xhwang): Shall we handle
strings like "mp4a.40.2"?
On 2013/08/28 18:41:56, ddorwin wrote:
> We should*. We'll need to see whether the upper layers pass this down or pass
> just the stripped version. If the latter, we should DCHECK that the value does
> not start with "mp4a.".
> 
> On second thought: The reason I say we should is that it's possible that the
> platform (this code) can do something with the additional information.
However,
> it doesn't appear Android differentiates, so maybe we should just simplify
> things in the browser. And maybe we can use an enum (made common with the
above
> conversions).
> 
> * I know Chrome allows this for avc1, but I'm not sure about mp4a. The TODO
> should probably be above line 56.

Currently mime_util doesn't have this granularity. We need to figure out at what
layer we strip out the codec list and at what precision we keep those strings. 

TODO moved up and we'll address that in a later CL.

https://codereview.chromium.org/23517002/diff/28001/media/base/android/media_...
File media/base/android/media_codec_bridge.h (right):

https://codereview.chromium.org/23517002/diff/28001/media/base/android/media_...
media/base/android/media_codec_bridge.h:42: // Returns if MediaCodecBridge has a
decoder that |is_secure| and can decode
On 2013/08/28 18:41:56, ddorwin wrote:
> nit: "if" > "true if" or "whether"

Done.

https://codereview.chromium.org/23517002/diff/28001/media/base/android/media_...
File media/base/android/media_source_player_unittest.cc (right):

https://codereview.chromium.org/23517002/diff/28001/media/base/android/media_...
media/base/android/media_source_player_unittest.cc:188: return
MediaSourcePlayer::IsTypeSupported(
On 2013/08/28 18:41:57, ddorwin wrote:
> Would a using statement avoid the need for this function?

We can only "using foo_namespace::Func1", not "using bar_class::Func2", sigh!

[commit-bot: I haz the power, 2013-08-30 16:36:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/23517002/46001

[ddorwin, 2013-08-30 16:59:15]

Just a question for future consideration.

https://codereview.chromium.org/23517002/diff/46001/media/base/android/media_...
File media/base/android/media_codec_bridge.cc (right):

https://codereview.chromium.org/23517002/diff/46001/media/base/android/media_...
media/base/android/media_codec_bridge.cc:55: static const std::string
CodecTypeToMimeType(const std::string& codec) {
What do you think of using the same type for this and the two methods above?

[commit-bot: I haz the power, 2013-08-30 19:35:47]

Change committed as 220630

[antoniooh47, 2013-12-14 03:48:48]

On 2013/08/30 19:35:47, I haz the power (commit-bot) wrote:
> Change committed as 220630

Xk vas a cambiar as 220630 xk yo estoy biendo lo k disen