Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2173593002:
[Chromecast] Limit number of concurrent audio and video decoders (Closed)
|
Created:
4 years, 5 months ago by halliwell Modified:
4 years, 5 months ago CC:
chromium-reviews, alokp+watch_chromium.org, lcwu+watch_chromium.org, halliwell+watch_chromium.org, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Chromecast] Limit number of concurrent audio and video decoders
Currently, an app can exceed the number of decoders that vendor backends
support (for example, by creating multiple video elements), leading to
unpredictable behaviour including crashes. While most apps don't do
this, we have seen some problems crop up e.g. when ads are injected into
an existing page.
For now, restrict to 1 video decoder and 2 audio decoders.
BUG=internal b/26910920
Committed: https://crrev.com/54e165ed71447f2d66ffbbc29f69569f9b332084
Cr-Commit-Position: refs/heads/master@{#407215}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Increment/decrement API #
Total comments: 10
Patch Set 3 : use enum for decoder type #
Messages
Total messages: 18 (6 generated)
Description was changed from ========== [Chromecast] Limit number of concurrent audio and video decoders Currently, an app can exceed the number of decoders that vendor backends support (for example, by creating multiple video elements), leading to unpredictable behaviour including crashes. While most apps don't do this, we have seen some problems crop up e.g. when ads are injected into an existing page. For now, restrict to 1 video decoder and 2 audio decoders. BUG=internal b/26910920 ========== to ========== [Chromecast] Limit number of concurrent audio and video decoders Currently, an app can exceed the number of decoders that vendor backends support (for example, by creating multiple video elements), leading to unpredictable behaviour including crashes. While most apps don't do this, we have seen some problems crop up e.g. when ads are injected into an existing page. For now, restrict to 1 video decoder and 2 audio decoders. BUG=internal b/26910920 ==========
halliwell@chromium.org changed reviewers: + alokp@chromium.org, kmackay@chromium.org
https://codereview.chromium.org/2173593002/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/media_pipeline_backend_manager.cc (right): https://codereview.chromium.org/2173593002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/media_pipeline_backend_manager.cc:46: bool MediaPipelineBackendManager::CanCreateAudioDecoder( The function name is a bit weird. In addition to replying yes it also assumes that the decoder has been created. Would it make more sense to: 1. Move the decoder count for each backend from BackendInfo into MediaPipelineBackendWrapper? 2. Keep the global decoder counts here but expose functions to increment/decrement audio/video decoder count. 3. Let MediaPipelineBackendWrapper decide whether it should create decoder or not. That way you can also handle the case when one of the decoders is deleted from a backend (which you do not seem to handle currently)
https://codereview.chromium.org/2173593002/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/media_pipeline_backend_manager.cc (right): https://codereview.chromium.org/2173593002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/media_pipeline_backend_manager.cc:46: bool MediaPipelineBackendManager::CanCreateAudioDecoder( On 2016/07/22 04:59:46, alokp wrote: > The function name is a bit weird. In addition to replying yes it also assumes > that the decoder has been created. > > Would it make more sense to: > 1. Move the decoder count for each backend from BackendInfo into > MediaPipelineBackendWrapper? > 2. Keep the global decoder counts here but expose functions to > increment/decrement audio/video decoder count. > 3. Let MediaPipelineBackendWrapper decide whether it should create decoder or > not. > > That way you can also handle the case when one of the decoders is deleted from a > backend (which you do not seem to handle currently) Agreed :) I moved the count into the wrapper (actually just a flag, wrappers don't currently support multiple decoders) and rephrased the API as increment/decrement.
lgtm https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/media_pipeline_backend_manager.cc (right): https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/media_pipeline_backend_manager.cc:46: bool MediaPipelineBackendManager::IncrementAudioDecoderCount() { nit: You may want to merge the implementation for audio/video by passing decoder type into Increment/DecrementDecoderCount(DecoderType). https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/media_pipeline_backend_manager.cc:70: audio_decoder_count_--; DCHECK(audio_decoder_count_ > 0) https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/media_pipeline_backend_manager.cc:75: video_decoder_count_--; ditto
https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/media_pipeline_backend_wrapper.cc (right): https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/media_pipeline_backend_wrapper.cc:34: backend_manager_->DecrementVideoDecoderCount(); Can't we have multiple video decoders (for DolbyVision?)
https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/media_pipeline_backend_wrapper.cc (right): https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/media_pipeline_backend_wrapper.cc:34: backend_manager_->DecrementVideoDecoderCount(); On 2016/07/22 16:50:07, kmackay wrote: > Can't we have multiple video decoders (for DolbyVision?) I think, since the API allows you to create multiple video/audio decoders from a single backend instance, you should track how many have been created in this class, and then decrement by N in the destructor here.
https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/media_pipeline_backend_manager.cc (right): https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/media_pipeline_backend_manager.cc:46: bool MediaPipelineBackendManager::IncrementAudioDecoderCount() { On 2016/07/22 16:46:44, alokp wrote: > nit: You may want to merge the implementation for audio/video by passing decoder > type into Increment/DecrementDecoderCount(DecoderType). Done. https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/media_pipeline_backend_manager.cc:70: audio_decoder_count_--; On 2016/07/22 16:46:44, alokp wrote: > DCHECK(audio_decoder_count_ > 0) Done. https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/media_pipeline_backend_manager.cc:75: video_decoder_count_--; On 2016/07/22 16:46:44, alokp wrote: > ditto Done. https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/media_pipeline_backend_wrapper.cc (right): https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/media_pipeline_backend_wrapper.cc:34: backend_manager_->DecrementVideoDecoderCount(); On 2016/07/22 16:50:07, kmackay wrote: > Can't we have multiple video decoders (for DolbyVision?) DV is handled by a single video decoder. You can see the single video decoder assumption baked into MediaPipelinImpl. https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/media_pipeline_backend_wrapper.cc:34: backend_manager_->DecrementVideoDecoderCount(); On 2016/07/22 16:53:08, kmackay wrote: > On 2016/07/22 16:50:07, kmackay wrote: > > Can't we have multiple video decoders (for DolbyVision?) > > I think, since the API allows you to create multiple video/audio decoders from a > single backend instance, you should track how many have been created in this > class, and then decrement by N in the destructor here. Although the API does, the implementation doesn't allow multiple audio decoders (see audio_decoder_wrapper_ here). I added a DCHECK to enforce a similar thing for video, which matches current usage. i.e. if we want to support multiple decoders in a single backend in future, we will have other work to do anyway. At that point we can change this to count them as you suggest.
On 2016/07/22 17:16:30, halliwell wrote: > https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... > File chromecast/media/cma/backend/media_pipeline_backend_manager.cc (right): > > https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... > chromecast/media/cma/backend/media_pipeline_backend_manager.cc:46: bool > MediaPipelineBackendManager::IncrementAudioDecoderCount() { > On 2016/07/22 16:46:44, alokp wrote: > > nit: You may want to merge the implementation for audio/video by passing > decoder > > type into Increment/DecrementDecoderCount(DecoderType). > > Done. > > https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... > chromecast/media/cma/backend/media_pipeline_backend_manager.cc:70: > audio_decoder_count_--; > On 2016/07/22 16:46:44, alokp wrote: > > DCHECK(audio_decoder_count_ > 0) > > Done. > > https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... > chromecast/media/cma/backend/media_pipeline_backend_manager.cc:75: > video_decoder_count_--; > On 2016/07/22 16:46:44, alokp wrote: > > ditto > > Done. > > https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... > File chromecast/media/cma/backend/media_pipeline_backend_wrapper.cc (right): > > https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... > chromecast/media/cma/backend/media_pipeline_backend_wrapper.cc:34: > backend_manager_->DecrementVideoDecoderCount(); > On 2016/07/22 16:50:07, kmackay wrote: > > Can't we have multiple video decoders (for DolbyVision?) > > DV is handled by a single video decoder. You can see the single video decoder > assumption baked into MediaPipelinImpl. > > https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... > chromecast/media/cma/backend/media_pipeline_backend_wrapper.cc:34: > backend_manager_->DecrementVideoDecoderCount(); > On 2016/07/22 16:53:08, kmackay wrote: > > On 2016/07/22 16:50:07, kmackay wrote: > > > Can't we have multiple video decoders (for DolbyVision?) > > > > I think, since the API allows you to create multiple video/audio decoders from > a > > single backend instance, you should track how many have been created in this > > class, and then decrement by N in the destructor here. > > Although the API does, the implementation doesn't allow multiple audio decoders > (see audio_decoder_wrapper_ here). I added a DCHECK to enforce a similar thing > for video, which matches current usage. > > i.e. if we want to support multiple decoders in a single backend in future, we > will have other work to do anyway. At that point we can change this to count > them as you suggest. lgtm. If we don't need multiple audio/video decoders per backend for any reason, maybe we should make that part of the API (ie, document it in chromecast/public/media/media_pipeline_backend.h). It would simplify the backend implementation too.
On 2016/07/22 17:20:49, kmackay wrote: > On 2016/07/22 17:16:30, halliwell wrote: > > > https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... > > File chromecast/media/cma/backend/media_pipeline_backend_manager.cc (right): > > > > > https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... > > chromecast/media/cma/backend/media_pipeline_backend_manager.cc:46: bool > > MediaPipelineBackendManager::IncrementAudioDecoderCount() { > > On 2016/07/22 16:46:44, alokp wrote: > > > nit: You may want to merge the implementation for audio/video by passing > > decoder > > > type into Increment/DecrementDecoderCount(DecoderType). > > > > Done. > > > > > https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... > > chromecast/media/cma/backend/media_pipeline_backend_manager.cc:70: > > audio_decoder_count_--; > > On 2016/07/22 16:46:44, alokp wrote: > > > DCHECK(audio_decoder_count_ > 0) > > > > Done. > > > > > https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... > > chromecast/media/cma/backend/media_pipeline_backend_manager.cc:75: > > video_decoder_count_--; > > On 2016/07/22 16:46:44, alokp wrote: > > > ditto > > > > Done. > > > > > https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... > > File chromecast/media/cma/backend/media_pipeline_backend_wrapper.cc (right): > > > > > https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... > > chromecast/media/cma/backend/media_pipeline_backend_wrapper.cc:34: > > backend_manager_->DecrementVideoDecoderCount(); > > On 2016/07/22 16:50:07, kmackay wrote: > > > Can't we have multiple video decoders (for DolbyVision?) > > > > DV is handled by a single video decoder. You can see the single video decoder > > assumption baked into MediaPipelinImpl. > > > > > https://codereview.chromium.org/2173593002/diff/20001/chromecast/media/cma/ba... > > chromecast/media/cma/backend/media_pipeline_backend_wrapper.cc:34: > > backend_manager_->DecrementVideoDecoderCount(); > > On 2016/07/22 16:53:08, kmackay wrote: > > > On 2016/07/22 16:50:07, kmackay wrote: > > > > Can't we have multiple video decoders (for DolbyVision?) > > > > > > I think, since the API allows you to create multiple video/audio decoders > from > > a > > > single backend instance, you should track how many have been created in this > > > class, and then decrement by N in the destructor here. > > > > Although the API does, the implementation doesn't allow multiple audio > decoders > > (see audio_decoder_wrapper_ here). I added a DCHECK to enforce a similar > thing > > for video, which matches current usage. > > > > i.e. if we want to support multiple decoders in a single backend in future, we > > will have other work to do anyway. At that point we can change this to count > > them as you suggest. > > lgtm. If we don't need multiple audio/video decoders per backend for any reason, > maybe we should make that part of the API (ie, document it in > chromecast/public/media/media_pipeline_backend.h). It would simplify the backend > implementation too. Agreed.
The CQ bit was checked by halliwell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alokp@chromium.org Link to the patchset: https://codereview.chromium.org/2173593002/#ps40001 (title: "use enum for decoder type")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Limit number of concurrent audio and video decoders Currently, an app can exceed the number of decoders that vendor backends support (for example, by creating multiple video elements), leading to unpredictable behaviour including crashes. While most apps don't do this, we have seen some problems crop up e.g. when ads are injected into an existing page. For now, restrict to 1 video decoder and 2 audio decoders. BUG=internal b/26910920 ========== to ========== [Chromecast] Limit number of concurrent audio and video decoders Currently, an app can exceed the number of decoders that vendor backends support (for example, by creating multiple video elements), leading to unpredictable behaviour including crashes. While most apps don't do this, we have seen some problems crop up e.g. when ads are injected into an existing page. For now, restrict to 1 video decoder and 2 audio decoders. BUG=internal b/26910920 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Limit number of concurrent audio and video decoders Currently, an app can exceed the number of decoders that vendor backends support (for example, by creating multiple video elements), leading to unpredictable behaviour including crashes. While most apps don't do this, we have seen some problems crop up e.g. when ads are injected into an existing page. For now, restrict to 1 video decoder and 2 audio decoders. BUG=internal b/26910920 ========== to ========== [Chromecast] Limit number of concurrent audio and video decoders Currently, an app can exceed the number of decoders that vendor backends support (for example, by creating multiple video elements), leading to unpredictable behaviour including crashes. While most apps don't do this, we have seen some problems crop up e.g. when ads are injected into an existing page. For now, restrict to 1 video decoder and 2 audio decoders. BUG=internal b/26910920 Committed: https://crrev.com/54e165ed71447f2d66ffbbc29f69569f9b332084 Cr-Commit-Position: refs/heads/master@{#407215} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/54e165ed71447f2d66ffbbc29f69569f9b332084 Cr-Commit-Position: refs/heads/master@{#407215} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
