video_decode_accelerator_unittest: enable test on ozone surfaceless

video_decode_accelerator_unittest: enable test on ozone surfaceless

TEST=run video_decode_accelerator_unittest
BUG=447798

[llandwerlin-old, 2015-02-19 13:19:09]

lionel.g.landwerlin@intel.com changed reviewers:
+ dnicoara@chromium.org, posciak@chromium.org

[llandwerlin-old, 2015-02-19 13:19:09]

dnicoara@ posciak@ : Feel free to start another CL if that's not the way you
would like to see this fixed.

[dnicoara, 2015-02-19 15:14:33]

On 2015/02/19 13:19:09, llandwerlin wrote:
> dnicoara@ posciak@ : Feel free to start another CL if that's not the way you
> would like to see this fixed.

Hi Lionel, Albert is already working on a solution that's more transparent to
the unittests (https://codereview.chromium.org/938873002/).

We'll likely need to do something like this in the future, but we'll want to
share the renderers with ozone_demo and GbmSurfaceless::OnSwapBuffers will need
to be implemented differently since the use of nested message loops is
discouraged.

[dnicoara, 2015-02-20 04:02:46]

dnicoara@chromium.org changed reviewers:
+ achaulk@chromium.org

[dnicoara, 2015-02-20 04:02:47]

+Albert just in case he thinks this is going to be easier/faster.

[Pawel Osciak, 2015-02-20 04:04:35]

On 2015/02/19 13:19:09, llandwerlin wrote:
> dnicoara@ posciak@ : Feel free to start another CL if that's not the way you
> would like to see this fixed.

Just to doublecheck: this doesn't break non-ozone platforms, right?

[llandwerlin-old, 2015-02-20 10:40:05]

On 2015/02/20 04:04:35, Pawel Osciak wrote:
> On 2015/02/19 13:19:09, llandwerlin wrote:
> > dnicoara@ posciak@ : Feel free to start another CL if that's not the way you
> > would like to see this fixed.
> 
> Just to doublecheck: this doesn't break non-ozone platforms, right?

Pawel, I tested this on chromiumos X11 and chromiumos freon surfaceless and non
surfaceless, all tests pass.
I don't have a Windows or Android setup to run this. Are they any try bots
actually running these tests?

Thanks

[Pawel Osciak, 2015-02-23 05:59:51]

On 2015/02/20 10:40:05, llandwerlin wrote:
> On 2015/02/20 04:04:35, Pawel Osciak wrote:
> > On 2015/02/19 13:19:09, llandwerlin wrote:
> > > dnicoara@ posciak@ : Feel free to start another CL if that's not the way
you
> > > would like to see this fixed.
> > 
> > Just to doublecheck: this doesn't break non-ozone platforms, right?
> 
> Pawel, I tested this on chromiumos X11 and chromiumos freon surfaceless and
non
> surfaceless, all tests pass.
> I don't have a Windows or Android setup to run this. Are they any try bots
> actually running these tests?

I'm not sure, we'll have to check.

Thumbnail test still doesn't work for me with this though. GL error in
RenderingHelper::RenderTexture(). Did it work for you?

[llandwerlin-old, 2015-02-23 09:05:14]

On 2015/02/23 05:59:51, Pawel Osciak wrote:
> On 2015/02/20 10:40:05, llandwerlin wrote:
> > On 2015/02/20 04:04:35, Pawel Osciak wrote:
> > > On 2015/02/19 13:19:09, llandwerlin wrote:
> > > > dnicoara@ posciak@ : Feel free to start another CL if that's not the way
> you
> > > > would like to see this fixed.
> > > 
> > > Just to doublecheck: this doesn't break non-ozone platforms, right?
> > 
> > Pawel, I tested this on chromiumos X11 and chromiumos freon surfaceless and
> non
> > surfaceless, all tests pass.
> > I don't have a Windows or Android setup to run this. Are they any try bots
> > actually running these tests?
> 
> I'm not sure, we'll have to check.
> 
> Thumbnail test still doesn't work for me with this though. GL error in
> RenderingHelper::RenderTexture(). Did it work for you?

Yes, it works for me. Can you paste the error? Thanks.

[Pawel Osciak, 2015-02-23 10:43:33]

On 2015/02/23 09:05:14, llandwerlin wrote:
> On 2015/02/23 05:59:51, Pawel Osciak wrote:
> > On 2015/02/20 10:40:05, llandwerlin wrote:
> > > On 2015/02/20 04:04:35, Pawel Osciak wrote:
> > > > On 2015/02/19 13:19:09, llandwerlin wrote:
> > > > > dnicoara@ posciak@ : Feel free to start another CL if that's not the
way
> > you
> > > > > would like to see this fixed.
> > > > 
> > > > Just to doublecheck: this doesn't break non-ozone platforms, right?
> > > 
> > > Pawel, I tested this on chromiumos X11 and chromiumos freon surfaceless
and
> > non
> > > surfaceless, all tests pass.
> > > I don't have a Windows or Android setup to run this. Are they any try bots
> > > actually running these tests?
> > 
> > I'm not sure, we'll have to check.
> > 
> > Thumbnail test still doesn't work for me with this though. GL error in
> > RenderingHelper::RenderTexture(). Did it work for you?
> 
> Yes, it works for me. Can you paste the error? Thanks.

I think glGetError() returns GL_INVALID_FRAMEBUFFER_OPERATION at the end of
RenderingHelper::RenderTexture()

[llandwerlin-old, 2015-02-23 10:57:30]

On 2015/02/23 10:43:33, Pawel Osciak wrote:
> On 2015/02/23 09:05:14, llandwerlin wrote:
> > On 2015/02/23 05:59:51, Pawel Osciak wrote:
> > > On 2015/02/20 10:40:05, llandwerlin wrote:
> > > > On 2015/02/20 04:04:35, Pawel Osciak wrote:
> > > > > On 2015/02/19 13:19:09, llandwerlin wrote:
> > > > > > dnicoara@ posciak@ : Feel free to start another CL if that's not the
> way
> > > you
> > > > > > would like to see this fixed.
> > > > > 
> > > > > Just to doublecheck: this doesn't break non-ozone platforms, right?
> > > > 
> > > > Pawel, I tested this on chromiumos X11 and chromiumos freon surfaceless
> and
> > > non
> > > > surfaceless, all tests pass.
> > > > I don't have a Windows or Android setup to run this. Are they any try
bots
> > > > actually running these tests?
> > > 
> > > I'm not sure, we'll have to check.
> > > 
> > > Thumbnail test still doesn't work for me with this though. GL error in
> > > RenderingHelper::RenderTexture(). Did it work for you?
> > 
> > Yes, it works for me. Can you paste the error? Thanks.
> 
> I think glGetError() returns GL_INVALID_FRAMEBUFFER_OPERATION at the end of
> RenderingHelper::RenderTexture()

Just to confirm we have the same setup, is your device using Mesa?

[Pawel Osciak, 2015-02-23 10:59:20]

On 2015/02/23 10:57:30, llandwerlin wrote:
> On 2015/02/23 10:43:33, Pawel Osciak wrote:
> > On 2015/02/23 09:05:14, llandwerlin wrote:
> > > On 2015/02/23 05:59:51, Pawel Osciak wrote:
> > > > On 2015/02/20 10:40:05, llandwerlin wrote:
> > > > > On 2015/02/20 04:04:35, Pawel Osciak wrote:
> > > > > > On 2015/02/19 13:19:09, llandwerlin wrote:
> > > > > > > dnicoara@ posciak@ : Feel free to start another CL if that's not
the
> > way
> > > > you
> > > > > > > would like to see this fixed.
> > > > > > 
> > > > > > Just to doublecheck: this doesn't break non-ozone platforms, right?
> > > > > 
> > > > > Pawel, I tested this on chromiumos X11 and chromiumos freon
surfaceless
> > and
> > > > non
> > > > > surfaceless, all tests pass.
> > > > > I don't have a Windows or Android setup to run this. Are they any try
> bots
> > > > > actually running these tests?
> > > > 
> > > > I'm not sure, we'll have to check.
> > > > 
> > > > Thumbnail test still doesn't work for me with this though. GL error in
> > > > RenderingHelper::RenderTexture(). Did it work for you?
> > > 
> > > Yes, it works for me. Can you paste the error? Thanks.
> > 
> > I think glGetError() returns GL_INVALID_FRAMEBUFFER_OPERATION at the end of
> > RenderingHelper::RenderTexture()
> 
> Just to confirm we have the same setup, is your device using Mesa?

Yes. Other tests work and render, just Thumbnail test doesn't.

[Pawel Osciak, 2015-02-23 11:32:32]

On 2015/02/23 10:59:20, Pawel Osciak wrote:
> On 2015/02/23 10:57:30, llandwerlin wrote:
> > On 2015/02/23 10:43:33, Pawel Osciak wrote:
> > > On 2015/02/23 09:05:14, llandwerlin wrote:
> > > > On 2015/02/23 05:59:51, Pawel Osciak wrote:
> > > > > On 2015/02/20 10:40:05, llandwerlin wrote:
> > > > > > On 2015/02/20 04:04:35, Pawel Osciak wrote:
> > > > > > > On 2015/02/19 13:19:09, llandwerlin wrote:
> > > > > > > > dnicoara@ posciak@ : Feel free to start another CL if that's not
> the
> > > way
> > > > > you
> > > > > > > > would like to see this fixed.
> > > > > > > 
> > > > > > > Just to doublecheck: this doesn't break non-ozone platforms,
right?
> > > > > > 
> > > > > > Pawel, I tested this on chromiumos X11 and chromiumos freon
> surfaceless
> > > and
> > > > > non
> > > > > > surfaceless, all tests pass.
> > > > > > I don't have a Windows or Android setup to run this. Are they any
try
> > bots
> > > > > > actually running these tests?
> > > > > 
> > > > > I'm not sure, we'll have to check.
> > > > > 
> > > > > Thumbnail test still doesn't work for me with this though. GL error in
> > > > > RenderingHelper::RenderTexture(). Did it work for you?
> > > > 
> > > > Yes, it works for me. Can you paste the error? Thanks.
> > > 
> > > I think glGetError() returns GL_INVALID_FRAMEBUFFER_OPERATION at the end
of
> > > RenderingHelper::RenderTexture()
> > 
> > Just to confirm we have the same setup, is your device using Mesa?
> 
> Yes. Other tests work and render, just Thumbnail test doesn't.

Hmm... actually, doing:

-  CHECK_EQ(static_cast<int>(glGetError()), GL_NO_ERROR);
+  CHECK_EQ(static_cast<int>(eglGetError()), EGL_SUCCESS);

over the whole file "fixed" it...

[llandwerlin-old, 2015-02-23 11:39:07]

On 2015/02/23 11:32:32, Pawel Osciak wrote:
> On 2015/02/23 10:59:20, Pawel Osciak wrote:
> > On 2015/02/23 10:57:30, llandwerlin wrote:
> > > On 2015/02/23 10:43:33, Pawel Osciak wrote:
> > > > On 2015/02/23 09:05:14, llandwerlin wrote:
> > > > > On 2015/02/23 05:59:51, Pawel Osciak wrote:
> > > > > > On 2015/02/20 10:40:05, llandwerlin wrote:
> > > > > > > On 2015/02/20 04:04:35, Pawel Osciak wrote:
> > > > > > > > On 2015/02/19 13:19:09, llandwerlin wrote:
> > > > > > > > > dnicoara@ posciak@ : Feel free to start another CL if that's
not
> > the
> > > > way
> > > > > > you
> > > > > > > > > would like to see this fixed.
> > > > > > > > 
> > > > > > > > Just to doublecheck: this doesn't break non-ozone platforms,
> right?
> > > > > > > 
> > > > > > > Pawel, I tested this on chromiumos X11 and chromiumos freon
> > surfaceless
> > > > and
> > > > > > non
> > > > > > > surfaceless, all tests pass.
> > > > > > > I don't have a Windows or Android setup to run this. Are they any
> try
> > > bots
> > > > > > > actually running these tests?
> > > > > > 
> > > > > > I'm not sure, we'll have to check.
> > > > > > 
> > > > > > Thumbnail test still doesn't work for me with this though. GL error
in
> > > > > > RenderingHelper::RenderTexture(). Did it work for you?
> > > > > 
> > > > > Yes, it works for me. Can you paste the error? Thanks.
> > > > 
> > > > I think glGetError() returns GL_INVALID_FRAMEBUFFER_OPERATION at the end
> of
> > > > RenderingHelper::RenderTexture()
> > > 
> > > Just to confirm we have the same setup, is your device using Mesa?
> > 
> > Yes. Other tests work and render, just Thumbnail test doesn't.
> 
> Hmm... actually, doing:
> 
> -  CHECK_EQ(static_cast<int>(glGetError()), GL_NO_ERROR);
> +  CHECK_EQ(static_cast<int>(eglGetError()), EGL_SUCCESS);
> 
> over the whole file "fixed" it...

I guess it's equivalent to not have the check.
My understanding is that eglGetError() only reports errors related to calls on
the EGL api.

[llandwerlin-old, 2015-02-26 12:02:25]

Pawel, 

I managed to reproduce the failure by just launching that particular test.
Having some context setup from the previous tests was making it work...

Here is an update to fix the thumbnail issues and also rework the synchronous
SwapBuffers().

I think it would be really good at some point to remove the application logic
(rendering_helper) from the GPU thread and use a command buffer. That would
remove the need for synchronous SwapBuffers.

[dnicoara, 2015-02-26 16:52:59]

Lionel, would you mind splitting out the Ozone-GBM change in a different patch?
I like this approach a lot more and we can land it in the meantime.

https://codereview.chromium.org/940903002/diff/60001/ui/ozone/platform/dri/dr...
File ui/ozone/platform/dri/dri_wrapper.cc (right):

https://codereview.chromium.org/940903002/diff/60001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_wrapper.cc:314: bool DriWrapper::PageFlip(uint32_t
crtc_id,
You can pass in an |is_sync| boolean in here and reuse most of the code and you
wouldn't have to make large changes in the callers.

[llandwerlin-old, 2015-02-26 17:37:22]

https://codereview.chromium.org/940903002/diff/60001/ui/ozone/platform/dri/dr...
File ui/ozone/platform/dri/dri_wrapper.cc (right):

https://codereview.chromium.org/940903002/diff/60001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_wrapper.cc:314: bool DriWrapper::PageFlip(uint32_t
crtc_id,
On 2015/02/26 16:52:59, dnicoara wrote:
> You can pass in an |is_sync| boolean in here and reuse most of the code and
you
> wouldn't have to make large changes in the callers.

Done. New CL : https://codereview.chromium.org/960273003

[Pawel Osciak, 2015-02-27 08:17:24]

On 2015/02/26 17:37:22, llandwerlin wrote:
>
https://codereview.chromium.org/940903002/diff/60001/ui/ozone/platform/dri/dr...
> File ui/ozone/platform/dri/dri_wrapper.cc (right):
> 
>
https://codereview.chromium.org/940903002/diff/60001/ui/ozone/platform/dri/dr...
> ui/ozone/platform/dri/dri_wrapper.cc:314: bool DriWrapper::PageFlip(uint32_t
> crtc_id,
> On 2015/02/26 16:52:59, dnicoara wrote:
> > You can pass in an |is_sync| boolean in here and reuse most of the code and
> you
> > wouldn't have to make large changes in the callers.
> 
> Done. New CL : https://codereview.chromium.org/960273003

Thanks Lionel. Apart from that new CL, is this CL also ready to review again?
I agree it would be good to go with this solution for now at least, to unblock
everyone.

[llandwerlin-old, 2015-02-27 10:35:53]

On 2015/02/27 08:17:24, Pawel Osciak wrote:
> On 2015/02/26 17:37:22, llandwerlin wrote:
> >
>
https://codereview.chromium.org/940903002/diff/60001/ui/ozone/platform/dri/dr...
> > File ui/ozone/platform/dri/dri_wrapper.cc (right):
> > 
> >
>
https://codereview.chromium.org/940903002/diff/60001/ui/ozone/platform/dri/dr...
> > ui/ozone/platform/dri/dri_wrapper.cc:314: bool DriWrapper::PageFlip(uint32_t
> > crtc_id,
> > On 2015/02/26 16:52:59, dnicoara wrote:
> > > You can pass in an |is_sync| boolean in here and reuse most of the code
and
> > you
> > > wouldn't have to make large changes in the callers.
> > 
> > Done. New CL : https://codereview.chromium.org/960273003
> 
> Thanks Lionel. Apart from that new CL, is this CL also ready to review again?
> I agree it would be good to go with this solution for now at least, to unblock
> everyone.

Pawel, here is the updated version of the CL ready for review.
This needs https://codereview.chromium.org/960273003 to work.

[dnicoara, 2015-02-27 16:58:47]

https://codereview.chromium.org/940903002/diff/80001/content/common/gpu/media...
File content/common/gpu/media/surfaceless_gl_renderer.h (right):

https://codereview.chromium.org/940903002/diff/80001/content/common/gpu/media...
content/common/gpu/media/surfaceless_gl_renderer.h:58:
ui::GpuMemoryBufferFactoryOzoneNativeBuffer* buffer_factory_;
Since we're in src/content, you should be able to use the GpuMemoryBufferFactory
interface. You can then call GpuMemoryBufferFactory::Create() to get an
implementation (you'll get the Ozone one if compiled with USE_OZONE). That way
you could drop most of the #if defined(USE_OZONE) around the surfaceless gl
renderer.

[llandwerlin-old, 2015-03-02 11:21:04]

https://codereview.chromium.org/940903002/diff/80001/content/common/gpu/media...
File content/common/gpu/media/surfaceless_gl_renderer.h (right):

https://codereview.chromium.org/940903002/diff/80001/content/common/gpu/media...
content/common/gpu/media/surfaceless_gl_renderer.h:58:
ui::GpuMemoryBufferFactoryOzoneNativeBuffer* buffer_factory_;
On 2015/02/27 16:58:47, dnicoara wrote:
> Since we're in src/content, you should be able to use the
GpuMemoryBufferFactory
> interface. You can then call GpuMemoryBufferFactory::Create() to get an
> implementation (you'll get the Ozone one if compiled with USE_OZONE). That way
> you could drop most of the #if defined(USE_OZONE) around the surfaceless gl
> renderer.

Done.

[Pawel Osciak, 2015-03-02 11:21:56]

posciak@chromium.org changed reviewers:
+ owenlin@chromium.org

[dnicoara, 2015-03-02 15:47:54]

lgtm

[achaulk, 2015-03-03 18:07:34]

With https://codereview.chromium.org/938873002/ going in this shouldn't be
needed anymore. I was seeing all tests pass

[Pawel Osciak, 2015-04-27 05:36:40]

On 2015/03/03 18:07:34, achaulk wrote:
> With https://codereview.chromium.org/938873002/ going in this shouldn't be
> needed anymore. I was seeing all tests pass

Should we close this?