Remove additional protection of kDisableAcceleratedVideoDecode in bpf_gpu_policy_linux.cc.

Remove additional protection of kDisableAcceleratedVideoDecode in bpf_gpu_policy_linux.cc.

The blacklist is the way we make features available or not. The control of
accelerated video decode should rely on GPU blacklist.

After this CL, Linux and Mac will output following error message when using --ignore-gpu-blacklist.
"[(pid):ERROR:gpu_video_decode_accelerator.cc(303)] Not implemented reached in void 
content::GpuVideoDecodeAccelerator::Initialize(media::VideoCodecProfile, IPC::Message*)
HW video decode acceleration not available."

In addition, match macro style in video_decode_accelerator_unittest.cc
to gpu_video_decode_accelerator.cc

BUG=N/A

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

[arekm, 2014-03-03 17:07:35]

On 2014/03/03 15:57:35, dshwang wrote:

+        I965DrvVideoPath = "/usr/lib/x86_64-linux-gnu/dri/i965_drv_video.so";

will these stay hardcoded? (invalid here on my distro)

[dshwang, 2014-03-03 17:14:47]

On 2014/03/03 17:07:35, arekm wrote:
> On 2014/03/03 15:57:35, dshwang wrote:
> 
> +        I965DrvVideoPath = "/usr/lib/x86_64-linux-gnu/dri/i965_drv_video.so";
> 
> will these stay hardcoded? (invalid here on my distro)

Thank you for noting. I guess I need to make a generating script.

I did it because sysroot-creator-debian.wheezy.sh has similar hardcode, but it
seems to be true in only debian..
chrome/installer/linux/sysroot_scripts/sysroot-creator-debian.wheezy.sh:        
   ${INSTALL_ROOT}/usr/lib/x86_64-linux-gnu/libc.so"
chrome/installer/linux/sysroot_scripts/sysroot-creator-debian.wheezy.sh:        
   ${INSTALL_ROOT}/usr/lib/i386-linux-gnu/libc.so"

[Ami GONE FROM CHROMIUM, 2014-03-03 17:28:11]

Does not lgtm. 
The rationale listed in the CL description fails to address the reason why this
is not already done.  Please see the reviewlog of
https://codereview.chromium.org/15955009/ for the meat of it.

https://codereview.chromium.org/176883018/diff/1/content/content_common.gypi
File content/content_common.gypi (right):

https://codereview.chromium.org/176883018/diff/1/content/content_common.gypi#...
content/content_common.gypi:570: ['(chromeos==1 or OS=="linux") and use_x11==1
and target_arch != "arm"', {
At least until https://codereview.chromium.org/179543002/ lands, this change
introduces a build-time dependency on libva, which means an edit to
build/install-build-deps.sh

[dshwang, 2014-03-03 18:15:44]

On 2014/03/03 17:28:11, Ami Fischman wrote:
> Does not lgtm. 
> The rationale listed in the CL description fails to address the reason why
this
> is not already done.  Please see the reviewlog of
> https://codereview.chromium.org/15955009/ for the meat of it.

Thank you for review. ah, haltonhuo@ already tried to enable it.

This CL does not enable it by default in Linux because Linux belongs to
blacklist.
If user want to take risk, they enable ignore-gpu-blacklist by themselves like
accelerated 2d canvas.
It means that we don't have responsibility to make sure it works in all linux
distros.

In addition, we can introduce gpu-video-decode-blacklist like gpu-blacklist if
we want more fine control for it.
I guess it makes it more convenient for us to maintain va-api for CrOS.

Could I listen to your opinion again?

> https://codereview.chromium.org/176883018/diff/1/content/content_common.gypi
> File content/content_common.gypi (right):
> 
>
https://codereview.chromium.org/176883018/diff/1/content/content_common.gypi#...
> content/content_common.gypi:570: ['(chromeos==1 or OS=="linux") and use_x11==1
> and target_arch != "arm"', {
> At least until https://codereview.chromium.org/179543002/ lands, this change
> introduces a build-time dependency on libva, which means an edit to
> build/install-build-deps.sh

It's good to know. I'll take care of not introducing build dependency.

[Ami GONE FROM CHROMIUM, 2014-03-03 18:26:27]

There is a history of users disabling the blacklist (entirely) because they
want a feature that is disabled.  That destabilizes the entire browser, and
users frequently forget about this action (and waste time trying to
re-stabilize their browser later).
If this landed I expect that sooner or later we'd get a rash of blog posts
explaining how to get HW decode on linux "for free" (by disabling the GPU
blacklist) and the overall result for our Linux userbase would be a worse
experience (because the blacklist will never be consulted on their system),
not better (b/c they'll have HW acceleration of h.264 decode).
This is a judgement call and I can certainly see how reasonable people can
disagree, but this is my personal judgement.

It means that we don't have responsibility to make sure it works in all
> linux distros.
>

That smells like a cop-out.  We don't ship code we consider to be
permanently "experimental" or "beta", only code we expect to be
stable/production-quality eventually, if not at landing.  This feature will
never graduate to that status, so this CL is effectively shipping a feature
that is known to be mostly-broken on most Linux installations.

In addition, we can introduce gpu-video-decode-blacklist like gpu-blacklist
> if we want more fine control for it.
>

This addresses my first paragraph above, but not the second.


> I guess it makes it more convenient for us to maintain va-api for CrOS.
>

I appreciate that there is non-zero friction cost to the Linux developer
experience imposed by the unavailability of HW decode on Linux (heck, _I_
am a Linux developer and I pay this cost! :)).  However I strongly prefer
that cost be paid by the (small handful) of developers working on
Linux/CrOS/HW decode rather than the costs described in the first two
paragraphs of this email be paid by the entire Linux userbase of Chrome.

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

[Jorge Lucangeli Obes, 2014-03-03 18:42:48]

On 2014/03/03 18:26:27, Ami Fischman wrote:
> There is a history of users disabling the blacklist (entirely) because they
> want a feature that is disabled.  That destabilizes the entire browser, and
> users frequently forget about this action (and waste time trying to
> re-stabilize their browser later).
> If this landed I expect that sooner or later we'd get a rash of blog posts
> explaining how to get HW decode on linux "for free" (by disabling the GPU
> blacklist) and the overall result for our Linux userbase would be a worse
> experience (because the blacklist will never be consulted on their system),
> not better (b/c they'll have HW acceleration of h.264 decode).
> This is a judgement call and I can certainly see how reasonable people can
> disagree, but this is my personal judgement.
> 
> It means that we don't have responsibility to make sure it works in all
> > linux distros.
> >
> 
> That smells like a cop-out.  We don't ship code we consider to be
> permanently "experimental" or "beta", only code we expect to be
> stable/production-quality eventually, if not at landing.  This feature will
> never graduate to that status, so this CL is effectively shipping a feature
> that is known to be mostly-broken on most Linux installations.
> 
> In addition, we can introduce gpu-video-decode-blacklist like gpu-blacklist
> > if we want more fine control for it.
> >
> 
> This addresses my first paragraph above, but not the second.
> 
> 
> > I guess it makes it more convenient for us to maintain va-api for CrOS.
> >
> 
> I appreciate that there is non-zero friction cost to the Linux developer
> experience imposed by the unavailability of HW decode on Linux (heck, _I_
> am a Linux developer and I pay this cost! :)).  However I strongly prefer
> that cost be paid by the (small handful) of developers working on
> Linux/CrOS/HW decode rather than the costs described in the first two
> paragraphs of this email be paid by the entire Linux userbase of Chrome.
>

I agree with Ami here. Supporting GPU features on Linux is a nightmare (I know
from dealing with the GPU sandbox). Enabling this feature should come after
thinking how we can make it available without making Chrome on Linux less
stable.

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

[dshwang, 2014-03-03 19:16:50]

On 2014/03/03 18:42:48, Jorge Lucangeli Obes wrote:
> On 2014/03/03 18:26:27, Ami Fischman wrote:
> > I appreciate that there is non-zero friction cost to the Linux developer
> > experience imposed by the unavailability of HW decode on Linux (heck, _I_
> > am a Linux developer and I pay this cost! :)).  However I strongly prefer
> > that cost be paid by the (small handful) of developers working on
> > Linux/CrOS/HW decode rather than the costs described in the first two
> > paragraphs of this email be paid by the entire Linux userbase of Chrome.
> >
> 
> I agree with Ami here. Supporting GPU features on Linux is a nightmare (I know
> from dealing with the GPU sandbox). Enabling this feature should come after
> thinking how we can make it available without making Chrome on Linux less
> stable.

Thank you for good explanation. I totally agree with your reasonable judgement.

I change this CL's goal. just clean up little more.
This CL is just following-up to https://chromiumcodereview.appspot.com/10832356

Could you review again?

[Ami GONE FROM CHROMIUM, 2014-03-03 20:38:37]

https://codereview.chromium.org/176883018/diff/70001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/176883018/diff/70001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_data_manager_impl_private.cc:325: #if
defined(OS_CHROMEOS) || defined(OS_WIN) || defined(OS_ANDROID)
Why is this #if needed, considering the feature is blacklisted on linux & mac?

[dshwang, 2014-03-03 20:45:30]

I have a good compromise plan.

Summary: introduce gpu-video-decode-whitelist. don't export it on chrome://flags
until we have confidence. only developers can use it like other non-exported
content switches.

This compromise is only for developers, because
1. we can test it on various linux distro, so we can get more real world
information.
2. It's more convenient to maintain gpu video decode.
3. It's different from submitting non-production quality code and then
forgetting, because linux chrome uses exactly same code to chrome os.

imo, Intel Ivy-bridge and Haswell can belong to gpu-video-decode-whitelist.
Could I listen to your opinion again?

P.S. I submitted this CL because I felt burdensome to work on CrOS when I
improved HW video compositing code: https://codereview.chromium.org/175223003/

[Ami GONE FROM CHROMIUM, 2014-03-03 21:15:59]

>
> Summary: introduce gpu-video-decode-whitelist. don't export it on
> chrome://flags
> until we have confidence.


I don't see what future developments would give us confidence.  How do you
see this play out?


> only developers can use it like other non-exported
> content switches.
>

We do not (or at least try very hard not to) ship things that are behind
flags indefinitely.  See my earlier comment.

This compromise is only for developers, because
> 1. we can test it on various linux distro, so we can get more real
> world information.
>

The problem space is far larger than just distros: it's the cartesian
product of:
distro * video HW * user flags
This testing would have to span a LOT of systems.  Are you volunteering to
do so, and to set up infrastructure to keep them green/blacklisted?

2. It's more convenient to maintain gpu video decode.
>

As I said earlier, we're talking about probably fewer than 20 developers
impacted.  That is a _tiny_ number to be taking any kind of user stability
risks.


> 3. It's different from submitting non-production quality code and then
> forgetting, because linux chrome uses exactly same code to chrome os.
>

The difference is that on CrOS we have a much smaller space to test with,
and we actually test the space quite extensively.


> imo, Intel Ivy-bridge and Haswell can belong to gpu-video-decode-whitelist.
>

IVB & HSW having the capability says nothing about the rest of the system
being ready/stable to take advantage of them.

P.S. I submitted this CL because I felt burdensome to work on CrOS when I
> improved HW video compositing code: https://codereview.chromium.
> org/175223003/


How much time did you spend with that setup?  Multiply that by 20.  That's
a ceiling on the total amount of developer time you're likely to save
(across the entire development team) over the next 2y (assuming we have
100% growth or turnover).
That still pales in comparison with the support burden and the hit to the
principle of not shipping disabled bits that accepting this proposal would
create.

(also, fwiw, I'm shocked that you consider it that large a burden; my own
experience of guiding people through
https://code.google.com/p/chromium/wiki/LinuxHWVideoDecode is that the
great majority of the time is taken by getting libva & vdpau installed in a
working situation on their system (and this would be necessary even if your
your proposal were accepted); once that's done the totality of the support
burden is to add chromeos=1 to $GYP_DEFINES and specifying LD_LIBRARY_PATH
on the chrome command-line; did you run into other pain?)

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

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

[dshwang, 2014-03-04 08:43:42]

On 2014/03/03 21:15:59, Ami Fischman wrote:
> I don't see what future developments would give us confidence.  How do you
> see this play out?

When I see my previous word, smell like politician. You're right.
I should say more obviously how about adding development switch, not white-list
marketing term.

> > only developers can use it like other non-exported
> > content switches.
> >
> 
> We do not (or at least try very hard not to) ship things that are behind
> flags indefinitely.  See my earlier comment.

We already have various switches for only developers.

CC_EXPORT extern const char kJankInsteadOfCheckerboard[];
CC_EXPORT extern const char kTraceOverdraw[];
CC_EXPORT extern const char kMaxTilesForInterestArea[];
CC_EXPORT extern const char kMaxUnusedResourceMemoryUsagePercentage[];
CC_EXPORT extern const char kStrictLayerPropertyChangeChecking[];

I thought it would be good, if we have that kind of switch. i.e.
kForceEnableGpuVideoDecode

> The problem space is far larger than just distros: it's the cartesian
> product of:
> distro * video HW * user flags
> This testing would have to span a LOT of systems.  Are you volunteering to
> do so, and to set up infrastructure to keep them green/blacklisted?

I understand why you are objective. Effort is way bigger than benefit. I agree.

> (also, fwiw, I'm shocked that you consider it that large a burden; my own
> experience of guiding people through

I just felt small burden, not large burden. don't be shocked plz.

> https://code.google.com/p/chromium/wiki/LinuxHWVideoDecode is that the
> great majority of the time is taken by getting libva & vdpau installed in a
> working situation on their system (and this would be necessary even if your
> your proposal were accepted); once that's done the totality of the support
> burden is to add chromeos=1 to $GYP_DEFINES and specifying LD_LIBRARY_PATH
> on the chrome command-line; did you run into other pain?)

my current development process:
1. sudo apt-get install libva1 libva-x11 libva-intel-vaapi-driver
2. apply my CL
3. build chrome or content_shell with proprietary_codecs=1
ffmpeg_branding=Chrome
no need chromeos=1 and LD_LIBRARY_PATH

I think it's not critical issue for dev. Hacking via wiki/LinuxHWVideoDecode is
not bad.
I don't want to make new burden to chromium dev or hit stability.
If you think it's not good idea to add something like
kForceEnableGpuVideoDecode, I agree.

Thank you for your time.

[Ami GONE FROM CHROMIUM, 2014-03-05 02:03:37]

The BUG= line in the CL description should probably be dropped (since the CL has
morphed away from that BUG).
Also note I sent a code comment I haven't seen a reply to yet.

[dshwang, 2014-03-06 12:44:37]

Thank you for review and sorry for delaying reply.

https://codereview.chromium.org/176883018/diff/90001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/176883018/diff/90001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_data_manager_impl_private.cc:328:
command_line->AppendSwitch(switches::kDisableAcceleratedVideoDecode);
> Why is this #if needed, considering the feature is blacklisted on linux & mac?

My previous CL is wrong. linux & mac must append kDisableAcceleratedVideoDecode
no matter whether blacklisted or not. So #if is needed.

[Jorge Lucangeli Obes, 2014-03-06 15:53:59]

https://codereview.chromium.org/176883018/diff/90001/content/common/sandbox_l...
File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (left):

https://codereview.chromium.org/176883018/diff/90001/content/common/sandbox_l...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:76: // Accelerated video
decode is currently enabled on Chrome OS,
Let's simplify this function to:
CommandLine::ForCurrentProcess()->HasSwitch(switches::kDisableAcceleratedVideoDecode)

And keep calling it below. Makes code easier to read.

[dshwang, 2014-03-06 16:15:06]

On 2014/03/06 15:53:59, Jorge Lucangeli Obes wrote:
>
https://codereview.chromium.org/176883018/diff/90001/content/common/sandbox_l...
> File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (left):
> 
>
https://codereview.chromium.org/176883018/diff/90001/content/common/sandbox_l...
> content/common/sandbox_linux/bpf_gpu_policy_linux.cc:76: // Accelerated video
> decode is currently enabled on Chrome OS,
> Let's simplify this function to:
>
CommandLine::ForCurrentProcess()->HasSwitch(switches::kDisableAcceleratedVideoDecode)
> 
> And keep calling it below. Makes code easier to read.

Yes, Done.

[Jorge Lucangeli Obes, 2014-03-06 16:28:33]

On 2014/03/06 16:15:06, dshwang wrote:
> On 2014/03/06 15:53:59, Jorge Lucangeli Obes wrote:
> >
>
https://codereview.chromium.org/176883018/diff/90001/content/common/sandbox_l...
> > File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (left):
> > 
> >
>
https://codereview.chromium.org/176883018/diff/90001/content/common/sandbox_l...
> > content/common/sandbox_linux/bpf_gpu_policy_linux.cc:76: // Accelerated
video
> > decode is currently enabled on Chrome OS,
> > Let's simplify this function to:
> >
>
CommandLine::ForCurrentProcess()->HasSwitch(switches::kDisableAcceleratedVideoDecode)
> > 
> > And keep calling it below. Makes code easier to read.
> 
> Yes, Done.

bpf_gpu_policy_linux.cc lgtm, but wait for Ami for the other bits.

[dshwang, 2014-03-06 16:35:16]

On 2014/03/06 16:28:33, Jorge Lucangeli Obes wrote:
> bpf_gpu_policy_linux.cc lgtm, but wait for Ami for the other bits.

yes, thank you for review.

[Ami GONE FROM CHROMIUM, 2014-03-06 17:29:05]

On 2014/03/06 12:44:37, dshwang wrote:
> Thank you for review and sorry for delaying reply.
> 
>
https://codereview.chromium.org/176883018/diff/90001/content/browser/gpu/gpu_...
> File content/browser/gpu/gpu_data_manager_impl_private.cc (right):
> 
>
https://codereview.chromium.org/176883018/diff/90001/content/browser/gpu/gpu_...
> content/browser/gpu/gpu_data_manager_impl_private.cc:328:
> command_line->AppendSwitch(switches::kDisableAcceleratedVideoDecode);
> > Why is this #if needed, considering the feature is blacklisted on linux &
mac?
> 
> My previous CL is wrong. linux & mac must append
kDisableAcceleratedVideoDecode
> no matter whether blacklisted or not. So #if is needed.

I'm sorry, I still don't understand why this #if is needed.  
Can you describe in as much detail as you can what scenario would break if it
was removed? (i.e. if the switch was unconditionally added to the cmdline when
the feature was blacklisted)

[dshwang, 2014-03-06 18:33:29]

On 2014/03/06 17:29:05, Ami Fischman wrote:
> On 2014/03/06 12:44:37, dshwang wrote:
> > Thank you for review and sorry for delaying reply.
> > 
> >
>
https://codereview.chromium.org/176883018/diff/90001/content/browser/gpu/gpu_...
> > File content/browser/gpu/gpu_data_manager_impl_private.cc (right):
> > 
> >
>
https://codereview.chromium.org/176883018/diff/90001/content/browser/gpu/gpu_...
> > content/browser/gpu/gpu_data_manager_impl_private.cc:328:
> > command_line->AppendSwitch(switches::kDisableAcceleratedVideoDecode);
> > > Why is this #if needed, considering the feature is blacklisted on linux &
> mac?
> > 
> > My previous CL is wrong. linux & mac must append
> kDisableAcceleratedVideoDecode
> > no matter whether blacklisted or not. So #if is needed.
> 
> I'm sorry, I still don't understand why this #if is needed.  
> Can you describe in as much detail as you can what scenario would break if it
> was removed? (i.e. if the switch was unconditionally added to the cmdline when
> the feature was blacklisted)

sorry for not clear explanation.

First of all, it declares Linux and Mac does not support Accelerated Video
Decode, no matter what.

Without #if, --ignore-gpu-blacklist switch can make Linux and Mac not include
kDisableAcceleratedVideoDecode switch.
Linux and Mac don't have Accelerated Video Decode implementation. It does not
make sense.

+#if defined(OS_CHROMEOS) || defined(OS_WIN) || defined(OS_ANDROID)
+  // Accelerated video decode is currently enabled on Chrome OS,
+  // but not on Linux: crbug.com/137247.
+  if (gpu_manager->IsFeatureBlacklisted(
+          gpu::GPU_FEATURE_TYPE_ACCELERATED_VIDEO_DECODE) &&
+      !command_line->HasSwitch(switches::kDisableAcceleratedVideoDecode))
+    command_line->AppendSwitch(switches::kDisableAcceleratedVideoDecode);
+#else
+  if (!command_line->HasSwitch(switches::kDisableAcceleratedVideoDecode))
+    command_line->AppendSwitch(switches::kDisableAcceleratedVideoDecode);
+#endif


In detail, absence of kDisableAcceleratedVideoDecode does not break anything,
because GpuVideoDecodeAccelerator::Initialize() in gpu process fail.
However, it's little bit overhead for runtime. In addition,
bpf_gpu_policy_linux.cc executes dlopen(libva). "#if" makes Linux avoid this
overhead.

[Ami GONE FROM CHROMIUM, 2014-03-06 19:18:21]

>
> First of all, it declares Linux and Mac does not support Accelerated Video
> Decode, no matter what.

Without #if, --ignore-gpu-blacklist switch can make Linux and Mac not
> include
> kDisableAcceleratedVideoDecode switch.
> Linux and Mac don't have Accelerated Video Decode implementation. It does
> not
> make sense.
>

We don't usually put in multiple layers of protection against foot-shooting
:)
The blacklist is the way we make features available or not.  I think it
would be a mistake to attempt to layer additional stopgaps like the #if
here.  In particular it makes it a lot harder to be sure that all codepaths
do the same thing (e.g. multiple test binaries, ppapi plugins, etc).

In detail, absence of kDisableAcceleratedVideoDecode does not break
> anything,
> because GpuVideoDecodeAccelerator::Initialize() in gpu process fail.
> However, it's little bit overhead for runtime. In addition,
> bpf_gpu_policy_linux.cc executes dlopen(libva). "#if" makes Linux avoid
> this
> overhead.
>

But no overhead is saved if the blacklist is working, right?

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

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

[dshwang, 2014-03-06 19:32:30]

On 2014/03/06 19:18:21, Ami Fischman wrote:
> We don't usually put in multiple layers of protection against foot-shooting
> :)
> The blacklist is the way we make features available or not.  I think it
> would be a mistake to attempt to layer additional stopgaps like the #if
> here.  In particular it makes it a lot harder to be sure that all codepaths
> do the same thing (e.g. multiple test binaries, ppapi plugins, etc).

Thank you for good explanation. I'll review #if. Linux and Mac would be the same
code to other in gpu_data_manager_impl_private.cc

I used the "#if" to preserve this logic. However, this additional protection can
be removed as you mentioned.
  75 bool IsAcceleratedVideoDecodeEnabled() {
  76   // Accelerated video decode is currently enabled on Chrome OS,	
  77   // but not on Linux: crbug.com/137247.	
  78   bool is_enabled = IsChromeOS();	
  79 	
  80   const CommandLine& command_line = *CommandLine::ForCurrentProcess();
  81   is_enabled &=
  82       !command_line.HasSwitch(switches::kDisableAcceleratedVideoDecode);	
  83 	
  84   return is_enabled;	
  85 }

> But no overhead is saved if the blacklist is working, right?

Right.

[dshwang, 2014-03-06 19:33:01]

On 2014/03/06 19:32:30, dshwang wrote:
> Thank you for good explanation. I'll review #if. Linux and Mac would be the
same

s/review/remove/

[dshwang, 2014-03-07 14:43:45]

I update code and description as your comment. Could you review again?

[Ami GONE FROM CHROMIUM, 2014-03-07 22:29:44]

LGTM assuming on linux the sandbox doesn't try to load the cros-only libs; i.e.
that the cmdline flag is set by the blacklist-driven code _before_ the sandbox
code runs.

[dshwang, 2014-03-10 11:43:06]

On 2014/03/07 22:29:44, Ami Fischman wrote:
> LGTM assuming on linux the sandbox doesn't try to load the cros-only libs;
i.e.
> that the cmdline flag is set by the blacklist-driven code _before_ the sandbox
> code runs.

blacklist-drive code is executed before gpu process launch. So, the time is
absolutely before gpu sandboxing.


bool GpuProcessHost::LaunchGpuProcess(const std::string& channel_id) {
  ....

  GpuDataManagerImpl::GetInstance()->AppendGpuCommandLine(cmd_line);   <- HERE

  ....

  process_->Launch(
#if defined(OS_WIN)
      new GpuSandboxedProcessLauncherDelegate(cmd_line),
      false,
#elif defined(OS_POSIX)
      false,
      base::EnvironmentMap(),
#endif
      cmd_line);
  process_launched_ = true;

  UMA_HISTOGRAM_ENUMERATION("GPU.GPUProcessLifetimeEvents",
                            LAUNCHED, GPU_PROCESS_LIFETIME_EVENT_MAX);
  return true;
}

[dshwang, 2014-03-10 11:43:11]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2014-03-10 11:43:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/176883018/13...

[commit-bot: I haz the power, 2014-03-10 13:00:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/176883018/13...

[commit-bot: I haz the power, 2014-03-10 14:18:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/176883018/13...

[commit-bot: I haz the power, 2014-03-10 15:24:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/176883018/13...

[commit-bot: I haz the power, 2014-03-10 17:33:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/176883018/13...

[commit-bot: I haz the power, 2014-03-10 17:59:07]

Change committed as 255968