Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
143803004:
android: Migrate old content readback to use async readback (and delegated renderer) (Closed)
|
Created:
6 years, 11 months ago by powei Modified:
6 years, 10 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, no sievers Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptionandroid: Migrate old content readback to use async readback (and delegated renderer)
This patch removes the use of a helper context for content readback. Instead,
we use the async readback API.
This patch is also setting delegated renderer as the default for android.
android= https://chrome-internal-review.googlesource.com/#/c/153746/
BUG=326363
TBR=sievers@chromium.org
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=248827
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249081
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249453
Patch Set 1 #Patch Set 2 : removed are_layers_attached_, added is_showing_ #Patch Set 3 : turn clipping off for readback #
Total comments: 25
Patch Set 4 : rebased #
Total comments: 9
Patch Set 5 : addressed sievers@ comments #Patch Set 6 : turn on delegated rendering #
Total comments: 5
Patch Set 7 : addressed comments #
Total comments: 18
Patch Set 8 : addressed aelias@ comments #
Total comments: 12
Patch Set 9 : aelias and dtrainor comments #Patch Set 10 : dtrainor comment leftover #
Total comments: 4
Patch Set 11 : changed GenerateCompressedUIResource details #Patch Set 12 : ETC1 UIResourceBitmap takes size #Patch Set 13 : nit #Patch Set 14 : consistent device scaling #Messages
Total messages: 38 (0 generated)
ptal thanks! https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.h (right): https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.h:64: virtual void SetNeedsCommit() OVERRIDE; Debug purpose only. Will be removed. https://codereview.chromium.org/143803004/diff/110001/content/public/browser/... File content/public/browser/android/compositor.h (right): https://codereview.chromium.org/143803004/diff/110001/content/public/browser/... content/public/browser/android/compositor.h:82: virtual void SetNeedsCommit() = 0; Will be removed
https://codereview.chromium.org/143803004/diff/110001/content/browser/android... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/143803004/diff/110001/content/browser/android... content/browser/android/content_view_core_impl.cc:620: base::Bind(compositor_callback, false, SkBitmap())); nit: you can just do compositor_callback.Run(false, SkBitmap()); https://codereview.chromium.org/143803004/diff/110001/content/browser/android... content/browser/android/content_view_core_impl.cc:626: base::Unretained(this), The Unretained() is a problem, since the ContentViewCore might go away in the meantime. To get the code functionally equivalent to what you're doing here and avoid the problem, you could just have RWHVAndroid imply the lock. However, isn't the problem that releasing the live layer when the readback callback is run is not good enough, because the application also wants to ETC1-compress the bitmap first which is another async task? It really only works if the live layer is replaced inside the callback with something suitable. And it seems silly to reupload the same texture again that we just read back if you could as well just keep the live layer around instead for another moment. In which case you want to move the lock/unlock further up (instead of further down) to be exposed to the app. https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:398: GLuint CompositorImpl::BuildBasicTexture() { remove this function https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:411: GLenum CompositorImpl::GetGLFormatForBitmap(gfx::JavaBitmap& bitmap) { remove this function https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:428: GLenum CompositorImpl::GetGLTypeForBitmap(gfx::JavaBitmap& bitmap) { remove this function https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.h (right): https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.h:92: private: these 3 functions can go away also https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (left): https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1416: DCHECK(result->HasTexture()); just wondering: when is this failing? https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:275: view->ResetClipping(); Not sure how this is used by the app, it's really not obvious unfortunately. But how comes this is not a problem to reset this arbitrarily? https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:315: return false; nit: Can we just check for !layer_ here. If the layer is not attached, it's not strictly an error. It just means that the readback will be delayed until the layer gets reattached to the root. We could allow this - it doesn't seem particularly illegal. If you are worried that it could cause unwanted bugs, how about putting a check for that into the app? Can we also DCHECK(!is_showing_ || layer_) before? Ideally even DCHECK((!is_showing_ && !is_locked_) || layer_); https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:387: AttachLayers(); Can this just call layer_->SetHideLayerAndSubtree(false) instead? https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:398: AttachLayers(); Can this just call layer_->SetHideLayerAndSubtree(true) instead? https://codereview.chromium.org/143803004/diff/130001/content/browser/android... File content/browser/android/transient_ui_resource.h (right): https://codereview.chromium.org/143803004/diff/130001/content/browser/android... content/browser/android/transient_ui_resource.h:22: // once. Subsequent GetBitmap() calls will return a small holder. Do we really need this class? The way this works, can't the Compositor just implement GetBitmap()? Then we don't have to pass around the weak pointers and such, and the lifetime oh objects is better defined. https://codereview.chromium.org/143803004/diff/130001/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/143803004/diff/130001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:217: ui_resource_map_.clear(); Does the application need to recreate all UI resources after it has hidden the compositor and before showing it again? https://codereview.chromium.org/143803004/diff/130001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:278: scoped_ptr<cc::ScopedUIResource> scoped = nit: |scoped| -> |resource| https://codereview.chromium.org/143803004/diff/130001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:288: NOTREACHED(); return 0;
https://codereview.chromium.org/143803004/diff/110001/content/browser/android... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/143803004/diff/110001/content/browser/android... content/browser/android/content_view_core_impl.cc:620: base::Bind(compositor_callback, false, SkBitmap())); On 2014/01/28 20:47:21, sievers wrote: > nit: you can just do compositor_callback.Run(false, SkBitmap()); Done. https://codereview.chromium.org/143803004/diff/110001/content/browser/android... content/browser/android/content_view_core_impl.cc:626: base::Unretained(this), On 2014/01/28 20:47:21, sievers wrote: > The Unretained() is a problem, since the ContentViewCore might go away in the > meantime. > > To get the code functionally equivalent to what you're doing here and avoid the > problem, you could just have RWHVAndroid imply the lock. > > However, isn't the problem that releasing the live layer when the readback > callback is run is not good enough, because the application also wants to > ETC1-compress the bitmap first which is another async task? > > It really only works if the live layer is replaced inside the callback with > something suitable. And it seems silly to reupload the same texture again that > we just read back if you could as well just keep the live layer around instead > for another moment. > > In which case you want to move the lock/unlock further up (instead of further > down) to be exposed to the app. I took out the locks here, but I'm not sure how to add it in RWHVA since don't we have the same problem with RWHVA's lifetime when the callback comes back? https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:398: GLuint CompositorImpl::BuildBasicTexture() { On 2014/01/28 20:47:21, sievers wrote: > remove this function Done. https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:411: GLenum CompositorImpl::GetGLFormatForBitmap(gfx::JavaBitmap& bitmap) { On 2014/01/28 20:47:21, sievers wrote: > remove this function Done. https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:428: GLenum CompositorImpl::GetGLTypeForBitmap(gfx::JavaBitmap& bitmap) { On 2014/01/28 20:47:21, sievers wrote: > remove this function Done. https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.h (right): https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.h:92: private: On 2014/01/28 20:47:21, sievers wrote: > these 3 functions can go away also Done. https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (left): https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1416: DCHECK(result->HasTexture()); On 2014/01/28 20:47:21, sievers wrote: > just wondering: when is this failing? Don't have a consistent repo, but I do hit this sometimes. I can put a TODO for investigation if you'd prefer that. https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:275: view->ResetClipping(); On 2014/01/28 20:47:21, sievers wrote: > Not sure how this is used by the app, it's really not obvious unfortunately. > But how comes this is not a problem to reset this arbitrarily? Done. Not affecting delegated rendering so removed. https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:315: return false; On 2014/01/28 20:47:21, sievers wrote: > nit: Can we just check for !layer_ here. > Done > If the layer is not attached, it's not strictly an error. It just means that the > readback will be delayed until the layer gets reattached to the root. > > We could allow this - it doesn't seem particularly illegal. If you are worried > that it could cause unwanted bugs, how about putting a check for that into the > app? > > Can we also DCHECK(!is_showing_ || layer_) before? > Ideally even DCHECK((!is_showing_ && !is_locked_) || layer_); > Not quite sure what you mean here. Why would we allow a readback without a layer to do the readback on? https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:387: AttachLayers(); On 2014/01/28 20:47:21, sievers wrote: > Can this just call layer_->SetHideLayerAndSubtree(false) instead? Done. https://codereview.chromium.org/143803004/diff/110001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:398: AttachLayers(); On 2014/01/28 20:47:21, sievers wrote: > Can this just call layer_->SetHideLayerAndSubtree(true) instead? Done. https://codereview.chromium.org/143803004/diff/130001/content/browser/android... File content/browser/android/transient_ui_resource.h (right): https://codereview.chromium.org/143803004/diff/130001/content/browser/android... content/browser/android/transient_ui_resource.h:22: // once. Subsequent GetBitmap() calls will return a small holder. On 2014/01/28 20:47:22, sievers wrote: > Do we really need this class? The way this works, can't the Compositor just > implement GetBitmap()? > > Then we don't have to pass around the weak pointers and such, and the lifetime > oh objects is better defined. Done. https://codereview.chromium.org/143803004/diff/130001/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/143803004/diff/130001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:217: ui_resource_map_.clear(); On 2014/01/28 20:47:22, sievers wrote: > Does the application need to recreate all UI resources after it has hidden the > compositor and before showing it again? Yes, right now the tabs resources are gone when showing again. This requires a bit more plumbing to the thumbnail cache. Again, would you mind if I put a TODO here? https://codereview.chromium.org/143803004/diff/130001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:278: scoped_ptr<cc::ScopedUIResource> scoped = On 2014/01/28 20:47:22, sievers wrote: > nit: |scoped| -> |resource| Done. https://codereview.chromium.org/143803004/diff/130001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:288: NOTREACHED(); On 2014/01/28 20:47:22, sievers wrote: > return 0; Done.
also turning on delegated rendering in this patch
https://codereview.chromium.org/143803004/diff/110001/content/browser/android... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/143803004/diff/110001/content/browser/android... content/browser/android/content_view_core_impl.cc:626: base::Unretained(this), On 2014/01/29 13:53:17, powei wrote: > On 2014/01/28 20:47:21, sievers wrote: > > The Unretained() is a problem, since the ContentViewCore might go away in the > > meantime. > > > > To get the code functionally equivalent to what you're doing here and avoid > the > > problem, you could just have RWHVAndroid imply the lock. > > > > However, isn't the problem that releasing the live layer when the readback > > callback is run is not good enough, because the application also wants to > > ETC1-compress the bitmap first which is another async task? > > > > It really only works if the live layer is replaced inside the callback with > > something suitable. And it seems silly to reupload the same texture again that > > we just read back if you could as well just keep the live layer around instead > > for another moment. > > > > In which case you want to move the lock/unlock further up (instead of further > > down) to be exposed to the app. > > I took out the locks here, but I'm not sure how to add it in RWHVA since don't > we have the same problem with RWHVA's lifetime when the callback comes back? You lock the live layer in CopyFromCompositingSurface() and then pass a weakptr (weak_ptr_factory_.GetWeakPtr()) to PrepareTextureCopyOutputResult as an argument. And in the latter you do: if (rwhv.get()) rwhv->UnlockResources(); But it might be better to push the responsiblity to the app sooner rather then later. It probably doesn't get us much in the meantime and if we start locking and unlocking the live layer from multiple places, things will likely get out of whack. https://codereview.chromium.org/143803004/diff/130001/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/143803004/diff/130001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:217: ui_resource_map_.clear(); On 2014/01/29 13:53:17, powei wrote: > On 2014/01/28 20:47:22, sievers wrote: > > Does the application need to recreate all UI resources after it has hidden the > > compositor and before showing it again? > > Yes, right now the tabs resources are gone when showing again. This requires a > bit more plumbing to the thumbnail cache. Again, would you mind if I put a TODO > here? Sure, no problem. Was mainly wondering what the app's responsibility will be. But do you really want to change that behavior at the level here? Currently, we destroy the LTH when we become invisible so everything is gone anyways. https://codereview.chromium.org/143803004/diff/190001/content/browser/android... File content/browser/android/content_startup_flags.cc (right): https://codereview.chromium.org/143803004/diff/190001/content/browser/android... content/browser/android/content_startup_flags.cc:89: parsed_command_line->AppendSwitch(switches::kEnableDelegatedRenderer); yay! https://codereview.chromium.org/143803004/diff/190001/content/browser/android... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/143803004/diff/190001/content/browser/android... content/browser/android/content_view_core_impl.cc:597: bool success, Looks like you don't need this method anymore but can use the original callback below. https://codereview.chromium.org/143803004/diff/190001/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/143803004/diff/190001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:282: transient_ui_bitmap_ = make_scoped_ptr(new cc::UIResourceBitmap(bitmap)); as discussed: an alternate 'GenerateUIResource(base::Callback<cc::UIResourceBitmap(void)> get_bitmap_callback)' instead of implying based on format here would be great for use cases where the bitmap is not to be kept around.
On 2014/01/29 21:13:46, sievers wrote: > https://codereview.chromium.org/143803004/diff/110001/content/browser/android... > File content/browser/android/content_view_core_impl.cc (right): > > https://codereview.chromium.org/143803004/diff/110001/content/browser/android... > content/browser/android/content_view_core_impl.cc:626: base::Unretained(this), > On 2014/01/29 13:53:17, powei wrote: > > On 2014/01/28 20:47:21, sievers wrote: > > > The Unretained() is a problem, since the ContentViewCore might go away in > the > > > meantime. > > > > > > To get the code functionally equivalent to what you're doing here and avoid > > the > > > problem, you could just have RWHVAndroid imply the lock. > > > > > > However, isn't the problem that releasing the live layer when the readback > > > callback is run is not good enough, because the application also wants to > > > ETC1-compress the bitmap first which is another async task? > > > > > > It really only works if the live layer is replaced inside the callback with > > > something suitable. And it seems silly to reupload the same texture again > that > > > we just read back if you could as well just keep the live layer around > instead > > > for another moment. > > > > > > In which case you want to move the lock/unlock further up (instead of > further > > > down) to be exposed to the app. > > > > I took out the locks here, but I'm not sure how to add it in RWHVA since don't > > we have the same problem with RWHVA's lifetime when the callback comes back? > > You lock the live layer in CopyFromCompositingSurface() and then pass a weakptr > (weak_ptr_factory_.GetWeakPtr()) to PrepareTextureCopyOutputResult as an > argument. And in the latter you do: > if (rwhv.get()) > rwhv->UnlockResources(); > > > But it might be better to push the responsiblity to the app sooner rather then > later. It probably doesn't get us much in the meantime and if we start locking > and unlocking the live layer from multiple places, things will likely get out of > whack. > Ok, I'm taking out locking for now. > https://codereview.chromium.org/143803004/diff/130001/content/browser/rendere... > File content/browser/renderer_host/compositor_impl_android.cc (right): > > https://codereview.chromium.org/143803004/diff/130001/content/browser/rendere... > content/browser/renderer_host/compositor_impl_android.cc:217: > ui_resource_map_.clear(); > On 2014/01/29 13:53:17, powei wrote: > > On 2014/01/28 20:47:22, sievers wrote: > > > Does the application need to recreate all UI resources after it has hidden > the > > > compositor and before showing it again? > > > > Yes, right now the tabs resources are gone when showing again. This requires > a > > bit more plumbing to the thumbnail cache. Again, would you mind if I put a > TODO > > here? > > Sure, no problem. Was mainly wondering what the app's responsibility will be. > > But do you really want to change that behavior at the level here? Currently, we > destroy the LTH when we become invisible so everything is gone anyways. > So the TODO will be on the android client of the compositor, which is signaled with OnLostResources(). > https://codereview.chromium.org/143803004/diff/190001/content/browser/android... > File content/browser/android/content_startup_flags.cc (right): > > https://codereview.chromium.org/143803004/diff/190001/content/browser/android... > content/browser/android/content_startup_flags.cc:89: > parsed_command_line->AppendSwitch(switches::kEnableDelegatedRenderer); > yay! > > https://codereview.chromium.org/143803004/diff/190001/content/browser/android... > File content/browser/android/content_view_core_impl.cc (right): > > https://codereview.chromium.org/143803004/diff/190001/content/browser/android... > content/browser/android/content_view_core_impl.cc:597: bool success, > Looks like you don't need this method anymore but can use the original callback > below. > > https://codereview.chromium.org/143803004/diff/190001/content/browser/rendere... > File content/browser/renderer_host/compositor_impl_android.cc (right): > > https://codereview.chromium.org/143803004/diff/190001/content/browser/rendere... > content/browser/renderer_host/compositor_impl_android.cc:282: > transient_ui_bitmap_ = make_scoped_ptr(new cc::UIResourceBitmap(bitmap)); > as discussed: an alternate > 'GenerateUIResource(base::Callback<cc::UIResourceBitmap(void)> > get_bitmap_callback)' instead of implying based on format here would be great > for use cases where the bitmap is not to be kept around.
https://codereview.chromium.org/143803004/diff/190001/content/browser/android... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/143803004/diff/190001/content/browser/android... content/browser/android/content_view_core_impl.cc:597: bool success, On 2014/01/29 21:13:48, sievers wrote: > Looks like you don't need this method anymore but can use the original callback > below. Done. https://codereview.chromium.org/143803004/diff/190001/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/143803004/diff/190001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:282: transient_ui_bitmap_ = make_scoped_ptr(new cc::UIResourceBitmap(bitmap)); On 2014/01/29 21:13:48, sievers wrote: > as discussed: an alternate > 'GenerateUIResource(base::Callback<cc::UIResourceBitmap(void)> > get_bitmap_callback)' instead of implying based on format here would be great > for use cases where the bitmap is not to be kept around. So I opted for the boolean flag for now since aelias@ also wanted the Skia allocation of ETC1 pixels to be upstream, so I added a GenerateCompressedUIResource. Also, UIResourceClient was intended to be the callback interface, but I'm not quite sure if passing a raw UIResourceClient pointer from the android side is a good idea right now; it ties the lifetime of the compositor |host_| with the thumbnail objects. Just to keep things simple for this patch, I added TransientUIResource as a private class here.
https://codereview.chromium.org/143803004/diff/200010/content/browser/android... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/143803004/diff/200010/content/browser/android... content/browser/android/content_view_core_impl.h:61: const base::Callback<void(bool, const SkBitmap&)>& callback) OVERRIDE; It's hard to understand from this API what this callback is for, could you comment it better and also typedef to a representative name? https://codereview.chromium.org/143803004/diff/200010/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/143803004/diff/200010/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:104: bool resource_lost) OVERRIDE { nit: indent https://codereview.chromium.org/143803004/diff/200010/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:110: tiny_bitmap.setConfig(SkBitmap::kARGB_8888_Config, 1, 1); , kOpaque_SkAlphaType https://codereview.chromium.org/143803004/diff/200010/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:111: tiny_bitmap.allocPixels(); This bitmap may actually be displayed on the screen. Could you also make the pixel white? tiny_bitmap.lockPixels(); tiny_bitmap.getAddr32(0, 0)[0] = SkPMColor(SkColor_WHITE); tiny_bitmap.unlockPixels(); or bind it to an SkCanvas and draw that way. https://codereview.chromium.org/143803004/diff/200010/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:332: gfx::Size& size, const gfx::Size& https://codereview.chromium.org/143803004/diff/200010/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:333: int data_size, Please use size_t instead of int. https://codereview.chromium.org/143803004/diff/200010/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:334: void* data, void* pixels https://codereview.chromium.org/143803004/diff/200010/content/public/browser/... File content/public/browser/android/compositor.h (right): https://codereview.chromium.org/143803004/diff/200010/content/public/browser/... content/public/browser/android/compositor.h:79: const cc::UIResourceBitmap& bitmap, Since GenerateCompressedUIResource takes a void* data, I think this should take an SkBitmap for consistency. (And maybe we can then avoid referencing UIResourceBitmap from downstream entirely?) Then what's currently in this method should be in a private method. https://codereview.chromium.org/143803004/diff/200010/content/public/browser/... content/public/browser/android/compositor.h:82: // Generates a compressed UIResource. See above for |is_transient|. May "ETC1 compressed"
https://codereview.chromium.org/143803004/diff/200010/content/browser/android... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/143803004/diff/200010/content/browser/android... content/browser/android/content_view_core_impl.h:61: const base::Callback<void(bool, const SkBitmap&)>& callback) OVERRIDE; On 2014/01/31 05:50:10, aelias wrote: > It's hard to understand from this API what this callback is for, could you > comment it better and also typedef to a representative name? Done. https://codereview.chromium.org/143803004/diff/200010/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/143803004/diff/200010/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:104: bool resource_lost) OVERRIDE { On 2014/01/31 05:50:10, aelias wrote: > nit: indent Done. https://codereview.chromium.org/143803004/diff/200010/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:110: tiny_bitmap.setConfig(SkBitmap::kARGB_8888_Config, 1, 1); On 2014/01/31 05:50:10, aelias wrote: > , kOpaque_SkAlphaType Done. https://codereview.chromium.org/143803004/diff/200010/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:111: tiny_bitmap.allocPixels(); On 2014/01/31 05:50:10, aelias wrote: > This bitmap may actually be displayed on the screen. Could you also make the > pixel white? > > tiny_bitmap.lockPixels(); > tiny_bitmap.getAddr32(0, 0)[0] = SkPMColor(SkColor_WHITE); > tiny_bitmap.unlockPixels(); > > or bind it to an SkCanvas and draw that way. Done. https://codereview.chromium.org/143803004/diff/200010/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:332: gfx::Size& size, On 2014/01/31 05:50:10, aelias wrote: > const gfx::Size& Done. https://codereview.chromium.org/143803004/diff/200010/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:333: int data_size, On 2014/01/31 05:50:10, aelias wrote: > Please use size_t instead of int. Done. https://codereview.chromium.org/143803004/diff/200010/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:334: void* data, On 2014/01/31 05:50:10, aelias wrote: > void* pixels Done. https://codereview.chromium.org/143803004/diff/200010/content/public/browser/... File content/public/browser/android/compositor.h (right): https://codereview.chromium.org/143803004/diff/200010/content/public/browser/... content/public/browser/android/compositor.h:79: const cc::UIResourceBitmap& bitmap, On 2014/01/31 05:50:10, aelias wrote: > Since GenerateCompressedUIResource takes a void* data, I think this should take > an SkBitmap for consistency. (And maybe we can then avoid referencing > UIResourceBitmap from downstream entirely?) Then what's currently in this > method should be in a private method. Done. Removed UIResourceBitmap references downstream. https://codereview.chromium.org/143803004/diff/200010/content/public/browser/... content/public/browser/android/compositor.h:82: // Generates a compressed UIResource. See above for |is_transient|. May On 2014/01/31 05:50:10, aelias wrote: > "ETC1 compressed" Done.
lgtm modulo minor comments https://codereview.chromium.org/143803004/diff/420001/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/143803004/diff/420001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:344: bool is_transient) { Please add some DCHECKs that "size" is valid for ETC1. https://codereview.chromium.org/143803004/diff/420001/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.h (right): https://codereview.chromium.org/143803004/diff/420001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.h:64: size_t data_size, On second thought, is this argument needed at all? Isn't it always size.width() * size.height() / 2? https://codereview.chromium.org/143803004/diff/420001/content/public/browser/... File content/public/browser/android/content_view_core.h (right): https://codereview.chromium.org/143803004/diff/420001/content/public/browser/... content/public/browser/android/content_view_core.h:58: typedef base::Callback<void(bool, const SkBitmap&)> ContentBitmapCallback; Hmm, sorry for changing my mind, but it looks like this callback is a pre-existing pattern in a lot of places. I guess we should keep using base::Callback<void(bool, const SkBitmap&)> for now and rename it in a future cleanup patch. I also think "PixelReadbackCallback" would be a better name.
lgtm https://chromiumcodereview.appspot.com/143803004/diff/420001/content/browser/... File content/browser/android/content_view_core_impl.cc (right): https://chromiumcodereview.appspot.com/143803004/diff/420001/content/browser/... content/browser/android/content_view_core_impl.cc:709: if (!view || !view->IsSurfaceAvailableForCopy()) { Should the view->IsSurfaceAvailableForCopy() check be internal to RWHVA::GetScaledContentBitmap()? https://chromiumcodereview.appspot.com/143803004/diff/420001/content/browser/... File content/browser/renderer_host/compositor_impl_android.h (right): https://chromiumcodereview.appspot.com/143803004/diff/420001/content/browser/... content/browser/renderer_host/compositor_impl_android.h:64: size_t data_size, IIRC valid content size is different from the size of the data (we had to pad it for the - potentially incorrect - compression algorithm requirements). Some drivers required powers of 2 or something. Not 100% sure if that's still necessary though? On 2014/02/01 02:09:20, aelias wrote: > On second thought, is this argument needed at all? Isn't it always size.width() > * size.height() / 2?
https://chromiumcodereview.appspot.com/143803004/diff/420001/content/browser/... File content/browser/renderer_host/compositor_impl_android.h (right): https://chromiumcodereview.appspot.com/143803004/diff/420001/content/browser/... content/browser/renderer_host/compositor_impl_android.h:64: size_t data_size, On 2014/02/02 18:38:53, David Trainor wrote: > IIRC valid content size is different from the size of the data (we had to pad it > for the - potentially incorrect - compression algorithm requirements). Some > drivers required powers of 2 or something. Not 100% sure if that's still > necessary though? It's likely still necessary since those devices likely never got updated requirements. My assumption was more that you are already handling all the adjustments needed for that padding downstream so nothing in Chromium-side needs to know about it? Is that correct?
https://codereview.chromium.org/143803004/diff/420001/content/browser/android... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/143803004/diff/420001/content/browser/android... content/browser/android/content_view_core_impl.cc:709: if (!view || !view->IsSurfaceAvailableForCopy()) { On 2014/02/02 18:38:53, David Trainor wrote: > Should the view->IsSurfaceAvailableForCopy() check be internal to > RWHVA::GetScaledContentBitmap()? Done. https://codereview.chromium.org/143803004/diff/420001/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/143803004/diff/420001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:344: bool is_transient) { On 2014/02/01 02:09:20, aelias wrote: > Please add some DCHECKs that "size" is valid for ETC1. Done. https://codereview.chromium.org/143803004/diff/420001/content/public/browser/... File content/public/browser/android/content_view_core.h (right): https://codereview.chromium.org/143803004/diff/420001/content/public/browser/... content/public/browser/android/content_view_core.h:58: typedef base::Callback<void(bool, const SkBitmap&)> ContentBitmapCallback; On 2014/02/01 02:09:20, aelias wrote: > Hmm, sorry for changing my mind, but it looks like this callback is a > pre-existing pattern in a lot of places. I guess we should keep using > base::Callback<void(bool, const SkBitmap&)> for now and rename it in a future > cleanup patch. I also think "PixelReadbackCallback" would be a better name. Done.
https://chromiumcodereview.appspot.com/143803004/diff/420001/content/browser/... File content/browser/renderer_host/compositor_impl_android.h (right): https://chromiumcodereview.appspot.com/143803004/diff/420001/content/browser/... content/browser/renderer_host/compositor_impl_android.h:64: size_t data_size, We handle adding the padding the compressed texture, but the size of the texture data might not represent the valid size of the content area after that, so I think this is needed so the resource layers can properly calculate the right uv. That stuff is all abstracted away from downstream now. On 2014/02/03 08:40:36, aelias wrote: > On 2014/02/02 18:38:53, David Trainor wrote: > > IIRC valid content size is different from the size of the data (we had to pad > it > > for the - potentially incorrect - compression algorithm requirements). Some > > drivers required powers of 2 or something. Not 100% sure if that's still > > necessary though? > > It's likely still necessary since those devices likely never got updated > requirements. My assumption was more that you are already handling all the > adjustments needed for that padding downstream so nothing in Chromium-side needs > to know about it? Is that correct?
https://chromiumcodereview.appspot.com/143803004/diff/420001/content/browser/... File content/browser/renderer_host/compositor_impl_android.h (right): https://chromiumcodereview.appspot.com/143803004/diff/420001/content/browser/... content/browser/renderer_host/compositor_impl_android.h:64: size_t data_size, On 2014/02/03 18:40:10, David Trainor wrote: > We handle adding the padding the compressed texture, but the size of the texture > data might not represent the valid size of the content area after that, so I > think this is needed so the resource layers can properly calculate the right uv. > That stuff is all abstracted away from downstream now. Well, the "size" value passed in this method isn't used for UV calculation but for memory allocation only, so I don't think we need to pass it in this particular method. https://chromiumcodereview.appspot.com/143803004/diff/450002/content/browser/... File content/browser/renderer_host/compositor_impl_android.cc (right): https://chromiumcodereview.appspot.com/143803004/diff/450002/content/browser/... content/browser/renderer_host/compositor_impl_android.cc:350: SkImageInfo info = {size.width(), size.height(), kPMColor_SkColorType, Wait, isn't this allocating 8x as much memory as is actually needed? Skia thinks this is RGBA8888 right?
https://chromiumcodereview.appspot.com/143803004/diff/450002/content/browser/... File content/browser/renderer_host/compositor_impl_android.cc (right): https://chromiumcodereview.appspot.com/143803004/diff/450002/content/browser/... content/browser/renderer_host/compositor_impl_android.cc:350: SkImageInfo info = {size.width(), size.height(), kPMColor_SkColorType, On 2014/02/03 18:51:14, aelias wrote: > Wait, isn't this allocating 8x as much memory as is actually needed? Skia > thinks this is RGBA8888 right? I think it should be: SkImageInfo info = {data_size, 1, kAlpha_8_SkColorType, kIgnore_SkAlphaType }; (a hack, but SkImageInfo doesn't understand ETC1).
https://codereview.chromium.org/143803004/diff/420001/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.h (right): https://codereview.chromium.org/143803004/diff/420001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.h:64: size_t data_size, On 2014/02/03 18:51:13, aelias wrote: > On 2014/02/03 18:40:10, David Trainor wrote: > > We handle adding the padding the compressed texture, but the size of the > texture > > data might not represent the valid size of the content area after that, so I > > think this is needed so the resource layers can properly calculate the right > uv. > > That stuff is all abstracted away from downstream now. > > Well, the "size" value passed in this method isn't used for UV calculation but > for memory allocation only, so I don't think we need to pass it in this > particular method. Done. I removed this argument for now since downstream code does pad. https://codereview.chromium.org/143803004/diff/450002/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/143803004/diff/450002/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:350: SkImageInfo info = {size.width(), size.height(), kPMColor_SkColorType, On 2014/02/03 19:25:55, aelias wrote: > On 2014/02/03 18:51:14, aelias wrote: > > Wait, isn't this allocating 8x as much memory as is actually needed? Skia > > thinks this is RGBA8888 right? > > I think it should be: > > SkImageInfo info = {data_size, 1, kAlpha_8_SkColorType, kIgnore_SkAlphaType }; > > (a hack, but SkImageInfo doesn't understand ETC1). So my original code came from following this pattern: https://codereview.chromium.org/138393002/diff/600004/cc/trees/layer_tree_hos... Can't really go with your suggestion since the SkImageInfo width/height are used as the bitmap width/height in UIResourceBitmap. So I'm going with SkImageInfo info = {size.width(), size.height(), kAlpha_8_SkColorType, kIgnore_SkAlphaType }; which is twice as big as it needs to be.
https://codereview.chromium.org/143803004/diff/450002/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/143803004/diff/450002/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:350: SkImageInfo info = {size.width(), size.height(), kPMColor_SkColorType, On 2014/02/03 23:39:11, powei wrote: > On 2014/02/03 19:25:55, aelias wrote: > > On 2014/02/03 18:51:14, aelias wrote: > > > Wait, isn't this allocating 8x as much memory as is actually needed? Skia > > > thinks this is RGBA8888 right? > > > > I think it should be: > > > > SkImageInfo info = {data_size, 1, kAlpha_8_SkColorType, kIgnore_SkAlphaType }; > > > > (a hack, but SkImageInfo doesn't understand ETC1). > > So my original code came from following this pattern: > https://codereview.chromium.org/138393002/diff/600004/cc/trees/layer_tree_hos... > > Can't really go with your suggestion since the SkImageInfo width/height are used > as the bitmap width/height in UIResourceBitmap. > > So I'm going with > > SkImageInfo info = {size.width(), size.height(), kAlpha_8_SkColorType, > kIgnore_SkAlphaType }; > > which is twice as big as it needs to be. We can't make it twice as big. The amount of memory taken up by these bitmaps is huge. Please fix UIResourceBitmap to stop using SkImageInfo for the size.
On 2014/02/04 01:09:59, aelias wrote: > https://codereview.chromium.org/143803004/diff/450002/content/browser/rendere... > File content/browser/renderer_host/compositor_impl_android.cc (right): > > https://codereview.chromium.org/143803004/diff/450002/content/browser/rendere... > content/browser/renderer_host/compositor_impl_android.cc:350: SkImageInfo info = > {size.width(), size.height(), kPMColor_SkColorType, > On 2014/02/03 23:39:11, powei wrote: > > On 2014/02/03 19:25:55, aelias wrote: > > > On 2014/02/03 18:51:14, aelias wrote: > > > > Wait, isn't this allocating 8x as much memory as is actually needed? Skia > > > > thinks this is RGBA8888 right? > > > > > > I think it should be: > > > > > > SkImageInfo info = {data_size, 1, kAlpha_8_SkColorType, kIgnore_SkAlphaType > }; > > > > > > (a hack, but SkImageInfo doesn't understand ETC1). > > > > So my original code came from following this pattern: > > > https://codereview.chromium.org/138393002/diff/600004/cc/trees/layer_tree_hos... > > > > Can't really go with your suggestion since the SkImageInfo width/height are > used > > as the bitmap width/height in UIResourceBitmap. > > > > So I'm going with > > > > SkImageInfo info = {size.width(), size.height(), kAlpha_8_SkColorType, > > kIgnore_SkAlphaType }; > > > > which is twice as big as it needs to be. > > We can't make it twice as big. The amount of memory taken up by these bitmaps > is huge. Please fix UIResourceBitmap to stop using SkImageInfo for the size. Done. Please review cc changes, aelias@. Thanks.
lgtm!
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/143803004/790001
Retried try job too often on linux_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/powei@chromium.org/143803004/790001
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/143803004/790001
Message was sent while issue was closed.
Change committed as 248827
On 2014/02/05 02:04:00, I haz the power (commit-bot) wrote: > Change committed as 248827 Reverted due to typo in downstream change: https://codereview.chromium.org/149653004/ Landing again.
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/143803004/790001
Message was sent while issue was closed.
Change committed as 249081
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/151103010/ by powei@chromium.org. The reason for reverting is: broke android build.
On 2014/02/06 00:04:10, powei wrote: > A revert of this CL has been created in > https://codereview.chromium.org/151103010/ by mailto:powei@chromium.org. > > The reason for reverting is: broke android build. Failed on dcheck at render_widget_host_view_anrdoid.cc:665. I spoke with pfeldman@ who wrote it, and he is ok with removing it. I made a two line change to remove it and make the scaling factors consistent. The try bots looked mostly good. Landing again.
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/143803004/1430001
Message was sent while issue was closed.
Change committed as 249453
Message was sent while issue was closed.
On 2014/02/06 18:31:11, I haz the power (commit-bot) wrote: > Change committed as 249453 Caused some failures because a test wasn't updated. Not sure why the CQ didn't catch it. http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%... |
