Add support to delay sending SwapbufferAck as needed.

Add support to delay sending SwapbufferAck as needed.

On platforms (i.e. ChromeOS using Ozone) where Chromium is responsible for
displaying the buffers, we want to delay SwapBufferAck till we know that
the buffer is displayed on screen. Currently, we block the GPU main thread till
that data is available. This patch introduces SwapBufferAsync apis, which are
similar to current SwapBuffer calls except that it adds a callback function
which can be used to delay sending SwapBufferAck as needed.

BUG=443543, chrome-os-partner:34292
Committed: https://crrev.com/ae4355c2148da9d549134da9d81d68fd1892724e
Cr-Commit-Position: refs/heads/master@{#309602}

[kalyank, 2014-12-18 10:23:06]

kalyan.kondapally@intel.com changed reviewers:
+ piman@chromium.org

[kalyank, 2014-12-18 10:25:40]

kalyan.kondapally@intel.com changed reviewers:
+ alexst@chromium.org

[kalyank, 2014-12-18 11:08:12]

On 2014/12/18 10:26:07, kalyank wrote:

Relevant discussion here: https://codereview.chromium.org/781683005/

[alexst (slow to review), 2014-12-18 13:27:39]

lgtm, we are working on hooking this up for ozone surfaceless.

[piman, 2014-12-19 04:10:37]

https://codereview.chromium.org/797843005/diff/1/content/common/gpu/image_tra...
File content/common/gpu/image_transport_surface.cc (right):

https://codereview.chromium.org/797843005/diff/1/content/common/gpu/image_tra...
content/common/gpu/image_transport_surface.cc:218: base::Unretained(this)));
Why is Unretained safe? Can you add a comment?

https://codereview.chromium.org/797843005/diff/1/content/common/gpu/image_tra...
content/common/gpu/image_transport_surface.cc:226: base::Unretained(this)));
same thing here wrt Unretained.

[kalyank, 2014-12-19 08:43:03]

https://codereview.chromium.org/797843005/diff/1/content/common/gpu/image_tra...
File content/common/gpu/image_transport_surface.cc (right):

https://codereview.chromium.org/797843005/diff/1/content/common/gpu/image_tra...
content/common/gpu/image_transport_surface.cc:218: base::Unretained(this)));
On 2014/12/19 04:10:36, piman (Very slow to review) wrote:
> Why is Unretained safe? Can you add a comment?

Weakptr has a requirement that the callback be posted on same sequenced thread
and with Unretained it's Manual life time management. After a second thought,
Weakptr  looks a better choice with an added restriction here that the callback
needs to be run in Gpu main thread ?

[piman, 2014-12-19 20:32:54]

https://codereview.chromium.org/797843005/diff/1/content/common/gpu/image_tra...
File content/common/gpu/image_transport_surface.cc (right):

https://codereview.chromium.org/797843005/diff/1/content/common/gpu/image_tra...
content/common/gpu/image_transport_surface.cc:218: base::Unretained(this)));
On 2014/12/19 08:43:03, kalyank wrote:
> On 2014/12/19 04:10:36, piman (Very slow to review) wrote:
> > Why is Unretained safe? Can you add a comment?
> 
> Weakptr has a requirement that the callback be posted on same sequenced thread
> and with Unretained it's Manual life time management. After a second thought,
> Weakptr  looks a better choice with an added restriction here that the
callback
> needs to be run in Gpu main thread ?

Right, that's a strong requirement. It wouldn't be ok to call
PassThroughImageTransportSurface::SwapBuffersCallBack on an arbitrary thread.

Weakptr is fine. Unretained is possible if we can ensure that the lifetime of
|this| outlives the callback - i.e. all implementations of GLSurface need to be
able to guarantee that.

[kalyank, 2014-12-22 09:35:17]

On 2014/12/19 20:32:54, piman (Very slow to review) wrote:
>
https://codereview.chromium.org/797843005/diff/1/content/common/gpu/image_tra...
> File content/common/gpu/image_transport_surface.cc (right):
> 
>
https://codereview.chromium.org/797843005/diff/1/content/common/gpu/image_tra...

> Weakptr is fine. Unretained is possible if we can ensure that the lifetime of
> |this| outlives the callback - i.e. all implementations of GLSurface need to
be
> able to guarantee that

K, I had to move latencyinfo to helper, so we could have swapbuffer callback
directly in helper
class and use weak ptr.

[piman, 2014-12-22 20:53:16]

https://codereview.chromium.org/797843005/diff/60001/content/common/gpu/image...
File content/common/gpu/image_transport_surface.cc (left):

https://codereview.chromium.org/797843005/diff/60001/content/common/gpu/image...
content/common/gpu/image_transport_surface.cc:182:
surface_->SetLatencyInfo(latency_info);
Why this change? I don't understand...
It will break cases where |surface_| is not a PassThroughImageTransportSurface
(e.g. mac's ImageTransportSurfaceFBO).

Why not keeping the code where it is, but taking a WeakPtr on the
PassThroughImageTransportSurface?

https://codereview.chromium.org/797843005/diff/60001/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://codereview.chromium.org/797843005/diff/60001/ui/gl/gl_surface.h#newco...
ui/gl/gl_surface.h:81: // GPUMainThread only.
nit: in ui/gl there's no notion of GpuMainThread.
I would phrase it as: the callback should be run on the calling thread (i.e.
same thread SwapBuffersAsync is called).

[kalyank, 2014-12-23 09:12:32]

https://codereview.chromium.org/797843005/diff/60001/content/common/gpu/image...
File content/common/gpu/image_transport_surface.cc (left):

https://codereview.chromium.org/797843005/diff/60001/content/common/gpu/image...
content/common/gpu/image_transport_surface.cc:182:
surface_->SetLatencyInfo(latency_info);
On 2014/12/22 20:53:15, piman (Very slow to review) wrote:
> Why this change? I don't understand...
> It will break cases where |surface_| is not a PassThroughImageTransportSurface
> (e.g. mac's ImageTransportSurfaceFBO).
> 
> Why not keeping the code where it is, but taking a WeakPtr on the
> PassThroughImageTransportSurface?

ImageTransportSurfaceAndroid inherits base::SupportsWeakPtr and
NullTransportSurface (NullTransportSurface inherits
PassThroughImageTransportSurface) . This was causing an issue, I tried changing
so that ImageTransportSurfaceAndroid doesn't inherit base::SupportsWeakPtr, but
it obviously complains in line 107 image_transport_surface_android.cc "
../../base/bind_internal.h:882:18: error: cannot initialize a parameter of type
'content::ImageTransportSurfaceAndroid *' with an rvalue of type
'content::PassThroughImageTransportSurface *'
    runnable.Run(weak_ptr.get());
"

Any suggestions ?

[kalyank, 2014-12-23 11:22:42]

https://codereview.chromium.org/797843005/diff/60001/content/common/gpu/image...
File content/common/gpu/image_transport_surface.cc (left):

https://codereview.chromium.org/797843005/diff/60001/content/common/gpu/image...
content/common/gpu/image_transport_surface.cc:182:
surface_->SetLatencyInfo(latency_info);
Added ImageTransportHelperAndroid, which will be used by
ImageTransportSurfaceAndroid to handle callbacks.

https://codereview.chromium.org/797843005/diff/60001/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://codereview.chromium.org/797843005/diff/60001/ui/gl/gl_surface.h#newco...
ui/gl/gl_surface.h:81: // GPUMainThread only.
On 2014/12/22 20:53:15, piman (Very slow to review) wrote:
> nit: in ui/gl there's no notion of GpuMainThread.
> I would phrase it as: the callback should be run on the calling thread (i.e.
> same thread SwapBuffersAsync is called).

Done.

[piman, 2014-12-23 18:54:23]

On Tue, Dec 23, 2014 at 1:12 AM, <kalyan.kondapally@intel.com> wrote:

>
> https://codereview.chromium.org/797843005/diff/60001/
> content/common/gpu/image_transport_surface.cc
> File content/common/gpu/image_transport_surface.cc (left):
>
> https://codereview.chromium.org/797843005/diff/60001/
> content/common/gpu/image_transport_surface.cc#oldcode182
> content/common/gpu/image_transport_surface.cc:182:
> surface_->SetLatencyInfo(latency_info);
> On 2014/12/22 20:53:15, piman (Very slow to review) wrote:
>
>> Why this change? I don't understand...
>> It will break cases where |surface_| is not a
>>
> PassThroughImageTransportSurface
>
>> (e.g. mac's ImageTransportSurfaceFBO).
>>
>
>  Why not keeping the code where it is, but taking a WeakPtr on the
>> PassThroughImageTransportSurface?
>>
>
> ImageTransportSurfaceAndroid inherits base::SupportsWeakPtr and
> NullTransportSurface (NullTransportSurface inherits
> PassThroughImageTransportSurface) . This was causing an issue, I tried
> changing so that ImageTransportSurfaceAndroid doesn't inherit
> base::SupportsWeakPtr, but it obviously complains in line 107
> image_transport_surface_android.cc " ../../base/bind_internal.h:882:18:
> error: cannot initialize a parameter of type
> 'content::ImageTransportSurfaceAndroid *' with an rvalue of type
> 'content::PassThroughImageTransportSurface *'
>     runnable.Run(weak_ptr.get());
> "
>
> Any suggestions ?
>

Use a WeakPtrFactory (as the last member) rather than SupportsWeakPtr. It's
safer too, generally.

>
> https://codereview.chromium.org/797843005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kalyank, 2014-12-24 00:01:29]

> > Any suggestions ?
> >
> 
> Use a WeakPtrFactory (as the last member) rather than SupportsWeakPtr. It's
> safer too, generally.

Thanks done.

[piman, 2014-12-24 00:13:42]

lgtm

[piman, 2014-12-24 00:13:45]

The CQ bit was checked by piman@chromium.org

[commit-bot: I haz the power, 2014-12-24 00:14:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/797843005/160001

[commit-bot: I haz the power, 2014-12-24 00:23:29]

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

[commit-bot: I haz the power, 2014-12-24 00:23:30]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[kalyank, 2014-12-24 02:01:04]

The CQ bit was checked by kalyan.kondapally@intel.com

[commit-bot: I haz the power, 2014-12-24 02:01:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/797843005/160001

[commit-bot: I haz the power, 2014-12-24 02:02:36]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2014-12-24 02:03:24]

Patchset 9 (id:??) landed as
https://crrev.com/ae4355c2148da9d549134da9d81d68fd1892724e
Cr-Commit-Position: refs/heads/master@{#309602}