video: On ChromeOS don't assume GLX always.

video: On ChromeOS don't assume GLX always.

Hardware video decode depends on GLX but we may be using OSMesa. In
this case fallback to non-hardware video decode gracefully instead
of static casting to an incorrect type.

This allows us to remove flags forcing non-OSMesa for some tests,
and they will test hardware decode on the ChromeOS VMs still.

These flags must go in order to support browser tests running
with a compositor instead of using legacy software mode, since
GLX inside the bot Xvfb environment can not run the compositor.
(This is a small piece of enabling us to test real code paths
in browser tests https://codereview.chromium.org/120313002/ .)

R=fischman@chromium.org, piman@chromium.org, piman
BUG=270918
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244111

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

[danakj, 2014-01-09 22:22:16]

piman@: please review
fischman@: please review the webrtc test changes

[piman, 2014-01-09 23:12:48]

lgtm

[Ami GONE FROM CHROMIUM, 2014-01-09 23:35:43]

Would it be better to do this via a blacklist entry than the change to gvda.cc?

Probably also want to add a LOG(FATAL) for this condition to
content/common/gpu/media/video_decode_accelerator_unittest.cc:GLRenderingVDAClient::CreateAndStartDecoder()
in case someone tries to run video_decode_accelerator_unittest on a bot that's
configured for mesa.

https://codereview.chromium.org/132503002/diff/170001/chrome/browser/media/ch...
File chrome/browser/media/chrome_webrtc_browsertest.cc (left):

https://codereview.chromium.org/132503002/diff/170001/chrome/browser/media/ch...
chrome/browser/media/chrome_webrtc_browsertest.cc:57:
command_line->AppendSwitch(switches::kUseGpuInTests);
This isn't being compensated for by a SetUp() change like the other files?

https://codereview.chromium.org/132503002/diff/170001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/132503002/diff/170001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:283: LOG(ERROR) << "HW
video decode acceleration not available without "
Your CL description makes it sound like this condition is going to be expected
on some bots.  s/ERROR/INFO/ here?  Or even VLOG(1)?

[piman, 2014-01-09 23:42:41]

On Thu, Jan 9, 2014 at 3:35 PM, <fischman@chromium.org> wrote:

> Would it be better to do this via a blacklist entry than the change to
> gvda.cc?
>

I think the code there is right. The static_cast is incorrect if
--use-gl=osmesa (i.e. potential security issue). The blacklist only results
in a command-line flag passed to the renderer, the GPU process doesn't do
anything with it. So in theory a malicious renderer could exploit that and
elevate privilege.


> Probably also want to add a LOG(FATAL) for this condition to
> content/common/gpu/media/video_decode_accelerator_unittest.cc:
> GLRenderingVDAClient::CreateAndStartDecoder()
> in case someone tries to run video_decode_accelerator_unittest on a bot
> that's
> configured for mesa.
>
>
> https://codereview.chromium.org/132503002/diff/170001/
> chrome/browser/media/chrome_webrtc_browsertest.cc
> File chrome/browser/media/chrome_webrtc_browsertest.cc (left):
>
> https://codereview.chromium.org/132503002/diff/170001/
> chrome/browser/media/chrome_webrtc_browsertest.cc#oldcode57
> chrome/browser/media/chrome_webrtc_browsertest.cc:57:
> command_line->AppendSwitch(switches::kUseGpuInTests);
> This isn't being compensated for by a SetUp() change like the other
> files?
>
> https://codereview.chromium.org/132503002/diff/170001/
> content/common/gpu/media/gpu_video_decode_accelerator.cc
> File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):
>
> https://codereview.chromium.org/132503002/diff/170001/
> content/common/gpu/media/gpu_video_decode_accelerator.cc#newcode283
> content/common/gpu/media/gpu_video_decode_accelerator.cc:283: LOG(ERROR)
>
> << "HW video decode acceleration not available without "
> Your CL description makes it sound like this condition is going to be
> expected on some bots.  s/ERROR/INFO/ here?  Or even VLOG(1)?
>
> https://codereview.chromium.org/132503002/
>

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

[Ami GONE FROM CHROMIUM, 2014-01-09 23:46:19]

Sold.


On Thu, Jan 9, 2014 at 3:42 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
>
> On Thu, Jan 9, 2014 at 3:35 PM, <fischman@chromium.org> wrote:
>
>> Would it be better to do this via a blacklist entry than the change to
>> gvda.cc?
>>
>
> I think the code there is right. The static_cast is incorrect if
> --use-gl=osmesa (i.e. potential security issue). The blacklist only results
> in a command-line flag passed to the renderer, the GPU process doesn't do
> anything with it. So in theory a malicious renderer could exploit that and
> elevate privilege.
>
>
>> Probably also want to add a LOG(FATAL) for this condition to
>> content/common/gpu/media/video_decode_accelerator_unittest.cc:
>> GLRenderingVDAClient::CreateAndStartDecoder()
>> in case someone tries to run video_decode_accelerator_unittest on a bot
>> that's
>> configured for mesa.
>>
>>
>> https://codereview.chromium.org/132503002/diff/170001/
>> chrome/browser/media/chrome_webrtc_browsertest.cc
>> File chrome/browser/media/chrome_webrtc_browsertest.cc (left):
>>
>> https://codereview.chromium.org/132503002/diff/170001/
>> chrome/browser/media/chrome_webrtc_browsertest.cc#oldcode57
>> chrome/browser/media/chrome_webrtc_browsertest.cc:57:
>> command_line->AppendSwitch(switches::kUseGpuInTests);
>> This isn't being compensated for by a SetUp() change like the other
>> files?
>>
>> https://codereview.chromium.org/132503002/diff/170001/
>> content/common/gpu/media/gpu_video_decode_accelerator.cc
>> File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):
>>
>> https://codereview.chromium.org/132503002/diff/170001/
>> content/common/gpu/media/gpu_video_decode_accelerator.cc#newcode283
>> content/common/gpu/media/gpu_video_decode_accelerator.cc:283: LOG(ERROR)
>>
>> << "HW video decode acceleration not available without "
>> Your CL description makes it sound like this condition is going to be
>> expected on some bots.  s/ERROR/INFO/ here?  Or even VLOG(1)?
>>
>> https://codereview.chromium.org/132503002/
>>
>
>

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

[danakj, 2014-01-10 01:10:40]

> Probably also want to add a LOG(FATAL) for this condition to
>
content/common/gpu/media/video_decode_accelerator_unittest.cc:GLRenderingVDAClient::CreateAndStartDecoder()
> in case someone tries to run video_decode_accelerator_unittest on a bot
> that's configured for mesa.

Ok, sure. I guess this is not running on linux_chromeos bots.

https://codereview.chromium.org/132503002/diff/170001/chrome/browser/media/ch...
File chrome/browser/media/chrome_webrtc_browsertest.cc (left):

https://codereview.chromium.org/132503002/diff/170001/chrome/browser/media/ch...
chrome/browser/media/chrome_webrtc_browsertest.cc:57:
command_line->AppendSwitch(switches::kUseGpuInTests);
On 2014/01/09 23:35:43, Ami Fischman wrote:
> This isn't being compensated for by a SetUp() change like the other files?

These tests pass without pixel results apparently.

https://codereview.chromium.org/132503002/diff/170001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/132503002/diff/170001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:283: LOG(ERROR) << "HW
video decode acceleration not available without "
On 2014/01/09 23:35:43, Ami Fischman wrote:
> Your CL description makes it sound like this condition is going to be expected
> on some bots.  s/ERROR/INFO/ here?  Or even VLOG(1)?

It's true, this will happen on linux_chromeos since that runs in a linux Xvfb,
not on a ChromeOS VM. VLOG(1) it is.

[Ami GONE FROM CHROMIUM, 2014-01-10 01:15:09]

LGTM % comment below.
(this test is run on the cros waterfall, but not on chromium bots, b/c there are
no chromium bots with the right OS+arch combo to make it go)

https://codereview.chromium.org/132503002/diff/330001/content/common/gpu/medi...
File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/132503002/diff/330001/content/common/gpu/medi...
content/common/gpu/media/video_decode_accelerator_unittest.cc:538:
CHECK_EQ(gfx::kGLImplementationDesktopGL, gfx::GetGLImplementation());
I expected you to put this inside the cros&&x86 #if (l.560 below).  As it is I
think it'll fail (incorrectly) on cros/arm, where we use EGL/GLES2.

[danakj, 2014-01-10 01:18:14]

https://codereview.chromium.org/132503002/diff/330001/content/common/gpu/medi...
File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/132503002/diff/330001/content/common/gpu/medi...
content/common/gpu/media/video_decode_accelerator_unittest.cc:538:
CHECK_EQ(gfx::kGLImplementationDesktopGL, gfx::GetGLImplementation());
On 2014/01/10 01:15:10, Ami Fischman wrote:
> I expected you to put this inside the cros&&x86 #if (l.560 below).  As it is I
> think it'll fail (incorrectly) on cros/arm, where we use EGL/GLES2.

Right, I just had changed it to check != OSMesa instead of == DesktopGL.  But I
can move it down to the #if also. Thanks!

[Ami GONE FROM CHROMIUM, 2014-01-10 01:20:46]

LGTM

[commit-bot: I haz the power, 2014-01-10 01:25:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/132503002/450001

[commit-bot: I haz the power, 2014-01-10 06:38:08]

Change committed as 244111

[Marijn Kruisselbrink, 2014-01-10 15:19:50]

A revert of this CL has been created in
https://codereview.chromium.org/134003003/ by mek@chromium.org.

The reason for reverting is: This seems to cause WebRtcBrowserTest to fail on
Win7 debug bots.

[danakj, 2014-01-10 16:36:20]

It appears that WebrtcBrowserTest.TestGetUserMediaConstraints is timing out, it
probably does not need pixel results. I'll try make this more fine-grained.

[danakj, 2014-01-10 16:51:17]

On 2014/01/10 16:36:20, danakj wrote:
> It appears that WebrtcBrowserTest.TestGetUserMediaConstraints is timing out,
it
> probably does not need pixel results. I'll try make this more fine-grained.

In the current browser test setup, we're using legacy software mode, and these
tests are okay to pass without the UI compositor drawing anything.

However once we get the renderer compositor running, these tests do time out and
make no progress if we aren't drawing. So they do seem to require pixel results.
Many of these tests are just extremely slow - testing large sets of
configurations at a time. Making the UI compositor draw pixels is not strictly
neccessary until we have the renderer running in a compositor, so maybe I'll
split that out of this CL.

[danakj, 2014-01-10 17:55:41]

https://codereview.chromium.org/132503002/diff/450001/content/browser/media/w...
File content/browser/media/webrtc_browsertest.cc (right):

https://codereview.chromium.org/132503002/diff/450001/content/browser/media/w...
content/browser/media/webrtc_browsertest.cc:93: UseRealGLContexts();
I removed this from this CL as it's not needed (yet). I'll do another CL with
this, and I'll split up some of these slow tests so they don't time out.

[commit-bot: I haz the power, 2014-01-10 18:04:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/132503002/800001

[danakj, 2014-01-10 20:01:35]

Committed patchset #7 manually as r244222 (presubmit successful).