Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1306843003:
CmaMediaPipelineClient to watch media pipeline status (Closed)
|
Created:
5 years, 4 months ago by yucliu1 Modified:
5 years, 3 months ago CC:
chromium-reviews, gunsch+watch_chromium.org, lcwu+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. |
DescriptionCmaMediaPipelineClient to watch media pipeline status
cma pipeline use CmaMediaPipelineClient to create media pipeline
backend and report creation and deletion of media pipeline backend.
BUG=internal 20612516
Committed: https://crrev.com/b26f03db16a4752572e136d3c531234bfa45eb63
Cr-Commit-Position: refs/heads/master@{#346977}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Nit #Patch Set 3 : CmaMediaPipelineClient #
Total comments: 4
Patch Set 4 : DISALLOW_COPY_AND_ASSIGN for CmaMediaPipelineClient #Patch Set 5 : CastResource #
Total comments: 4
Patch Set 6 : Move CastResource to chromecast/cast #
Total comments: 32
Patch Set 7 : Move CastResource to chromecast/base #Patch Set 8 : Dummy callback in cma renderer #
Total comments: 2
Patch Set 9 : Doc #Depends on Patchset: Messages
Total messages: 45 (5 generated)
yucliu@chromium.org changed reviewers: + erickung@chromium.org, halliwell@chromium.org, lcwu@chromium.org, maclellant@chromium.org
yucliu@chromium.org changed reviewers: + gfhuang@chromium.org, gunsch@chromium.org
+gunsch, gfhuang
On 2015/08/25 00:33:46, yucliu1 wrote: > +gunsch, gfhuang Mostly nits. Consider adding a test that callbacks are made correctly (cma_end_to_end_test has the infrastructure for setting up a cma pipeline).
https://codereview.chromium.org/1306843003/diff/1/chromecast/browser/cast_con... File chromecast/browser/cast_content_browser_client.h (right): https://codereview.chromium.org/1306843003/diff/1/chromecast/browser/cast_con... chromecast/browser/cast_content_browser_client.h:68: virtual void OnMediaPipelineBackendDestroy(); nit: created, destroyed. https://codereview.chromium.org/1306843003/diff/1/chromecast/browser/media/cm... File chromecast/browser/media/cma_message_filter_host.h (right): https://codereview.chromium.org/1306843003/diff/1/chromecast/browser/media/cm... chromecast/browser/media/cma_message_filter_host.h:55: const base::Closure& on_device_components_destroy_cb); I think we should replace 'device components' with 'pipeline backend' in this class - it's old terminology now (MediaPipelineBackend class used to be called something like DeviceComponents). https://codereview.chromium.org/1306843003/diff/1/chromecast/browser/media/cm... chromecast/browser/media/cma_message_filter_host.h:125: // Callbacks for device components' create and destroy nit: creation, destruction https://codereview.chromium.org/1306843003/diff/1/chromecast/media/cma/pipeli... File chromecast/media/cma/pipeline/media_pipeline_client.h (right): https://codereview.chromium.org/1306843003/diff/1/chromecast/media/cma/pipeli... chromecast/media/cma/pipeline/media_pipeline_client.h:33: // Callback used to report the create of MediaPipelineBackend nit: creation. and either comment destroy or expand this comment to cover both. https://codereview.chromium.org/1306843003/diff/1/chromecast/media/cma/pipeli... File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right): https://codereview.chromium.org/1306843003/diff/1/chromecast/media/cma/pipeli... chromecast/media/cma/pipeline/media_pipeline_impl.cc:77: client_.device_components_destroy_cb.Run(); I think you have to check is_null before calling? https://codereview.chromium.org/1306843003/diff/1/chromecast/media/cma/pipeli... chromecast/media/cma/pipeline/media_pipeline_impl.cc:87: client_.device_components_create_cb.Run(); same here.
https://codereview.chromium.org/1306843003/diff/1/chromecast/media/cma/pipeli... File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right): https://codereview.chromium.org/1306843003/diff/1/chromecast/media/cma/pipeli... chromecast/media/cma/pipeline/media_pipeline_impl.cc:77: client_.device_components_destroy_cb.Run(); On 2015/08/25 00:48:34, halliwell wrote: > I think you have to check is_null before calling? The SetClient requires that all the callbacks can not be null. And there's no other places reset the callbacks.
https://codereview.chromium.org/1306843003/diff/1/chromecast/media/cma/pipeli... File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right): https://codereview.chromium.org/1306843003/diff/1/chromecast/media/cma/pipeli... chromecast/media/cma/pipeline/media_pipeline_impl.cc:77: client_.device_components_destroy_cb.Run(); On 2015/08/25 00:53:11, yucliu1 wrote: > On 2015/08/25 00:48:34, halliwell wrote: > > I think you have to check is_null before calling? > > The SetClient requires that all the callbacks can not be null. And there's no > other places reset the callbacks. You didn't add DCHECKs to SetClient for these new CBs. And you will find a variety of other users of media pipeline (e.g. unit tests) need updating to set the new CBs in their clients. Given that these other users don't actually need the CB, you may want to consider allowing it to be null.
https://codereview.chromium.org/1306843003/diff/1/chromecast/browser/cast_con... File chromecast/browser/cast_content_browser_client.h (right): https://codereview.chromium.org/1306843003/diff/1/chromecast/browser/cast_con... chromecast/browser/cast_content_browser_client.h:68: virtual void OnMediaPipelineBackendDestroy(); On 2015/08/25 00:48:34, halliwell wrote: > nit: created, destroyed. Done. https://codereview.chromium.org/1306843003/diff/1/chromecast/browser/media/cm... File chromecast/browser/media/cma_message_filter_host.h (right): https://codereview.chromium.org/1306843003/diff/1/chromecast/browser/media/cm... chromecast/browser/media/cma_message_filter_host.h:55: const base::Closure& on_device_components_destroy_cb); On 2015/08/25 00:48:34, halliwell wrote: > I think we should replace 'device components' with 'pipeline backend' in this > class - it's old terminology now (MediaPipelineBackend class used to be called > something like DeviceComponents). Done. https://codereview.chromium.org/1306843003/diff/1/chromecast/browser/media/cm... chromecast/browser/media/cma_message_filter_host.h:125: // Callbacks for device components' create and destroy On 2015/08/25 00:48:34, halliwell wrote: > nit: creation, destruction Done. https://codereview.chromium.org/1306843003/diff/1/chromecast/media/cma/pipeli... File chromecast/media/cma/pipeline/media_pipeline_client.h (right): https://codereview.chromium.org/1306843003/diff/1/chromecast/media/cma/pipeli... chromecast/media/cma/pipeline/media_pipeline_client.h:33: // Callback used to report the create of MediaPipelineBackend On 2015/08/25 00:48:34, halliwell wrote: > nit: creation. and either comment destroy or expand this comment to cover both. Done. https://codereview.chromium.org/1306843003/diff/1/chromecast/media/cma/pipeli... File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right): https://codereview.chromium.org/1306843003/diff/1/chromecast/media/cma/pipeli... chromecast/media/cma/pipeline/media_pipeline_impl.cc:77: client_.device_components_destroy_cb.Run(); On 2015/08/25 01:02:49, halliwell wrote: > On 2015/08/25 00:53:11, yucliu1 wrote: > > On 2015/08/25 00:48:34, halliwell wrote: > > > I think you have to check is_null before calling? > > > > The SetClient requires that all the callbacks can not be null. And there's no > > other places reset the callbacks. > > You didn't add DCHECKs to SetClient for these new CBs. And you will find a > variety of other users of media pipeline (e.g. unit tests) need updating to set > the new CBs in their clients. Given that these other users don't actually need > the CB, you may want to consider allowing it to be null. Done. https://codereview.chromium.org/1306843003/diff/1/chromecast/media/cma/pipeli... chromecast/media/cma/pipeline/media_pipeline_impl.cc:87: client_.device_components_create_cb.Run(); On 2015/08/25 00:48:34, halliwell wrote: > same here. Done.
New patch uploaded.
Most of my comments are on the internal CL. I think the structure of that CL is the primary factor affecting the code is here. https://codereview.chromium.org/1306843003/diff/40001/chromecast/browser/medi... File chromecast/browser/media/cma_media_pipeline_client.h (right): https://codereview.chromium.org/1306843003/diff/40001/chromecast/browser/medi... chromecast/browser/media/cma_media_pipeline_client.h:33: friend class base::RefCounted<CmaMediaPipelineClient>; did you try adding DISALLOW_COPY_AND_ASSIGN in this header? https://codereview.chromium.org/1306843003/diff/40001/chromecast/media/cma/pi... File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right): https://codereview.chromium.org/1306843003/diff/40001/chromecast/media/cma/pi... chromecast/media/cma/pipeline/media_pipeline_impl.cc:88: if (!client_.pipeline_backend_created_cb.is_null()) can we remove these checks, since they're also checked in SetClient?
https://codereview.chromium.org/1306843003/diff/40001/chromecast/media/cma/pi... File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right): https://codereview.chromium.org/1306843003/diff/40001/chromecast/media/cma/pi... chromecast/media/cma/pipeline/media_pipeline_impl.cc:88: if (!client_.pipeline_backend_created_cb.is_null()) On 2015/08/31 15:56:52, gunsch wrote: > can we remove these checks, since they're also checked in SetClient? In some tests, SetClient is never called, which leads to empty callback.
https://codereview.chromium.org/1306843003/diff/40001/chromecast/browser/medi... File chromecast/browser/media/cma_media_pipeline_client.h (right): https://codereview.chromium.org/1306843003/diff/40001/chromecast/browser/medi... chromecast/browser/media/cma_media_pipeline_client.h:33: friend class base::RefCounted<CmaMediaPipelineClient>; On 2015/08/31 15:56:52, gunsch wrote: > did you try adding DISALLOW_COPY_AND_ASSIGN in this header? Just reviewed ref_counted.h: https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/ref_co..., the friend is required here. I'll add the DISALLOW here anyway.
Add DISALLOW_COPY_AND_ASSIGN and remove TODO in cma_media_pipeline_client.h
One thing that bothers me a lot about this structure is that we don't have a good path that can be followed from "pipeline release is requested" to "pipeline is released". When we release the pipeline, it doesn't go through MediaPipelineClient. It seems like we tear down the page and simply _assume_ that the pipeline will get torn down because of that, then MediaPipelineClient simply gets a notification when pipeline teardown has occurred. That makes the end-to-end path difficult to reason about, and nearly impossible to test. It also seems like it doesn't get us much closer to a point where we can release CMA resources without tearing down the page, which would hopefully be one of the goals of this new code path. I don't currently have a great suggestion for how to make this better, but I feel like this needs more thought and traceability. An end-to-end test demonstrating the flow (even in the internal CL) would go a long way toward convincing me that this is a good structure.
On 2015/08/31 22:03:08, gunsch wrote: > One thing that bothers me a lot about this structure is that we don't have a > good path that can be followed from "pipeline release is requested" to "pipeline > is released". > > When we release the pipeline, it doesn't go through MediaPipelineClient. It > seems like we tear down the page and simply _assume_ that the pipeline will get > torn down because of that, then MediaPipelineClient simply gets a notification > when pipeline teardown has occurred. > > That makes the end-to-end path difficult to reason about, and nearly impossible > to test. It also seems like it doesn't get us much closer to a point where we > can release CMA resources without tearing down the page, which would hopefully > be one of the goals of this new code path. > > I don't currently have a great suggestion for how to make this better, but I > feel like this needs more thought and traceability. An end-to-end test > demonstrating the flow (even in the internal CL) would go a long way toward > convincing me that this is a good structure. I think that's valid, but also that explicitly requesting pipeline teardown is outside the scope of this change. It's definitely something we should implement soon (yucliu: could you make sure there's a ticket entered for this?). But my understanding was that we need a way for vendors to start hooking into the 'resources have been released' event, and even if that's triggered implicitly by closing the page, it's a useful first step. A test on the internal CL would definitely be helpful, though.
On 2015/09/01 00:04:30, halliwell wrote: > On 2015/08/31 22:03:08, gunsch wrote: > > One thing that bothers me a lot about this structure is that we don't have a > > good path that can be followed from "pipeline release is requested" to > "pipeline > > is released". > > > > When we release the pipeline, it doesn't go through MediaPipelineClient. It > > seems like we tear down the page and simply _assume_ that the pipeline will > get > > torn down because of that, then MediaPipelineClient simply gets a notification > > when pipeline teardown has occurred. > > > > That makes the end-to-end path difficult to reason about, and nearly > impossible > > to test. It also seems like it doesn't get us much closer to a point where we > > can release CMA resources without tearing down the page, which would hopefully > > be one of the goals of this new code path. > > > > I don't currently have a great suggestion for how to make this better, but I > > feel like this needs more thought and traceability. An end-to-end test > > demonstrating the flow (even in the internal CL) would go a long way toward > > convincing me that this is a good structure. > > I think that's valid, but also that explicitly requesting pipeline teardown is > outside the scope of this change. It's definitely something we should implement > soon (yucliu: could you make sure there's a ticket entered for this?). But my > understanding was that we need a way for vendors to start hooking into the > 'resources have been released' event, and even if that's triggered implicitly by > closing the page, it's a useful first step. > I realize requesting pipeline teardown (and especially in a resumable way) is outside the scope of this CL. But if that's the inevitable next step, I'd like to make sure we have a design that doesn't box ourselves in before getting to that point. Yuchen and I talked offline for awhile about some ways this could be achieved. > A test on the internal CL would definitely be helpful, though.
https://codereview.chromium.org/1306843003/diff/80001/chromecast/base/cast_re... File chromecast/base/cast_resource.h (right): https://codereview.chromium.org/1306843003/diff/80001/chromecast/base/cast_re... chromecast/base/cast_resource.h:34: virtual void OnResourceReleased(CastResource* cast_resource) = 0; add virtual dtor, hide dtor as well? https://codereview.chromium.org/1306843003/diff/80001/chromecast/browser/medi... File chromecast/browser/media/cma_media_pipeline_client.cc (right): https://codereview.chromium.org/1306843003/diff/80001/chromecast/browser/medi... chromecast/browser/media/cma_media_pipeline_client.cc:11: CmaMediaPipelineClient::CmaMediaPipelineClient() {} client_ must be initialized.
New patch uploaded. Move CastResource to chromecast/cast. What's the best location for this file? https://codereview.chromium.org/1306843003/diff/80001/chromecast/base/cast_re... File chromecast/base/cast_resource.h (right): https://codereview.chromium.org/1306843003/diff/80001/chromecast/base/cast_re... chromecast/base/cast_resource.h:34: virtual void OnResourceReleased(CastResource* cast_resource) = 0; On 2015/09/01 15:24:05, halliwell wrote: > add virtual dtor, hide dtor as well? Done. https://codereview.chromium.org/1306843003/diff/80001/chromecast/browser/medi... File chromecast/browser/media/cma_media_pipeline_client.cc (right): https://codereview.chromium.org/1306843003/diff/80001/chromecast/browser/medi... chromecast/browser/media/cma_media_pipeline_client.cc:11: CmaMediaPipelineClient::CmaMediaPipelineClient() {} On 2015/09/01 15:24:05, halliwell wrote: > client_ must be initialized. Initialized to nullptr. Upstream don't set client.
I'm not very keen on introducing chromecast/cast. Always felt they were a confusing pair of names. gunsch@, do we have a plan already for how to name the cast/ code as we upstream it? https://codereview.chromium.org/1306843003/diff/100001/chromecast/browser/med... File chromecast/browser/media/cma_media_pipeline_client.cc (right): https://codereview.chromium.org/1306843003/diff/100001/chromecast/browser/med... chromecast/browser/media/cma_media_pipeline_client.cc:36: cast::CastResource::kResourceScreenPrimary); I actually think this is incorrect and we need to change the set of resource types? We already use kResourceScreenPrimary for the GPU relinquish flow. I think we need a different type for CMA backend ... MediaDecoder? Or Media if we want something more generic? https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... File chromecast/cast/cast_resource.h (right): https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... chromecast/cast/cast_resource.h:5: #define CAST_BASE_CAST_RESOURCE_H_ nit: this doesn't match path. https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... chromecast/cast/cast_resource.h:7: namespace cast { nit: really should be an outer namespace chromecast here? https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... chromecast/cast/cast_resource.h:10: class CastResource { nit: do we need it to be called CastResource if it's in cast? https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... chromecast/cast/cast_resource.h:17: kResourceAudio = 1 << 0, what is audio for and where is it used? I'm not convinced it actually maps to anything. https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... chromecast/cast/cast_resource.h:19: // pipeline, primary graphics plane, display, etc. see comment elsewhere, I think we need to separate images from video.
https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... File chromecast/cast/cast_resource.h (right): https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... chromecast/cast/cast_resource.h:7: namespace cast { On 2015/09/01 19:42:23, halliwell wrote: > nit: really should be an outer namespace chromecast here? If this is supposed in cast/, we'd better keep consistency with internal/cast/. https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... chromecast/cast/cast_resource.h:17: kResourceAudio = 1 << 0, On 2015/09/01 19:42:23, halliwell wrote: > what is audio for and where is it used? I'm not convinced it actually maps to > anything. internal code will use it. https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... chromecast/cast/cast_resource.h:19: // pipeline, primary graphics plane, display, etc. On 2015/09/01 19:42:23, halliwell wrote: > see comment elsewhere, I think we need to separate images from video. Single CastResource can hold multiple resources and multiple CastResource could have the same resource.
https://codereview.chromium.org/1306843003/diff/100001/chromecast/browser/med... File chromecast/browser/media/cma_media_pipeline_client.h (right): https://codereview.chromium.org/1306843003/diff/100001/chromecast/browser/med... chromecast/browser/media/cma_media_pipeline_client.h:18: public cast::CastResource { Is this class really necessary any more? I'm wondering if we could eliminate a bunch of code by: * Setting the CastResource::Client directly on the backend object * Call Acquired when the backend is initialised, Released when it's destroyed * Go back to having the browser client create the backend directly, remove this class and the internal subclass Thoughts?
https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... File chromecast/cast/cast_resource.h (right): https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... chromecast/cast/cast_resource.h:17: kResourceAudio = 1 << 0, On 2015/09/01 20:15:47, yucliu1 wrote: > On 2015/09/01 19:42:23, halliwell wrote: > > what is audio for and where is it used? I'm not convinced it actually maps to > > anything. > > internal code will use it. I don't believe it does anything useful with it, but we could discuss offline since all the relevant code is internal. https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... chromecast/cast/cast_resource.h:19: // pipeline, primary graphics plane, display, etc. On 2015/09/01 20:15:47, yucliu1 wrote: > On 2015/09/01 19:42:23, halliwell wrote: > > see comment elsewhere, I think we need to separate images from video. > > Single CastResource can hold multiple resources and multiple CastResource could > have the same resource. That seems ... useless. The whole point of the resource management API is that there are specific hardware resources that we have to release in order to give access to other applications. The primary display is one of those, it is managed by the GL driver, and we're just lumping it in with the media pipeline here. We should separate them so that applications can be precise about what hardware resources they need us to release.
https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... File chromecast/cast/cast_resource.h (right): https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... chromecast/cast/cast_resource.h:19: // pipeline, primary graphics plane, display, etc. On 2015/09/01 20:24:39, halliwell wrote: > On 2015/09/01 20:15:47, yucliu1 wrote: > > On 2015/09/01 19:42:23, halliwell wrote: > > > see comment elsewhere, I think we need to separate images from video. > > > > Single CastResource can hold multiple resources and multiple CastResource > could > > have the same resource. > > That seems ... useless. The whole point of the resource management API is that > there are specific hardware resources that we have to release in order to give > access to other applications. The primary display is one of those, it is > managed by the GL driver, and we're just lumping it in with the media pipeline > here. We should separate them so that applications can be precise about what > hardware resources they need us to release. Wait, should MediaPipeline use both audio and video?
On 2015/09/01 20:36:42, yucliu1 wrote: > https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... > File chromecast/cast/cast_resource.h (right): > > https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... > chromecast/cast/cast_resource.h:19: // pipeline, primary graphics plane, > display, etc. > On 2015/09/01 20:24:39, halliwell wrote: > > On 2015/09/01 20:15:47, yucliu1 wrote: > > > On 2015/09/01 19:42:23, halliwell wrote: > > > > see comment elsewhere, I think we need to separate images from video. > > > > > > Single CastResource can hold multiple resources and multiple CastResource > > could > > > have the same resource. > > > > That seems ... useless. The whole point of the resource management API is > that > > there are specific hardware resources that we have to release in order to give > > access to other applications. The primary display is one of those, it is > > managed by the GL driver, and we're just lumping it in with the media pipeline > > here. We should separate them so that applications can be precise about what > > hardware resources they need us to release. > > Wait, should MediaPipeline use both audio and video? MediaPipeline does use audio and video hardware, yes. But our resource types seem to group video playback with primary graphics plane, which is incorrect. Also, what will happen when we have multiple media pipelines? e.g. someone requests the video/audio resources to be released, the pipelines get torn down ... will the resource manager know that it should wait for all media pipelines to tear down before notifying that the resource is actually released?
On 2015/09/01 20:53:33, halliwell wrote: > On 2015/09/01 20:36:42, yucliu1 wrote: > > > https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... > > File chromecast/cast/cast_resource.h (right): > > > > > https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... > > chromecast/cast/cast_resource.h:19: // pipeline, primary graphics plane, > > display, etc. > > On 2015/09/01 20:24:39, halliwell wrote: > > > On 2015/09/01 20:15:47, yucliu1 wrote: > > > > On 2015/09/01 19:42:23, halliwell wrote: > > > > > see comment elsewhere, I think we need to separate images from video. > > > > > > > > Single CastResource can hold multiple resources and multiple CastResource > > > could > > > > have the same resource. > > > > > > That seems ... useless. The whole point of the resource management API is > > that > > > there are specific hardware resources that we have to release in order to > give > > > access to other applications. The primary display is one of those, it is > > > managed by the GL driver, and we're just lumping it in with the media > pipeline > > > here. We should separate them so that applications can be precise about > what > > > hardware resources they need us to release. > > > > Wait, should MediaPipeline use both audio and video? > > MediaPipeline does use audio and video hardware, yes. But our resource types > seem to group video playback with primary graphics plane, which is incorrect. > > Also, what will happen when we have multiple media pipelines? e.g. someone > requests the video/audio resources to be released, the pipelines get torn down > ... will the resource manager know that it should wait for all media pipelines > to tear down before notifying that the resource is actually released? Currently it only works for single media pipeline. But we can support multiple media pipeline by either: 1. CmaMediaPipelineClient records how many media pipelines are still alive. 2. Add wrapper for MediaPipelineBackend, which is responsible for notify resource manager.
On 2015/09/01 21:12:09, yucliu1 wrote: > On 2015/09/01 20:53:33, halliwell wrote: > > On 2015/09/01 20:36:42, yucliu1 wrote: > > > > > > https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... > > > File chromecast/cast/cast_resource.h (right): > > > > > > > > > https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... > > > chromecast/cast/cast_resource.h:19: // pipeline, primary graphics plane, > > > display, etc. > > > On 2015/09/01 20:24:39, halliwell wrote: > > > > On 2015/09/01 20:15:47, yucliu1 wrote: > > > > > On 2015/09/01 19:42:23, halliwell wrote: > > > > > > see comment elsewhere, I think we need to separate images from video. > > > > > > > > > > Single CastResource can hold multiple resources and multiple > CastResource > > > > could > > > > > have the same resource. > > > > > > > > That seems ... useless. The whole point of the resource management API is > > > that > > > > there are specific hardware resources that we have to release in order to > > give > > > > access to other applications. The primary display is one of those, it is > > > > managed by the GL driver, and we're just lumping it in with the media > > pipeline > > > > here. We should separate them so that applications can be precise about > > what > > > > hardware resources they need us to release. > > > > > > Wait, should MediaPipeline use both audio and video? > > > > MediaPipeline does use audio and video hardware, yes. But our resource types > > seem to group video playback with primary graphics plane, which is incorrect. > > > > Also, what will happen when we have multiple media pipelines? e.g. someone > > requests the video/audio resources to be released, the pipelines get torn down > > ... will the resource manager know that it should wait for all media pipelines > > to tear down before notifying that the resource is actually released? > > Currently it only works for single media pipeline. But we can support multiple > media pipeline by either: > > 1. CmaMediaPipelineClient records how many media pipelines are still alive. > 2. Add wrapper for MediaPipelineBackend, which is responsible for notify > resource manager. Discussed my most recent comments offline with yucliu, to summarise: * I don't think the resource types are quite right, but changing them may involve vendor changes. Given that this CL isn't actually changing them, let's not hold it up for that. But yucliu to file a ticket for followup on this. * Similarly for multiple media pipelines: file ticket for followup. This will need to come soon with alok's Web Audio changes. * Setting the client on the backend object won't work, since backend is an interface which vendors implement. So I'm happy keeping CmaMediaPipelineClient.
On 2015/09/01 22:37:26, halliwell wrote: > On 2015/09/01 21:12:09, yucliu1 wrote: > > On 2015/09/01 20:53:33, halliwell wrote: > > > On 2015/09/01 20:36:42, yucliu1 wrote: > > > > > > > > > > https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... > > > > File chromecast/cast/cast_resource.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... > > > > chromecast/cast/cast_resource.h:19: // pipeline, primary graphics plane, > > > > display, etc. > > > > On 2015/09/01 20:24:39, halliwell wrote: > > > > > On 2015/09/01 20:15:47, yucliu1 wrote: > > > > > > On 2015/09/01 19:42:23, halliwell wrote: > > > > > > > see comment elsewhere, I think we need to separate images from > video. > > > > > > > > > > > > Single CastResource can hold multiple resources and multiple > > CastResource > > > > > could > > > > > > have the same resource. > > > > > > > > > > That seems ... useless. The whole point of the resource management API > is > > > > that > > > > > there are specific hardware resources that we have to release in order > to > > > give > > > > > access to other applications. The primary display is one of those, it > is > > > > > managed by the GL driver, and we're just lumping it in with the media > > > pipeline > > > > > here. We should separate them so that applications can be precise about > > > what > > > > > hardware resources they need us to release. > > > > > > > > Wait, should MediaPipeline use both audio and video? > > > > > > MediaPipeline does use audio and video hardware, yes. But our resource > types > > > seem to group video playback with primary graphics plane, which is > incorrect. > > > > > > Also, what will happen when we have multiple media pipelines? e.g. someone > > > requests the video/audio resources to be released, the pipelines get torn > down > > > ... will the resource manager know that it should wait for all media > pipelines > > > to tear down before notifying that the resource is actually released? > > > > Currently it only works for single media pipeline. But we can support multiple > > media pipeline by either: > > > > 1. CmaMediaPipelineClient records how many media pipelines are still alive. > > 2. Add wrapper for MediaPipelineBackend, which is responsible for notify > > resource manager. > > Discussed my most recent comments offline with yucliu, to summarise: > * I don't think the resource types are quite right, but changing them may > involve vendor changes. Given that this CL isn't actually changing them, let's > not hold it up for that. But yucliu to file a ticket for followup on this. > * Similarly for multiple media pipelines: file ticket for followup. This will > need to come soon with alok's Web Audio changes. > * Setting the client on the backend object won't work, since backend is an > interface which vendors implement. So I'm happy keeping CmaMediaPipelineClient. This lgtm, check if gunsch has any further comments.
https://codereview.chromium.org/1306843003/diff/100001/chromecast/browser/med... File chromecast/browser/media/cma_media_pipeline_client.cc (right): https://codereview.chromium.org/1306843003/diff/100001/chromecast/browser/med... chromecast/browser/media/cma_media_pipeline_client.cc:33: if (!(resource & cast::CastResource::kResourceScreenPrimary)) { What threads are these called on? Should OnResourceReleased be callable from any thread? https://codereview.chromium.org/1306843003/diff/100001/chromecast/browser/med... File chromecast/browser/media/cma_message_filter_host.cc (right): https://codereview.chromium.org/1306843003/diff/100001/chromecast/browser/med... chromecast/browser/media/cma_message_filter_host.cc:384: client.pipeline_backend_created_cb = base::Bind( Since there's one CmaMediaPipelineClient (as opposed to clients being one-to-one with backend instances, or backends being tracked by CmaMediaPipelineClient), this couples us to some assumptions. For example, we now can't have two media pipelines. (You might say we couldn't before, but that's not quite accurate---IIRC we should be able to have one video-only pipeline and one audio-only). There's still the weirdness of not being able to trace an easy create or destroy path in a way that ties to resource acquisition/release, but we've agreed we can punt that slightly for now. Could we have CmaMediaPipelineClient keep references to the Backend instances it creates, or at least manage references when OnMediaPipelineBackendCreated and OnMediaPipelineBackendDestroyed are called, so that it only calls OnResourcesReleased when _all_ pipelines are destroyed? https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... File chromecast/cast/cast_resource.h (right): https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... chromecast/cast/cast_resource.h:4: #ifndef CAST_BASE_CAST_RESOURCE_H_ nit: blank line above https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... chromecast/cast/cast_resource.h:5: #define CAST_BASE_CAST_RESOURCE_H_ On 2015/09/01 19:42:23, halliwell wrote: > nit: this doesn't match path. nit: move this back to chromecast/base/ :) https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... chromecast/cast/cast_resource.h:7: namespace cast { On 2015/09/01 20:15:47, yucliu1 wrote: > On 2015/09/01 19:42:23, halliwell wrote: > > nit: really should be an outer namespace chromecast here? > > If this is supposed in cast/, we'd better keep consistency with internal/cast/. per offline, namespace is chromecast https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... chromecast/cast/cast_resource.h:13: enum Resource { optional naming q: if the class is Resource, should this be ResourceType? https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... chromecast/cast/cast_resource.h:44: virtual void SetCastResourceClient(Client* client) = 0; nit: can we put this first, to represent the general order that these calls occur https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... chromecast/cast/cast_resource.h:47: virtual ~CastResource() {} why is this protected? Doesn't this limit CastResource ownership options? https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... chromecast/cast/cast_resource.h:48: }; DISALLOW_COPY_AND_ASSIGN(CastResource); plus #include "base/macros.h"
Oh sorry, I didn't read the rest of the thread. I just noticed there was a discussion about single vs. multiple pipelines. Filing a bug inline for now and resolving the rest of the comments SGTM.
https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... File chromecast/cast/cast_resource.h (right): https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... chromecast/cast/cast_resource.h:35: Resource resource) = 0; I just noticed in the internal CL that "resource" here is documented as "remain". That seems backwards to me---the obvious thing to do here would be to indicate which resource was released. Can you clarify in docs how each of |Client|'s methods are intended to be used?
How difficult would it be to add a test that a) creates a pipeline using CmaMediaPipelineClient, b) destroys the pipeline, c) verifies that we get the ResourceReleased notificatoin?
https://codereview.chromium.org/1306843003/diff/100001/chromecast/browser/med... File chromecast/browser/media/cma_media_pipeline_client.cc (right): https://codereview.chromium.org/1306843003/diff/100001/chromecast/browser/med... chromecast/browser/media/cma_media_pipeline_client.cc:33: if (!(resource & cast::CastResource::kResourceScreenPrimary)) { On 2015/09/01 22:56:08, gunsch wrote: > What threads are these called on? > > Should OnResourceReleased be callable from any thread? Yes, OnResourceReleased can be called from any thread. https://codereview.chromium.org/1306843003/diff/100001/chromecast/browser/med... File chromecast/browser/media/cma_media_pipeline_client.h (right): https://codereview.chromium.org/1306843003/diff/100001/chromecast/browser/med... chromecast/browser/media/cma_media_pipeline_client.h:18: public cast::CastResource { On 2015/09/01 20:16:34, halliwell wrote: > Is this class really necessary any more? > > I'm wondering if we could eliminate a bunch of code by: > * Setting the CastResource::Client directly on the backend object > * Call Acquired when the backend is initialised, Released when it's destroyed > * Go back to having the browser client create the backend directly, remove this > class and the internal subclass > > Thoughts? This needs to create another class to wrap the media pipeline. https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... File chromecast/cast/cast_resource.h (right): https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... chromecast/cast/cast_resource.h:4: #ifndef CAST_BASE_CAST_RESOURCE_H_ On 2015/09/01 22:56:08, gunsch wrote: > nit: blank line above Done. https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... chromecast/cast/cast_resource.h:5: #define CAST_BASE_CAST_RESOURCE_H_ On 2015/09/01 22:56:08, gunsch wrote: > On 2015/09/01 19:42:23, halliwell wrote: > > nit: this doesn't match path. > > nit: move this back to chromecast/base/ :) Done. https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... chromecast/cast/cast_resource.h:7: namespace cast { On 2015/09/01 22:56:08, gunsch wrote: > On 2015/09/01 20:15:47, yucliu1 wrote: > > On 2015/09/01 19:42:23, halliwell wrote: > > > nit: really should be an outer namespace chromecast here? > > > > If this is supposed in cast/, we'd better keep consistency with > internal/cast/. > > per offline, namespace is chromecast Done. https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... chromecast/cast/cast_resource.h:13: enum Resource { On 2015/09/01 22:56:08, gunsch wrote: > optional naming q: if the class is Resource, should this be ResourceType? I think it's better to use Resource since it's a union of different bits. https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... chromecast/cast/cast_resource.h:47: virtual ~CastResource() {} On 2015/09/01 22:56:08, gunsch wrote: > why is this protected? Doesn't this limit CastResource ownership options? With current implementation, CastResource is owned by creator. We don't want to delete them as "CastResource*".
The CQ bit was checked by yucliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1306843003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1306843003/120001
On 2015/09/01 23:06:28, gunsch wrote: > How difficult would it be to add a test that a) creates a pipeline using > CmaMediaPipelineClient, b) destroys the pipeline, c) verifies that we get the > ResourceReleased notificatoin? The difficult parts are: 1. CmaMessageFilterHost is supposed to run on browser io thread, I'm not sure if we could do this without launching the whole browser process. 2. CmaMessageFilterHost is driven by renderer, looks like we need to expose some api (at least CreateMedia and DestroyMedia) to allow media creation and destroy in test.
In the spirit of "testability is good design", I think the difficult parts you raise speak to the complexity of this solution. Since we're getting pressure to merge this sooner rather than continue iterating to a better solution, can you please file a bug to track simplifying this path and adding an end-to-end test, so that the code doesn't remain in this state? https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... File chromecast/cast/cast_resource.h (right): https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... chromecast/cast/cast_resource.h:47: virtual ~CastResource() {} On 2015/09/01 23:42:13, yucliu1 wrote: > On 2015/09/01 22:56:08, gunsch wrote: > > why is this protected? Doesn't this limit CastResource ownership options? > > With current implementation, CastResource is owned by creator. We don't want to > delete them as "CastResource*". Agreed, but I think this means that an owner can't have a scoped_ptr<CastResource>? https://codereview.chromium.org/1306843003/diff/140001/chromecast/base/cast_r... File chromecast/base/cast_resource.h (right): https://codereview.chromium.org/1306843003/diff/140001/chromecast/base/cast_r... chromecast/base/cast_resource.h:39: // Client. Please indicate that these can be called on any thread. My question wasn't just idle curiosity but also for documentation :)
https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... File chromecast/cast/cast_resource.h (right): https://codereview.chromium.org/1306843003/diff/100001/chromecast/cast/cast_r... chromecast/cast/cast_resource.h:47: virtual ~CastResource() {} On 2015/09/02 17:10:50, gunsch wrote: > On 2015/09/01 23:42:13, yucliu1 wrote: > > On 2015/09/01 22:56:08, gunsch wrote: > > > why is this protected? Doesn't this limit CastResource ownership options? > > > > With current implementation, CastResource is owned by creator. We don't want > to > > delete them as "CastResource*". > > Agreed, but I think this means that an owner can't have a > scoped_ptr<CastResource>? The owner should know the type of CastResource. If they want to manage multiple CastResources, these CastResource should share some common parts, which means there's another base class. https://codereview.chromium.org/1306843003/diff/140001/chromecast/base/cast_r... File chromecast/base/cast_resource.h (right): https://codereview.chromium.org/1306843003/diff/140001/chromecast/base/cast_r... chromecast/base/cast_resource.h:39: // Client. On 2015/09/02 17:10:50, gunsch wrote: > Please indicate that these can be called on any thread. My question wasn't just > idle curiosity but also for documentation :) Done.
On 2015/09/02 17:10:50, gunsch wrote: > In the spirit of "testability is good design", I think the difficult parts you > raise speak to the complexity of this solution. Since we're getting pressure to > merge this sooner rather than continue iterating to a better solution, can you > please file a bug to track simplifying this path and adding an end-to-end test, > so that the code doesn't remain in this state? I think this can be fixed by allowing shutdown/suspend of media pipeline from browser side. Anyway, I'll file a bug.
lgtm
The CQ bit was checked by yucliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from halliwell@chromium.org Link to the patchset: https://codereview.chromium.org/1306843003/#ps160001 (title: "Doc")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1306843003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1306843003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/b26f03db16a4752572e136d3c531234bfa45eb63 Cr-Commit-Position: refs/heads/master@{#346977} |
