EglRenderer: Trigger framelisteners even on dropped frames by default.

Trigger framelisteners even on frames dropped by the FPS reduction by default.

Modification affects EglRenderer on Android. Moves frame dropping to the
renderer thread. Frame listeners are triggered even when FPS reduction is
active unless applyFpsReduction is set to true.

BUG=webrtc:7149
Review-Url: https://codereview.webrtc.org/2688843002
Cr-Commit-Position: refs/heads/master@{#17206}
Committed: https://chromium.googlesource.com/external/webrtc/+/d15165222f688d6ec779747361d95884b62a2dc8

[sakal, 2017-02-10 12:38:21]

Description was changed from

==========
EglRenderer: Trigger framelisteners even on dropped frames.

BUG=
==========

to

==========
Trigger framelisteners even on frames dropped by the FPS reduction.

Modification affects EglRenderer on Android. Moves frame dropping to the
renderer thread. Framelisteners are triggered even when FPS reduction is
active.

BUG=webrtc:6470
==========

[sakal, 2017-02-10 12:39:05]

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

[sakal, 2017-02-10 12:39:11]

PTAL

[magjed_webrtc, 2017-02-13 12:00:10]

https://codereview.webrtc.org/2688843002/diff/1/webrtc/sdk/android/api/org/we...
File webrtc/sdk/android/api/org/webrtc/EglRenderer.java (right):

https://codereview.webrtc.org/2688843002/diff/1/webrtc/sdk/android/api/org/we...
webrtc/sdk/android/api/org/webrtc/EglRenderer.java:585:
yuvUploader.uploadYuvData(
This will currently crash, because we haven't allocated the YUV textures yet.
Also, we shouldn't do this for the common case frameListeners.isEmpty(). I would
recommend updating YuvUploader so that it owns the textures and move the texture
allocation into that class. Then can put the code here unchanged under 'if
(shouldRenderFrame) {' and then add an else-clause where you add:
if (shouldRenderFrame) {'
  ...old code...
} else if (!frameListeners.isEmpty() && frame.yuvFrame) }
  // Need to upload the YUV data to textures for the frame listener.
  yuvUploader.uploadYuvData(...);
}

[sakal, 2017-02-13 13:05:57]

Description was changed from

==========
Trigger framelisteners even on frames dropped by the FPS reduction.

Modification affects EglRenderer on Android. Moves frame dropping to the
renderer thread. Framelisteners are triggered even when FPS reduction is
active.

BUG=webrtc:6470
==========

to

==========
Trigger framelisteners even on frames dropped by the FPS reduction.

Modification affects EglRenderer on Android. Moves frame dropping to the
renderer thread. Framelisteners are triggered even when FPS reduction is
active.

BUG=webrtc:7149
==========

[sakal, 2017-02-13 13:06:10]

https://codereview.webrtc.org/2688843002/diff/1/webrtc/sdk/android/api/org/we...
File webrtc/sdk/android/api/org/webrtc/EglRenderer.java (right):

https://codereview.webrtc.org/2688843002/diff/1/webrtc/sdk/android/api/org/we...
webrtc/sdk/android/api/org/webrtc/EglRenderer.java:585:
yuvUploader.uploadYuvData(
On 2017/02/13 12:00:10, magjed_webrtc wrote:
> This will currently crash, because we haven't allocated the YUV textures yet.
> Also, we shouldn't do this for the common case frameListeners.isEmpty(). I
would
> recommend updating YuvUploader so that it owns the textures and move the
texture
> allocation into that class. Then can put the code here unchanged under 'if
> (shouldRenderFrame) {' and then add an else-clause where you add:
> if (shouldRenderFrame) {'
>   ...old code...
> } else if (!frameListeners.isEmpty() && frame.yuvFrame) }
>   // Need to upload the YUV data to textures for the frame listener.
>   yuvUploader.uploadYuvData(...);
> }

Done. I haven't tested VideoRendererGui since it is not really used though.

[sakal, 2017-02-13 13:06:30]

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

[commit-bot: I haz the power, 2017-02-13 13:06:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[magjed_webrtc, 2017-02-13 15:19:47]

lgtm

https://codereview.webrtc.org/2688843002/diff/80001/webrtc/sdk/android/api/or...
File webrtc/sdk/android/api/org/webrtc/EglRenderer.java (right):

https://codereview.webrtc.org/2688843002/diff/80001/webrtc/sdk/android/api/or...
webrtc/sdk/android/api/org/webrtc/EglRenderer.java:579: final int[] yuvTextures
= frame.yuvFrame && (shouldRenderFrame || !frameListeners.isEmpty())
nit: Give a comment for this since it's quite complicated logic.

[commit-bot: I haz the power, 2017-02-13 16:07:18]

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

[commit-bot: I haz the power, 2017-02-13 16:07:19]

Dry run: This issue passed the CQ dry run.

[sakal, 2017-03-07 08:34:26]

I added a applyFpsReduction param to control whether to apply this new behavior.
Please take a look.

[sakal, 2017-03-07 09:59:49]

Description was changed from

==========
Trigger framelisteners even on frames dropped by the FPS reduction.

Modification affects EglRenderer on Android. Moves frame dropping to the
renderer thread. Framelisteners are triggered even when FPS reduction is
active.

BUG=webrtc:7149
==========

to

==========
Trigger framelisteners even on frames dropped by the FPS reduction by default.

Modification affects EglRenderer on Android. Moves frame dropping to the
renderer thread. Frame listeners are triggered even when FPS reduction is
active unless applyFpsReduction is set to true.

BUG=webrtc:7149
==========

[magjed_webrtc, 2017-03-07 12:48:14]

https://codereview.webrtc.org/2688843002/diff/120001/webrtc/sdk/android/api/o...
File webrtc/sdk/android/api/org/webrtc/EglRenderer.java (right):

https://codereview.webrtc.org/2688843002/diff/120001/webrtc/sdk/android/api/o...
webrtc/sdk/android/api/org/webrtc/EglRenderer.java:416: postToRenderThread(new
Runnable() {
Can you add a thread check here that we are _not_ on the render thead with a
comment that it would deadlock the render thread. Maybe we should add a public
comment that it's not allowed to call removeFrameListener inside FrameListener
callbacks :/ Kind of a strange requirement.

We could handle it properly if we add an if-statement on the calling thread and
remove the FrameListener immediately if we are on the render thread AND remove
any pending posted addFrameListener. We can remove pending addFrameListener by
posting with token=listener and then call removeCallbacksAndMessages also with
token=listener.

https://codereview.webrtc.org/2688843002/diff/120001/webrtc/sdk/android/api/o...
webrtc/sdk/android/api/org/webrtc/EglRenderer.java:546: } else if
(minRenderPeriodNs > 0) {
nit: I think it's easier to understand if it's in this order:
if (minRenderPeriodNs == Long.MAX_VALUE) {
  shouldRenderFrame = false;
} else if (minRenderPeriodNs <= 0) {
  shouldRenderFrame = true;
} else {
  ...general logic...
}

https://codereview.webrtc.org/2688843002/diff/120001/webrtc/sdk/android/api/o...
webrtc/sdk/android/api/org/webrtc/EglRenderer.java:595: final int[] yuvTextures
= frame.yuvFrame && (shouldRenderFrame || !frameListeners.isEmpty())
You should check if any of the frameListeners have !applyFpsReduction before
uploading. The whole purpose is optimize performance. I think it might be
cleaner to have them in two different lists.

[sakal, 2017-03-08 11:50:12]

I decided to remove the applyFpsReduction parameter for now.

https://codereview.webrtc.org/2688843002/diff/120001/webrtc/sdk/android/api/o...
File webrtc/sdk/android/api/org/webrtc/EglRenderer.java (right):

https://codereview.webrtc.org/2688843002/diff/120001/webrtc/sdk/android/api/o...
webrtc/sdk/android/api/org/webrtc/EglRenderer.java:416: postToRenderThread(new
Runnable() {
On 2017/03/07 12:48:13, magjed_webrtc wrote:
> Can you add a thread check here that we are _not_ on the render thead with a
> comment that it would deadlock the render thread. Maybe we should add a public
> comment that it's not allowed to call removeFrameListener inside FrameListener
> callbacks :/ Kind of a strange requirement.
> 
> We could handle it properly if we add an if-statement on the calling thread
and
> remove the FrameListener immediately if we are on the render thread AND remove
> any pending posted addFrameListener. We can remove pending addFrameListener by
> posting with token=listener and then call removeCallbacksAndMessages also with
> token=listener.

Done.

https://codereview.webrtc.org/2688843002/diff/120001/webrtc/sdk/android/api/o...
webrtc/sdk/android/api/org/webrtc/EglRenderer.java:546: } else if
(minRenderPeriodNs > 0) {
On 2017/03/07 12:48:13, magjed_webrtc wrote:
> nit: I think it's easier to understand if it's in this order:
> if (minRenderPeriodNs == Long.MAX_VALUE) {
>   shouldRenderFrame = false;
> } else if (minRenderPeriodNs <= 0) {
>   shouldRenderFrame = true;
> } else {
>   ...general logic...
> }

Done.

https://codereview.webrtc.org/2688843002/diff/120001/webrtc/sdk/android/api/o...
webrtc/sdk/android/api/org/webrtc/EglRenderer.java:595: final int[] yuvTextures
= frame.yuvFrame && (shouldRenderFrame || !frameListeners.isEmpty())
On 2017/03/07 12:48:13, magjed_webrtc wrote:
> You should check if any of the frameListeners have !applyFpsReduction before
> uploading. The whole purpose is optimize performance. I think it might be
> cleaner to have them in two different lists.

I implemented the strictest possible check.

[magjed_webrtc, 2017-03-13 10:13:14]

lgtm if comments are addressed.

https://codereview.webrtc.org/2688843002/diff/120001/webrtc/sdk/android/api/o...
File webrtc/sdk/android/api/org/webrtc/EglRenderer.java (right):

https://codereview.webrtc.org/2688843002/diff/120001/webrtc/sdk/android/api/o...
webrtc/sdk/android/api/org/webrtc/EglRenderer.java:626: VideoRenderer.I420Frame
frame, int[] yuvTextures, float[] texMatrix, boolean rendered) {
Remove the |rendered| argument since it's not used now.

https://codereview.webrtc.org/2688843002/diff/140001/webrtc/sdk/android/api/o...
File webrtc/sdk/android/api/org/webrtc/SurfaceViewRenderer.java (right):

https://codereview.webrtc.org/2688843002/diff/140001/webrtc/sdk/android/api/o...
webrtc/sdk/android/api/org/webrtc/SurfaceViewRenderer.java:125: * Register a
callback to be invoked when a new video frame has been received.
Move this function back to where it used to be in order to reduce the diff.

[sakal, 2017-03-13 10:53:36]

https://codereview.webrtc.org/2688843002/diff/120001/webrtc/sdk/android/api/o...
File webrtc/sdk/android/api/org/webrtc/EglRenderer.java (right):

https://codereview.webrtc.org/2688843002/diff/120001/webrtc/sdk/android/api/o...
webrtc/sdk/android/api/org/webrtc/EglRenderer.java:626: VideoRenderer.I420Frame
frame, int[] yuvTextures, float[] texMatrix, boolean rendered) {
On 2017/03/13 10:13:14, magjed_webrtc wrote:
> Remove the |rendered| argument since it's not used now.

Done.

https://codereview.webrtc.org/2688843002/diff/140001/webrtc/sdk/android/api/o...
File webrtc/sdk/android/api/org/webrtc/SurfaceViewRenderer.java (right):

https://codereview.webrtc.org/2688843002/diff/140001/webrtc/sdk/android/api/o...
webrtc/sdk/android/api/org/webrtc/SurfaceViewRenderer.java:125: * Register a
callback to be invoked when a new video frame has been received.
On 2017/03/13 10:13:14, magjed_webrtc wrote:
> Move this function back to where it used to be in order to reduce the diff.

Done.

[sakal, 2017-03-13 10:53:40]

The CQ bit was checked by sakal@webrtc.org

[sakal, 2017-03-13 10:53:40]

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/2688843002/#ps160001 (title:
"Changes according to magjed's comments.")

[commit-bot: I haz the power, 2017-03-13 10:53:48]

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-03-13 12:11:49]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1489402420168850,
"parent_rev": "7be1dcb98e17b6ac9228a6f44e1c5d8a570a966a", "commit_rev":
"d15165222f688d6ec779747361d95884b62a2dc8"}

[commit-bot: I haz the power, 2017-03-13 12:11:51]

Description was changed from

==========
Trigger framelisteners even on frames dropped by the FPS reduction by default.

Modification affects EglRenderer on Android. Moves frame dropping to the
renderer thread. Frame listeners are triggered even when FPS reduction is
active unless applyFpsReduction is set to true.

BUG=webrtc:7149
==========

to

==========
Trigger framelisteners even on frames dropped by the FPS reduction by default.

Modification affects EglRenderer on Android. Moves frame dropping to the
renderer thread. Frame listeners are triggered even when FPS reduction is
active unless applyFpsReduction is set to true.

BUG=webrtc:7149

Review-Url: https://codereview.webrtc.org/2688843002
Cr-Commit-Position: refs/heads/master@{#17206}
Committed:
https://chromium.googlesource.com/external/webrtc/+/d15165222f688d6ec77974736...
==========

[commit-bot: I haz the power, 2017-03-13 12:11:53]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/external/webrtc/+/d15165222f688d6ec77974736...