Avoid compositing when the surface texture is still empty.

Avoid compositing when the surface texture is still empty.

This avoids rendering a white layer in the surface view before the first frames
from blink are available and instead we continue rendering the theme
background in the Android activity until then.

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

[Ignacio Solla, 2013-11-14 10:18:30]

jam@chromium.org: Please review changes in

[jam, 2013-11-14 17:29:19]

On 2013/11/14 10:18:30, igsolla wrote:
> jam@chromium.org: Please review changes in

removing myself as a reviewer, as I'm not familiar at all with this code. please
pick someone from content/public/android/OWNERS

[bulach, 2013-11-14 18:10:22]

+sami for compositing..

if this render() function is called for every single frame, surely we don't want
to test if the surface is ready by doing two function calls at that stage?
would it be possible to the other way around, i.e., do not render anything until
some sort of callback tells that it's ready, and then just keep render()ing from
them on?

Sami and sievers can certainly give better info, I'm not too familiar with this
part..

[Sami, 2013-11-14 19:05:38]

On 2013/11/14 18:10:22, bulach wrote:
> +sami for compositing..
> 
> if this render() function is called for every single frame, surely we don't
want
> to test if the surface is ready by doing two function calls at that stage?
> would it be possible to the other way around, i.e., do not render anything
until
> some sort of callback tells that it's ready, and then just keep render()ing
from
> them on?

Unfortunately we're already calling ContentViewCore.isReady() on a per-frame
basis in didCompositeOnDraw() :(

How about we move this to ContentViewCore and only set mPendingRendererFrame[1]
if there's actually a pending frame? RWHVA could then notify ContentViewCore
whenever the HasValidFrame[2] status changes.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/and...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

[Sami, 2013-11-15 10:43:22]

On 2013/11/14 19:05:38, Sami wrote:
> How about we move this to ContentViewCore and only set
mPendingRendererFrame[1]
> if there's actually a pending frame? RWHVA could then notify ContentViewCore
> whenever the HasValidFrame[2] status changes.

Oops, scratch that; we could still composite the blank frame at the next vsync.
I think it's fine to have the check where you put it originally.

One thing to note here is that HasValidFrame doesn't just go from false => true,
but could go back to false again for instance if the renderer goes to the
background. In that sense, the polling justified.

With that, lgtm.

[Sami, 2013-11-15 10:43:31]

https://codereview.chromium.org/62333022/diff/30001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://codereview.chromium.org/62333022/diff/30001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:286:
// Waiting for the content view contents to be ready avoids compositing
Nit: this could be infinitesimally cleaner as an early-out instead of a scope
around everything.

[Ignacio Solla, 2013-11-15 11:13:51]

On 2013/11/14 19:05:38, Sami wrote:
> On 2013/11/14 18:10:22, bulach wrote:
> > +sami for compositing..
> > 
> > if this render() function is called for every single frame, surely we don't
> want
> > to test if the surface is ready by doing two function calls at that stage?
> > would it be possible to the other way around, i.e., do not render anything
> until
> > some sort of callback tells that it's ready, and then just keep render()ing
> from
> > them on?
> 
> Unfortunately we're already calling ContentViewCore.isReady() on a per-frame
> basis in didCompositeOnDraw() :(

To avoid the duplicate check I have now combined render() and
didCompositeAndDraw(). This works because:
- render is the only caller of didCompositeAndDraw
- and render would only call didCompositeAndDraw if the check passes, so no
point in checking again.

[Ignacio Solla, 2013-11-15 11:14:54]

https://codereview.chromium.org/62333022/diff/30001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://codereview.chromium.org/62333022/diff/30001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:286:
// Waiting for the content view contents to be ready avoids compositing
On 2013/11/15 10:43:32, Sami wrote:
> Nit: this could be infinitesimally cleaner as an early-out instead of a scope
> around everything.

Done.

[Sami, 2013-11-15 13:44:53]

lgtm

[bulach, 2013-11-15 15:54:01]

lgtm, if sami is happy so am I :)

one extra question for you both:

https://codereview.chromium.org/62333022/diff/150001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://codereview.chromium.org/62333022/diff/150001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:282:
post(new Runnable() {
while at it then.... is churning out new Runnable() everytime necessary? can we
keep this as member and just post it?

[Sami, 2013-11-15 16:06:28]

https://codereview.chromium.org/62333022/diff/150001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://codereview.chromium.org/62333022/diff/150001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:282:
post(new Runnable() {
On 2013/11/15 15:54:02, bulach wrote:
> while at it then.... is churning out new Runnable() everytime necessary? can
we
> keep this as member and just post it?

This runnable is just removing the static background resource some time after
startup, so we only do this once.

[bulach, 2013-11-15 16:07:39]

lgtm, thanks for the clarification!

https://codereview.chromium.org/62333022/diff/150001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://codereview.chromium.org/62333022/diff/150001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:282:
post(new Runnable() {
On 2013/11/15 16:06:28, Sami wrote:
> On 2013/11/15 15:54:02, bulach wrote:
> > while at it then.... is churning out new Runnable() everytime necessary? can
> we
> > keep this as member and just post it?
> 
> This runnable is just removing the static background resource some time after
> startup, so we only do this once.

got it, thanks!

[no sievers, 2013-11-15 19:04:37]

lgtm

[commit-bot: I haz the power, 2013-11-19 09:49:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/igsolla@chromium.org/62333022/150001

[commit-bot: I haz the power, 2013-11-19 12:39:17]

Change committed as 235984