Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2021003002:
LockCompositingSurface to make sure frame is not evicted. (Closed)
DescriptionLockCompositingSurface to make sure frame is not evicted.
1.LockCompositingSurface before placing a CopyOutputRquest on SurfaceLayer.
2.When receiving texture as response in PrepareTextureCopyOutputResult call
UnlockCompositingSurface.
3.Do early return if checks are not met.
4.Now CopyFromCompositingSurface internally locks and unlocks the surface when
needed, so remove any external lock/unlock usage.
BUG=NONE.
Committed: https://crrev.com/d9f1d790cc10adcf036fd9a0d48102a12329e81b
Cr-Commit-Position: refs/heads/master@{#397610}
Patch Set 1 #Patch Set 2 : Lock/unlock surface. #Patch Set 3 : #
Total comments: 6
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Messages
Total messages: 41 (21 generated)
Description was changed from ========== Lock and Unlock compositing surface for GetContentBitmap. Lock and Unlock compositing surface for GetContentBitmap. BUG=NONE. ========== to ========== Lock and Unlock compositing surface for GetContentBitmap. LockCompositingSurface when requesting GetContentBitmap as done in tab_content_manager.cc. BUG=NONE. ==========
The CQ bit was checked by siva.gunturi@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2021003002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2021003002/1
Description was changed from ========== Lock and Unlock compositing surface for GetContentBitmap. LockCompositingSurface when requesting GetContentBitmap as done in tab_content_manager.cc. BUG=NONE. ========== to ========== Lock and Unlock compositing surface for GetContentBitmap. LockCompositingSurface when requesting GetContentBitmap as done in tab_content_manager.cc. Unlock it when we receive a response callback. BUG=NONE. ==========
Description was changed from ========== Lock and Unlock compositing surface for GetContentBitmap. LockCompositingSurface when requesting GetContentBitmap as done in tab_content_manager.cc. Unlock it when we receive a response callback. BUG=NONE. ========== to ========== Lock and Unlock compositing surface for GetContentBitmap. LockCompositingSurface when requesting GetContentBitmap as done in tab_content_manager.cc. Unlock it when we receive a response callback. BUG=NONE. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
siva.gunturi@samsung.com changed reviewers: + sievers@chromium.org, tedchoc@chromium.org
ptal..
On 2016/05/30 14:02:19, sivag wrote: > ptal.. this is sievers@ area (no idea if this is something we should be doing in this case)
On 2016/05/31 18:06:39, Ted C wrote: > On 2016/05/30 14:02:19, sivag wrote: > > ptal.. > > this is sievers@ area (no idea if this is something we should > be doing in this case) I guess there is a race where the layer could get evicted (because the web contents gets hidden) before the CopyOutputRequest completes. Is that the bug you're trying to fix? But you only need to lock it in between RWHVAndroid::CopyFromCompositingSurface() and PrepareTextureCopyOutputResult(). Because after that it's in a texture with its own release callback (for scale+readback).
The CQ bit was checked by siva.gunturi@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2021003002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2021003002/20001
The CQ bit was checked by siva.gunturi@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2021003002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2021003002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Lock and Unlock compositing surface for GetContentBitmap. LockCompositingSurface when requesting GetContentBitmap as done in tab_content_manager.cc. Unlock it when we receive a response callback. BUG=NONE. ========== to ========== LockCompositinSurface to make sure frame is not evicted. 1.LockCompositingSurface before placing a CopyOutputRquest on SurfaceLayer. 2.When receiving texture as response in PrepareTextureCopyOutputResult call UnlockCompositingSurface. 3.Do early return if checks are not met. 4.Now CopyFromCompositingSurface internally locks and unlocks the surface when needed so, remove any external lock/unlock usage. BUG=NONE. ==========
Description was changed from ========== LockCompositinSurface to make sure frame is not evicted. 1.LockCompositingSurface before placing a CopyOutputRquest on SurfaceLayer. 2.When receiving texture as response in PrepareTextureCopyOutputResult call UnlockCompositingSurface. 3.Do early return if checks are not met. 4.Now CopyFromCompositingSurface internally locks and unlocks the surface when needed so, remove any external lock/unlock usage. BUG=NONE. ========== to ========== LockCompositinSurface to make sure frame is not evicted. 1.LockCompositingSurface before placing a CopyOutputRquest on SurfaceLayer. 2.When receiving texture as response in PrepareTextureCopyOutputResult call UnlockCompositingSurface. 3.Do early return if checks are not met. 4.Now CopyFromCompositingSurface internally locks and unlocks the surface when needed so, remove any external lock/unlock usage. BUG=NONE. ==========
Description was changed from ========== LockCompositinSurface to make sure frame is not evicted. 1.LockCompositingSurface before placing a CopyOutputRquest on SurfaceLayer. 2.When receiving texture as response in PrepareTextureCopyOutputResult call UnlockCompositingSurface. 3.Do early return if checks are not met. 4.Now CopyFromCompositingSurface internally locks and unlocks the surface when needed so, remove any external lock/unlock usage. BUG=NONE. ========== to ========== LockCompositinSurface to make sure frame is not evicted. 1.LockCompositingSurface before placing a CopyOutputRquest on SurfaceLayer. 2.When receiving texture as response in PrepareTextureCopyOutputResult call UnlockCompositingSurface. 3.Do early return if checks are not met. 4.Now CopyFromCompositingSurface internally locks and unlocks the surface when needed, so remove any external lock/unlock usage. BUG=NONE. ==========
Description was changed from ========== LockCompositinSurface to make sure frame is not evicted. 1.LockCompositingSurface before placing a CopyOutputRquest on SurfaceLayer. 2.When receiving texture as response in PrepareTextureCopyOutputResult call UnlockCompositingSurface. 3.Do early return if checks are not met. 4.Now CopyFromCompositingSurface internally locks and unlocks the surface when needed, so remove any external lock/unlock usage. BUG=NONE. ========== to ========== LockCompositingSurface to make sure frame is not evicted. 1.LockCompositingSurface before placing a CopyOutputRquest on SurfaceLayer. 2.When receiving texture as response in PrepareTextureCopyOutputResult call UnlockCompositingSurface. 3.Do early return if checks are not met. 4.Now CopyFromCompositingSurface internally locks and unlocks the surface when needed, so remove any external lock/unlock usage. BUG=NONE. ==========
On 2016/05/31 19:51:01, sievers wrote: > On 2016/05/31 18:06:39, Ted C wrote: > > On 2016/05/30 14:02:19, sivag wrote: > > > ptal.. > > > > this is sievers@ area (no idea if this is something we should > > be doing in this case) > > > I guess there is a race where the layer could get evicted (because the web > contents gets hidden) before the CopyOutputRequest completes. > Is that the bug you're trying to fix? > > But you only need to lock it in between > RWHVAndroid::CopyFromCompositingSurface() and PrepareTextureCopyOutputResult(). > Because after that it's in a texture with its own release callback (for > scale+readback). Yes that is what i am trying to fix. I added the changes as suggested. ptal..
https://codereview.chromium.org/2021003002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2021003002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:909: // Lock the surface before placing the CopyOutputRequest. nit: I'd say 'Make sure the layer doesn't get deleted until we fulfill the request' or something like that https://codereview.chromium.org/2021003002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1940: if (!rwhva) When RWHVA goes away the layer goes away, which means the CopyOutputRequest goes away. Then again we have to be careful with what we're doing when inside the RWHVA destructor. RWHVA already unlocks the compositing surface during teardown. Maybe it should just be? if (rwhva) rwhva->UnlockCompositingSurface(); Because we can still return the result at this point if it's valid. https://codereview.chromium.org/2021003002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2021003002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.h:77: public base::SupportsWeakPtr<RenderWidgetHostViewAndroid> { There is already a |weak_ptr_factory_| in this class that you can use.
The CQ bit was checked by siva.gunturi@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2021003002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2021003002/60001
The CQ bit was checked by siva.gunturi@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2021003002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2021003002/100001
ptal.. https://codereview.chromium.org/2021003002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2021003002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:909: // Lock the surface before placing the CopyOutputRequest. On 2016/06/01 22:32:06, sievers wrote: > nit: I'd say 'Make sure the layer doesn't get deleted until we fulfill the > request' or something like that Done. https://codereview.chromium.org/2021003002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1940: if (!rwhva) On 2016/06/01 22:32:06, sievers wrote: > When RWHVA goes away the layer goes away, which means the CopyOutputRequest goes > away. Then again we have to be careful with what we're doing when inside the > RWHVA destructor. RWHVA already unlocks the compositing surface during teardown. > > Maybe it should just be? > if (rwhva) > rwhva->UnlockCompositingSurface(); > > Because we can still return the result at this point if it's valid. Done. https://codereview.chromium.org/2021003002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2021003002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.h:77: public base::SupportsWeakPtr<RenderWidgetHostViewAndroid> { On 2016/06/01 22:32:06, sievers wrote: > There is already a |weak_ptr_factory_| in this class that you can use. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
siva.gunturi@samsung.com changed reviewers: + dtrainor@chromium.org
siva.gunturi@samsung.com changed reviewers: + changwan@chromium.org
changwan, dtrainor ptal @tab_content_manager.cc
On 2016/06/02 20:03:48, sivag wrote: > changwan, dtrainor > ptal @tab_content_manager.cc compositor/ lgtm
The CQ bit was checked by siva.gunturi@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2021003002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2021003002/100001
Message was sent while issue was closed.
Description was changed from ========== LockCompositingSurface to make sure frame is not evicted. 1.LockCompositingSurface before placing a CopyOutputRquest on SurfaceLayer. 2.When receiving texture as response in PrepareTextureCopyOutputResult call UnlockCompositingSurface. 3.Do early return if checks are not met. 4.Now CopyFromCompositingSurface internally locks and unlocks the surface when needed, so remove any external lock/unlock usage. BUG=NONE. ========== to ========== LockCompositingSurface to make sure frame is not evicted. 1.LockCompositingSurface before placing a CopyOutputRquest on SurfaceLayer. 2.When receiving texture as response in PrepareTextureCopyOutputResult call UnlockCompositingSurface. 3.Do early return if checks are not met. 4.Now CopyFromCompositingSurface internally locks and unlocks the surface when needed, so remove any external lock/unlock usage. BUG=NONE. ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== LockCompositingSurface to make sure frame is not evicted. 1.LockCompositingSurface before placing a CopyOutputRquest on SurfaceLayer. 2.When receiving texture as response in PrepareTextureCopyOutputResult call UnlockCompositingSurface. 3.Do early return if checks are not met. 4.Now CopyFromCompositingSurface internally locks and unlocks the surface when needed, so remove any external lock/unlock usage. BUG=NONE. ========== to ========== LockCompositingSurface to make sure frame is not evicted. 1.LockCompositingSurface before placing a CopyOutputRquest on SurfaceLayer. 2.When receiving texture as response in PrepareTextureCopyOutputResult call UnlockCompositingSurface. 3.Do early return if checks are not met. 4.Now CopyFromCompositingSurface internally locks and unlocks the surface when needed, so remove any external lock/unlock usage. BUG=NONE. Committed: https://crrev.com/d9f1d790cc10adcf036fd9a0d48102a12329e81b Cr-Commit-Position: refs/heads/master@{#397610} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/d9f1d790cc10adcf036fd9a0d48102a12329e81b Cr-Commit-Position: refs/heads/master@{#397610} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
