Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
174323003:
Expose locks for CopyFromCompositingSurface/CopyFromBackingStore API (Closed)
|
Created:
6 years, 10 months ago by powei Modified:
6 years, 9 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, jam, penghuang+watch_chromium.org, joi+watch-content_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, James Su, miu+watch_chromium.org, aelias_OOO_until_Jul13 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionExpose locks for CopyFromCompositingSurface/CopyFromBackingStore API
For async implementation of CopyFromCompositingSurface/CopyFromBackingStore in
RenderWidgetHostView and RenderWidgetHost, we need locks to ensure that the
content cannot be evicted during copy.
BUG=326363
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258476
Patch Set 1 #Patch Set 2 : upload error #
Total comments: 4
Patch Set 3 : addressed comments #
Total comments: 2
Patch Set 4 : reworded and added comments #
Total comments: 7
Patch Set 5 : added android-side change #
Total comments: 2
Patch Set 6 : added NOTIMPLEMENTED() and put back CanCopyFromBackingStore() #
Total comments: 4
Patch Set 7 : changed lock/unlock implementation to accommodate for CVC being swapped out #Patch Set 8 : rebased #Patch Set 9 : addressed comments #
Total comments: 6
Patch Set 10 : addressed comments #
Total comments: 2
Patch Set 11 : addressed comment about saving last frame during lock #
Total comments: 2
Patch Set 12 : nit and removed incorrect early out #
Total comments: 8
Patch Set 13 : addressed comments #
Total comments: 4
Patch Set 14 : addressed comment about comment #Patch Set 15 : LockSurface -> LockCompositingSurface #Patch Set 16 : fix android clang bot #Patch Set 17 : fix android clang bot try 2 #Messages
Total messages: 60 (0 generated)
ptal sievers@. piman@ is OOO for a little over a week. Would you recommend I wait or is there another suitable reviewer?
https://codereview.chromium.org/174323003/diff/90001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/174323003/diff/90001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.cc:753: view_->IsSurfaceAvailableForCopy(); return view_->IsSurfaceAvailableForCopy(); https://codereview.chromium.org/174323003/diff/90001/content/public/browser/r... File content/public/browser/render_widget_host_view.h (right): https://codereview.chromium.org/174323003/diff/90001/content/public/browser/r... content/public/browser/render_widget_host_view.h:90: virtual void LockSurfaceForCopy() = 0; Can this be in RenderWidgetHostViewPort instead, so it's not public?
https://codereview.chromium.org/174323003/diff/90001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/174323003/diff/90001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.cc:753: view_->IsSurfaceAvailableForCopy(); On 2014/02/21 02:01:37, sievers wrote: > return view_->IsSurfaceAvailableForCopy(); Done. (oops) https://codereview.chromium.org/174323003/diff/90001/content/public/browser/r... File content/public/browser/render_widget_host_view.h (right): https://codereview.chromium.org/174323003/diff/90001/content/public/browser/r... content/public/browser/render_widget_host_view.h:90: virtual void LockSurfaceForCopy() = 0; On 2014/02/21 02:01:37, sievers wrote: > Can this be in RenderWidgetHostViewPort instead, so it's not public? Done.
+jam https://codereview.chromium.org/174323003/diff/220002/content/port/browser/re... File content/port/browser/render_widget_host_view_port.h (right): https://codereview.chromium.org/174323003/diff/220002/content/port/browser/re... content/port/browser/render_widget_host_view_port.h:188: // content is not evicted while copying. I'd say that it instructs the view to not drop the surface even when the view is hidden. I'd also put a comment in the public interface.
https://codereview.chromium.org/174323003/diff/220002/content/port/browser/re... File content/port/browser/render_widget_host_view_port.h (right): https://codereview.chromium.org/174323003/diff/220002/content/port/browser/re... content/port/browser/render_widget_host_view_port.h:188: // content is not evicted while copying. On 2014/02/21 02:34:10, sievers wrote: > I'd say that it instructs the view to not drop the surface even when the view is > hidden. > > I'd also put a comment in the public interface. Done.
lgtm
needs jam@ for OWNERS
https://codereview.chromium.org/174323003/diff/350001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/174323003/diff/350001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.cc:596: void RenderWidgetHostViewBase::LockSurfaceForCopy() {} where is the implementation of this? https://codereview.chromium.org/174323003/diff/350001/content/public/browser/... File content/public/browser/render_widget_host.h (right): https://codereview.chromium.org/174323003/diff/350001/content/public/browser/... content/public/browser/render_widget_host.h:203: virtual bool CanCopyFromBackingStore() = 0; so to be clear, this is in the public api since chrome on android, which isn't in this repository, is calling these methods? https://codereview.chromium.org/174323003/diff/350001/content/public/browser/... content/public/browser/render_widget_host.h:206: virtual void UnlockForCopyFromBackingStore() = 0; I'm sure there's a reason why you're exposing these methods, but right now I'm not seeing it. Per the cl subject, you're doing this so that the content isn't evicted when calling async CopyFromBackingStore/CopyFromCompositingSurface. But I don't understand why you need public locking methods. Why can't the implementation of CopyFromBackingStore/CopyFromCompositingSurface do this internally since they know when there's an oustanding callback?
https://codereview.chromium.org/174323003/diff/350001/content/public/browser/... File content/public/browser/render_widget_host.h (right): https://codereview.chromium.org/174323003/diff/350001/content/public/browser/... content/public/browser/render_widget_host.h:203: virtual bool CanCopyFromBackingStore() = 0; On 2014/02/26 00:09:54, jam wrote: > so to be clear, this is in the public api since chrome on android, which isn't > in this repository, is calling these methods? Actually we don't have to add this one. It already exists in content/public/render_widget_host_view.h: // Returns true is the current display surface is available. virtual bool IsSurfaceAvailableForCopy() const = 0; https://codereview.chromium.org/174323003/diff/350001/content/public/browser/... content/public/browser/render_widget_host.h:206: virtual void UnlockForCopyFromBackingStore() = 0; On 2014/02/26 00:09:54, jam wrote: > I'm sure there's a reason why you're exposing these methods, but right now I'm > not seeing it. > > Per the cl subject, you're doing this so that the content isn't evicted when > calling async CopyFromBackingStore/CopyFromCompositingSurface. But I don't > understand why you need public locking methods. Why can't the implementation of > CopyFromBackingStore/CopyFromCompositingSurface do this internally since they > know when there's an oustanding callback? The problem is that the application wants to replace the live tab with a placeholder approximation which needs to happen asynchronously. While this happens and until this is ready and added to the compositor tree, we cannot let RWHV release the live surface. It's sort of an override for 'frontbuffer' memory management which the app is not really involved in.
On 2014/02/26 00:25:55, sievers wrote: > https://codereview.chromium.org/174323003/diff/350001/content/public/browser/... > File content/public/browser/render_widget_host.h (right): > > https://codereview.chromium.org/174323003/diff/350001/content/public/browser/... > content/public/browser/render_widget_host.h:203: virtual bool > CanCopyFromBackingStore() = 0; > On 2014/02/26 00:09:54, jam wrote: > > so to be clear, this is in the public api since chrome on android, which isn't > > in this repository, is calling these methods? > > Actually we don't have to add this one. It already exists in > content/public/render_widget_host_view.h: > // Returns true is the current display surface is available. > virtual bool IsSurfaceAvailableForCopy() const = 0; > > https://codereview.chromium.org/174323003/diff/350001/content/public/browser/... > content/public/browser/render_widget_host.h:206: virtual void > UnlockForCopyFromBackingStore() = 0; > On 2014/02/26 00:09:54, jam wrote: > > I'm sure there's a reason why you're exposing these methods, but right now I'm > > not seeing it. > > > > Per the cl subject, you're doing this so that the content isn't evicted when > > calling async CopyFromBackingStore/CopyFromCompositingSurface. But I don't > > understand why you need public locking methods. Why can't the implementation > of > > CopyFromBackingStore/CopyFromCompositingSurface do this internally since they > > know when there's an oustanding callback? > > The problem is that the application wants to replace the live tab with a > placeholder approximation which needs to happen asynchronously. > > While this happens and until this is ready and added to the compositor tree, we > cannot let RWHV release the live surface. > > It's sort of an override for 'frontbuffer' memory management which the app is > not really involved in. And yes, a content_browser_test that exercises the lock/unlock API on Android would be great. Some similar ones for these readback paths actually already exist. Unfortunately, content_browser_tests is not really functional enough on Android for this.
https://codereview.chromium.org/174323003/diff/350001/content/public/browser/... File content/public/browser/render_widget_host.h (right): https://codereview.chromium.org/174323003/diff/350001/content/public/browser/... content/public/browser/render_widget_host.h:206: virtual void UnlockForCopyFromBackingStore() = 0; On 2014/02/26 00:25:56, sievers wrote: > On 2014/02/26 00:09:54, jam wrote: > > I'm sure there's a reason why you're exposing these methods, but right now I'm > > not seeing it. > > > > Per the cl subject, you're doing this so that the content isn't evicted when > > calling async CopyFromBackingStore/CopyFromCompositingSurface. But I don't > > understand why you need public locking methods. Why can't the implementation > of > > CopyFromBackingStore/CopyFromCompositingSurface do this internally since they > > know when there's an oustanding callback? > > The problem is that the application wants to replace the live tab with a > placeholder approximation which needs to happen asynchronously. > > While this happens and until this is ready and added to the compositor tree, we > cannot let RWHV release the live surface. I'm not familiar with this part of the code, so I have more questions. When this "replace the live tab with a placeholder" is happening, at that point a pending CopyFromBackingStore/CopyFromCompositingSurface is happening? I'm just trying to convince myself that we can't implement this internally without a public API. It's hard to see the whole picture because I don't see the calling code. I also don't understand why these lock methods are empty (see my other comment) > > It's sort of an override for 'frontbuffer' memory management which the app is > not really involved in.
On 2014/02/26 17:37:29, jam wrote: > https://codereview.chromium.org/174323003/diff/350001/content/public/browser/... > File content/public/browser/render_widget_host.h (right): > > https://codereview.chromium.org/174323003/diff/350001/content/public/browser/... > content/public/browser/render_widget_host.h:206: virtual void > UnlockForCopyFromBackingStore() = 0; > On 2014/02/26 00:25:56, sievers wrote: > > On 2014/02/26 00:09:54, jam wrote: > > > I'm sure there's a reason why you're exposing these methods, but right now > I'm > > > not seeing it. > > > > > > Per the cl subject, you're doing this so that the content isn't evicted when > > > calling async CopyFromBackingStore/CopyFromCompositingSurface. But I don't > > > understand why you need public locking methods. Why can't the implementation > > of > > > CopyFromBackingStore/CopyFromCompositingSurface do this internally since > they > > > know when there's an oustanding callback? > > > > The problem is that the application wants to replace the live tab with a > > placeholder approximation which needs to happen asynchronously. > > > > While this happens and until this is ready and added to the compositor tree, > we > > cannot let RWHV release the live surface. > > I'm not familiar with this part of the code, so I have more questions. When this > "replace the live tab with a placeholder" is happening, at that point a pending > CopyFromBackingStore/CopyFromCompositingSurface is happening? I'm just trying to > convince myself that we can't implement this internally without a public API. While the copy is happening, the surface (and intermediate buffers in the case of a readback with multiple steps, such as GPU scaling or format conversion followed by CPU readback) is correctly referenced so that it doesn't go away and break the readback. The problem is that on Android we are trying to be very efficient with memory, and we start other background work after the readback completes to further compress the bitmap. In the tab stack > It's hard to see the whole picture because I don't see the calling code. I also > don't understand why these lock methods are empty (see my other comment) > > > > > > It's sort of an override for 'frontbuffer' memory management which the app is > > not really involved in.
On 2014/02/26 17:43:44, sievers wrote: > On 2014/02/26 17:37:29, jam wrote: > > > https://codereview.chromium.org/174323003/diff/350001/content/public/browser/... > > File content/public/browser/render_widget_host.h (right): > > > > > https://codereview.chromium.org/174323003/diff/350001/content/public/browser/... > > content/public/browser/render_widget_host.h:206: virtual void > > UnlockForCopyFromBackingStore() = 0; > > On 2014/02/26 00:25:56, sievers wrote: > > > On 2014/02/26 00:09:54, jam wrote: > > > > I'm sure there's a reason why you're exposing these methods, but right now > > I'm > > > > not seeing it. > > > > > > > > Per the cl subject, you're doing this so that the content isn't evicted > when > > > > calling async CopyFromBackingStore/CopyFromCompositingSurface. But I don't > > > > understand why you need public locking methods. Why can't the > implementation > > > of > > > > CopyFromBackingStore/CopyFromCompositingSurface do this internally since > > they > > > > know when there's an oustanding callback? > > > > > > The problem is that the application wants to replace the live tab with a > > > placeholder approximation which needs to happen asynchronously. > > > > > > While this happens and until this is ready and added to the compositor tree, > > we > > > cannot let RWHV release the live surface. > > > > I'm not familiar with this part of the code, so I have more questions. When > this > > "replace the live tab with a placeholder" is happening, at that point a > pending > > CopyFromBackingStore/CopyFromCompositingSurface is happening? I'm just trying > to > > convince myself that we can't implement this internally without a public API. > > While the copy is happening, the surface (and intermediate buffers in the case > of a readback with multiple steps, such as GPU scaling or format conversion > followed by CPU readback) is correctly referenced so that it doesn't go away and > break the readback. > > The problem is that on Android we are trying to be very efficient with memory, > and we start other background work after the readback completes to further > compress the bitmap. In the tab stack In the tab stack however we always need a representation for all tabs. Since we cannot keep all tabs live and updating, we keep the latest surface until we have a compressed representation ready for upload. We could potentially move the ETC1 compression to happen as part of the readback. However, we currently return an SkBitmap which does not support this format. > > > > It's hard to see the whole picture because I don't see the calling code. I > also > > don't understand why these lock methods are empty (see my other comment) > > > > > > > > > > It's sort of an override for 'frontbuffer' memory management which the app > is > > > not really involved in.
On 2014/02/26 17:45:45, sievers wrote: > On 2014/02/26 17:43:44, sievers wrote: > > On 2014/02/26 17:37:29, jam wrote: > > > > > > https://codereview.chromium.org/174323003/diff/350001/content/public/browser/... > > > File content/public/browser/render_widget_host.h (right): > > > > > > > > > https://codereview.chromium.org/174323003/diff/350001/content/public/browser/... > > > content/public/browser/render_widget_host.h:206: virtual void > > > UnlockForCopyFromBackingStore() = 0; > > > On 2014/02/26 00:25:56, sievers wrote: > > > > On 2014/02/26 00:09:54, jam wrote: > > > > > I'm sure there's a reason why you're exposing these methods, but right > now > > > I'm > > > > > not seeing it. > > > > > > > > > > Per the cl subject, you're doing this so that the content isn't evicted > > when > > > > > calling async CopyFromBackingStore/CopyFromCompositingSurface. But I > don't > > > > > understand why you need public locking methods. Why can't the > > implementation > > > > of > > > > > CopyFromBackingStore/CopyFromCompositingSurface do this internally since > > > they > > > > > know when there's an oustanding callback? > > > > > > > > The problem is that the application wants to replace the live tab with a > > > > placeholder approximation which needs to happen asynchronously. > > > > > > > > While this happens and until this is ready and added to the compositor > tree, > > > we > > > > cannot let RWHV release the live surface. > > > > > > I'm not familiar with this part of the code, so I have more questions. When > > this > > > "replace the live tab with a placeholder" is happening, at that point a > > pending > > > CopyFromBackingStore/CopyFromCompositingSurface is happening? I'm just > trying > > to > > > convince myself that we can't implement this internally without a public > API. > > > > While the copy is happening, the surface (and intermediate buffers in the case > > of a readback with multiple steps, such as GPU scaling or format conversion > > followed by CPU readback) is correctly referenced so that it doesn't go away > and > > break the readback. > > > > The problem is that on Android we are trying to be very efficient with memory, > > and we start other background work after the readback completes to further > > compress the bitmap. In the tab stack > > In the tab stack however we always need a representation for all tabs. Since we > cannot keep all tabs live and updating, we keep the latest surface until we have > a compressed representation ready for upload. > > We could potentially move the ETC1 compression to happen as part of the > readback. However, we currently return an SkBitmap which does not support this > format. > > > > > > > > It's hard to see the whole picture because I don't see the calling code. I > > also > > > don't understand why these lock methods are empty (see my other comment) > > > I think the plan probably was to rename RWHVAndroid::Lock/UnlockResources to implement these methods. It should probably be done in this patch. > > > > > > > > > > > It's sort of an override for 'frontbuffer' memory management which the app > > is > > > > not really involved in.
On 2014/02/26 17:47:21, sievers wrote: > On 2014/02/26 17:45:45, sievers wrote: > > On 2014/02/26 17:43:44, sievers wrote: > > > On 2014/02/26 17:37:29, jam wrote: > > > > > > > > > > https://codereview.chromium.org/174323003/diff/350001/content/public/browser/... > > > > File content/public/browser/render_widget_host.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/174323003/diff/350001/content/public/browser/... > > > > content/public/browser/render_widget_host.h:206: virtual void > > > > UnlockForCopyFromBackingStore() = 0; > > > > On 2014/02/26 00:25:56, sievers wrote: > > > > > On 2014/02/26 00:09:54, jam wrote: > > > > > > I'm sure there's a reason why you're exposing these methods, but right > > now > > > > I'm > > > > > > not seeing it. > > > > > > > > > > > > Per the cl subject, you're doing this so that the content isn't > evicted > > > when > > > > > > calling async CopyFromBackingStore/CopyFromCompositingSurface. But I > > don't > > > > > > understand why you need public locking methods. Why can't the > > > implementation > > > > > of > > > > > > CopyFromBackingStore/CopyFromCompositingSurface do this internally > since > > > > they > > > > > > know when there's an oustanding callback? > > > > > > > > > > The problem is that the application wants to replace the live tab with a > > > > > placeholder approximation which needs to happen asynchronously. > > > > > > > > > > While this happens and until this is ready and added to the compositor > > tree, > > > > we > > > > > cannot let RWHV release the live surface. > > > > > > > > I'm not familiar with this part of the code, so I have more questions. > When > > > this > > > > "replace the live tab with a placeholder" is happening, at that point a > > > pending > > > > CopyFromBackingStore/CopyFromCompositingSurface is happening? I'm just > > trying > > > to > > > > convince myself that we can't implement this internally without a public > > API. > > > > > > While the copy is happening, the surface (and intermediate buffers in the > case > > > of a readback with multiple steps, such as GPU scaling or format conversion > > > followed by CPU readback) is correctly referenced so that it doesn't go away > > and > > > break the readback. > > > > > > The problem is that on Android we are trying to be very efficient with > memory, > > > and we start other background work after the readback completes to further > > > compress the bitmap. In the tab stack > > > > In the tab stack however we always need a representation for all tabs. Since > we > > cannot keep all tabs live and updating, we keep the latest surface until we > have > > a compressed representation ready for upload. > > > > We could potentially move the ETC1 compression to happen as part of the > > readback. However, we currently return an SkBitmap which does not support this > > format. > > > > > > > > > > > > It's hard to see the whole picture because I don't see the calling code. I > > > also > > > > don't understand why these lock methods are empty (see my other comment) > > > > > > > I think the plan probably was to rename RWHVAndroid::Lock/UnlockResources to > implement these methods. > It should probably be done in this patch. > Yes I was going to rework this patch https://codereview.chromium.org/170783014/ to implement the api in RWHVAndroid. I'll merge it to this. > > > > > > > > > > > > > > It's sort of an override for 'frontbuffer' memory management which the > app > > > is > > > > > not really involved in.
https://codereview.chromium.org/174323003/diff/350001/content/public/browser/... File content/public/browser/render_widget_host.h (right): https://codereview.chromium.org/174323003/diff/350001/content/public/browser/... content/public/browser/render_widget_host.h:203: virtual bool CanCopyFromBackingStore() = 0; On 2014/02/26 00:25:56, sievers wrote: > On 2014/02/26 00:09:54, jam wrote: > > so to be clear, this is in the public api since chrome on android, which isn't > > in this repository, is calling these methods? > > Actually we don't have to add this one. It already exists in > content/public/render_widget_host_view.h: > // Returns true is the current display surface is available. > virtual bool IsSurfaceAvailableForCopy() const = 0; I thought it would make sense that the user would just interact with RenderWidgetHost and not have to worry about whether the view is there or not. WDYT?
https://codereview.chromium.org/174323003/diff/390001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/174323003/diff/390001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.cc:596: void RenderWidgetHostViewBase::LockSurfaceForCopy() {} I'd put NOTIMPLEMENTED() here and below though.
On 2014/02/26 18:42:38, powei wrote: > https://codereview.chromium.org/174323003/diff/350001/content/public/browser/... > File content/public/browser/render_widget_host.h (right): > > https://codereview.chromium.org/174323003/diff/350001/content/public/browser/... > content/public/browser/render_widget_host.h:203: virtual bool > CanCopyFromBackingStore() = 0; > On 2014/02/26 00:25:56, sievers wrote: > > On 2014/02/26 00:09:54, jam wrote: > > > so to be clear, this is in the public api since chrome on android, which > isn't > > > in this repository, is calling these methods? > > > > Actually we don't have to add this one. It already exists in > > content/public/render_widget_host_view.h: > > // Returns true is the current display surface is available. > > virtual bool IsSurfaceAvailableForCopy() const = 0; > > I thought it would make sense that the user would just interact with > RenderWidgetHost and not have to worry about whether the view is there or not. > WDYT? I'll let jam comment, but having the related API in one place makes sense to me. For example, in chrome/browser/thumbnails/thumbnail_tab_helper.cc it's slightly confusing: It checks view->IsSurfaceAvailableForCopy() to decide to see whether it can calll render_widget_host->CopyFromBackingStore().
+piman@ for RendererFrameManager/DelegatedFrameEvictor sievers@, not sure if you looked over RWHVAndroid yet. If not, would you please check that ignoring frame swaps like I'm doing is not going to cause problems? Thanks. https://codereview.chromium.org/174323003/diff/390001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/174323003/diff/390001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.cc:596: void RenderWidgetHostViewBase::LockSurfaceForCopy() {} On 2014/02/26 19:49:39, sievers wrote: > I'd put NOTIMPLEMENTED() here and below though. Done.
just some nits I don't really understand this code, so I feel like I'm not a good owners reviewer. if you're going to wait for piman anyways (i see you have some questions for him), that'd be best. if you weren't going to wait for him, here's a rubberstamp lgtm for the public file and I defer to sievers who's an owner in content/browser/renderer_host for the guts. https://codereview.chromium.org/174323003/diff/410001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/174323003/diff/410001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.h:220: virtual void LockSurfaceForCopy() OVERRIDE; these should be put in the right section https://codereview.chromium.org/174323003/diff/410001/content/public/browser/... File content/public/browser/render_widget_host.h (right): https://codereview.chromium.org/174323003/diff/410001/content/public/browser/... content/public/browser/render_widget_host.h:203: // Ensures that the view does not drop the backing store even when hidden. perhaps these methods should be in an OS_ANDROID ifdef since they're only implemented on that platform?
https://codereview.chromium.org/174323003/diff/410001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/174323003/diff/410001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.h:220: virtual void LockSurfaceForCopy() OVERRIDE; On 2014/02/26 22:57:52, jam wrote: > these should be put in the right section Done. https://codereview.chromium.org/174323003/diff/410001/content/public/browser/... File content/public/browser/render_widget_host.h (right): https://codereview.chromium.org/174323003/diff/410001/content/public/browser/... content/public/browser/render_widget_host.h:203: // Ensures that the view does not drop the backing store even when hidden. On 2014/02/26 22:57:52, jam wrote: > perhaps these methods should be in an OS_ANDROID ifdef since they're only > implemented on that platform? Done.
https://codereview.chromium.org/174323003/diff/490001/content/browser/rendere... File content/browser/renderer_host/delegated_frame_evictor.cc (right): https://codereview.chromium.org/174323003/diff/490001/content/browser/rendere... content/browser/renderer_host/delegated_frame_evictor.cc:52: if (!has_frame_) return 0; nit: return on next line https://codereview.chromium.org/174323003/diff/490001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/174323003/diff/490001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:394: if (frame_evictor_->FrameLockCount() == target_lock_count) This feels iffy. If some other code starts talking to the frame evictor or the renderer frame manager to lock/unlock, this code will become wrong. It's similar to looking at the refcount of an object and deciding whether to ref/unref it. At a high level, the RWHVA should only concern itself with the locks it itself takes. It can keep track of that. It would remove some amount of code in the other classes, so that you can only, at worst, keep a counter that you increment in LockSurfaceForCopy and decrement in UnlockSurfaceForCopy, and reset in ReleaseLocksOnSurface, as you do the LockFrame/UnlockFrame operations. https://codereview.chromium.org/174323003/diff/490001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/174323003/diff/490001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.h:295: // ContentViewCore is gone or when the context has been lost. nit: can you describe what it does rather than when to use it?
https://codereview.chromium.org/174323003/diff/490001/content/browser/rendere... File content/browser/renderer_host/delegated_frame_evictor.cc (right): https://codereview.chromium.org/174323003/diff/490001/content/browser/rendere... content/browser/renderer_host/delegated_frame_evictor.cc:52: if (!has_frame_) return 0; On 2014/03/04 19:09:55, piman (OOO back 2014-3-4) wrote: > nit: return on next line Done. removed. https://codereview.chromium.org/174323003/diff/490001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/174323003/diff/490001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:394: if (frame_evictor_->FrameLockCount() == target_lock_count) On 2014/03/04 19:09:55, piman (OOO back 2014-3-4) wrote: > This feels iffy. > > If some other code starts talking to the frame evictor or the renderer frame > manager to lock/unlock, this code will become wrong. > It's similar to looking at the refcount of an object and deciding whether to > ref/unref it. > > At a high level, the RWHVA should only concern itself with the locks it itself > takes. It can keep track of that. It would remove some amount of code in the > other classes, so that you can only, at worst, keep a counter that you increment > in LockSurfaceForCopy and decrement in UnlockSurfaceForCopy, and reset in > ReleaseLocksOnSurface, as you do the LockFrame/UnlockFrame operations. Done. https://codereview.chromium.org/174323003/diff/490001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/174323003/diff/490001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.h:295: // ContentViewCore is gone or when the context has been lost. On 2014/03/04 19:09:55, piman (OOO back 2014-3-4) wrote: > nit: can you describe what it does rather than when to use it? Done.
https://codereview.chromium.org/174323003/diff/530001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/174323003/diff/530001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:840: return; So, if my understanding is correct, while there is a lock, we discard incoming frames from the renderer? Should we keep the last one around to swap in when the lock count drops to 0?
https://codereview.chromium.org/174323003/diff/530001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/174323003/diff/530001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:840: return; On 2014/03/05 20:14:36, piman wrote: > So, if my understanding is correct, while there is a lock, we discard incoming > frames from the renderer? Yes > Should we keep the last one around to swap in when > the lock count drops to 0? Done. PTAL.
LGTM+nit. Daniel should review the latest logic though. https://codereview.chromium.org/174323003/diff/560001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/174323003/diff/560001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:868: void RenderWidgetHostViewAndroid::DropFrame( nit: RetainFrame or something would be a better name? We're not dropping the frame, really.
https://codereview.chromium.org/174323003/diff/560001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/174323003/diff/560001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:868: void RenderWidgetHostViewAndroid::DropFrame( On 2014/03/06 21:11:28, piman wrote: > nit: RetainFrame or something would be a better name? We're not dropping the > frame, really. Done.
ping sievers@ for another scan of render_widget_host_view_android.cc. Thanks!
On 2014/03/10 05:38:39, powei wrote: > ping sievers@ for another scan of render_widget_host_view_android.cc. Thanks! Do we even need the complexity or can we just accept newly swapped frames as before (when not hidden)? It shouldn't mess with any copy in progress or does it? I might be missing some issues, but do we need to worry about issues other than: - when a surface was hidden but ignored eviction due to the lock, we need to drop the frontbuffer when becoming unlocked if we are still hidden - hide/show races, such as would be causes by discarding new frames when the RWHV is locked but visible
On 2014/03/17 19:18:21, sievers wrote: > On 2014/03/10 05:38:39, powei wrote: > > ping sievers@ for another scan of render_widget_host_view_android.cc. > Thanks! > > Do we even need the complexity or can we just accept newly swapped frames as > before (when not hidden)? It shouldn't mess with any copy in progress or does > it? > > I might be missing some issues, but do we need to worry about issues other than: > - when a surface was hidden but ignored eviction due to the lock, we need to > drop the frontbuffer when becoming unlocked if we are still hidden > - hide/show races, such as would be causes by discarding new frames when the > RWHV is locked but visible Also not sure if Unlock/LockSurfaceFor'Copy' is maybe a bit misleading, since we are actually not locking it for the copy (everything should be properly referenced until the async copy completes and not need an explicit lock), but we are locking the frontbuffer because we want to show static contents in the tab stack although we told the RWHV to Hide (so that we free resources in the renderer).
On 2014/03/17 19:18:21, sievers wrote: > On 2014/03/10 05:38:39, powei wrote: > > ping sievers@ for another scan of render_widget_host_view_android.cc. > Thanks! > > Do we even need the complexity or can we just accept newly swapped frames as > before (when not hidden)? It shouldn't mess with any copy in progress or does > it? > For the actual copy, there should be no problem. That said, DelegatedFrameEvictor/RendererFrameManager assumes that the user would not swap out a locked frame. As you mentioned in the follow-up post, we need the lock to display *something* while readback is in flight. Following the semantics DelegatedFrameEvictor just simplifies the logic of what we put on the screen during readback, IMO. > I might be missing some issues, but do we need to worry about issues other than: > - when a surface was hidden but ignored eviction due to the lock, we need to > drop the frontbuffer when becoming unlocked if we are still hidden > - hide/show races, such as would be causes by discarding new frames when the > RWHV is locked but visible one addition: - content view core being swapped out while the frame is locked. My approach is just to drop all locks when that happens. Due to this case, user might unlock an unlocked frame, I keep a lock counter in RWHVAndroid for this reason.
On 2014/03/17 19:20:28, sievers wrote: > On 2014/03/17 19:18:21, sievers wrote: > > On 2014/03/10 05:38:39, powei wrote: > > > ping sievers@ for another scan of render_widget_host_view_android.cc. > > Thanks! > > > > Do we even need the complexity or can we just accept newly swapped frames as > > before (when not hidden)? It shouldn't mess with any copy in progress or does > > it? > > > > I might be missing some issues, but do we need to worry about issues other > than: > > - when a surface was hidden but ignored eviction due to the lock, we need to > > drop the frontbuffer when becoming unlocked if we are still hidden > > - hide/show races, such as would be causes by discarding new frames when the > > RWHV is locked but visible > > Also not sure if Unlock/LockSurfaceFor'Copy' is maybe a bit misleading, since we > are actually not locking it for the copy (everything should be properly > referenced until the async copy completes and not need an explicit lock), but we > are locking the frontbuffer because we want to show static contents in the tab > stack although we told the RWHV to Hide (so that we free resources in the > renderer). Any suggestion for method names? How about just Unlock/LockSurface?
Thinking about it a bit more, retaining the frame but inhibiting the swap sounds better just so that the current frontbuffer does not change while you are holding the lock so that when you replace it with the static layer it stays consistent. > Any suggestion for method names? How about just Unlock/LockSurface? Yea, I'd maybe just drop the 'ForCopy' notation since it's really rather a generic freeze API https://codereview.chromium.org/174323003/diff/580001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/174323003/diff/580001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:779: ack_callback.Run(); Do we need to call RunAckCallbacks() here instead now? Since you might have this: - Swap, push ack - Lock - Hide - Swap, push ack for retained frame - Unlock - |-> calls Swap, now runs ack callback for retained frame but not orig How about: ack_callbacks_.push(ack_callback); if (host_->is_hidden()) RunAckCallbacks(); https://codereview.chromium.org/174323003/diff/580001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:874: // been released. If there is already a stored frame, then send nit: extra space https://codereview.chromium.org/174323003/diff/580001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:882: ack_callback.Run(); The code does actually not match the comment above. We only send the ack if we are hidden, otherwise it's deferred. I'm wondering if that can have unwanted consequences if we run the ack here and return unused child resources for the current frame (the one supposed to be locked, not the one received here). Can we always just push the ack here? https://codereview.chromium.org/174323003/diff/580001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:990: ack_callback.Run(); see comment in SwapDelegatedFrame(), although this is deprecated.
Also removed "ForCopy" notation https://codereview.chromium.org/174323003/diff/580001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/174323003/diff/580001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:779: ack_callback.Run(); On 2014/03/18 23:44:07, sievers wrote: > Do we need to call RunAckCallbacks() here instead now? > Since you might have this: > > - Swap, push ack > - Lock > - Hide > - Swap, push ack for retained frame > - Unlock > - |-> calls Swap, now runs ack callback for retained frame but not orig > > How about: > ack_callbacks_.push(ack_callback); > if (host_->is_hidden()) > RunAckCallbacks(); Done. https://codereview.chromium.org/174323003/diff/580001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:874: // been released. If there is already a stored frame, then send On 2014/03/18 23:44:07, sievers wrote: > nit: extra space Done. https://codereview.chromium.org/174323003/diff/580001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:882: ack_callback.Run(); On 2014/03/18 23:44:07, sievers wrote: > The code does actually not match the comment above. We only send the ack if we > are hidden, otherwise it's deferred. > > I'm wondering if that can have unwanted consequences if we run the ack here and > return unused child resources for the current frame (the one supposed to be > locked, not the one received here). Can we always just push the ack here? > > > > > > Done. https://codereview.chromium.org/174323003/diff/580001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:990: ack_callback.Run(); On 2014/03/18 23:44:07, sievers wrote: > see comment in SwapDelegatedFrame(), although this is deprecated. Done.
LGTM, nice. https://codereview.chromium.org/174323003/diff/600001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/174323003/diff/600001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:879: // acknowledgement and drop it. nit: comment, 'If there is already a stored frame, then replace and skip the previous one but make sure we still eventually send the ACK. Holding the ACK also blocks the renderer when its max_frames_pending is reached.' https://codereview.chromium.org/174323003/diff/600001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/174323003/diff/600001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.h:93: virtual void UnlockSurface() OVERRIDE; nit: Maybe check with the OWNERS if they prefer LockCompositingSurface() to match ResizeCompositingSurface() above and CopyFromCompositingSurface().
ping jam@ and piman@. Any preference between the naming scheme of Lock/UnlockSurface and Lock/UnlockCompositingSurface ?
On 2014/03/19 19:38:07, powei wrote: > ping jam@ and piman@. > > Any preference between the naming scheme of Lock/UnlockSurface and > Lock/UnlockCompositingSurface ? Slight preference for consistency as sievers suggests
https://codereview.chromium.org/174323003/diff/600001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/174323003/diff/600001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:879: // acknowledgement and drop it. On 2014/03/19 19:22:26, sievers wrote: > nit: comment, 'If there is already a stored frame, then replace and skip the > previous one but make sure we still eventually send the ACK. Holding the ACK > also blocks the renderer when its max_frames_pending is reached.' Done. https://codereview.chromium.org/174323003/diff/600001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/174323003/diff/600001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.h:93: virtual void UnlockSurface() OVERRIDE; On 2014/03/19 19:22:26, sievers wrote: > nit: Maybe check with the OWNERS if they prefer LockCompositingSurface() to > match ResizeCompositingSurface() above and CopyFromCompositingSurface(). Done.
On 2014/03/19 20:35:05, piman wrote: > On 2014/03/19 19:38:07, powei wrote: > > ping jam@ and piman@. > > > > Any preference between the naming scheme of Lock/UnlockSurface and > > Lock/UnlockCompositingSurface ? > > Slight preference for consistency as sievers suggests Done. And thank you all. commiting...
The CQ bit was checked by powei@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/powei@chromium.org/174323003/660001
The CQ bit was unchecked by powei@chromium.org
The CQ bit was checked by powei@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/powei@chromium.org/174323003/680001
The CQ bit was unchecked by powei@chromium.org
The CQ bit was checked by powei@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/powei@chromium.org/174323003/700001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_clang_dbg
The CQ bit was checked by powei@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/powei@chromium.org/174323003/700001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_clang_dbg
The CQ bit was checked by powei@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/powei@chromium.org/174323003/700001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by powei@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/powei@chromium.org/174323003/700001
Message was sent while issue was closed.
Change committed as 258476 |
