Add support for EXT_image_flush_external extension

Add support for EXT_image_flush_external extension

BUG=chromium:478339
TEST=Boot to UI

Signed-off-by: Vince Hsu <vince.h@nvidia.com>;
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/8baf5bd7df5891470b8dfe4c6de7998bd60170b1
Cr-Commit-Position: refs/heads/master@{#405659}

[vinceh, 2016-06-29 05:58:11]

Description was changed from

==========
Add support for EXT_image_flush_external extension

BUG=chromium:478339
TEST=Boot to UI

Signed-off-by: Vince Hsu <vince.h@nvidia.com>
==========

to

==========
Add support for EXT_image_flush_external extension

BUG=chromium:478339
TEST=Boot to UI

Signed-off-by: Vince Hsu <vince.h@nvidia.com>
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[vinceh, 2016-06-29 06:02:08]

vince.h@nvidia.com changed reviewers:
+ dnicoara@chromium.org, marcheu@chromium.org, piman@chromium.org,
reveman@chromium.org

[vinceh, 2016-06-29 06:02:08]

Added reviewers.

[reveman, 2016-06-29 07:37:51]

https://codereview.chromium.org/2109803003/diff/20001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/2109803003/diff/20001/ui/gl/gl_image.h#newcode96
ui/gl/gl_image.h:96: virtual void Flush() = 0;
Do we need to add GLImage API function for this? It looks like it's only used
for overlay scheduling. Can we handle this as part of the
GLImage::ScheduleOverlayPlane implementation instead?

[vinceh, 2016-06-29 07:44:37]

https://codereview.chromium.org/2109803003/diff/20001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/2109803003/diff/20001/ui/gl/gl_image.h#newcode96
ui/gl/gl_image.h:96: virtual void Flush() = 0;
On 2016/06/29 07:37:51, reveman wrote:
> Do we need to add GLImage API function for this? It looks like it's only used
> for overlay scheduling. Can we handle this as part of the
> GLImage::ScheduleOverlayPlane implementation instead?

That's actually implemented according to Daniel's suggestion if I understood his
words correctly. Daniel, any comments?

[vinceh, 2016-06-29 10:07:58]

vince.h@nvidia.com changed reviewers:
+ jam@chromium.org

[vinceh, 2016-06-29 10:07:59]

Added jam as reviewer as well.

[dnicoara, 2016-06-29 14:21:56]

https://codereview.chromium.org/2109803003/diff/20001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/2109803003/diff/20001/ui/gl/gl_image.h#newcode96
ui/gl/gl_image.h:96: virtual void Flush() = 0;
On 2016/06/29 07:44:36, vinceh wrote:
> On 2016/06/29 07:37:51, reveman wrote:
> > Do we need to add GLImage API function for this? It looks like it's only
used
> > for overlay scheduling. Can we handle this as part of the
> > GLImage::ScheduleOverlayPlane implementation instead?
> 
> That's actually implemented according to Daniel's suggestion if I understood
his
> words correctly. Daniel, any comments?

My thought was to make it a generic API since it is implemented as an EGLImage
extension (and not couple overlay scheduling with image flushing). But perhaps
this is overly generalizing (especially since it is changing quite a bit of
files)?

https://codereview.chromium.org/2109803003/diff/60001/ui/gl/gl_image_ozone_na...
File ui/gl/gl_image_ozone_native_pixmap.cc (right):

https://codereview.chromium.org/2109803003/diff/60001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.cc:250: EGLDisplay display =
eglGetDisplay(EGL_DEFAULT_DISPLAY);
I believe you should be using "GLSurfaceEGL::GetHardwareDisplay()" here.

[piman, 2016-06-29 17:34:50]

https://codereview.chromium.org/2109803003/diff/60001/third_party/khronos/EGL...
File third_party/khronos/EGL/eglext.h (right):

https://codereview.chromium.org/2109803003/diff/60001/third_party/khronos/EGL...
third_party/khronos/EGL/eglext.h:581: #define EGL_DECOMPRESSED_NVX		0x3331
How come an EXT extension has _NVX enums in them?
This doesn't look like an official EXT extension, it's not in the Khronos
registry...

Is there documentation for that extension somewhere?

https://codereview.chromium.org/2109803003/diff/60001/third_party/khronos/REA...
File third_party/khronos/README.chromium (right):

https://codereview.chromium.org/2109803003/diff/60001/third_party/khronos/REA...
third_party/khronos/README.chromium:39: - Added supprot for
EGL_EXT_image_flush_external extension
nit: typo supprot->support

https://codereview.chromium.org/2109803003/diff/60001/ui/gl/gl_image_ozone_na...
File ui/gl/gl_image_ozone_native_pixmap.cc (right):

https://codereview.chromium.org/2109803003/diff/60001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.cc:251: if
(!eglImageFlushExternalEXT(display, egl_image_, attribs)) {
What if the extension is not supported?

https://codereview.chromium.org/2109803003/diff/60001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.cc:256: GLFence* fence = GLFence::Create();
nit: use std::unique_ptr<GLFence>

https://codereview.chromium.org/2109803003/diff/60001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.cc:257: fence->ClientWait();
So, you're doing this on all drivers, even though all current ones don't need
this. This is a big hammer (which is not reflected in the "Flush" name nor the
documentation for the API), and will likely cause significant regressions on
those other drivers.

Even on NVIDIA, this looks like it would break pipelining and cause poor
performance.

Can we have a high level discussion about what's needed - maybe in a bug - and
how we might go around to solve it in a more efficient / more general way?

[piman, 2016-06-29 17:34:50]

https://codereview.chromium.org/2109803003/diff/60001/third_party/khronos/EGL...
File third_party/khronos/EGL/eglext.h (right):

https://codereview.chromium.org/2109803003/diff/60001/third_party/khronos/EGL...
third_party/khronos/EGL/eglext.h:581: #define EGL_DECOMPRESSED_NVX		0x3331
How come an EXT extension has _NVX enums in them?
This doesn't look like an official EXT extension, it's not in the Khronos
registry...

Is there documentation for that extension somewhere?

https://codereview.chromium.org/2109803003/diff/60001/third_party/khronos/REA...
File third_party/khronos/README.chromium (right):

https://codereview.chromium.org/2109803003/diff/60001/third_party/khronos/REA...
third_party/khronos/README.chromium:39: - Added supprot for
EGL_EXT_image_flush_external extension
nit: typo supprot->support

https://codereview.chromium.org/2109803003/diff/60001/ui/gl/gl_image_ozone_na...
File ui/gl/gl_image_ozone_native_pixmap.cc (right):

https://codereview.chromium.org/2109803003/diff/60001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.cc:251: if
(!eglImageFlushExternalEXT(display, egl_image_, attribs)) {
What if the extension is not supported?

https://codereview.chromium.org/2109803003/diff/60001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.cc:256: GLFence* fence = GLFence::Create();
nit: use std::unique_ptr<GLFence>

https://codereview.chromium.org/2109803003/diff/60001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.cc:257: fence->ClientWait();
So, you're doing this on all drivers, even though all current ones don't need
this. This is a big hammer (which is not reflected in the "Flush" name nor the
documentation for the API), and will likely cause significant regressions on
those other drivers.

Even on NVIDIA, this looks like it would break pipelining and cause poor
performance.

Can we have a high level discussion about what's needed - maybe in a bug - and
how we might go around to solve it in a more efficient / more general way?

[jam, 2016-06-29 18:15:30]

On 2016/06/29 10:07:59, vinceh wrote:
> Added jam as reviewer as well.

for what? you should pick owners from ui/ and gpu/ who will be better than me.

[vinceh, 2016-06-30 09:22:48]

https://codereview.chromium.org/2109803003/diff/20001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/2109803003/diff/20001/ui/gl/gl_image.h#newcode96
ui/gl/gl_image.h:96: virtual void Flush() = 0;
On 2016/06/29 14:21:56, dnicoara wrote:
> On 2016/06/29 07:44:36, vinceh wrote:
> > On 2016/06/29 07:37:51, reveman wrote:
> > > Do we need to add GLImage API function for this? It looks like it's only
> used
> > > for overlay scheduling. Can we handle this as part of the
> > > GLImage::ScheduleOverlayPlane implementation instead?
> > 
> > That's actually implemented according to Daniel's suggestion if I understood
> his
> > words correctly. Daniel, any comments?
> 
> My thought was to make it a generic API since it is implemented as an EGLImage
> extension (and not couple overlay scheduling with image flushing). But perhaps
> this is overly generalizing (especially since it is changing quite a bit of
> files)?

Then let's implement this inside GLImageOzoneNativePixmap::ScheduleOverlayPlane,
OK?

https://codereview.chromium.org/2109803003/diff/60001/third_party/khronos/EGL...
File third_party/khronos/EGL/eglext.h (right):

https://codereview.chromium.org/2109803003/diff/60001/third_party/khronos/EGL...
third_party/khronos/EGL/eglext.h:581: #define EGL_DECOMPRESSED_NVX		0x3331
On 2016/06/29 17:34:50, piman wrote:
> How come an EXT extension has _NVX enums in them?
> This doesn't look like an official EXT extension, it's not in the Khronos
> registry...
> 
> Is there documentation for that extension somewhere?

It's in
https://docs.google.com/document/d/1Z3P00qyjI7R5LYA2T9qYzmXymm1NOTbcoFbEod_GD...
.

The enums is actually from NVIDIA's EGL_EXT_image_flush_external extension since
I did not have ideas for the names. Could you suggest in the Google doc above?

https://codereview.chromium.org/2109803003/diff/60001/ui/gl/gl_image_ozone_na...
File ui/gl/gl_image_ozone_native_pixmap.cc (right):

https://codereview.chromium.org/2109803003/diff/60001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.cc:250: EGLDisplay display =
eglGetDisplay(EGL_DEFAULT_DISPLAY);
On 2016/06/29 14:21:56, dnicoara wrote:
> I believe you should be using "GLSurfaceEGL::GetHardwareDisplay()" here.

OK, will fix.

https://codereview.chromium.org/2109803003/diff/60001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.cc:251: if
(!eglImageFlushExternalEXT(display, egl_image_, attribs)) {
On 2016/06/29 17:34:50, piman wrote:
> What if the extension is not supported?

I should add check for the extension. Thanks.

https://codereview.chromium.org/2109803003/diff/60001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.cc:257: fence->ClientWait();
On 2016/06/29 17:34:50, piman wrote:
> So, you're doing this on all drivers, even though all current ones don't need
> this. This is a big hammer (which is not reflected in the "Flush" name nor the
> documentation for the API), and will likely cause significant regressions on
> those other drivers.
I should do this only when the extension is supported.

> 
> Even on NVIDIA, this looks like it would break pipelining and cause poor
> performance.
> 
> Can we have a high level discussion about what's needed - maybe in a bug - and
> how we might go around to solve it in a more efficient / more general way?

Could you go to
https://docs.google.com/document/d/1Z3P00qyjI7R5LYA2T9qYzmXymm1NOTbcoFbEod_GD...
and comment there? If that's not convenient then I will file a bug?

[vinceh, 2016-06-30 09:39:31]

On 2016/06/29 18:15:30, jam wrote:
> On 2016/06/29 10:07:59, vinceh wrote:
> > Added jam as reviewer as well.
> 
> for what? you should pick owners from ui/ and gpu/ who will be better than me.

I got your name from "git cl owners", but probably I made some mistake. Sorry
for the confusion.

[piman, 2016-06-30 18:18:59]

https://codereview.chromium.org/2109803003/diff/60001/third_party/khronos/EGL...
File third_party/khronos/EGL/eglext.h (right):

https://codereview.chromium.org/2109803003/diff/60001/third_party/khronos/EGL...
third_party/khronos/EGL/eglext.h:581: #define EGL_DECOMPRESSED_NVX		0x3331
On 2016/06/30 09:22:48, vinceh wrote:
> On 2016/06/29 17:34:50, piman wrote:
> > How come an EXT extension has _NVX enums in them?
> > This doesn't look like an official EXT extension, it's not in the Khronos
> > registry...
> > 
> > Is there documentation for that extension somewhere?
> 
> It's in
>
https://docs.google.com/document/d/1Z3P00qyjI7R5LYA2T9qYzmXymm1NOTbcoFbEod_GD...
> .
> 
> The enums is actually from NVIDIA's EGL_EXT_image_flush_external extension
since
> I did not have ideas for the names. Could you suggest in the Google doc above?

EGL_EXT_image_flush_external, as described in the document, and being an EXT
extension, doesn't/shouldn't include those enums.
Perhaps you mean you want a layered extension on top of it that adds
NVIDIA-specific attributes, but it should be specced separately, and queried
separately.
From my discussions with marcheu@, eglImageFlushExternalEXT is intended to work
without driver-specific attributes.

https://codereview.chromium.org/2109803003/diff/60001/ui/gl/gl_image_ozone_na...
File ui/gl/gl_image_ozone_native_pixmap.cc (right):

https://codereview.chromium.org/2109803003/diff/60001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.cc:257: fence->ClientWait();
On 2016/06/30 09:22:48, vinceh wrote:
> On 2016/06/29 17:34:50, piman wrote:
> > So, you're doing this on all drivers, even though all current ones don't
need
> > this. This is a big hammer (which is not reflected in the "Flush" name nor
the
> > documentation for the API), and will likely cause significant regressions on
> > those other drivers.
> I should do this only when the extension is supported.
> 
> > 
> > Even on NVIDIA, this looks like it would break pipelining and cause poor
> > performance.
> > 
> > Can we have a high level discussion about what's needed - maybe in a bug -
and
> > how we might go around to solve it in a more efficient / more general way?
> 
> Could you go to
>
https://docs.google.com/document/d/1Z3P00qyjI7R5LYA2T9qYzmXymm1NOTbcoFbEod_GD...
> and comment there? If that's not convenient then I will file a bug?

Commented there. I think this should integrate with the code in
GLSurfaceOzoneSurfaceless::SwapBuffersAsync to wait for the fence in a worker
thread

[marcheu1, 2016-07-01 06:45:58]

marcheu@google.com changed reviewers:
+ marcheu@google.com

[marcheu1, 2016-07-01 06:45:58]

https://codereview.chromium.org/2109803003/diff/20001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/2109803003/diff/20001/ui/gl/gl_image.h#newcode96
ui/gl/gl_image.h:96: virtual void Flush() = 0;
On 2016/06/30 09:22:48, vinceh wrote:
> On 2016/06/29 14:21:56, dnicoara wrote:
> > On 2016/06/29 07:44:36, vinceh wrote:
> > > On 2016/06/29 07:37:51, reveman wrote:
> > > > Do we need to add GLImage API function for this? It looks like it's only
> > used
> > > > for overlay scheduling. Can we handle this as part of the
> > > > GLImage::ScheduleOverlayPlane implementation instead?
> > > 
> > > That's actually implemented according to Daniel's suggestion if I
understood
> > his
> > > words correctly. Daniel, any comments?
> > 
> > My thought was to make it a generic API since it is implemented as an
EGLImage
> > extension (and not couple overlay scheduling with image flushing). But
perhaps
> > this is overly generalizing (especially since it is changing quite a bit of
> > files)?
> 
> Then let's implement this inside
GLImageOzoneNativePixmap::ScheduleOverlayPlane,
> OK?

If we do this as part of ScheduleOverlayPlane, wouldn't we have to return a
fence that the async swap can use? If so that seems as complicated to me as
implementing a new entry point for images...

[vinceh, 2016-07-07 08:00:37]

On 2016/07/01 06:45:58, marcheu1 wrote:
> https://codereview.chromium.org/2109803003/diff/20001/ui/gl/gl_image.h
> File ui/gl/gl_image.h (right):
> 
>
https://codereview.chromium.org/2109803003/diff/20001/ui/gl/gl_image.h#newcode96
> ui/gl/gl_image.h:96: virtual void Flush() = 0;
> On 2016/06/30 09:22:48, vinceh wrote:
> > On 2016/06/29 14:21:56, dnicoara wrote:
> > > On 2016/06/29 07:44:36, vinceh wrote:
> > > > On 2016/06/29 07:37:51, reveman wrote:
> > > > > Do we need to add GLImage API function for this? It looks like it's
only
> > > used
> > > > > for overlay scheduling. Can we handle this as part of the
> > > > > GLImage::ScheduleOverlayPlane implementation instead?
> > > > 
> > > > That's actually implemented according to Daniel's suggestion if I
> understood
> > > his
> > > > words correctly. Daniel, any comments?
> > > 
> > > My thought was to make it a generic API since it is implemented as an
> EGLImage
> > > extension (and not couple overlay scheduling with image flushing). But
> perhaps
> > > this is overly generalizing (especially since it is changing quite a bit
of
> > > files)?
> > 
> > Then let's implement this inside
> GLImageOzoneNativePixmap::ScheduleOverlayPlane,
> > OK?
> 
> If we do this as part of ScheduleOverlayPlane, wouldn't we have to return a
> fence that the async swap can use? If so that seems as complicated to me as
> implementing a new entry point for images...

Hi Daniel/piman/reveman,

What do you think about Stephane's comments?

[piman, 2016-07-07 21:42:51]

On 2016/07/07 08:00:37, vinceh wrote:
> On 2016/07/01 06:45:58, marcheu1 wrote:
> > https://codereview.chromium.org/2109803003/diff/20001/ui/gl/gl_image.h
> > File ui/gl/gl_image.h (right):
> > 
> >
>
https://codereview.chromium.org/2109803003/diff/20001/ui/gl/gl_image.h#newcode96
> > ui/gl/gl_image.h:96: virtual void Flush() = 0;
> > On 2016/06/30 09:22:48, vinceh wrote:
> > > On 2016/06/29 14:21:56, dnicoara wrote:
> > > > On 2016/06/29 07:44:36, vinceh wrote:
> > > > > On 2016/06/29 07:37:51, reveman wrote:
> > > > > > Do we need to add GLImage API function for this? It looks like it's
> only
> > > > used
> > > > > > for overlay scheduling. Can we handle this as part of the
> > > > > > GLImage::ScheduleOverlayPlane implementation instead?
> > > > > 
> > > > > That's actually implemented according to Daniel's suggestion if I
> > understood
> > > > his
> > > > > words correctly. Daniel, any comments?
> > > > 
> > > > My thought was to make it a generic API since it is implemented as an
> > EGLImage
> > > > extension (and not couple overlay scheduling with image flushing). But
> > perhaps
> > > > this is overly generalizing (especially since it is changing quite a bit
> of
> > > > files)?
> > > 
> > > Then let's implement this inside
> > GLImageOzoneNativePixmap::ScheduleOverlayPlane,
> > > OK?
> > 
> > If we do this as part of ScheduleOverlayPlane, wouldn't we have to return a
> > fence that the async swap can use? If so that seems as complicated to me as
> > implementing a new entry point for images...
> 
> Hi Daniel/piman/reveman,
> 
> What do you think about Stephane's comments?

I don't think we can do it as part of ScheduleOverlayPlane if we don't want to
block the main thread - it's called too late, i.e. after we're supposed to have
waited on the fence.
So either we have to add another function on GLImage, or we need to invent a
ScheduleOverlayPlaneAsync that returns a fence and callback to call when the
fence is passed - which is fairly equivalent.

[reveman, 2016-07-08 09:18:26]

On 2016/07/07 at 21:42:51, piman wrote:
> On 2016/07/07 08:00:37, vinceh wrote:
> > On 2016/07/01 06:45:58, marcheu1 wrote:
> > > https://codereview.chromium.org/2109803003/diff/20001/ui/gl/gl_image.h
> > > File ui/gl/gl_image.h (right):
> > > 
> > >
> >
https://codereview.chromium.org/2109803003/diff/20001/ui/gl/gl_image.h#newcode96
> > > ui/gl/gl_image.h:96: virtual void Flush() = 0;
> > > On 2016/06/30 09:22:48, vinceh wrote:
> > > > On 2016/06/29 14:21:56, dnicoara wrote:
> > > > > On 2016/06/29 07:44:36, vinceh wrote:
> > > > > > On 2016/06/29 07:37:51, reveman wrote:
> > > > > > > Do we need to add GLImage API function for this? It looks like
it's
> > only
> > > > > used
> > > > > > > for overlay scheduling. Can we handle this as part of the
> > > > > > > GLImage::ScheduleOverlayPlane implementation instead?
> > > > > > 
> > > > > > That's actually implemented according to Daniel's suggestion if I
> > > understood
> > > > > his
> > > > > > words correctly. Daniel, any comments?
> > > > > 
> > > > > My thought was to make it a generic API since it is implemented as an
> > > EGLImage
> > > > > extension (and not couple overlay scheduling with image flushing). But
> > > perhaps
> > > > > this is overly generalizing (especially since it is changing quite a
bit
> > of
> > > > > files)?
> > > > 
> > > > Then let's implement this inside
> > > GLImageOzoneNativePixmap::ScheduleOverlayPlane,
> > > > OK?
> > > 
> > > If we do this as part of ScheduleOverlayPlane, wouldn't we have to return
a
> > > fence that the async swap can use? If so that seems as complicated to me
as
> > > implementing a new entry point for images...
> > 
> > Hi Daniel/piman/reveman,
> > 
> > What do you think about Stephane's comments?
> 
> I don't think we can do it as part of ScheduleOverlayPlane if we don't want to
block the main thread - it's called too late, i.e. after we're supposed to have
waited on the fence.
> So either we have to add another function on GLImage, or we need to invent a
ScheduleOverlayPlaneAsync that returns a fence and callback to call when the
fence is passed - which is fairly equivalent.

Adding a Flush function to GLImage sgtm if that's needed to get synchronization
right.

[vinceh, 2016-07-11 09:39:57]

Hello reviewers,

Could you take a look at v6? If we return fence for each GLImgae, we have to
track all of the fences in SwapBuffer. So I re-use the fence which was used for
implicit external sync previously.

Thanks,
Vince

[piman, 2016-07-11 16:06:26]

I'm happy with the approach. One thing, and otherwise lgtm.

https://codereview.chromium.org/2109803003/diff/100001/ui/gl/gl_image_ozone_n...
File ui/gl/gl_image_ozone_native_pixmap.cc (right):

https://codereview.chromium.org/2109803003/diff/100001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.cc:253: if
(GLSurfaceEGL::HasEGLExtension("EGL_EXT_image_flush_external")) {
nit: parsing strings on every frame seems a bit unfortunate. Can we check this
at creation time and keep in a field?

[reveman, 2016-07-11 18:59:06]

lgtm % nits

https://codereview.chromium.org/2109803003/diff/20001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/2109803003/diff/20001/ui/gl/gl_image.h#newcode96
ui/gl/gl_image.h:96: virtual void Flush() = 0;
On 2016/06/29 at 14:21:56, dnicoara wrote:
> On 2016/06/29 07:44:36, vinceh wrote:
> > On 2016/06/29 07:37:51, reveman wrote:
> > > Do we need to add GLImage API function for this? It looks like it's only
used
> > > for overlay scheduling. Can we handle this as part of the
> > > GLImage::ScheduleOverlayPlane implementation instead?
> > 
> > That's actually implemented according to Daniel's suggestion if I understood
his
> > words correctly. Daniel, any comments?
> 
> My thought was to make it a generic API since it is implemented as an EGLImage
extension (and not couple overlay scheduling with image flushing). But perhaps
this is overly generalizing (especially since it is changing quite a bit of
files)?

Is this flush needed in the sw raster case? In the arc++ case, it's not, as
we're using explicit synchronization and I think this might result in a
significant performance regression.

https://codereview.chromium.org/2109803003/diff/100001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/2109803003/diff/100001/ui/gl/gl_image.h#newco...
ui/gl/gl_image.h:96: virtual void Flush() = 0;
nit: EmulatingRGB and GetType are temporary workarounds but I think this Flush
function is part of the core API. Please move it below ScheduleOverlayPlane and
maintain the sorting order where implemented.

https://codereview.chromium.org/2109803003/diff/100001/ui/gl/gl_image_glx.h
File ui/gl/gl_image_glx.h (right):

https://codereview.chromium.org/2109803003/diff/100001/ui/gl/gl_image_glx.h#n...
ui/gl/gl_image_glx.h:42: void Flush() override {}
nit: move below ScheduleOverlayPlane

https://codereview.chromium.org/2109803003/diff/100001/ui/gl/gl_image_io_surf...
File ui/gl/gl_image_io_surface.h (right):

https://codereview.chromium.org/2109803003/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.h:77: void Flush() override {}
nit: move below ScheduleOverlayPlane

https://codereview.chromium.org/2109803003/diff/100001/ui/gl/gl_image_memory.h
File ui/gl/gl_image_memory.h (right):

https://codereview.chromium.org/2109803003/diff/100001/ui/gl/gl_image_memory....
ui/gl/gl_image_memory.h:44: void Flush() override {}
nit: move below ScheduleOverlayPlane

https://codereview.chromium.org/2109803003/diff/100001/ui/gl/gl_image_ozone_n...
File ui/gl/gl_image_ozone_native_pixmap.cc (right):

https://codereview.chromium.org/2109803003/diff/100001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.cc:208: 
nit: no need to add this blank line

https://codereview.chromium.org/2109803003/diff/100001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.cc:252: void
GLImageOzoneNativePixmap::Flush() {
nit: move below ScheduleOverlayPlane

https://codereview.chromium.org/2109803003/diff/100001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.cc:264: return;
nit: remove unnecessary return statement

https://codereview.chromium.org/2109803003/diff/100001/ui/gl/gl_image_ozone_n...
File ui/gl/gl_image_ozone_native_pixmap.h (right):

https://codereview.chromium.org/2109803003/diff/100001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.h:38: void Flush() override;
nit: move below ScheduleOverlayPlane

https://codereview.chromium.org/2109803003/diff/100001/ui/gl/gl_image_stub.h
File ui/gl/gl_image_stub.h (right):

https://codereview.chromium.org/2109803003/diff/100001/ui/gl/gl_image_stub.h#...
ui/gl/gl_image_stub.h:38: void Flush() override {}
nit: move below ScheduleOverlayPlane

https://codereview.chromium.org/2109803003/diff/100001/ui/gl/gl_image_surface...
File ui/gl/gl_image_surface_texture.h (right):

https://codereview.chromium.org/2109803003/diff/100001/ui/gl/gl_image_surface...
ui/gl/gl_image_surface_texture.h:45: void Flush() override {}
nit: move below ScheduleOverlayPlane

[vinceh, 2016-07-12 07:30:11]

The CQ bit was checked by vince.h@nvidia.com

[vinceh, 2016-07-12 07:30:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2109803003/#ps160001
(title: "Add support for EXT_image_flush_external extension")

[commit-bot: I haz the power, 2016-07-12 07:31:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 07:43:34]

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

[commit-bot: I haz the power, 2016-07-12 07:43:35]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[vinceh, 2016-07-12 07:48:33]

Hi jam,

Could you please review the change for media/gpu/avda_codec_image.h? Thanks!

[jam, 2016-07-12 21:24:49]

On 2016/07/12 07:48:33, vinceh wrote:
> Hi jam,
> 
> Could you please review the change for media/gpu/avda_codec_image.h? Thanks!

please pick an owner from media/

[vinceh, 2016-07-13 00:38:36]

vince.h@nvidia.com changed reviewers:
+ ben@chromium.org

[vinceh, 2016-07-13 00:38:37]

Hello ben,

Could you please review the change under media/ ? Thanks!

[vinceh, 2016-07-13 00:39:54]

On 2016/07/12 21:24:49, jam wrote:
> On 2016/07/12 07:48:33, vinceh wrote:
> > Hi jam,
> > 
> > Could you please review the change for media/gpu/avda_codec_image.h? Thanks!
> 
> please pick an owner from media/

Somehow you're the first name I got from "git cl owners" for media/. I've added
ben.

[vinceh, 2016-07-13 07:32:43]

vince.h@nvidia.com changed reviewers:
+ wuchengli@chromium.org

[vinceh, 2016-07-13 07:32:44]

Added wuchengli.

Hi wuchengli, could you please review the change of media/gpu/avda_codec_image.h
? Thanks!

[wuchengli, 2016-07-14 08:52:29]

wuchengli@chromium.org changed reviewers:
+ liberato@chromium.org, watk@chromium.org

[wuchengli, 2016-07-14 08:52:30]

I'm not familiar with that file. From media/gpu/OWNERS, liberato@ and watk@ can
review *avda*. I'm adding them.

[wuchengli, 2016-07-14 09:15:16]

Description was changed from

==========
Add support for EXT_image_flush_external extension

BUG=chromium:478339
TEST=Boot to UI

Signed-off-by: Vince Hsu <vince.h@nvidia.com>
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Add support for EXT_image_flush_external extension

BUG=chromium:478339
TEST=Boot to UI

Signed-off-by: Vince Hsu <vince.h@nvidia.com>
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[wuchengli, 2016-07-14 09:15:16]

wuchengli@chromium.org changed reviewers:
- wuchengli@chromium.org

[liberato (no reviews please), 2016-07-14 14:38:35]

lgtm

https://codereview.chromium.org/2109803003/diff/160001/ui/gl/gl_image_ozone_n...
File ui/gl/gl_image_ozone_native_pixmap.cc (right):

https://codereview.chromium.org/2109803003/diff/160001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.cc:215: if (has_image_flush_external_) {
random nit: can early out and avoid the indent.

[vinceh, 2016-07-14 16:13:32]

The CQ bit was checked by vince.h@nvidia.com

[vinceh, 2016-07-14 16:13:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, piman@chromium.org, liberato@chromium.org
Link to the patchset: https://codereview.chromium.org/2109803003/#ps180001
(title: "Add support for EXT_image_flush_external extension")

[commit-bot: I haz the power, 2016-07-14 16:13:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 16:14:02]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
     tryserver.chromium.mac
     tryserver.chromium.win
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-14 17:29:23]

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

[commit-bot: I haz the power, 2016-07-14 17:29:25]

Failed to apply patch for gpu/command_buffer/service/gl_stream_texture_image.h:
While running git apply --index -3 -p1;
  error: patch failed: gpu/command_buffer/service/gl_stream_texture_image.h:31
  Falling back to three-way merge...
  Applied patch to 'gpu/command_buffer/service/gl_stream_texture_image.h' with
conflicts.
  U gpu/command_buffer/service/gl_stream_texture_image.h

Patch:       gpu/command_buffer/service/gl_stream_texture_image.h
Index: gpu/command_buffer/service/gl_stream_texture_image.h
diff --git a/gpu/command_buffer/service/gl_stream_texture_image.h
b/gpu/command_buffer/service/gl_stream_texture_image.h
index
b5bc370c5ba58af945a348050c9229c9f7fc5b40..0525a37adeceb22822b7a77ff60d165976bfd64c
100644
--- a/gpu/command_buffer/service/gl_stream_texture_image.h
+++ b/gpu/command_buffer/service/gl_stream_texture_image.h
@@ -31,6 +31,9 @@ class GPU_EXPORT GLStreamTextureImage : public gl::GLImage {
     matrix[13] += matrix[5];
     matrix[5] = -matrix[5];
   }
+
+  void Flush() override {}
+
  protected:
   ~GLStreamTextureImage() override {}

[commit-bot: I haz the power, 2016-07-14 17:29:37]

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

[commit-bot: I haz the power, 2016-07-14 17:29:38]

CQ bit was unchecked.

[vinceh, 2016-07-15 01:04:33]

The CQ bit was checked by vince.h@nvidia.com

[commit-bot: I haz the power, 2016-07-15 01:05:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-15 01:05:06]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
     tryserver.chromium.mac
     tryserver.chromium.win
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-15 01:13:12]

Description was changed from

==========
Add support for EXT_image_flush_external extension

BUG=chromium:478339
TEST=Boot to UI

Signed-off-by: Vince Hsu <vince.h@nvidia.com>
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Add support for EXT_image_flush_external extension

BUG=chromium:478339
TEST=Boot to UI

Signed-off-by: Vince Hsu <vince.h@nvidia.com>
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-07-15 01:13:16]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-07-15 01:13:38]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-15 01:17:50]

Description was changed from

==========
Add support for EXT_image_flush_external extension

BUG=chromium:478339
TEST=Boot to UI

Signed-off-by: Vince Hsu <vince.h@nvidia.com>
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Add support for EXT_image_flush_external extension

BUG=chromium:478339
TEST=Boot to UI

Signed-off-by: Vince Hsu <vince.h@nvidia.com>
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/8baf5bd7df5891470b8dfe4c6de7998bd60170b1
Cr-Commit-Position: refs/heads/master@{#405659}
==========

[commit-bot: I haz the power, 2016-07-15 01:17:52]

Patchset 10 (id:??) landed as
https://crrev.com/8baf5bd7df5891470b8dfe4c6de7998bd60170b1
Cr-Commit-Position: refs/heads/master@{#405659}