Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10824019:
Tweak the GPU process sandbox to allow accelerated video decode. (Closed)
|
Created:
8 years, 5 months ago by Jorge Lucangeli Obes Modified:
8 years, 5 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, jln+watch_chromium.org, apatrick_chromium Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionTweak the GPU process sandbox to allow accelerated video decode.
BUG=127664
TEST=Accelerated .mp4 video works on Chrome OS.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148649
Patch Set 1 #Patch Set 2 : Properly scope k965DrvVideoPath_64. #Patch Set 3 : Fix incorrect path for shared object. #
Total comments: 8
Patch Set 4 : Addressed jln's comments. #
Total comments: 2
Patch Set 5 : Added TODO as per piman's comment. #Patch Set 6 : Rebased over VA decode flag change. #
Total comments: 8
Patch Set 7 : Addressed jln's comments. #Messages
Total messages: 28 (0 generated)
PTAL
https://chromiumcodereview.appspot.com/10824019/diff/7001/content/common/sand... File content/common/sandbox_init_linux.cc (right): https://chromiumcodereview.appspot.com/10824019/diff/7001/content/common/sand... content/common/sandbox_init_linux.cc:150: bool EnabledAcceleratedVideoDecode() { Rename to AcceleratedVideoDecodeIsEnabled() to better match the general style ? https://chromiumcodereview.appspot.com/10824019/diff/7001/content/common/sand... content/common/sandbox_init_linux.cc:280: if (EnabledAcceleratedVideoDecode()) Style: please add braces around these multi lines statements. https://chromiumcodereview.appspot.com/10824019/diff/7001/content/common/sand... content/common/sandbox_init_linux.cc:282: // dup()'ing an already open fd does not work. Can you add a comment along the lines of: "This essentially disables the sandbox for security purposes but allows us to properly test it until a proper fix is developed." ? https://chromiumcodereview.appspot.com/10824019/diff/7001/content/common/sand... content/common/sandbox_init_linux.cc:403: "/usr/lib64/va/drivers/i965_drv_video.so"; This should be scoped in the if with the dlopen below. More importantly: is there any chance that on the target platform this path would be either in LD_LIBRARY_PATH or in ld.so.conf ? It would be nice to not have to include the full path.
PTAL https://chromiumcodereview.appspot.com/10824019/diff/7001/content/common/sand... File content/common/sandbox_init_linux.cc (right): https://chromiumcodereview.appspot.com/10824019/diff/7001/content/common/sand... content/common/sandbox_init_linux.cc:150: bool EnabledAcceleratedVideoDecode() { On 2012/07/25 23:47:56, Julien Tinnes wrote: > Rename to AcceleratedVideoDecodeIsEnabled() to better match the general style ? Done. https://chromiumcodereview.appspot.com/10824019/diff/7001/content/common/sand... content/common/sandbox_init_linux.cc:280: if (EnabledAcceleratedVideoDecode()) On 2012/07/25 23:47:56, Julien Tinnes wrote: > Style: please add braces around these multi lines statements. Done. https://chromiumcodereview.appspot.com/10824019/diff/7001/content/common/sand... content/common/sandbox_init_linux.cc:282: // dup()'ing an already open fd does not work. On 2012/07/25 23:47:56, Julien Tinnes wrote: > Can you add a comment along the lines of: "This essentially disables the sandbox > for security purposes but allows us to properly test it until a proper fix is > developed." ? Done. https://chromiumcodereview.appspot.com/10824019/diff/7001/content/common/sand... content/common/sandbox_init_linux.cc:403: "/usr/lib64/va/drivers/i965_drv_video.so"; On 2012/07/25 23:47:56, Julien Tinnes wrote: > This should be scoped in the if with the dlopen below. > > More importantly: is there any chance that on the target platform this path > would be either in LD_LIBRARY_PATH or in ld.so.conf ? It would be nice to not > have to include the full path. Unfortunately no. libva uses the full path when it dlopen()'s.
LGTM
apatrick, piman: OWNERS LGTM for gpu_process_host.cc? Forwarding the --enable-accelerated-video-decode flag to the GPU process, needed to tweak the sandbox so that it doesn't break accelerated video decoding on Chrome OS.
https://chromiumcodereview.appspot.com/10824019/diff/5004/content/common/sand... File content/common/sandbox_init_linux.cc (right): https://chromiumcodereview.appspot.com/10824019/diff/5004/content/common/sand... content/common/sandbox_init_linux.cc:415: "/usr/lib64/va/drivers/i965_drv_video.so"; What about i915 (Alex, etc.)?
On 2012/07/26 01:09:52, piman wrote: > https://chromiumcodereview.appspot.com/10824019/diff/5004/content/common/sand... > File content/common/sandbox_init_linux.cc (right): > > https://chromiumcodereview.appspot.com/10824019/diff/5004/content/common/sand... > content/common/sandbox_init_linux.cc:415: > "/usr/lib64/va/drivers/i965_drv_video.so"; > What about i915 (Alex, etc.)? The file is still called "i965_drv_video.so" on Alex, although of course it doesn't live in lib64. But were not enabling the sandbox on 32-bit architectures so far.
LGTM for now, but please add a todo. https://chromiumcodereview.appspot.com/10824019/diff/5004/content/common/sand... File content/common/sandbox_init_linux.cc (right): https://chromiumcodereview.appspot.com/10824019/diff/5004/content/common/sand... content/common/sandbox_init_linux.cc:415: "/usr/lib64/va/drivers/i965_drv_video.so"; On 2012/07/26 01:09:52, piman wrote: > What about i915 (Alex, etc.)? Mmh, we're still whipping it as x86 for now, so maybe it's a non-issue and we can add a TODO? This will break / be wrong later as we bring up new platforms. Also on non-chromeos.
On 2012/07/26 01:13:05, piman wrote: > LGTM for now, but please add a todo. > > https://chromiumcodereview.appspot.com/10824019/diff/5004/content/common/sand... > File content/common/sandbox_init_linux.cc (right): > > https://chromiumcodereview.appspot.com/10824019/diff/5004/content/common/sand... > content/common/sandbox_init_linux.cc:415: > "/usr/lib64/va/drivers/i965_drv_video.so"; > On 2012/07/26 01:09:52, piman wrote: > > What about i915 (Alex, etc.)? > > Mmh, we're still whipping it as x86 for now, so maybe it's a non-issue and we > can add a TODO? > This will break / be wrong later as we bring up new platforms. Also on > non-chromeos. I had assumed this was not enabled on Linux Chrome: http://code.google.com/searchframe#wZuuyuB8jKQ/chromium/src/content/common/gp... #if defined(OS_WIN) ... #elif defined(OS_CHROMEOS) && defined(ARCH_CPU_ARMEL) ... #elif defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY) ... #elif defined(OS_MACOSX) ... #else NOTIMPLEMENTED() << "HW video decode acceleration not available."; NotifyError(media::VideoDecodeAccelerator::PLATFORM_FAILURE); return; #endif
On Wed, Jul 25, 2012 at 6:26 PM, <jorgelo@chromium.org> wrote: > On 2012/07/26 01:13:05, piman wrote: > >> LGTM for now, but please add a todo. >> > > > https://chromiumcodereview.**appspot.com/10824019/diff/** > 5004/content/common/sandbox_**init_linux.cc<https://chromiumcodereview.appspot.com/10824019/diff/5004/content/common/sandbox_init_linux.cc> > >> File content/common/sandbox_init_**linux.cc (right): >> > > > https://chromiumcodereview.**appspot.com/10824019/diff/** > 5004/content/common/sandbox_**init_linux.cc#newcode415<https://chromiumcodereview.appspot.com/10824019/diff/5004/content/common/sandbox_init_linux.cc#newcode415> > >> content/common/sandbox_init_**linux.cc:415: >> "/usr/lib64/va/drivers/i965_**drv_video.so"; >> On 2012/07/26 01:09:52, piman wrote: >> > What about i915 (Alex, etc.)? >> > > Mmh, we're still whipping it as x86 for now, so maybe it's a non-issue >> and we >> can add a TODO? >> This will break / be wrong later as we bring up new platforms. Also on >> non-chromeos. >> > > I had assumed this was not enabled on Linux Chrome: > Yet? Those ifdefs will go away hopefully at some point, in favor for flags. Antoine > > http://code.google.com/**searchframe#wZuuyuB8jKQ/** > chromium/src/content/common/**gpu/media/gpu_video_decode_** > accelerator.cc&exact_package=**chromiumos&q=**VaapiVideoDecodeAccelerator& > **type=cs&l=163<http://code.google.com/searchframe#wZuuyuB8jKQ/chromium/src/content/common/gpu/media/gpu_video_decode_accelerator.cc&exact_package=chromiumos&q=VaapiVideoDecodeAccelerator&type=cs&l=163> > > #if defined(OS_WIN) > ... > #elif defined(OS_CHROMEOS) && defined(ARCH_CPU_ARMEL) > ... > #elif defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY) > ... > #elif defined(OS_MACOSX) > ... > #else > NOTIMPLEMENTED() << "HW video decode acceleration not available."; > NotifyError(media::**VideoDecodeAccelerator::**PLATFORM_FAILURE); > return; > #endif > > https://chromiumcodereview.**appspot.com/10824019/<https://chromiumcodereview... >
On 2012/07/26 01:28:04, piman wrote: > On Wed, Jul 25, 2012 at 6:26 PM, <mailto:jorgelo@chromium.org> wrote: > > > On 2012/07/26 01:13:05, piman wrote: > > > >> LGTM for now, but please add a todo. > >> > > > > > > https://chromiumcodereview.**appspot.com/10824019/diff/** > > > 5004/content/common/sandbox_**init_linux.cc<https://chromiumcodereview.appspot.com/10824019/diff/5004/content/common/sandbox_init_linux.cc> > > > >> File content/common/sandbox_init_**linux.cc (right): > >> > > > > > > https://chromiumcodereview.**appspot.com/10824019/diff/** > > > 5004/content/common/sandbox_**init_linux.cc#newcode415<https://chromiumcodereview.appspot.com/10824019/diff/5004/content/common/sandbox_init_linux.cc#newcode415> > > > >> content/common/sandbox_init_**linux.cc:415: > >> "/usr/lib64/va/drivers/i965_**drv_video.so"; > >> On 2012/07/26 01:09:52, piman wrote: > >> > What about i915 (Alex, etc.)? > >> > > > > Mmh, we're still whipping it as x86 for now, so maybe it's a non-issue > >> and we > >> can add a TODO? > >> This will break / be wrong later as we bring up new platforms. Also on > >> non-chromeos. > >> > > > > I had assumed this was not enabled on Linux Chrome: > > > > Yet? Those ifdefs will go away hopefully at some point, in favor for flags. > Absolutely, it's clear that the ifdefs should go away. Just checking that my understanding of that file was correct. > > > > http://code.google.com/**searchframe#wZuuyuB8jKQ/** > > chromium/src/content/common/**gpu/media/gpu_video_decode_** > > accelerator.cc&exact_package=**chromiumos&q=**VaapiVideoDecodeAccelerator& > > > **type=cs&l=163<http://code.google.com/searchframe#wZuuyuB8jKQ/chromium/src/content/common/gpu/media/gpu_video_decode_accelerator.cc&exact_package=chromiumos&q=VaapiVideoDecodeAccelerator&type=cs&l=163> > > > > #if defined(OS_WIN) > > ... > > #elif defined(OS_CHROMEOS) && defined(ARCH_CPU_ARMEL) > > ... > > #elif defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY) > > ... > > #elif defined(OS_MACOSX) > > ... > > #else > > NOTIMPLEMENTED() << "HW video decode acceleration not available."; > > NotifyError(media::**VideoDecodeAccelerator::**PLATFORM_FAILURE); > > return; > > #endif > > > > > https://chromiumcodereview.**appspot.com/10824019/%3Chttps://chromiumcoderevi...> > >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jorgelo@chromium.org/10824019/11002
Try job failure for 10824019-11002 (retry) on android for steps "compile, build" (clobber build). It's a second try, previously, steps "compile, build" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...
On 2012/07/26 05:25:35, I haz the power (commit-bot) wrote: > Try job failure for 10824019-11002 (retry) on android for steps "compile, build" > (clobber build). > It's a second try, previously, steps "compile, build" failed. > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu... Hmmm looks like the flag got flipped between the trybot runs and the CQ. Will rebase tomorrow.
Please take another look. Mostly clean rebase over the flag switch, keeping the same semantics.
lgtm
https://chromiumcodereview.appspot.com/10824019/diff/6004/content/common/sand... File content/common/sandbox_init_linux.cc (right): https://chromiumcodereview.appspot.com/10824019/diff/6004/content/common/sand... content/common/sandbox_init_linux.cc:150: bool IsAcceleratedVideoDecodeEnabled() { Any chance that gpu/ could export a function like this instead? It looks like dangerous code duplication, doesn't it? https://chromiumcodereview.appspot.com/10824019/diff/6004/content/common/sand... content/common/sandbox_init_linux.cc:154: #if defined(OS_CHROMEOS) Please use IsChromeOS(). https://chromiumcodereview.appspot.com/10824019/diff/6004/content/common/sand... content/common/sandbox_init_linux.cc:159: is_enabled &= !command_line.HasSwitch( please, use && here so that we benefit from lazy evaluation.
https://chromiumcodereview.appspot.com/10824019/diff/6004/content/common/sand... File content/common/sandbox_init_linux.cc (right): https://chromiumcodereview.appspot.com/10824019/diff/6004/content/common/sand... content/common/sandbox_init_linux.cc:150: bool IsAcceleratedVideoDecodeEnabled() { On 2012/07/26 17:53:54, Julien Tinnes wrote: > Any chance that gpu/ could export a function like this instead? It looks like > dangerous code duplication, doesn't it? The code does not live in gpu/, it lives in content/renderer/render_view_impl.cc. So I don't think so. It is duplicated, but the only thing that can change is Linux being enabled, and we should be informed of that in the linked bug. https://chromiumcodereview.appspot.com/10824019/diff/6004/content/common/sand... content/common/sandbox_init_linux.cc:154: #if defined(OS_CHROMEOS) On 2012/07/26 17:53:54, Julien Tinnes wrote: > Please use IsChromeOS(). Done. https://chromiumcodereview.appspot.com/10824019/diff/6004/content/common/sand... content/common/sandbox_init_linux.cc:159: is_enabled &= !command_line.HasSwitch( On 2012/07/26 17:53:54, Julien Tinnes wrote: > please, use && here so that we benefit from lazy evaluation. Done.
content/browser/gpu/gpu_process_host.cc LGTM
LGTM https://chromiumcodereview.appspot.com/10824019/diff/6004/content/common/sand... File content/common/sandbox_init_linux.cc (right): https://chromiumcodereview.appspot.com/10824019/diff/6004/content/common/sand... content/common/sandbox_init_linux.cc:150: bool IsAcceleratedVideoDecodeEnabled() { On 2012/07/26 18:33:36, Jorge Lucangeli Obes wrote: > On 2012/07/26 17:53:54, Julien Tinnes wrote: > > Any chance that gpu/ could export a function like this instead? It looks like > > dangerous code duplication, doesn't it? > > The code does not live in gpu/, it lives in > content/renderer/render_view_impl.cc. So I don't think so. > It is duplicated, but the only thing that can change is Linux being enabled, and > we should be informed of that in the linked bug. Ok, we don't want to start refactoring code just for this, but could you at least add a comment in renderer_view_impl.cc around use_accelerated_video_decode and link to the bug?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jorgelo@chromium.org/10824019/2005
jln: https://chromiumcodereview.appspot.com/10821049/ "https://chromiumcodereview.appspot.com/10821049/"
That should have been: "Add a comment for Linux video decode acceleration."
https://chromiumcodereview.appspot.com/10824019/diff/6004/content/common/sand... File content/common/sandbox_init_linux.cc (right): https://chromiumcodereview.appspot.com/10824019/diff/6004/content/common/sand... content/common/sandbox_init_linux.cc:154: #if defined(OS_CHROMEOS) On 2012/07/26 18:33:36, Jorge Lucangeli Obes wrote: > On 2012/07/26 17:53:54, Julien Tinnes wrote: > > Please use IsChromeOS(). > > Done. FYI, IsChromeOS is different from #ifdef OS_CHROMEOS. IsChromeOS is a run-time "detection" of whether you're running on a chrome OS device - in practice, if $USER==chronos OS_CHROMEOS is a compile-time define that's set if chromeos=1 is set in GYP_DEFINES. We can, and regularly do (including on bots) run a chromeos=1 build on linux, where OS_CHROMEOS is defined but IsChromeOS is false. Be sure to take that into account here.
On 2012/07/26 20:33:52, piman wrote: > https://chromiumcodereview.appspot.com/10824019/diff/6004/content/common/sand... > File content/common/sandbox_init_linux.cc (right): > > https://chromiumcodereview.appspot.com/10824019/diff/6004/content/common/sand... > content/common/sandbox_init_linux.cc:154: #if defined(OS_CHROMEOS) > On 2012/07/26 18:33:36, Jorge Lucangeli Obes wrote: > > On 2012/07/26 17:53:54, Julien Tinnes wrote: > > > Please use IsChromeOS(). > > > > Done. > > FYI, IsChromeOS is different from #ifdef OS_CHROMEOS. > > IsChromeOS is a run-time "detection" of whether you're running on a chrome OS > device - in practice, if $USER==chronos > OS_CHROMEOS is a compile-time define that's set if chromeos=1 is set in > GYP_DEFINES. > We can, and regularly do (including on bots) run a chromeos=1 build on linux, > where OS_CHROMEOS is defined but IsChromeOS is false. Be sure to take that into > account here. We have a private IsChromeOS function in sandbox_init_linux.cc: inline bool IsChromeOS() { #if defined(OS_CHROMEOS) return true; #else return false; #endif } So we should be OK.
On 2012/07/26 20:36:37, Jorge Lucangeli Obes wrote: > On 2012/07/26 20:33:52, piman wrote: > > > https://chromiumcodereview.appspot.com/10824019/diff/6004/content/common/sand... > > File content/common/sandbox_init_linux.cc (right): > > > > > https://chromiumcodereview.appspot.com/10824019/diff/6004/content/common/sand... > > content/common/sandbox_init_linux.cc:154: #if defined(OS_CHROMEOS) > > On 2012/07/26 18:33:36, Jorge Lucangeli Obes wrote: > > > On 2012/07/26 17:53:54, Julien Tinnes wrote: > > > > Please use IsChromeOS(). > > > > > > Done. > > > > FYI, IsChromeOS is different from #ifdef OS_CHROMEOS. > > > > IsChromeOS is a run-time "detection" of whether you're running on a chrome OS > > device - in practice, if $USER==chronos > > OS_CHROMEOS is a compile-time define that's set if chromeos=1 is set in > > GYP_DEFINES. > > We can, and regularly do (including on bots) run a chromeos=1 build on linux, > > where OS_CHROMEOS is defined but IsChromeOS is false. Be sure to take that > into > > account here. > > We have a private IsChromeOS function in sandbox_init_linux.cc: > > inline bool IsChromeOS() { > #if defined(OS_CHROMEOS) > return true; > #else > return false; > #endif > } > > So we should be OK. Ah, ok.
On 2012/07/26 20:40:26, piman wrote: > On 2012/07/26 20:36:37, Jorge Lucangeli Obes wrote: > > On 2012/07/26 20:33:52, piman wrote: > > > > > > https://chromiumcodereview.appspot.com/10824019/diff/6004/content/common/sand... > > > File content/common/sandbox_init_linux.cc (right): > > > > > > > > > https://chromiumcodereview.appspot.com/10824019/diff/6004/content/common/sand... > > > content/common/sandbox_init_linux.cc:154: #if defined(OS_CHROMEOS) > > > On 2012/07/26 18:33:36, Jorge Lucangeli Obes wrote: > > > > On 2012/07/26 17:53:54, Julien Tinnes wrote: > > > > > Please use IsChromeOS(). > > > > > > > > Done. > > > > > > FYI, IsChromeOS is different from #ifdef OS_CHROMEOS. > > > > > > IsChromeOS is a run-time "detection" of whether you're running on a chrome > OS > > > device - in practice, if $USER==chronos > > > OS_CHROMEOS is a compile-time define that's set if chromeos=1 is set in > > > GYP_DEFINES. > > > We can, and regularly do (including on bots) run a chromeos=1 build on > linux, > > > where OS_CHROMEOS is defined but IsChromeOS is false. Be sure to take that > > into > > > account here. > > > > We have a private IsChromeOS function in sandbox_init_linux.cc: > > > > inline bool IsChromeOS() { > > #if defined(OS_CHROMEOS) > > return true; > > #else > > return false; > > #endif > > } > > > > So we should be OK. > > Ah, ok. Yeah, the rationale is that this file is plagued by #ifdefs, and using this instead means that we ensure that all code compiles all the time (but still gets optimized-out when relevant).
Change committed as 148649 |
