Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
723993003:
Sandbox initialization for VideoToolbox. (Closed)
|
Created:
6 years, 1 month ago by sandersd (OOO until July 31) Modified:
6 years ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@vt_queue_frames Project:
chromium Visibility:
Public. |
DescriptionSandbox initialization for VideoToolbox.
Creates a VideoToolbox decompression session during sandbox startup so that VideoToolbox can load all of its libraries before the sandbox goes into effect.
Note: Once this CL is committed, VTVideoDecodeAccelerator can be enabled using only --ignore-gpu-blacklist.
BUG=133828
Committed: https://crrev.com/592cdf068b0e4dfabe9a6e44af55b3ee1e24fbc7
Cr-Commit-Position: refs/heads/master@{#305938}
Patch Set 1 #Patch Set 2 : Fix comment. #Patch Set 3 : Reduce code duplication. #
Total comments: 10
Patch Set 4 : Comment block for initialization function. #Patch Set 5 : Header comment #
Total comments: 8
Patch Set 6 : Logging #
Total comments: 10
Patch Set 7 : Nits. #Patch Set 8 : Rebase. #
Messages
Total messages: 28 (8 generated)
Patchset #2 (id:20001) has been deleted
jeremy@chromium.org changed reviewers: + jeremy@chromium.org
Thanks for looking into this! A couple of high level remarks: * Does this work on OSs from 10.6 onwards? * Can you use the perf trybots to see that doesn't regress startup time - http://www.chromium.org/developers/performance-try-bots ?
rsesek@chromium.org changed reviewers: + rsesek@chromium.org
On 2014/11/16 12:01:39, jeremy wrote: > Thanks for looking into this! > A couple of high level remarks: > * Does this work on OSs from 10.6 onwards? Depending on your definition of "work". It will fail gracefully (with a VLOG(1) message) for versions less than 10.9. There are two planned eventual fixes to eventually get 10.6 support: 1) Remove reiliance on CMVideoFormatDescriptionCreateFromH264ParameterSets(), instead generate the avcC data and configure the session manually. (This gives 10.8 support.) 2) Enable fallback to the PrivateFrameworks version of VideoToolbox. (Should work back to 10.6.) > * Can you use the perf trybots to see that doesn't regress startup time - > http://www.chromium.org/developers/performance-try-bots ? I'm not sure what the best benchmark is for GPU process startup. Assuming that the startup.*.blank_page choices actually measure that, I get: - cold: http://storage.googleapis.com/chromium-telemetry/html-results/results-2014-11... (no significant difference, 12ms absolute difference) - warm: http://storage.googleapis.com/chromium-telemetry/html-results/results-2014-11... (5ms worse)
jeremy: ping
sandersd@chromium.org changed reviewers: + dalecurtis@chromium.org
sandersd@chromium.org changed required reviewers: + dalecurtis@chromium.org
dalecurtis: Please review for non-sandbox related issues.
Can you please elaborate in the changelist description why you need to fake an initialization outside of the sandbox? https://codereview.chromium.org/723993003/diff/60001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/723993003/diff/60001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:88: // Create a decoding session. This seems a bit crazy, wouldn't we normally handle this during Initialize() with actual data?
https://codereview.chromium.org/723993003/diff/60001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/723993003/diff/60001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:88: // Create a decoding session. On 2014/11/20 23:08:33, DaleCurtis wrote: > This seems a bit crazy, wouldn't we normally handle this during Initialize() > with actual data? Yes, but we don't have actual data because no video is being decoded yet, and if we call it then it will be too late to initialize VideoToolbox. [Side note: actually no, it normally happens after the first call to Decode() because we don't get the initialization data in Initialize(). It's a problematic feature of the VDA API.] We just need some data (any data, as long as it is compatible with hardware decoding) that VideoToolbox will accept to create a decompression session.
https://codereview.chromium.org/723993003/diff/60001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/723993003/diff/60001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:88: // Create a decoding session. On 2014/11/21 00:04:52, sandersd wrote: > On 2014/11/20 23:08:33, DaleCurtis wrote: > > This seems a bit crazy, wouldn't we normally handle this during Initialize() > > with actual data? > > Yes, but we don't have actual data because no video is being decoded yet, and if > we call it then it will be too late to initialize VideoToolbox. > > [Side note: actually no, it normally happens after the first call to Decode() > because we don't get the initialization data in Initialize(). It's a problematic > feature of the VDA API.] > > We just need some data (any data, as long as it is compatible with hardware > decoding) that VideoToolbox will accept to create a decompression session. Is there any impact on the ability to play two videos at once?
https://codereview.chromium.org/723993003/diff/60001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/723993003/diff/60001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:88: // Create a decoding session. On 2014/11/21 02:04:13, DaleCurtis wrote: > On 2014/11/21 00:04:52, sandersd wrote: > > On 2014/11/20 23:08:33, DaleCurtis wrote: > > > This seems a bit crazy, wouldn't we normally handle this during Initialize() > > > with actual data? > > > > Yes, but we don't have actual data because no video is being decoded yet, and > if > > we call it then it will be too late to initialize VideoToolbox. > > > > [Side note: actually no, it normally happens after the first call to Decode() > > because we don't get the initialization data in Initialize(). It's a > problematic > > feature of the VDA API.] > > > > We just need some data (any data, as long as it is compatible with hardware > > decoding) that VideoToolbox will accept to create a decompression session. > > Is there any impact on the ability to play two videos at once? No, this initialization is called once per GPU process and is entirely separate from the per-stream initialization.
https://codereview.chromium.org/723993003/diff/60001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/723993003/diff/60001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:70: void InitializeVideoToolbox() { What's the impact on gpu process startup time with this call? https://codereview.chromium.org/723993003/diff/60001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:88: // Create a decoding session. On 2014/11/21 02:07:07, sandersd wrote: > On 2014/11/21 02:04:13, DaleCurtis wrote: > > On 2014/11/21 00:04:52, sandersd wrote: > > > On 2014/11/20 23:08:33, DaleCurtis wrote: > > > > This seems a bit crazy, wouldn't we normally handle this during > Initialize() > > > > with actual data? > > > > > > Yes, but we don't have actual data because no video is being decoded yet, > and > > if > > > we call it then it will be too late to initialize VideoToolbox. > > > > > > [Side note: actually no, it normally happens after the first call to > Decode() > > > because we don't get the initialization data in Initialize(). It's a > > problematic > > > feature of the VDA API.] > > > > > > We just need some data (any data, as long as it is compatible with hardware > > > decoding) that VideoToolbox will accept to create a decompression session. > > > > Is there any impact on the ability to play two videos at once? > > No, this initialization is called once per GPU process and is entirely separate > from the per-stream initialization. To be clear, you're running this here so that all the necessary library calls will be preloaded into the process prior to lockdown? Seems like that could fail in odd ways if this misses calls that say a 1080p high profile decoding session might use... If this is the only way to do this, you at least need a much stronger comment about what's happening here and how it could fail. https://codereview.chromium.org/723993003/diff/60001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:90: uint8_t sps[] = {0x67, 0x64, 0x00, 0x1e, 0xac, 0xd9, 0x80, 0xd4, 0x3d, 0xa1, Will the API let you const all these?
https://codereview.chromium.org/723993003/diff/60001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/723993003/diff/60001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:70: void InitializeVideoToolbox() { On 2014/11/21 19:01:02, DaleCurtis wrote: > What's the impact on gpu process startup time with this call? Based on the benchmarking I just did, the perf test I ran was not being helpful. On my MacBook Pro, a cold start is about 60ms and a warm start 30ms. https://codereview.chromium.org/723993003/diff/60001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:88: // Create a decoding session. On 2014/11/21 19:01:02, DaleCurtis wrote: > On 2014/11/21 02:07:07, sandersd wrote: > > On 2014/11/21 02:04:13, DaleCurtis wrote: > > > On 2014/11/21 00:04:52, sandersd wrote: > > > > On 2014/11/20 23:08:33, DaleCurtis wrote: > > > > > This seems a bit crazy, wouldn't we normally handle this during > > Initialize() > > > > > with actual data? > > > > > > > > Yes, but we don't have actual data because no video is being decoded yet, > > and > > > if > > > > we call it then it will be too late to initialize VideoToolbox. > > > > > > > > [Side note: actually no, it normally happens after the first call to > > Decode() > > > > because we don't get the initialization data in Initialize(). It's a > > > problematic > > > > feature of the VDA API.] > > > > > > > > We just need some data (any data, as long as it is compatible with > hardware > > > > decoding) that VideoToolbox will accept to create a decompression session. > > > > > > Is there any impact on the ability to play two videos at once? > > > > No, this initialization is called once per GPU process and is entirely > separate > > from the per-stream initialization. > > To be clear, you're running this here so that all the necessary library calls > will be preloaded into the process prior to lockdown? Seems like that could fail > in odd ways if this misses calls that say a 1080p high profile decoding session > might use... > > If this is the only way to do this, you at least need a much stronger comment > about what's happening here and how it could fail. Done. https://codereview.chromium.org/723993003/diff/60001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:90: uint8_t sps[] = {0x67, 0x64, 0x00, 0x1e, 0xac, 0xd9, 0x80, 0xd4, 0x3d, 0xa1, On 2014/11/21 19:01:02, DaleCurtis wrote: > Will the API let you const all these? Done.
https://codereview.chromium.org/723993003/diff/100001/content/common/gpu/medi... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/723993003/diff/100001/content/common/gpu/medi... content/common/gpu/media/vt_video_decode_accelerator.cc:76: // not documented), then VideoToolbox will fall back on software decoding Is there a way you record when this happens as a UMA statistic? Otherwise it seems this could happen and we'd never know. https://codereview.chromium.org/723993003/diff/100001/content/common/gpu/medi... content/common/gpu/media/vt_video_decode_accelerator.cc:115: LOG(ERROR) << "Failed to create CMVideoFormatDescription while " Check out OSSTATUS_LOG and OSSTATUS_DLOG. You can see example using in audio_manager_mac. I take it you're intentionally using LOG here? https://codereview.chromium.org/723993003/diff/100001/content/common/gpu/medi... content/common/gpu/media/vt_video_decode_accelerator.cc:137: VTDecompressionOutputCallbackRecord callback; Worth using = {0} to zero all potential fields? https://codereview.chromium.org/723993003/diff/100001/content/common/gpu/medi... content/common/gpu/media/vt_video_decode_accelerator.cc:153: } Is the session automatically dropped once all refs are lost? Do you need to close it cleanly? I see a VTDecompressionSessionInvalidate() method...
https://codereview.chromium.org/723993003/diff/100001/content/common/gpu/medi... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/723993003/diff/100001/content/common/gpu/medi... content/common/gpu/media/vt_video_decode_accelerator.cc:76: // not documented), then VideoToolbox will fall back on software decoding On 2014/11/21 21:00:49, DaleCurtis wrote: > Is there a way you record when this happens as a UMA statistic? Otherwise it > seems this could happen and we'd never know. It's possible, but is also expected when the resolution changes below the hardware decoder's limit. A better solution is to require hardware decoding and fail otherwise, but we can't do that until fallback to Chromium's software path works smoothly. (That is what RequireHardwareAcceleratedVideoDecoder configures; I don't use it in the regular initialization path though.) https://codereview.chromium.org/723993003/diff/100001/content/common/gpu/medi... content/common/gpu/media/vt_video_decode_accelerator.cc:115: LOG(ERROR) << "Failed to create CMVideoFormatDescription while " On 2014/11/21 21:00:49, DaleCurtis wrote: > Check out OSSTATUS_LOG and OSSTATUS_DLOG. You can see example using in > audio_manager_mac. I take it you're intentionally using LOG here? Done. https://codereview.chromium.org/723993003/diff/100001/content/common/gpu/medi... content/common/gpu/media/vt_video_decode_accelerator.cc:137: VTDecompressionOutputCallbackRecord callback; On 2014/11/21 21:00:49, DaleCurtis wrote: > Worth using = {0} to zero all potential fields? Done. (Although the regular path does not do this.) https://codereview.chromium.org/723993003/diff/100001/content/common/gpu/medi... content/common/gpu/media/vt_video_decode_accelerator.cc:153: } On 2014/11/21 21:00:49, DaleCurtis wrote: > Is the session automatically dropped once all refs are lost? Do you need to > close it cleanly? I see a VTDecompressionSessionInvalidate() method... It is, VTDecompressionSessionInvalidate() just lets you do it earlier.
Patchset #6 (id:120001) has been deleted
lgtm
A few very small nits, otherwise looks fine - I'd like rsesek@ to look at this too. https://codereview.chromium.org/723993003/diff/140001/content/common/gpu/medi... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/723993003/diff/140001/content/common/gpu/medi... content/common/gpu/media/vt_video_decode_accelerator.cc:44: static CFMutableDictionaryRef BuildImageConfig( return a ScopedCFTypeRef ? https://codereview.chromium.org/723993003/diff/140001/content/common/gpu/medi... content/common/gpu/media/vt_video_decode_accelerator.cc:45: CMVideoDimensions coded_dimensions) { nit: will this fit on the line above? https://codereview.chromium.org/723993003/diff/140001/content/common/gpu/medi... content/common/gpu/media/vt_video_decode_accelerator.cc:91: // TODO(sandersd): Fallback to PrivateFrameworks. Can you expand on this TODO a bit, why do we need PrivateFrameworks ? https://codereview.chromium.org/723993003/diff/140001/content/common/gpu/medi... content/common/gpu/media/vt_video_decode_accelerator.cc:119: content_common_gpu_media::UninitializeVt(); nit: I'd feel better if there was a scoped object that did this automagically but perhaps that's overkill in this case. https://codereview.chromium.org/723993003/diff/140001/content/common/gpu/medi... File content/common/gpu/media/vt_video_decode_accelerator.h (right): https://codereview.chromium.org/723993003/diff/140001/content/common/gpu/medi... content/common/gpu/media/vt_video_decode_accelerator.h:31: // Preload VideoToolbox libraries. nit: s/./, needed for sandbox warmup./
https://codereview.chromium.org/723993003/diff/140001/content/common/gpu/medi... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/723993003/diff/140001/content/common/gpu/medi... content/common/gpu/media/vt_video_decode_accelerator.cc:44: static CFMutableDictionaryRef BuildImageConfig( On 2014/11/23 10:29:16, jeremy wrote: > return a ScopedCFTypeRef ? Done. https://codereview.chromium.org/723993003/diff/140001/content/common/gpu/medi... content/common/gpu/media/vt_video_decode_accelerator.cc:45: CMVideoDimensions coded_dimensions) { On 2014/11/23 10:29:16, jeremy wrote: > nit: will this fit on the line above? Nope. https://codereview.chromium.org/723993003/diff/140001/content/common/gpu/medi... content/common/gpu/media/vt_video_decode_accelerator.cc:45: CMVideoDimensions coded_dimensions) { On 2014/11/23 10:29:16, jeremy wrote: > nit: will this fit on the line above? Nope. (And especially not anymore.) https://codereview.chromium.org/723993003/diff/140001/content/common/gpu/medi... content/common/gpu/media/vt_video_decode_accelerator.cc:91: // TODO(sandersd): Fallback to PrivateFrameworks. On 2014/11/23 10:29:16, jeremy wrote: > Can you expand on this TODO a bit, why do we need PrivateFrameworks ? Done. https://codereview.chromium.org/723993003/diff/140001/content/common/gpu/medi... content/common/gpu/media/vt_video_decode_accelerator.cc:119: content_common_gpu_media::UninitializeVt(); On 2014/11/23 10:29:16, jeremy wrote: > nit: I'd feel better if there was a scoped object that did this automagically > but perhaps that's overkill in this case. A good point, but I think it's overkill in this case where there are exactly two failure paths. I'll keep that in mind if more get added.
LGTM
LGTM
Patchset #8 (id:180001) has been deleted
The CQ bit was checked by sandersd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/723993003/200001
Message was sent while issue was closed.
Committed patchset #8 (id:200001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/592cdf068b0e4dfabe9a6e44af55b3ee1e24fbc7 Cr-Commit-Position: refs/heads/master@{#305938} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
