Add Create() function to AudioCodecBridge and VideoCodecBridge to allow return of null pointers

Add Create() function to AudioCodecBridge and VideoCodecBridge to allow return of null pointers

If codec is not supported, we should allow null pointers to be returned when trying to create a MediaCodecBridge.

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

[qinmin, 2013-05-16 00:58:40]

PTAL

[dwkang1, 2013-05-16 01:53:53]

https://chromiumcodereview.appspot.com/14932020/diff/1/content/common/gpu/med...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/14932020/diff/1/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator.cc:379: void
AndroidVideoDecodeAccelerator::ConfigureMediaCodec() {
Could you make this return bool and make Initialize() return false when
VideoCodecBridge::Create() returns null?

[qinmin, 2013-05-16 04:07:43]

https://chromiumcodereview.appspot.com/14932020/diff/1/content/common/gpu/med...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/14932020/diff/1/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator.cc:379: void
AndroidVideoDecodeAccelerator::ConfigureMediaCodec() {
On 2013/05/16 01:53:54, dwkang1 wrote:
> Could you make this return bool and make Initialize() return false when
> VideoCodecBridge::Create() returns null?

Done.

[dwkang1, 2013-05-16 04:43:14]

LGTM

[acolwell GONE FROM CHROMIUM, 2013-05-16 14:28:32]

Please add a test that verifies the failure condition that you're adding support
for.

[qinmin, 2013-05-16 14:58:14]

On 2013/05/16 14:28:32, acolwell wrote:
> Please add a test that verifies the failure condition that you're adding
support
> for.

done. test added

[acolwell GONE FROM CHROMIUM, 2013-05-16 15:52:38]

On 2013/05/16 14:58:14, qinmin wrote:
> On 2013/05/16 14:28:32, acolwell wrote:
> > Please add a test that verifies the failure condition that you're adding
> support
> > for.
> 
> done. test added

Sorry. I actually meant adding a test that exercises the changes to
AndroidVideoDecodeAccelerator, but I'll take these ones too. :)

[qinmin, 2013-05-16 22:47:58]

On 2013/05/16 15:52:38, acolwell wrote:
> On 2013/05/16 14:58:14, qinmin wrote:
> > On 2013/05/16 14:28:32, acolwell wrote:
> > > Please add a test that verifies the failure condition that you're adding
> > support
> > > for.
> > 
> > done. test added
> 
> Sorry. I actually meant adding a test that exercises the changes to
> AndroidVideoDecodeAccelerator, but I'll take these ones too. :)

added unit tests for ConfigureMediaCodec(). The class is not very friendly for
testing.
1. Initialize() only accepts VP8 for now, so ConfigureMediaCodec() should always
pass.
2. There are some GL calls in Initialize(). We need gl bindings so tha t it can
work properly.
The tests i wrote bypasses the Initialize() call, only testing the
ConfigureMediaCodec() call I modified. I think we need to add more test for
other functions of the file, http://crbug.com/178647 was filed some time ago to
keep track of that.

[acolwell GONE FROM CHROMIUM, 2013-05-17 14:17:18]

lgtm

[qinmin, 2013-05-17 16:03:57]

+brettw for content/content_tests.gypi

[qinmin, 2013-05-20 19:24:18]

ping. brettw, would you please take a look? I need OWNER stamp for
content/content_tests.gypi

[brettw, 2013-05-21 17:39:05]

https://chromiumcodereview.appspot.com/14932020/diff/13001/content/content_te...
File content/content_tests.gypi (right):

https://chromiumcodereview.appspot.com/14932020/diff/13001/content/content_te...
content/content_tests.gypi:992: 
I think our rule is that the sources section should list all the sources, and
then if some are inappropriate for some platform, then remove them later (rather
than including them in the conditional).

https://chromiumcodereview.appspot.com/14932020/diff/13001/media/base/android...
File media/base/android/media_codec_bridge.h (right):

https://chromiumcodereview.appspot.com/14932020/diff/13001/media/base/android...
media/base/android/media_codec_bridge.h:117: 
Can you document above these two create functions that they can return NULL when
the codec is invalid (or whatever).

[qinmin, 2013-05-21 21:36:39]

https://chromiumcodereview.appspot.com/14932020/diff/13001/content/content_te...
File content/content_tests.gypi (right):

https://chromiumcodereview.appspot.com/14932020/diff/13001/content/content_te...
content/content_tests.gypi:992: 
On 2013/05/21 17:39:05, brettw wrote:
> I think our rule is that the sources section should list all the sources, and
> then if some are inappropriate for some platform, then remove them later
(rather
> than including them in the conditional).

Done.

https://chromiumcodereview.appspot.com/14932020/diff/13001/media/base/android...
File media/base/android/media_codec_bridge.h (right):

https://chromiumcodereview.appspot.com/14932020/diff/13001/media/base/android...
media/base/android/media_codec_bridge.h:117: 
On 2013/05/21 17:39:05, brettw wrote:
> Can you document above these two create functions that they can return NULL
when
> the codec is invalid (or whatever).

Done.

[brettw, 2013-05-22 18:33:07]

https://chromiumcodereview.appspot.com/14932020/diff/34001/content/common/gpu...
File content/common/gpu/media/android_video_decode_accelerator_unittest.cc
(right):

https://chromiumcodereview.appspot.com/14932020/diff/34001/content/common/gpu...
content/common/gpu/media/android_video_decode_accelerator_unittest.cc:20:
namespace {
Namespace formatting: blank lines around beginning & end, "// namespace" comment
after the }

https://chromiumcodereview.appspot.com/14932020/diff/34001/content/common/gpu...
content/common/gpu/media/android_video_decode_accelerator_unittest.cc:26:
namespace content {
Blank line after.

https://chromiumcodereview.appspot.com/14932020/diff/34001/content/common/gpu...
content/common/gpu/media/android_video_decode_accelerator_unittest.cc:96: int
main(int argc, char **argv) {
I don't understand why this individual test file has a main() in it.

[qinmin, 2013-05-22 19:02:17]

https://chromiumcodereview.appspot.com/14932020/diff/34001/content/common/gpu...
File content/common/gpu/media/android_video_decode_accelerator_unittest.cc
(right):

https://chromiumcodereview.appspot.com/14932020/diff/34001/content/common/gpu...
content/common/gpu/media/android_video_decode_accelerator_unittest.cc:20:
namespace {
On 2013/05/22 18:33:07, brettw wrote:
> Namespace formatting: blank lines around beginning & end, "// namespace"
comment
> after the }

Done.

https://chromiumcodereview.appspot.com/14932020/diff/34001/content/common/gpu...
content/common/gpu/media/android_video_decode_accelerator_unittest.cc:26:
namespace content {
On 2013/05/22 18:33:07, brettw wrote:
> Blank line after.

Done.

https://chromiumcodereview.appspot.com/14932020/diff/34001/content/common/gpu...
content/common/gpu/media/android_video_decode_accelerator_unittest.cc:96: int
main(int argc, char **argv) {
This is because the file is built as a single test executable/apk. Similar to
video_decode_accelerator_test.cc, it also needs a main() in it.
If a test is included in a big test suite, then only one main() is needed for
the whole suite. For content eg., a main() is defined in test_launcher.cc.

On 2013/05/22 18:33:07, brettw wrote:
> I don't understand why this individual test file has a main() in it.

[brettw, 2013-05-22 19:38:03]

Normally I would expect a dependency on base run_all_unittests instead but I see
the non-Android one needs some special setup. So I guess this is fine. LGTM

[commit-bot: I haz the power, 2013-05-22 20:23:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/14932020/41001

[commit-bot: I haz the power, 2013-05-23 08:34:50]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android_dbg, revision is HEAD

[commit-bot: I haz the power, 2013-05-23 16:09:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/14932020/41001

[commit-bot: I haz the power, 2013-05-23 23:27:48]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android_dbg, revision is HEAD

[commit-bot: I haz the power, 2013-05-24 00:21:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/14932020/77001

[commit-bot: I haz the power, 2013-05-24 06:18:56]

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

[commit-bot: I haz the power, 2013-05-24 18:59:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/14932020/104001

[commit-bot: I haz the power, 2013-05-24 19:14:07]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-25 21:45:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/14932020/137001

[commit-bot: I haz the power, 2013-05-26 02:04:26]

Change committed as 202323