Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1531543002:
[Chromecast] Adding pause/resume operations to VideoPlaneController. (Closed)
|
Created:
5 years ago by esum Modified:
4 years, 11 months ago CC:
chromium-reviews, gunsch+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] Adding pause/resume operations to VideoPlaneController.
Also adding method for initializing resources after they have been
granted to cast.
BUG=internal b/25812819
TEST=Revoke/Grant resources during youtube playback on redwood.
Youtube/Netflix playback on earth (no regressions).
Committed: https://crrev.com/8d560a8c717968a3391be2341dc9a5170a4be310
Cr-Commit-Position: refs/heads/master@{#369937}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Modifying comment. #
Total comments: 10
Patch Set 3 : #Patch Set 4 : Just modified a comment back the way it was. #
Total comments: 12
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #Patch Set 7 : Just rebase of Patch Set 6. #
Total comments: 14
Patch Set 8 : Just changed comments. No code changes. #Patch Set 9 : Rebase of Patch Set 8. #
Messages
Total messages: 44 (5 generated)
esum@chromium.org changed reviewers: + derekjchow@chromium.org, gfhuang@chromium.org, halliwell@chromium.org, yucliu@chromium.org
https://codereview.chromium.org/1531543002/diff/1/chromecast/browser/media/cm... File chromecast/browser/media/cma_media_pipeline_client.h (right): https://codereview.chromium.org/1531543002/diff/1/chromecast/browser/media/cm... chromecast/browser/media/cma_media_pipeline_client.h:32: void InitializeResource(CastResource::Resource resource) override {} I don't think anything needs to go here. Can you guys confirm?
https://codereview.chromium.org/1531543002/diff/20001/chromecast/base/cast_re... File chromecast/base/cast_resource.h (right): https://codereview.chromium.org/1531543002/diff/20001/chromecast/base/cast_re... chromecast/base/cast_resource.h:59: virtual void InitializeResource(Resource resource) = 0; What thread does this get called on? If this gets called on the UI thread we should make this API asynchronous in case if initialization becomes a long operation. Also another reason to make this asynchronous would be to mirror ReleaseResource(). https://codereview.chromium.org/1531543002/diff/20001/chromecast/media/base/v... File chromecast/media/base/video_plane_controller.cc (right): https://codereview.chromium.org/1531543002/diff/20001/chromecast/media/base/v... chromecast/media/base/video_plane_controller.cc:205: void VideoPlaneController::Pause() { Since this is a public function, you should thread check this. https://codereview.chromium.org/1531543002/diff/20001/chromecast/media/base/v... chromecast/media/base/video_plane_controller.cc:210: void VideoPlaneController::Resume() { ditto. https://codereview.chromium.org/1531543002/diff/20001/chromecast/media/base/v... chromecast/media/base/video_plane_controller.cc:215: void VideoPlaneController::ClearGeometryState() { ditto.
https://codereview.chromium.org/1531543002/diff/20001/chromecast/base/cast_re... File chromecast/base/cast_resource.h (right): https://codereview.chromium.org/1531543002/diff/20001/chromecast/base/cast_re... chromecast/base/cast_resource.h:59: virtual void InitializeResource(Resource resource) = 0; On 2015/12/15 22:40:35, derekjchow1 wrote: > What thread does this get called on? If this gets called on the UI thread we > should make this API asynchronous in case if initialization becomes a long > operation. > > Also another reason to make this asynchronous would be to mirror > ReleaseResource(). Probably we can make a contract here: InitializeResource should have a corresponding Client::OnResourceAcquired. This requires add other two APIs to add/remove CastResource to/from watch list. Otherwise, this function may not be called, right? Can you add comments here?
On 2015/12/15 22:40:36, derekjchow1 wrote: > https://codereview.chromium.org/1531543002/diff/20001/chromecast/base/cast_re... > File chromecast/base/cast_resource.h (right): > > https://codereview.chromium.org/1531543002/diff/20001/chromecast/base/cast_re... > chromecast/base/cast_resource.h:59: virtual void InitializeResource(Resource > resource) = 0; > What thread does this get called on? If this gets called on the UI thread we > should make this API asynchronous in case if initialization becomes a long > operation. > > Also another reason to make this asynchronous would be to mirror > ReleaseResource(). > > https://codereview.chromium.org/1531543002/diff/20001/chromecast/media/base/v... > File chromecast/media/base/video_plane_controller.cc (right): > > https://codereview.chromium.org/1531543002/diff/20001/chromecast/media/base/v... > chromecast/media/base/video_plane_controller.cc:205: void > VideoPlaneController::Pause() { > Since this is a public function, you should thread check this. > > https://codereview.chromium.org/1531543002/diff/20001/chromecast/media/base/v... > chromecast/media/base/video_plane_controller.cc:210: void > VideoPlaneController::Resume() { > ditto. > > https://codereview.chromium.org/1531543002/diff/20001/chromecast/media/base/v... > chromecast/media/base/video_plane_controller.cc:215: void > VideoPlaneController::ClearGeometryState() { > ditto. Making InitializeResource asynchronous reminds me of TCP handshake. CastResource is the client and CastResource::Client is the server. "initialization or connect" uses three way handshake started with CastResource sending the first request message. But "termination or disconnect" is slightly different in our model. CastResource::Client doesn't need to "ack" CastResource's "close" message. We can imagine client is closed immediately after sending the close message. So the initialization logic should be, 1. CastResource request Resource from Client (OnResourceAcquired, need a better name). Initialization can only be triggered by CastResource. 2. When Resource is ready, Client calls InitializeResource. 3. When initialization is finished, CastResource calls the new API (find a proper name). The destroy logic should be, 1. If destroy is triggered by Client, Client calls ReleaseResource. 2. CastResource release Resource and calls OnResourceReleased, at this point, CastResource is safe to delete itself if it doesn't hold any Resource. 3. Client thinks the CastResource is completely released. Also, is it better to change the name Client to Server or something similar?
esum@chromium.org changed reviewers: + byungchul@chromium.org, maclellant@chromium.org
Please have a look at PatchSet 4. +Tavis/Byungchul for info. and review if you want. https://codereview.chromium.org/1531543002/diff/20001/chromecast/base/cast_re... File chromecast/base/cast_resource.h (right): https://codereview.chromium.org/1531543002/diff/20001/chromecast/base/cast_re... chromecast/base/cast_resource.h:59: virtual void InitializeResource(Resource resource) = 0; On 2015/12/15 23:05:58, yucliu1 wrote: > On 2015/12/15 22:40:35, derekjchow1 wrote: > > What thread does this get called on? If this gets called on the UI thread we > > should make this API asynchronous in case if initialization becomes a long > > operation. > > > > Also another reason to make this asynchronous would be to mirror > > ReleaseResource(). > > Probably we can make a contract here: InitializeResource should have a > corresponding Client::OnResourceAcquired. This requires add other two APIs to > add/remove CastResource to/from watch list. > > Otherwise, this function may not be called, right? Can you add comments here? Agreed it should be made asynchronous, but I would really like to tackle this after 1.18 in the interest of fixing the actual bug at hand for OEM's soon. Especially because this is not sounding like a straightforward change. I think Tavis has some thoughts on this also. Is there a problem with the way the code is now? I know it's not ideal, but it's simple and all the VideoPlane calls are very quick. Also Luke/Gaofeng, what do you think? https://codereview.chromium.org/1531543002/diff/20001/chromecast/media/base/v... File chromecast/media/base/video_plane_controller.cc (right): https://codereview.chromium.org/1531543002/diff/20001/chromecast/media/base/v... chromecast/media/base/video_plane_controller.cc:205: void VideoPlaneController::Pause() { On 2015/12/15 22:40:35, derekjchow1 wrote: > Since this is a public function, you should thread check this. Done. https://codereview.chromium.org/1531543002/diff/20001/chromecast/media/base/v... chromecast/media/base/video_plane_controller.cc:210: void VideoPlaneController::Resume() { On 2015/12/15 22:40:35, derekjchow1 wrote: > ditto. Done. https://codereview.chromium.org/1531543002/diff/20001/chromecast/media/base/v... chromecast/media/base/video_plane_controller.cc:215: void VideoPlaneController::ClearGeometryState() { On 2015/12/15 22:40:35, derekjchow1 wrote: > ditto. Done.
> Making InitializeResource asynchronous reminds me of TCP handshake. CastResource > is the client and CastResource::Client is the server. "initialization or > connect" uses three way handshake started with CastResource sending the first > request message. But "termination or disconnect" is slightly different in our > model. CastResource::Client doesn't need to "ack" CastResource's "close" > message. We can imagine client is closed immediately after sending the close > message. > > So the initialization logic should be, > > 1. CastResource request Resource from Client (OnResourceAcquired, need a better > name). Initialization can only be triggered by CastResource. > 2. When Resource is ready, Client calls InitializeResource. > 3. When initialization is finished, CastResource calls the new API (find a > proper name). > > The destroy logic should be, > > 1. If destroy is triggered by Client, Client calls ReleaseResource. > 2. CastResource release Resource and calls OnResourceReleased, at this point, > CastResource is safe to delete itself if it doesn't hold any Resource. > 3. Client thinks the CastResource is completely released. > > Also, is it better to change the name Client to Server or something similar? Yuchen, this sounds very interesting, but I'm not sure I fully understand it. When we do implement this (see my response in cast_resource.h), whether it's now or later, I'll discuss with you in person.
https://codereview.chromium.org/1531543002/diff/20001/chromecast/base/cast_re... File chromecast/base/cast_resource.h (right): https://codereview.chromium.org/1531543002/diff/20001/chromecast/base/cast_re... chromecast/base/cast_resource.h:59: virtual void InitializeResource(Resource resource) = 0; On 2015/12/17 08:28:12, esum wrote: > On 2015/12/15 23:05:58, yucliu1 wrote: > > On 2015/12/15 22:40:35, derekjchow1 wrote: > > > What thread does this get called on? If this gets called on the UI thread we > > > should make this API asynchronous in case if initialization becomes a long > > > operation. > > > > > > Also another reason to make this asynchronous would be to mirror > > > ReleaseResource(). > > > > Probably we can make a contract here: InitializeResource should have a > > corresponding Client::OnResourceAcquired. This requires add other two APIs to > > add/remove CastResource to/from watch list. > > > > Otherwise, this function may not be called, right? Can you add comments here? > > Agreed it should be made asynchronous, but I would really like to tackle this > after 1.18 in the interest of fixing the actual bug at hand for OEM's soon. > Especially because this is not sounding like a straightforward change. I think > Tavis has some thoughts on this also. > > Is there a problem with the way the code is now? I know it's not ideal, but it's > simple and all the VideoPlane calls are very quick. > > Also Luke/Gaofeng, what do you think? > Calling VideoPlane synchronously is fine. I'm ok with following up on async API afterwards. https://codereview.chromium.org/1531543002/diff/60001/chromecast/media/base/v... File chromecast/media/base/video_plane_controller.cc (right): https://codereview.chromium.org/1531543002/diff/60001/chromecast/media/base/v... chromecast/media/base/video_plane_controller.cc:227: } So ... it looks like from the internal CL that when we get resources back, we ClearGeometryState and Resume. That does not sound right to me as it depends on receiving further SetGeometry calls, which may be true in some cases but not necessarily always. I was expecting that we would want to keep previously cached video plane geometry, and to post a SetGeometry call to VideoPlane at this point to re-establish that VideoPlane's state matches VideoPlaneController.
https://codereview.chromium.org/1531543002/diff/60001/chromecast/browser/medi... File chromecast/browser/media/cma_media_pipeline_client.cc (right): https://codereview.chromium.org/1531543002/diff/60001/chromecast/browser/medi... chromecast/browser/media/cma_media_pipeline_client.cc:40: CastResource::Resource resource) {} wrap line between { and }? https://codereview.chromium.org/1531543002/diff/60001/chromecast/media/base/v... File chromecast/media/base/video_plane_controller.h (right): https://codereview.chromium.org/1531543002/diff/60001/chromecast/media/base/v... chromecast/media/base/video_plane_controller.h:63: // The Set methods will however updated cached parameters that will take s/updated/update
https://codereview.chromium.org/1531543002/diff/60001/chromecast/base/cast_re... File chromecast/base/cast_resource.h (right): https://codereview.chromium.org/1531543002/diff/60001/chromecast/base/cast_re... chromecast/base/cast_resource.h:13: class CastResource { Can we rename this as CastResourceContainer or something? It's getting confusing. https://codereview.chromium.org/1531543002/diff/60001/chromecast/base/cast_re... chromecast/base/cast_resource.h:63: virtual void InitializeResource(Resource resource) = 0; GrantResource?
Please have a look at Patch Set 5. Thanks. https://codereview.chromium.org/1531543002/diff/60001/chromecast/base/cast_re... File chromecast/base/cast_resource.h (right): https://codereview.chromium.org/1531543002/diff/60001/chromecast/base/cast_re... chromecast/base/cast_resource.h:13: class CastResource { On 2015/12/18 00:53:21, byungchul wrote: > Can we rename this as CastResourceContainer or something? It's getting > confusing. Agreed. Doing later in another CL though since the CastResource class gets used a bunch of different places. Made b/26279969. https://codereview.chromium.org/1531543002/diff/60001/chromecast/base/cast_re... chromecast/base/cast_resource.h:63: virtual void InitializeResource(Resource resource) = 0; On 2015/12/18 00:53:21, byungchul wrote: > GrantResource? I actually prefer InitializeResource since this serves a different purpose than OEM's granting resources to cast. If we call this GrantResource too, it would get confusing IMO. https://codereview.chromium.org/1531543002/diff/60001/chromecast/browser/medi... File chromecast/browser/media/cma_media_pipeline_client.cc (right): https://codereview.chromium.org/1531543002/diff/60001/chromecast/browser/medi... chromecast/browser/media/cma_media_pipeline_client.cc:40: CastResource::Resource resource) {} On 2015/12/18 00:28:28, byungchul wrote: > wrap line between { and }? git cl format wants the braces on the same line. https://codereview.chromium.org/1531543002/diff/60001/chromecast/media/base/v... File chromecast/media/base/video_plane_controller.cc (right): https://codereview.chromium.org/1531543002/diff/60001/chromecast/media/base/v... chromecast/media/base/video_plane_controller.cc:227: } On 2015/12/17 14:46:31, halliwell wrote: > So ... it looks like from the internal CL that when we get resources back, we > ClearGeometryState and Resume. That does not sound right to me as it depends on > receiving further SetGeometry calls, which may be true in some cases but not > necessarily always. > > I was expecting that we would want to keep previously cached video plane > geometry, and to post a SetGeometry call to VideoPlane at this point to > re-establish that VideoPlane's state matches VideoPlaneController. Done. https://codereview.chromium.org/1531543002/diff/60001/chromecast/media/base/v... File chromecast/media/base/video_plane_controller.h (right): https://codereview.chromium.org/1531543002/diff/60001/chromecast/media/base/v... chromecast/media/base/video_plane_controller.h:63: // The Set methods will however updated cached parameters that will take On 2015/12/18 00:28:28, byungchul wrote: > s/updated/update Done.
https://codereview.chromium.org/1531543002/diff/80001/chromecast/media/base/v... File chromecast/media/base/video_plane_controller.h (right): https://codereview.chromium.org/1531543002/diff/80001/chromecast/media/base/v... chromecast/media/base/video_plane_controller.h:49: void ResetGeometry(); nit: blank line before comment. I don't quite like 'ResetGeometry' function name. It makes it sound like the geometry is being reset to a default or initial value. How about something like ApplyToVideoPlane? Or even ResetVideoPlane.
https://codereview.chromium.org/1531543002/diff/60001/chromecast/base/cast_re... File chromecast/base/cast_resource.h (right): https://codereview.chromium.org/1531543002/diff/60001/chromecast/base/cast_re... chromecast/base/cast_resource.h:63: virtual void InitializeResource(Resource resource) = 0; On 2015/12/21 00:39:21, esum wrote: > On 2015/12/18 00:53:21, byungchul wrote: > > GrantResource? > > I actually prefer InitializeResource since this serves a different purpose than > OEM's granting resources to cast. If we call this GrantResource too, it would > get confusing IMO. How about AcquireResource, since this is the opposite of ReleaseResource? Alternatively, we can rename ReleaseResource DeinitializeResource.
Rebase coming shortly. https://codereview.chromium.org/1531543002/diff/60001/chromecast/base/cast_re... File chromecast/base/cast_resource.h (right): https://codereview.chromium.org/1531543002/diff/60001/chromecast/base/cast_re... chromecast/base/cast_resource.h:63: virtual void InitializeResource(Resource resource) = 0; On 2016/01/04 17:29:10, derekjchow1 wrote: > On 2015/12/21 00:39:21, esum wrote: > > On 2015/12/18 00:53:21, byungchul wrote: > > > GrantResource? > > > > I actually prefer InitializeResource since this serves a different purpose > than > > OEM's granting resources to cast. If we call this GrantResource too, it would > > get confusing IMO. > > How about AcquireResource, since this is the opposite of ReleaseResource? > Alternatively, we can rename ReleaseResource DeinitializeResource. Done. Had to rearrange some names to make things clearer. Discussed with Yuchen offline just to be sure. https://codereview.chromium.org/1531543002/diff/80001/chromecast/media/base/v... File chromecast/media/base/video_plane_controller.h (right): https://codereview.chromium.org/1531543002/diff/80001/chromecast/media/base/v... chromecast/media/base/video_plane_controller.h:49: void ResetGeometry(); On 2015/12/28 22:22:31, halliwell wrote: > nit: blank line before comment. > > I don't quite like 'ResetGeometry' function name. It makes it sound like the > geometry is being reset to a default or initial value. > > How about something like ApplyToVideoPlane? Or even ResetVideoPlane. Method no longer needed since resetting the video plane is now an automatic part of Resume() per Derek's comment: https://eureka-internal-review.git.corp.google.com/#/c/42141/4/service/resour...
Patch Set 7 is just rebase of Patch Set 6. Please have a look. Sorry for delay.
Looking really close, although just reminded me how much the resource classes need cleanup and clarification :) https://codereview.chromium.org/1531543002/diff/120001/chromecast/base/cast_r... File chromecast/base/cast_resource.h (right): https://codereview.chromium.org/1531543002/diff/120001/chromecast/base/cast_r... chromecast/base/cast_resource.h:12: // Interface for resources needed to run application. This comment is terrible, btw. https://codereview.chromium.org/1531543002/diff/120001/chromecast/base/cast_r... chromecast/base/cast_resource.h:36: class Client { Could we clarify what a Client is? It's very much not clear from just looking at this header. https://codereview.chromium.org/1531543002/diff/120001/chromecast/base/cast_r... chromecast/base/cast_resource.h:38: // Called to register CastResource with client. CastResource should not be what does "should not be owned by client" mean? Is that trying to say "Client does not take ownership of CastResource?" https://codereview.chromium.org/1531543002/diff/120001/chromecast/base/cast_r... chromecast/base/cast_resource.h:52: Resource remain) = 0; Not a comment on your changes, but I'm having trouble with the naming in this header. What is a "CastResource". Why are there two types here? A "CastResource" is released, and 'Resource' remains ... we should clean up all the naming and concepts here :) https://codereview.chromium.org/1531543002/diff/120001/chromecast/browser/med... File chromecast/browser/media/cma_media_pipeline_client.cc (right): https://codereview.chromium.org/1531543002/diff/120001/chromecast/browser/med... chromecast/browser/media/cma_media_pipeline_client.cc:28: RegisterWithClient(); This makes me think RegisterWIthClient should be given a more explicit name (at least mention resources?). Or scope it with the class name here. When you're just reading CmaMediaPipelineClient, this callsite is pretty mysterious. https://codereview.chromium.org/1531543002/diff/120001/chromecast/media/base/... File chromecast/media/base/video_plane_controller.h (right): https://codereview.chromium.org/1531543002/diff/120001/chromecast/media/base/... chromecast/media/base/video_plane_controller.h:63: // made except for any pending calls already scheduled on the media thread. Technically, doesn't this mean we're not implementing resource relinquish correctly? I.e. we could be saying we've relinquished when there are some pending calls still? We could probably follow up on that separately given this addresses the resume behaviour.
Let me think how best to rename/reword things in CastResource for 1.18 tomorrow. Byungchul, Tavis, and I spent 1.5 hours today discussing this and did not quite reach a definitive model (or set of names). Also Derek has refactor ideas for 1.19, so need to consider that too. https://codereview.chromium.org/1531543002/diff/120001/chromecast/media/base/... File chromecast/media/base/video_plane_controller.h (right): https://codereview.chromium.org/1531543002/diff/120001/chromecast/media/base/... chromecast/media/base/video_plane_controller.h:63: // made except for any pending calls already scheduled on the media thread. On 2016/01/12 03:25:07, halliwell wrote: > Technically, doesn't this mean we're not implementing resource relinquish > correctly? I.e. we could be saying we've relinquished when there are some > pending calls still? > > We could probably follow up on that separately given this addresses the resume > behaviour. Ah, so I wasn't sure if this was worth the extra code, but it sounds like it is. What actually happens if a couple SetGeometry calls on the media thread slip through after the OEM revokes resources? Is it just implementation dependent? To fix, my first thought is Pause/Resume become asynchronous, so CastResource::Acquire/RevokeResource are asynchronous, which complicates the logic in ResourceManagerImpl even more.
https://codereview.chromium.org/1531543002/diff/120001/chromecast/media/base/... File chromecast/media/base/video_plane_controller.h (right): https://codereview.chromium.org/1531543002/diff/120001/chromecast/media/base/... chromecast/media/base/video_plane_controller.h:63: // made except for any pending calls already scheduled on the media thread. On 2016/01/12 04:51:47, esum wrote: > On 2016/01/12 03:25:07, halliwell wrote: > > Technically, doesn't this mean we're not implementing resource relinquish > > correctly? I.e. we could be saying we've relinquished when there are some > > pending calls still? > > > > We could probably follow up on that separately given this addresses the resume > > behaviour. > > Ah, so I wasn't sure if this was worth the extra code, but it sounds like it is. > What actually happens if a couple SetGeometry calls on the media thread slip > through after the OEM revokes resources? Is it just implementation dependent? > It is implementation-dependent, yes. I don't think it is worth holding up this change though. Remember that we currently don't handle media resource relinquish in a fine-grained way at all. In order to release video decoder resources, we just stop the current application. > To fix, my first thought is Pause/Resume become asynchronous, so > CastResource::Acquire/RevokeResource are asynchronous, which complicates the > logic in ResourceManagerImpl even more. I think we should leave this issue for now, but add a TODO.
This is just my take on the CURRENT model. Yuchen, please correct me if I'm wrong. The biggest confusion is that the term "resource" is overloaded with 2 definitions: a) A resource native to the PLATFORM that both the OEM and cast_shell need to operate. Because both parties care about these types of resources, the cast_control api is needed to regulate ownership. Current name: CastResource::Resource enum. Ex: Primary Screen, Secondary Screen, Audio b) A CAST-SPECIFIC resource that only cast_shell has knowledge of; OEM's don't know about these, nor do they care. Each of these resources USES 1 or more of the platform resources. Now because they use platform resources, they need to know when platform resources are getting granted/revoked by the OEM so that they can do any cast-specific initialization/deinitialization. And conversely, ResourceManager needs to know when these resources are DONE doing said initialization/deinitialization. Current name: CastResource Ex: The cast media pipeline uses Primary Screen and Audio, The gpu process uses Primary Screen With those definitions in mind, the major short-term changes I propose: 1) For resource a, CastResource::Resource -> PlatformResource. I don't see a conceptual reason why it should be encapsulated in the class representing resource b. How about we move it out? 2) For resource b, CastResource actually might be OK with more commenting. Alternatively, since it USES the platform resources, maybe PlatformResourceConsumer? 3) ResourceProviderChromecast -> GraphicsResourceConsumer. It initializes/deinitializes things that require graphics-related platform resources. 4) CmaMediaPipelineClient. Unsure on new name. Please recommend a change if any. 5) CastResource::Client. This is basically used as an interface for CastResource to notify someone when it's done initializing/deinitializing resources. Since this is the recipient of notifications, how about we name it Observer instead of Client. Note that this is basically ResourceManager. We might need other smaller changes, but these are the biggest. PLEASE APPROVE/DISAPPROVE BEFORE I MAKE CHANGES.
On 2016/01/12 19:58:15, esum wrote: > This is just my take on the CURRENT model. Yuchen, please correct me if I'm > wrong. > > The biggest confusion is that the term "resource" is overloaded with 2 > definitions: > > a) A resource native to the PLATFORM that both the OEM and cast_shell need to > operate. Because both parties care about these types of resources, the > cast_control api is needed to regulate ownership. > > Current name: CastResource::Resource enum. > Ex: Primary Screen, Secondary Screen, Audio > > b) A CAST-SPECIFIC resource that only cast_shell has knowledge of; OEM's don't > know about these, nor do they care. Each of these resources USES 1 or more of > the platform resources. Now because they use platform resources, they need to > know when platform resources are getting granted/revoked by the OEM so that they > can do any cast-specific initialization/deinitialization. And conversely, > ResourceManager needs to know when these resources are DONE doing said > initialization/deinitialization. > > Current name: CastResource > Ex: The cast media pipeline uses Primary Screen and Audio, The gpu process uses > Primary Screen > > With those definitions in mind, the major short-term changes I propose: > > 1) For resource a, CastResource::Resource -> PlatformResource. I don't see a > conceptual reason why it should be encapsulated in the class representing > resource b. How about we move it out? > > 2) For resource b, CastResource actually might be OK with more commenting. > Alternatively, since it USES the platform resources, maybe > PlatformResourceConsumer? > > 3) ResourceProviderChromecast -> GraphicsResourceConsumer. It > initializes/deinitializes things that require graphics-related platform > resources. > > 4) CmaMediaPipelineClient. Unsure on new name. Please recommend a change if any. > > 5) CastResource::Client. This is basically used as an interface for CastResource > to notify someone when it's done initializing/deinitializing resources. Since > this is the recipient of notifications, how about we name it Observer instead of > Client. Note that this is basically ResourceManager. > > We might need other smaller changes, but these are the biggest. PLEASE > APPROVE/DISAPPROVE BEFORE I MAKE CHANGES. CastResource is a holder/container for CastResource::Resource (I don't like the name consumer, because CastResource will return the resource eventually. Resource is never consumed, it's transferred). Now in our code, CastResource(s) are some cast_shell components. In the initial design, CastResource::Client is used to decouple CastResource from ResourceManager, since it's not upstreamed. CmaMediaPipelineClient is designed as the interface for notifying browser process what happens in the media pipeline. It decouples media pipeline from the rest of the world. CmaMediaPipelineClient now works as a bridge between resource management and media pipeline. For this reason, I'd like to keep the name CmaMediaPipelineClient unchanged.
> CastResource is a holder/container for CastResource::Resource (I don't like the > name consumer, because CastResource will return the resource eventually. > Resource is never consumed, it's transferred). Now in our code, CastResource(s) > are some cast_shell components. Maybe I don't understand CastResource then. What does being a "holder/container" for CastResource::Resource mean? Can you clarify because that sounds to me like it's supposed to store them internally somehow. Alos, does this mean you don't like proposed change 1)?
On 2016/01/12 21:25:03, esum wrote: > > CastResource is a holder/container for CastResource::Resource (I don't like > the > > name consumer, because CastResource will return the resource eventually. > > Resource is never consumed, it's transferred). Now in our code, > CastResource(s) > > are some cast_shell components. > > Maybe I don't understand CastResource then. What does being a "holder/container" > for CastResource::Resource mean? Can you clarify because that sounds to me like > it's supposed to store them internally somehow. Alos, does this mean you don't > like proposed change 1)? For example, cma media pipeline (client) can hold/own hardware resource (primary screen and audio) when playing movie. Calling it a container may be misleading. During that time, the resource is owned/used by media pipeline. I'm ok with proposal 1.
> For example, cma media pipeline (client) can hold/own hardware resource (primary > screen and audio) when playing movie. Calling it a container may be misleading. > During that time, the resource is owned/used by media pipeline. I'm ok with > proposal 1. Ah thanks. I think I see what you're saying. In that case, since an instance of the class "owns" the hardware resource, why don't we: 1) Forget CastResource::Resource -> PlatformResource. Just call the enum Resource, and move it out of CastResource scope. 2) CastResource -> ResourceOwner. Conceptually, it sounds like a "Resource" can have multiple owners. 3) ResourceProviderChromecast -> GraphicsResourceOwner. (We can create an AudioResourceOwner, etc. if we need more owners). 4) Keep CmaMediaPipelineClient the same. (But we can think of it as MediaResourceOwner). 5) What do you think of proposal 5 (Client -> Observer)? Personally, I'm actually OK with "Client", but that term is pretty generic, so we need to add more comments.
Also if "Resource is never consumed, it's transferred" is the terminology we are using, then I think ResourceManagerImpl::Consumer needs to get renamed.
On 2016/01/12 22:01:21, esum wrote: > > For example, cma media pipeline (client) can hold/own hardware resource > (primary > > screen and audio) when playing movie. Calling it a container may be > misleading. > > During that time, the resource is owned/used by media pipeline. I'm ok with > > proposal 1. > > Ah thanks. I think I see what you're saying. In that case, since an instance of > the class "owns" the hardware resource, why don't we: > > 1) Forget CastResource::Resource -> PlatformResource. Just call the enum > Resource, and move it out of CastResource scope. > 2) CastResource -> ResourceOwner. Conceptually, it sounds like a "Resource" can > have multiple owners. > 3) ResourceProviderChromecast -> GraphicsResourceOwner. (We can create an > AudioResourceOwner, etc. if we need more owners). > 4) Keep CmaMediaPipelineClient the same. (But we can think of it as > MediaResourceOwner). > 5) What do you think of proposal 5 (Client -> Observer)? Personally, I'm > actually OK with "Client", but that term is pretty generic, so we need to add > more comments. I would recommend to add prefix/namespace/outer class to Resource, otherwise it will be mixed up with "resources bundle". I just find a cast_resource_delegate in chromecast/common, which means we do need to rename CastResource. For proposal 5, we can keep Client, but maybe we need to add more comments.
> I would recommend to add prefix/namespace/outer class to Resource, otherwise it > will be mixed up with "resources bundle". I just find a cast_resource_delegate > in chromecast/common, which means we do need to rename CastResource. Great even more usage of the term "resource" :). Let me just prefix the enum, and call it PlatformResource, and the owner class will be called PlatformResourceOwner. > For proposal 5, we can keep Client, but maybe we need to add more comments. OK. Definitely more commenting. Others, please confirm ultimate changes. Again to summarize: 1) CastResource::Resource moves out of class to PlatformResource. 2) CastResource -> PlatformResourceOwner. 3) ResourceProviderChromecast -> GraphicsPlatformResourceOwner.
On 2016/01/12 22:27:40, esum wrote:
> > I would recommend to add prefix/namespace/outer class to Resource, otherwise
> it
> > will be mixed up with "resources bundle". I just find a
cast_resource_delegate
> > in chromecast/common, which means we do need to rename CastResource.
>
> Great even more usage of the term "resource" :). Let me just prefix the enum,
> and call it PlatformResource, and the owner class will be called
> PlatformResourceOwner.
>
> > For proposal 5, we can keep Client, but maybe we need to add more comments.
>
> OK. Definitely more commenting.
>
> Others, please confirm ultimate changes. Again to summarize:
>
> 1) CastResource::Resource moves out of class to PlatformResource.
> 2) CastResource -> PlatformResourceOwner.
> 3) ResourceProviderChromecast -> GraphicsPlatformResourceOwner.
SGTM for PlatformResource though I want more change like:
class PlatformResource {
enum Resource { // previous CastResource::Resource
None, Audio, ScreenPrimary, ScreenSecondary, ...
};
class User { // previous CastResource
User(Resource resource);
virtual OnResourceAvailable(Resource resource) = 0; // or
OnResourceGranted, previous CastResource::AcquireResource
virtual OnResourceUnavailable(Resource resource) = 0; // or
OnResourceRevoked, previous CastResource::ReleaseResource
};
class Manager { // previous CastResource::Client
virtual void RegisterUser(User* user) = 0; // previous
CastResource::Client::RegisterCastResource
virtual void LockResource(Resource resource) = 0; // or UseResource,
previous CastResource::Client::OnResourceAcquired
virtual void UnlockResource(Resource resource) = 0; // or UnuseResource,
previous CastResource::Client::OnResourceReleased
};
};
Basically, PlatformResource::Manager or ResourceManager needs do ref-counting
for each platform resource with Lock/UnlockResource. When ref-count is 0 for
platform resource, it can release the resource effectively.
> SGTM for PlatformResource though I want more change like:
>
> class PlatformResource {
> enum Resource { // previous CastResource::Resource
> None, Audio, ScreenPrimary, ScreenSecondary, ...
> };
>
> class User { // previous CastResource
> User(Resource resource);
> virtual OnResourceAvailable(Resource resource) = 0; // or
> OnResourceGranted, previous CastResource::AcquireResource
> virtual OnResourceUnavailable(Resource resource) = 0; // or
> OnResourceRevoked, previous CastResource::ReleaseResource
> };
>
> class Manager { // previous CastResource::Client
> virtual void RegisterUser(User* user) = 0; // previous
> CastResource::Client::RegisterCastResource
> virtual void LockResource(Resource resource) = 0; // or UseResource,
> previous CastResource::Client::OnResourceAcquired
> virtual void UnlockResource(Resource resource) = 0; // or UnuseResource,
> previous CastResource::Client::OnResourceReleased
> };
> };
>
> Basically, PlatformResource::Manager or ResourceManager needs do ref-counting
> for each platform resource with Lock/UnlockResource. When ref-count is 0 for
> platform resource, it can release the resource effectively.
Discussed with Byungchul, and this idea overall SGTM.
Yuchen/Tavis/Derek/Luke, please respond with approval/disapproval and share
thoughts. I need everyone's approval to proceed with this CL.
If approved, I'm going to change JUST the names and leave the ref-counting logic
for after 1.18.
On 2016/01/13 23:27:48, esum wrote:
> > SGTM for PlatformResource though I want more change like:
> >
> > class PlatformResource {
> > enum Resource { // previous CastResource::Resource
> > None, Audio, ScreenPrimary, ScreenSecondary, ...
> > };
> >
> > class User { // previous CastResource
> > User(Resource resource);
> > virtual OnResourceAvailable(Resource resource) = 0; // or
> > OnResourceGranted, previous CastResource::AcquireResource
> > virtual OnResourceUnavailable(Resource resource) = 0; // or
> > OnResourceRevoked, previous CastResource::ReleaseResource
> > };
> >
> > class Manager { // previous CastResource::Client
> > virtual void RegisterUser(User* user) = 0; // previous
> > CastResource::Client::RegisterCastResource
> > virtual void LockResource(Resource resource) = 0; // or UseResource,
> > previous CastResource::Client::OnResourceAcquired
> > virtual void UnlockResource(Resource resource) = 0; // or
UnuseResource,
> > previous CastResource::Client::OnResourceReleased
> > };
> > };
> >
> > Basically, PlatformResource::Manager or ResourceManager needs do
ref-counting
> > for each platform resource with Lock/UnlockResource. When ref-count is 0 for
> > platform resource, it can release the resource effectively.
>
>
> Discussed with Byungchul, and this idea overall SGTM.
>
> Yuchen/Tavis/Derek/Luke, please respond with approval/disapproval and share
> thoughts. I need everyone's approval to proceed with this CL.
>
> If approved, I'm going to change JUST the names and leave the ref-counting
logic
> for after 1.18.
Please rename PlatformResource::Resource to PlatformResource::ResourceId.
*Resource::Resource is confusing.
User and Manager names SGTM.
On 2016/01/13 23:27:48, esum wrote:
> > SGTM for PlatformResource though I want more change like:
> >
> > class PlatformResource {
> > enum Resource { // previous CastResource::Resource
> > None, Audio, ScreenPrimary, ScreenSecondary, ...
> > };
> >
> > class User { // previous CastResource
> > User(Resource resource);
> > virtual OnResourceAvailable(Resource resource) = 0; // or
> > OnResourceGranted, previous CastResource::AcquireResource
> > virtual OnResourceUnavailable(Resource resource) = 0; // or
> > OnResourceRevoked, previous CastResource::ReleaseResource
> > };
> >
> > class Manager { // previous CastResource::Client
> > virtual void RegisterUser(User* user) = 0; // previous
> > CastResource::Client::RegisterCastResource
> > virtual void LockResource(Resource resource) = 0; // or UseResource,
> > previous CastResource::Client::OnResourceAcquired
> > virtual void UnlockResource(Resource resource) = 0; // or
UnuseResource,
> > previous CastResource::Client::OnResourceReleased
> > };
> > };
> >
> > Basically, PlatformResource::Manager or ResourceManager needs do
ref-counting
> > for each platform resource with Lock/UnlockResource. When ref-count is 0 for
> > platform resource, it can release the resource effectively.
>
>
> Discussed with Byungchul, and this idea overall SGTM.
>
> Yuchen/Tavis/Derek/Luke, please respond with approval/disapproval and share
> thoughts. I need everyone's approval to proceed with this CL.
>
> If approved, I'm going to change JUST the names and leave the ref-counting
logic
> for after 1.18.
I'm fine with just renaming for now. Maybe can run the other plan by me later
when you get around to implementing it? :)
On 2016/01/13 23:32:37, halliwell wrote:
> On 2016/01/13 23:27:48, esum wrote:
> > > SGTM for PlatformResource though I want more change like:
> > >
> > > class PlatformResource {
> > > enum Resource { // previous CastResource::Resource
> > > None, Audio, ScreenPrimary, ScreenSecondary, ...
> > > };
> > >
> > > class User { // previous CastResource
> > > User(Resource resource);
> > > virtual OnResourceAvailable(Resource resource) = 0; // or
> > > OnResourceGranted, previous CastResource::AcquireResource
> > > virtual OnResourceUnavailable(Resource resource) = 0; // or
> > > OnResourceRevoked, previous CastResource::ReleaseResource
> > > };
> > >
> > > class Manager { // previous CastResource::Client
> > > virtual void RegisterUser(User* user) = 0; // previous
> > > CastResource::Client::RegisterCastResource
> > > virtual void LockResource(Resource resource) = 0; // or UseResource,
> > > previous CastResource::Client::OnResourceAcquired
> > > virtual void UnlockResource(Resource resource) = 0; // or
> UnuseResource,
> > > previous CastResource::Client::OnResourceReleased
> > > };
> > > };
> > >
> > > Basically, PlatformResource::Manager or ResourceManager needs do
> ref-counting
> > > for each platform resource with Lock/UnlockResource. When ref-count is 0
for
> > > platform resource, it can release the resource effectively.
> >
> >
> > Discussed with Byungchul, and this idea overall SGTM.
> >
> > Yuchen/Tavis/Derek/Luke, please respond with approval/disapproval and share
> > thoughts. I need everyone's approval to proceed with this CL.
> >
> > If approved, I'm going to change JUST the names and leave the ref-counting
> logic
> > for after 1.18.
>
> I'm fine with just renaming for now. Maybe can run the other plan by me later
> when you get around to implementing it? :)
Is PlatformResource just a namespace?
> > I'm fine with just renaming for now. Maybe can run the other plan by me
later
> > when you get around to implementing it? :)
Sure :).
> Is PlatformResource just a namespace?
Yes, good point. Let's do this:
namespace chromecast {
// Named PlatformResource to avoid confusion with "Resource" usage upstream
enum PlatformResource {
None, Audio, ScreenPrimary, ScreenSecondary, ...
}
class PlatformResourceUser { // previous CastResource
PlatformResourceUser(PlatformResource resource);
virtual OnResourceAvailable(PlatformResource resource) = 0; // previous
CastResource::AcquireResource
virtual OnResourceUnavailable(PlatformResource resource) = 0; // previous
CastResource::ReleaseResource
};
class PlatformResourceManager { // previous CastResource::Client
virtual void RegisterPlatformResourceUser(PlatformResourceUser* user) = 0;
// previous CastResource::Client::RegisterCastResource
virtual void LockResource(PlatformResource resource) = 0; // previous
CastResource::Client::OnResourceAcquired
virtual void UnlockResource(PlatformResource resource) = 0; // previous
CastResource::Client::OnResourceReleased
};
}
Admittedly, PlatformResource is a little verbose to see everywhere. Is that OK?
On 2016/01/13 23:56:22, esum wrote:
> > > I'm fine with just renaming for now. Maybe can run the other plan by me
> later
> > > when you get around to implementing it? :)
>
> Sure :).
>
> > Is PlatformResource just a namespace?
>
> Yes, good point. Let's do this:
>
> namespace chromecast {
>
> // Named PlatformResource to avoid confusion with "Resource" usage upstream
> enum PlatformResource {
> None, Audio, ScreenPrimary, ScreenSecondary, ...
> }
>
> class PlatformResourceUser { // previous CastResource
> PlatformResourceUser(PlatformResource resource);
> virtual OnResourceAvailable(PlatformResource resource) = 0; // previous
> CastResource::AcquireResource
> virtual OnResourceUnavailable(PlatformResource resource) = 0; // previous
> CastResource::ReleaseResource
> };
>
> class PlatformResourceManager { // previous CastResource::Client
> virtual void RegisterPlatformResourceUser(PlatformResourceUser* user) = 0;
> // previous CastResource::Client::RegisterCastResource
> virtual void LockResource(PlatformResource resource) = 0; // previous
> CastResource::Client::OnResourceAcquired
> virtual void UnlockResource(PlatformResource resource) = 0; // previous
> CastResource::Client::OnResourceReleased
> };
>
> }
>
> Admittedly, PlatformResource is a little verbose to see everywhere. Is that
OK?
Also, Derek, this should bypass PlatformResource::Resource. Agreed that would
look confusing.
> > namespace chromecast {
> >
> > // Named PlatformResource to avoid confusion with "Resource" usage
upstream
> > enum PlatformResource {
> > None, Audio, ScreenPrimary, ScreenSecondary, ...
> > }
> >
> > class PlatformResourceUser { // previous CastResource
> > PlatformResourceUser(PlatformResource resource);
> > virtual OnResourceAvailable(PlatformResource resource) = 0; // previous
> > CastResource::AcquireResource
> > virtual OnResourceUnavailable(PlatformResource resource) = 0; //
previous
> > CastResource::ReleaseResource
> > };
> >
> > class PlatformResourceManager { // previous CastResource::Client
> > virtual void RegisterPlatformResourceUser(PlatformResourceUser* user) =
0;
>
> > // previous CastResource::Client::RegisterCastResource
> > virtual void LockResource(PlatformResource resource) = 0; // previous
> > CastResource::Client::OnResourceAcquired
> > virtual void UnlockResource(PlatformResource resource) = 0; // previous
> > CastResource::Client::OnResourceReleased
> > };
> >
> > }
The actual bug needs to get fixed soon, and we are having a hard time agreeing
on a model.
I need to do SOMETHING now. Either just use naming above for the time being, or
keep names as is with more commenting? What's best?
On 2016/01/15 01:35:26, esum wrote:
> > > namespace chromecast {
> > >
> > > // Named PlatformResource to avoid confusion with "Resource" usage
> upstream
> > > enum PlatformResource {
> > > None, Audio, ScreenPrimary, ScreenSecondary, ...
> > > }
> > >
> > > class PlatformResourceUser { // previous CastResource
> > > PlatformResourceUser(PlatformResource resource);
> > > virtual OnResourceAvailable(PlatformResource resource) = 0; //
previous
> > > CastResource::AcquireResource
> > > virtual OnResourceUnavailable(PlatformResource resource) = 0; //
> previous
> > > CastResource::ReleaseResource
> > > };
> > >
> > > class PlatformResourceManager { // previous CastResource::Client
> > > virtual void RegisterPlatformResourceUser(PlatformResourceUser* user)
=
> 0;
> >
> > > // previous CastResource::Client::RegisterCastResource
> > > virtual void LockResource(PlatformResource resource) = 0; // previous
> > > CastResource::Client::OnResourceAcquired
> > > virtual void UnlockResource(PlatformResource resource) = 0; //
previous
> > > CastResource::Client::OnResourceReleased
> > > };
> > >
> > > }
>
> The actual bug needs to get fixed soon, and we are having a hard time agreeing
> on a model.
>
> I need to do SOMETHING now. Either just use naming above for the time being,
or
> keep names as is with more commenting? What's best?
Let's just leave naming, sorry your review has been pretty derailed by
discussion of other problems :)
I'm going to leave a lgtm here, double check if anyone has remaining comments on
your actual bug fix, otherwise please submit and we'll follow up on resource
system after.
> Let's just leave naming, sorry your review has been pretty derailed by > discussion of other problems :) > > I'm going to leave a lgtm here, double check if anyone has remaining comments on > your actual bug fix, otherwise please submit and we'll follow up on resource > system after. No worries. I'm still adding comments where there was the most confusion for the time being.
Rebase coming shortly. Just adding more comments. https://codereview.chromium.org/1531543002/diff/120001/chromecast/base/cast_r... File chromecast/base/cast_resource.h (right): https://codereview.chromium.org/1531543002/diff/120001/chromecast/base/cast_r... chromecast/base/cast_resource.h:12: // Interface for resources needed to run application. On 2016/01/12 03:25:06, halliwell wrote: > This comment is terrible, btw. Modified to be a little clearer for the time being. https://codereview.chromium.org/1531543002/diff/120001/chromecast/base/cast_r... chromecast/base/cast_resource.h:36: class Client { On 2016/01/12 03:25:07, halliwell wrote: > Could we clarify what a Client is? It's very much not clear from just looking > at this header. Done. https://codereview.chromium.org/1531543002/diff/120001/chromecast/base/cast_r... chromecast/base/cast_resource.h:38: // Called to register CastResource with client. CastResource should not be On 2016/01/12 03:25:06, halliwell wrote: > what does "should not be owned by client" mean? Is that trying to say "Client > does not take ownership of CastResource?" Yes. Done. https://codereview.chromium.org/1531543002/diff/120001/chromecast/base/cast_r... chromecast/base/cast_resource.h:52: Resource remain) = 0; On 2016/01/12 03:25:06, halliwell wrote: > Not a comment on your changes, but I'm having trouble with the naming in this > header. What is a "CastResource". Why are there two types here? A > "CastResource" is released, and 'Resource' remains ... we should clean up all > the naming and concepts here :) Discussed and will leave as is now. Will be renamed in coming refactor. https://codereview.chromium.org/1531543002/diff/120001/chromecast/browser/med... File chromecast/browser/media/cma_media_pipeline_client.cc (right): https://codereview.chromium.org/1531543002/diff/120001/chromecast/browser/med... chromecast/browser/media/cma_media_pipeline_client.cc:28: RegisterWithClient(); On 2016/01/12 03:25:07, halliwell wrote: > This makes me think RegisterWIthClient should be given a more explicit name (at > least mention resources?). Or scope it with the class name here. When you're > just reading CmaMediaPipelineClient, this callsite is pretty mysterious. Done. Scoped it with class name. Also added comments to CastResource::RegisterWithClient. Hope that helps a little. https://codereview.chromium.org/1531543002/diff/120001/chromecast/media/base/... File chromecast/media/base/video_plane_controller.h (right): https://codereview.chromium.org/1531543002/diff/120001/chromecast/media/base/... chromecast/media/base/video_plane_controller.h:63: // made except for any pending calls already scheduled on the media thread. On 2016/01/12 15:40:38, halliwell wrote: > On 2016/01/12 04:51:47, esum wrote: > > On 2016/01/12 03:25:07, halliwell wrote: > > > Technically, doesn't this mean we're not implementing resource relinquish > > > correctly? I.e. we could be saying we've relinquished when there are some > > > pending calls still? > > > > > > We could probably follow up on that separately given this addresses the > resume > > > behaviour. > > > > Ah, so I wasn't sure if this was worth the extra code, but it sounds like it > is. > > What actually happens if a couple SetGeometry calls on the media thread slip > > through after the OEM revokes resources? Is it just implementation dependent? > > > It is implementation-dependent, yes. I don't think it is worth holding up this > change though. Remember that we currently don't handle media resource > relinquish in a fine-grained way at all. In order to release video decoder > resources, we just stop the current application. > > > To fix, my first thought is Pause/Resume become asynchronous, so > > CastResource::Acquire/RevokeResource are asynchronous, which complicates the > > logic in ResourceManagerImpl even more. > > I think we should leave this issue for now, but add a TODO. Done.
The CQ bit was checked by esum@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/1531543002/#ps160001 (title: "Rebase of Patch Set 8.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1531543002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1531543002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from
==========
[Chromecast] Adding pause/resume operations to VideoPlaneController.
Also adding method for initializing resources after they have been
granted to cast.
BUG=internal b/25812819
TEST=Revoke/Grant resources during youtube playback on redwood.
Youtube/Netflix playback on earth (no regressions).
==========
to
==========
[Chromecast] Adding pause/resume operations to VideoPlaneController.
Also adding method for initializing resources after they have been
granted to cast.
BUG=internal b/25812819
TEST=Revoke/Grant resources during youtube playback on redwood.
Youtube/Netflix playback on earth (no regressions).
Committed: https://crrev.com/8d560a8c717968a3391be2341dc9a5170a4be310
Cr-Commit-Position: refs/heads/master@{#369937}
==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/8d560a8c717968a3391be2341dc9a5170a4be310 Cr-Commit-Position: refs/heads/master@{#369937} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
