Bindings for injectable Java video encoders.

Bindings for injectable Java video encoders.

BUG=webrtc:7760
Review-Url: https://codereview.webrtc.org/3003873002
Cr-Commit-Position: refs/heads/master@{#19651}
Committed: https://chromium.googlesource.com/external/webrtc/+/07a3bd7c4b54399e2abd4713619427f9638ab6d9

[sakal, 2017-08-28 10:51:30]

Patchset #2 (id:20001) has been deleted

[sakal, 2017-08-28 10:51:36]

Patchset #1 (id:1) has been deleted

[sakal, 2017-08-28 10:51:49]

sakal@webrtc.org changed reviewers:
+ magjed@webrtc.org

[sakal, 2017-08-28 10:51:49]

PTAL

[sakal, 2017-08-28 10:53:13]

Patchset #1 (id:40001) has been deleted

[magjed_webrtc, 2017-08-28 16:54:23]

https://codereview.webrtc.org/3003873002/diff/60001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/videoencoderfactorywrapper.cc (right):

https://codereview.webrtc.org/3003873002/diff/60001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/videoencoderfactorywrapper.cc:89: // TODO(sakal):
Parse params
Is this not needed?

https://codereview.webrtc.org/3003873002/diff/60001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/videoencoderfactorywrapper.h (right):

https://codereview.webrtc.org/3003873002/diff/60001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/videoencoderfactorywrapper.h:20: namespace webrtc_jni
{
Nest the namespaces instead.

https://codereview.webrtc.org/3003873002/diff/60001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/videoencoderwrapper.cc (right):

https://codereview.webrtc.org/3003873002/diff/60001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/videoencoderwrapper.cc:24: namespace webrtc_jni {
Nest the namespaces instead.

https://codereview.webrtc.org/3003873002/diff/60001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/videoencoderwrapper.cc:178: ScopedLocalRefFrame
local_ref_frame(jni);
Remove this if it's not needed since this is called for every frame.

https://codereview.webrtc.org/3003873002/diff/60001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/videoencoderwrapper.cc:266: // TOOD(sakal): Maybe
this should be a different timestamp?
Sounds like something we should figure out. Is it possible to keep the current
behavior from MediaCodecVideoEncoder?

https://codereview.webrtc.org/3003873002/diff/60001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/videoencoderwrapper.cc:337: int qp;
I would like to write it like this instead:
int qp = -1;
bool success = false;
switch (codec_settings_.codecType) {
  case webrtc::kVideoCodecVP8: 
    success = webrtc::vp8::GetQp(buffer, buffer_size, &qp);
    break;
  ...
}
return success ? qp : -1;

https://codereview.webrtc.org/3003873002/diff/60001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/videoencoderwrapper.h (right):

https://codereview.webrtc.org/3003873002/diff/60001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/videoencoderwrapper.h:24: namespace webrtc_jni {
Nest the namespaces instead.

https://codereview.webrtc.org/3003873002/diff/60001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/videoencoderwrapper.h:33: // Initialize the encoder
with the information from the codecSettings
You don't have to copy the documentation for overrides.

[sakal, 2017-08-29 12:59:10]

Patchset #2 (id:80001) has been deleted

[sakal, 2017-08-29 12:59:39]

https://codereview.webrtc.org/3003873002/diff/60001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/videoencoderfactorywrapper.cc (right):

https://codereview.webrtc.org/3003873002/diff/60001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/videoencoderfactorywrapper.cc:89: // TODO(sakal):
Parse params
On 2017/08/28 16:54:21, magjed_webrtc wrote:
> Is this not needed?

Missed this, implemented it now.

https://codereview.webrtc.org/3003873002/diff/60001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/videoencoderfactorywrapper.h (right):

https://codereview.webrtc.org/3003873002/diff/60001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/videoencoderfactorywrapper.h:20: namespace webrtc_jni
{
On 2017/08/28 16:54:21, magjed_webrtc wrote:
> Nest the namespaces instead.

Done.

https://codereview.webrtc.org/3003873002/diff/60001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/videoencoderwrapper.cc (right):

https://codereview.webrtc.org/3003873002/diff/60001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/videoencoderwrapper.cc:24: namespace webrtc_jni {
On 2017/08/28 16:54:22, magjed_webrtc wrote:
> Nest the namespaces instead.

Done.

https://codereview.webrtc.org/3003873002/diff/60001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/videoencoderwrapper.cc:178: ScopedLocalRefFrame
local_ref_frame(jni);
On 2017/08/28 16:54:22, magjed_webrtc wrote:
> Remove this if it's not needed since this is called for every frame.

This is needed because we call methods that return objects?

https://codereview.webrtc.org/3003873002/diff/60001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/videoencoderwrapper.cc:266: // TOOD(sakal): Maybe
this should be a different timestamp?
On 2017/08/28 16:54:22, magjed_webrtc wrote:
> Sounds like something we should figure out. Is it possible to keep the current
> behavior from MediaCodecVideoEncoder?

Done.

https://codereview.webrtc.org/3003873002/diff/60001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/videoencoderwrapper.cc:337: int qp;
On 2017/08/28 16:54:22, magjed_webrtc wrote:
> I would like to write it like this instead:
> int qp = -1;
> bool success = false;
> switch (codec_settings_.codecType) {
>   case webrtc::kVideoCodecVP8: 
>     success = webrtc::vp8::GetQp(buffer, buffer_size, &qp);
>     break;
>   ...
> }
> return success ? qp : -1;

Done.

https://codereview.webrtc.org/3003873002/diff/60001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/videoencoderwrapper.h (right):

https://codereview.webrtc.org/3003873002/diff/60001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/videoencoderwrapper.h:24: namespace webrtc_jni {
On 2017/08/28 16:54:23, magjed_webrtc wrote:
> Nest the namespaces instead.

Done.

https://codereview.webrtc.org/3003873002/diff/60001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/videoencoderwrapper.h:33: // Initialize the encoder
with the information from the codecSettings
On 2017/08/28 16:54:23, magjed_webrtc wrote:
> You don't have to copy the documentation for overrides.

Done.

[sakal, 2017-08-29 13:00:52]

I also moved encoded frame delivery to encoder queue. This is the preferred way
nowadays and we don't want to regress in this aspect.

[magjed_webrtc, 2017-08-30 10:34:43]

lgtm

https://codereview.webrtc.org/3003873002/diff/60001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/videoencoderwrapper.cc (right):

https://codereview.webrtc.org/3003873002/diff/60001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/videoencoderwrapper.cc:178: ScopedLocalRefFrame
local_ref_frame(jni);
On 2017/08/29 12:59:38, sakal wrote:
> On 2017/08/28 16:54:22, magjed_webrtc wrote:
> > Remove this if it's not needed since this is called for every frame.
> 
> This is needed because we call methods that return objects?

Acknowledged.

[sakal, 2017-08-31 09:37:50]

The CQ bit was checked by sakal@webrtc.org

[commit-bot: I haz the power, 2017-08-31 09:37:55]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-08-31 09:46:04]

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

[commit-bot: I haz the power, 2017-08-31 09:46:05]

Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/27225)

[sakal, 2017-08-31 10:32:46]

The CQ bit was checked by sakal@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-31 10:32:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-08-31 10:36:23]

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

[commit-bot: I haz the power, 2017-08-31 10:36:24]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
  android_compile_x86_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...)
  android_compile_x86_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_rel...)

[sakal, 2017-08-31 10:44:41]

The CQ bit was checked by sakal@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-31 10:44:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-08-31 10:48:13]

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

[commit-bot: I haz the power, 2017-08-31 10:48:15]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
  android_compile_mips_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_mips_db...)
  android_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/27230)

[sakal, 2017-09-01 08:27:27]

The CQ bit was checked by sakal@webrtc.org

[sakal, 2017-09-01 08:27:27]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org
Link to the patchset: https://codereview.webrtc.org/3003873002/#ps160001 (title:
"Add dep")

[commit-bot: I haz the power, 2017-09-01 08:27:36]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-09-01 08:32:27]

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

[commit-bot: I haz the power, 2017-09-01 08:32:27]

Try jobs failed on following builders:
  android_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...)
  android_compile_x86_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...)
  android_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/27260)

[sakal, 2017-09-01 08:37:20]

The CQ bit was checked by sakal@webrtc.org

[sakal, 2017-09-01 08:37:21]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org
Link to the patchset: https://codereview.webrtc.org/3003873002/#ps180001 (title:
"Fix tests")

[commit-bot: I haz the power, 2017-09-01 08:37:28]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-09-01 09:09:17]

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

[commit-bot: I haz the power, 2017-09-01 09:09:18]

Try jobs failed on following builders:
  linux_ubsan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/15282)

[sakal, 2017-09-01 11:04:27]

The CQ bit was checked by sakal@webrtc.org

[commit-bot: I haz the power, 2017-09-01 11:04:35]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-09-01 11:27:24]

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

[commit-bot: I haz the power, 2017-09-01 11:27:25]

Try jobs failed on following builders:
  linux_ubsan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/15297)

[sakal, 2017-09-01 11:28:38]

The CQ bit was checked by sakal@webrtc.org

[commit-bot: I haz the power, 2017-09-01 11:28:44]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-09-01 11:59:13]

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

[commit-bot: I haz the power, 2017-09-01 11:59:14]

Try jobs failed on following builders:
  linux_msan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/23497)

[sakal, 2017-09-01 17:22:41]

The CQ bit was checked by sakal@webrtc.org

[commit-bot: I haz the power, 2017-09-01 17:22:50]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-09-01 17:52:39]

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

[commit-bot: I haz the power, 2017-09-01 17:52:45]

Try jobs failed on following builders:
  linux_msan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/23527)

[sakal, 2017-09-04 07:56:14]

The CQ bit was checked by sakal@webrtc.org

[commit-bot: I haz the power, 2017-09-04 07:56:25]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-09-04 10:57:22]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1504511774231060,
"parent_rev": "75204c5ccdda561ccffbfa3a8fa94feac8e19e30", "commit_rev":
"07a3bd7c4b54399e2abd4713619427f9638ab6d9"}

[commit-bot: I haz the power, 2017-09-04 10:57:27]

Description was changed from

==========
Bindings for injectable Java video encoders.

BUG=webrtc:7760
==========

to

==========
Bindings for injectable Java video encoders.

BUG=webrtc:7760

Review-Url: https://codereview.webrtc.org/3003873002
Cr-Commit-Position: refs/heads/master@{#19651}
Committed:
https://chromium.googlesource.com/external/webrtc/+/07a3bd7c4b54399e2abd47136...
==========

[commit-bot: I haz the power, 2017-09-04 10:57:29]

Committed patchset #6 (id:180001) as
https://chromium.googlesource.com/external/webrtc/+/07a3bd7c4b54399e2abd47136...