Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
21777003:
DevTools: [Android] implement RenderWidgetHostViewAndroid::CopyFromCompositingSurface (Closed)
|
Created:
7 years, 4 months ago by pfeldman Modified:
7 years, 4 months ago CC:
chromium-reviews, vsevik, jam, yurys, joi+watch-content_chromium.org, paulirish+reviews_chromium.org, darin-cc_chromium.org, devtools-reviews_chromium.org, aandrey+blink_chromium.org, aelias_OOO_until_Jul13, piman, David Trainor- moved to gerrit, danakj Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDevTools: [Android] implement RenderWidgetHostViewAndroid::CopyFromCompositingSurface
BUG=242299
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217073
Patch Set 1 #
Total comments: 6
Patch Set 2 : Make bots a bit more happy #Patch Set 3 : Fix clang bots. #
Total comments: 7
Patch Set 4 : Using RequestCopyOfOutput async API. #
Total comments: 17
Patch Set 5 : Review comments addressed. #
Total comments: 8
Patch Set 6 : For landing #
Total comments: 6
Patch Set 7 : For landing #
Messages
Total messages: 29 (0 generated)
I think this is doing the right thing, but what if we changed the organization around a bit so that GrabSnapshot() returns a gfx::JavaBitmap() and then we add a method there to encode as PNG (and a recycle() too). WDYT? https://codereview.chromium.org/21777003/diff/1/content/browser/android/conte... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/21777003/diff/1/content/browser/android/conte... content/browser/android/content_view_core_impl.h:299: bool GrabSnapshot(std::vector<unsigned uint8>* png); _unsigned_ uint8 seems a little redundant :) https://codereview.chromium.org/21777003/diff/1/content/browser/devtools/rend... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/21777003/diff/1/content/browser/devtools/rend... content/browser/devtools/renderer_overrides_handler.cc:163: std::vector<unsigned uint8> png; No "unsigned" here either. https://codereview.chromium.org/21777003/diff/1/content/public/android/java/s... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/21777003/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:327: bitmap.compress(Bitmap.CompressFormat.PNG, 80, baos); Do a bitmap.recycle() here to make sure the memory is freed sooner.
>> I think this is doing the right thing, but what if we >> changed the organization around a bit so that >> GrabSnapshot() returns a gfx::JavaBitmap() and then we >> add a method there to encode as PNG (and a recycle() too). >> WDYT? Whatever results in less overhead. It is dead slow now (measuring as we speak). So I'll play around with the code to optimize it. https://codereview.chromium.org/21777003/diff/1/content/browser/android/conte... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/21777003/diff/1/content/browser/android/conte... content/browser/android/content_view_core_impl.h:299: bool GrabSnapshot(std::vector<unsigned uint8>* png); Heh :) I copy-pasted it from somewhere and then deleted it everywhere (as I thought) https://codereview.chromium.org/21777003/diff/1/content/browser/devtools/rend... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/21777003/diff/1/content/browser/devtools/rend... content/browser/devtools/renderer_overrides_handler.cc:163: std::vector<unsigned uint8> png; On 2013/08/02 10:53:23, Sami wrote: > No "unsigned" here either. Done. https://codereview.chromium.org/21777003/diff/1/content/public/android/java/s... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/21777003/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:327: bitmap.compress(Bitmap.CompressFormat.PNG, 80, baos); On 2013/08/02 10:53:23, Sami wrote: > Do a bitmap.recycle() here to make sure the memory is freed sooner. Done.
On 2013/08/02 12:25:30, pfeldman wrote: > >> I think this is doing the right thing, but what if we > >> changed the organization around a bit so that > >> GrabSnapshot() returns a gfx::JavaBitmap() and then we > >> add a method there to encode as PNG (and a recycle() too). > >> WDYT? > > Whatever results in less overhead. It is dead slow now (measuring as we speak). > So I'll play around with the code to optimize it. I'm guessing the getBitmap() part in ContentViewCore.java is where most time is spent. To make that faster I think we'd need to change the way the texture readback is done, most likely by avoiding glReadPixels().
PTAL. Capturing pixels from native now via readback. Java is only used for compressing.
I wonder if we should try to reimagine this to work asynchronously as needed by http://crbug.com/251936. Daniel, WDYT? https://codereview.chromium.org/21777003/diff/49001/content/browser/android/c... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/21777003/diff/49001/content/browser/android/c... content/browser/android/content_view_core_impl.cc:717: scale)); Check that no dimension of |size| is zero here. https://codereview.chromium.org/21777003/diff/49001/content/browser/android/c... content/browser/android/content_view_core_impl.cc:718: SkBitmap bitmap; To avoid the extra allocation and copy on line 735, let's flip this around so that we allocate the java bitmap from the beginning and point PopulateBitmapWithContents directly into it. Something like this: jobject bitmapObject = <call into java to create a bitmap of the right size> gfx::JavaBitmap javaBitmap(bitmapObject); SkBitmap bitmap; bitmap.setConfig(SkBitmap::kARGB_8888_Config, size.width(), size.height()); bitmap.setPixels(javaBitmap.pixels()); // call PopulateBitmapWithContents as below Make sure you destroy |javaBitmap| before try compressing the Java bitmap. https://codereview.chromium.org/21777003/diff/49001/content/browser/android/c... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/21777003/diff/49001/content/browser/android/c... content/browser/android/content_view_core_impl.h:303: bool GrabSnapshot(const std::string& format, Nit: call this CaptureScreenshot for consistency?
+alex, piman See comments. I feel really bad now pointing this out. It looks like if you wanted to implement GetView/WindowSnapshot() now in the right way, you'd have to make it asynchronous first (see also crbug.com/252037). What are the specific call sites we care about for this bug? Maybe we could focus on making those async and it wouldn't be so bad? Looking at the amount of plumbing and unwanted cruft we'd be adding with this patch here though, it seems like it might be worth doing it right (async) now. We really want to get rid of - synchronous readback entry points in the compositor - entry points in Android RWHV that use the helper GL context - entry points that pass in Java bitmaps (they might require pixel copies to pass them around JNI boundaries) https://codereview.chromium.org/21777003/diff/49001/content/browser/android/c... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/21777003/diff/49001/content/browser/android/c... content/browser/android/content_view_core_impl.cc:724: if (!view->PopulateBitmapWithContents(size, &bitmap)) We'd like to deprecate this API and make all readbacks asynchronous. Essentially you invoke the readback and you'd eventually get a callback that passes you an SkBitmap. I think crbug.com/252037 tracks that for GrabWindow/ViewSnapshot(). https://codereview.chromium.org/21777003/diff/49001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/21777003/diff/49001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:274: bool RenderWidgetHostViewAndroid::PopulateBitmapWithContents( Oh no... we are trying to deprecate any readbacks in here that are not RWHV::CopyFromCopositingSurface(). https://codereview.chromium.org/21777003/diff/49001/content/browser/web_conte... File content/browser/web_contents/web_contents_view_android.h (right): https://codereview.chromium.org/21777003/diff/49001/content/browser/web_conte... content/browser/web_contents/web_contents_view_android.h:85: std::vector<uint8>* data) OVERRIDE; Ideally, we'd just implement RWHV::CopyFromCompositingSurface(). https://codereview.chromium.org/21777003/diff/49001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/21777003/diff/49001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:341: private byte[] compressBitmap(Bitmap bitmap, String format, int quality) { ContentViewCore like an odd place for this function. Would java_bitmap.cc work?
>> See comments. I feel really bad now pointing this out. It looks like if you >> wanted to implement GetView/WindowSnapshot() now in the right way, you'd >> have to make it asynchronous first (see also crbug.com/252037). Don't feel bad! >> What are the specific call sites we care about for this bug? Maybe we could >> focus on making those async and it wouldn't be so bad? That's mostly Telemetry via DevTools protocol. I will try making Page.captureScreenshot true async (although it would require more plumbing and will break present serialization). I am sure there are no clients relying upon it yet. >> Looking at the amount of plumbing and unwanted cruft we'd be adding with >> this patch here though, it seems like it might be worth doing it right (async) now. sgtm >> We really want to get rid of >> - synchronous readback entry points in the compositor >> - entry points in Android RWHV that use the helper GL context >> - entry points that pass in Java bitmaps (they might require pixel copies to >> pass them around JNI boundaries) The only reason we even go into Java is image compression. Sounds like Skia is linked without those on Android. Is there a workaround?
On 2013/08/05 21:56:54, pfeldman wrote: > >> See comments. I feel really bad now pointing this out. It looks like if you > >> wanted to implement GetView/WindowSnapshot() now in the right way, you'd > >> have to make it asynchronous first (see also crbug.com/252037). > > Don't feel bad! > > >> What are the specific call sites we care about for this bug? Maybe we could > >> focus on making those async and it wouldn't be so bad? > > That's mostly Telemetry via DevTools protocol. I will try making > Page.captureScreenshot true async (although it would require more plumbing and > will break present serialization). I am sure there are no clients relying upon > it yet. That sounds great. > > >> Looking at the amount of plumbing and unwanted cruft we'd be adding with > >> this patch here though, it seems like it might be worth doing it right > (async) now. > > sgtm > > >> We really want to get rid of > >> - synchronous readback entry points in the compositor > >> - entry points in Android RWHV that use the helper GL context > >> - entry points that pass in Java bitmaps (they might require pixel copies to > >> pass them around JNI boundaries) > > The only reason we even go into Java is image compression. Sounds like Skia is > linked without those on Android. Is there a workaround? To compress natively you can probably look at ui/snapshot/snapshot_aura.cc: gfx::PNGCodec::Encode(pixels,...) There is one for JPEG also.
Based on the discussion... I managed to make this protocol command async (that is already landed). This patch is still a straw man: - Uses CopyFromCompositingSurface to capture on all ports (should only use it for Android?) - Copies a bunch of code from RenderWidgetHostViewAura to RenderWidgetHostViewAndroid that compiles and works in Chrome (GTK), but not in Android (captures nothing, bails out). Before I figure out new plumbing, I'd like to make it work on Android.
On 2013/08/07 17:15:50, pfeldman wrote: > Based on the discussion... > > I managed to make this protocol command async (that is already landed). > > This patch is still a straw man: > - Uses CopyFromCompositingSurface to capture on all ports (should only use it > for Android?) I believe this is the common readback path for where accelerated compositing is available. Also see the unit tests here: content/browser/renderer_host/render_widget_host_view_browsertest.cc It has a function to determine whether that's the case (IsForceCompositingModeEnabled()), although the logic looks a little convoluted. Otherwise it uses CopyFromBackingStore(). Furthermore, you can query IsSurfaceAvailableForCopy() to see if a copy can be done. If you add RenderWidgetHostViewAndroid::CopyFromCompositingSurface(), then RenderWidgetHostViewAndroid::HasValidFrame() should probably be renamed to implement the virtual IsSurfaceAvailableForCopy() also (which currently is NOTIMPLEMENTED()). > - Copies a bunch of code from RenderWidgetHostViewAura to > RenderWidgetHostViewAndroid > > that compiles and works in Chrome (GTK), but not in Android (captures nothing, > bails out). > > Before I figure out new plumbing, I'd like to make it work on Android.
On 2013/08/07 17:33:51, sievers wrote: > On 2013/08/07 17:15:50, pfeldman wrote: > > Based on the discussion... > > > > I managed to make this protocol command async (that is already landed). > > > > This patch is still a straw man: > > - Uses CopyFromCompositingSurface to capture on all ports (should only use it > > for Android?) > > I believe this is the common readback path for where accelerated compositing is > available. Actually, it's probably only true for Aura (incl. win_aura) and Android. > > Also see the unit tests here: > content/browser/renderer_host/render_widget_host_view_browsertest.cc > It has a function to determine whether that's the case > (IsForceCompositingModeEnabled()), although the logic looks a little convoluted. > Otherwise it uses CopyFromBackingStore(). > > Furthermore, you can query IsSurfaceAvailableForCopy() to see if a copy can be > done. > If you add RenderWidgetHostViewAndroid::CopyFromCompositingSurface(), then > RenderWidgetHostViewAndroid::HasValidFrame() should probably be renamed to > implement the virtual IsSurfaceAvailableForCopy() also (which currently is > NOTIMPLEMENTED()). > > > > > - Copies a bunch of code from RenderWidgetHostViewAura to > > RenderWidgetHostViewAndroid > > > > that compiles and works in Chrome (GTK), but not in Android (captures nothing, > > bails out). > > > > Before I figure out new plumbing, I'd like to make it work on Android.
+dana for some questions about cc::Layer copy requests: Looks like this does not support GPU scaling. Are there any plans for it? On Android, I think we want to avoid scaling on the CPU. Therefore we probably need to route everything through the GLHelper path for now (which can scale on the GPU into a new texture and then readback a potentially smaller copy). Therefore the only motivation to go through the layer's copy request might be to have a codepath that's compatible with ubercomp (layer_ is a cc::DelegatedRendererLayer). But that's also nothing you should necessarily have to worry about in this patch if it makes things harder. https://codereview.chromium.org/21777003/diff/75001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/21777003/diff/75001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:283: bool RenderWidgetHostViewAndroid::PopulateBitmapWithContents( So this is unneeded then?
On Wed, Aug 7, 2013 at 11:41 AM, <sievers@chromium.org> wrote: > +dana for some questions about cc::Layer copy requests: > > Looks like this does not support GPU scaling. Are there any plans for it? > There's a path to return a texture. We can scale/crop/... after that with the GLHelper. That's what we do on Aura. > > On Android, I think we want to avoid scaling on the CPU. > Therefore we probably need to route everything through the GLHelper path > for now > (which can scale on the GPU into a new texture and then readback a > potentially > smaller copy). > > Therefore the only motivation to go through the layer's copy request might > be to > have a codepath that's compatible with ubercomp (layer_ is a > cc::DelegatedRendererLayer). But that's also nothing you should > necessarily have > to worry about in this patch if it makes things harder. > > > > > > > https://codereview.chromium.**org/21777003/diff/75001/** > content/browser/renderer_host/**render_widget_host_view_**android.cc<https://codereview.chromium.org/21777003/diff/75001/content/browser/renderer_host/render_widget_host_view_android.cc> > File content/browser/renderer_host/**render_widget_host_view_**android.cc > (right): > > https://codereview.chromium.**org/21777003/diff/75001/** > content/browser/renderer_host/**render_widget_host_view_** > android.cc#newcode283<https://codereview.chromium.org/21777003/diff/75001/content/browser/renderer_host/render_widget_host_view_android.cc#newcode283> > content/browser/renderer_host/**render_widget_host_view_**android.cc:283: > bool RenderWidgetHostViewAndroid::**PopulateBitmapWithContents( > So this is unneeded then? > > https://codereview.chromium.**org/21777003/<https://codereview.chromium.org/2... >
On 2013/08/07 19:06:01, piman wrote: > On Wed, Aug 7, 2013 at 11:41 AM, <mailto:sievers@chromium.org> wrote: > > > +dana for some questions about cc::Layer copy requests: > > > > Looks like this does not support GPU scaling. Are there any plans for it? > > > > There's a path to return a texture. We can scale/crop/... after that with > the GLHelper. That's what we do on Aura. Yes, but then we also have an extra copy (could be expensive on a high-res device). But for ubercomp we'd have to go through the cc layer API first anyhow. We might want to avoid the GPU copy for non-ubercomp though. > > > > > On Android, I think we want to avoid scaling on the CPU. > > Therefore we probably need to route everything through the GLHelper path > > for now > > (which can scale on the GPU into a new texture and then readback a > > potentially > > smaller copy). > > > > Therefore the only motivation to go through the layer's copy request might > > be to > > have a codepath that's compatible with ubercomp (layer_ is a > > cc::DelegatedRendererLayer). But that's also nothing you should > > necessarily have > > to worry about in this patch if it makes things harder. > > > > > > > > > > > > > > https://codereview.chromium.**org/21777003/diff/75001/** > > > content/browser/renderer_host/**render_widget_host_view_**android.cc<https://codereview.chromium.org/21777003/diff/75001/content/browser/renderer_host/render_widget_host_view_android.cc> > > File content/browser/renderer_host/**render_widget_host_view_**android.cc > > (right): > > > > https://codereview.chromium.**org/21777003/diff/75001/** > > content/browser/renderer_host/**render_widget_host_view_** > > > android.cc#newcode283<https://codereview.chromium.org/21777003/diff/75001/content/browser/renderer_host/render_widget_host_view_android.cc#newcode283> > > content/browser/renderer_host/**render_widget_host_view_**android.cc:283: > > bool RenderWidgetHostViewAndroid::**PopulateBitmapWithContents( > > So this is unneeded then? > > > > > https://codereview.chromium.**org/21777003/%3Chttps://codereview.chromium.org...> > >
On Wed, Aug 7, 2013 at 12:11 PM, <sievers@chromium.org> wrote: > On 2013/08/07 19:06:01, piman wrote: > > On Wed, Aug 7, 2013 at 11:41 AM, <mailto:sievers@chromium.org> wrote: >> > > > +dana for some questions about cc::Layer copy requests: >> > >> > Looks like this does not support GPU scaling. Are there any plans for >> it? >> > >> > > There's a path to return a texture. We can scale/crop/... after that with >> the GLHelper. That's what we do on Aura. >> > > Yes, but then we also have an extra copy (could be expensive on a high-res > device). Does it matter? Is this used in the critical path? The glReadPixels is likely going to be slower than anything else anyway... > But for ubercomp we'd have to go through the cc layer API first anyhow. We might want to avoid the GPU copy for non-ubercomp though. > > > > > >> > On Android, I think we want to avoid scaling on the CPU. >> > Therefore we probably need to route everything through the GLHelper path >> > for now >> > (which can scale on the GPU into a new texture and then readback a >> > potentially >> > smaller copy). >> > >> > Therefore the only motivation to go through the layer's copy request >> might >> > be to >> > have a codepath that's compatible with ubercomp (layer_ is a >> > cc::DelegatedRendererLayer). But that's also nothing you should >> > necessarily have >> > to worry about in this patch if it makes things harder. >> > >> > >> > >> > >> > >> > >> > https://codereview.chromium.****org/21777003/diff/75001/** >> > >> > > content/browser/renderer_host/****render_widget_host_view_****android.cc< > https://codereview.**chromium.org/21777003/diff/**75001/content/browser/** > renderer_host/render_widget_**host_view_android.cc<https://codereview.chromium.org/21777003/diff/75001/content/browser/renderer_host/render_widget_host_view_android.cc> > > > >> > File content/browser/renderer_host/****render_widget_host_view_**** >> android.cc >> > (right): >> > >> > https://codereview.chromium.****org/21777003/diff/75001/** >> > content/browser/renderer_host/****render_widget_host_view_** >> > >> > > android.cc#newcode283<https://**codereview.chromium.org/** > 21777003/diff/75001/content/**browser/renderer_host/render_** > widget_host_view_android.cc#**newcode283<https://codereview.chromium.org/21777003/diff/75001/content/browser/renderer_host/render_widget_host_view_android.cc#newcode283> > > > >> > content/browser/renderer_host/****render_widget_host_view_**** >> android.cc:283: >> > bool RenderWidgetHostViewAndroid::****PopulateBitmapWithContents( >> >> > So this is unneeded then? >> > >> > >> > > https://codereview.chromium.****org/21777003/%3Chttps://codere** > view.chromium.org/21777003/ <http://codereview.chromium.org/21777003/>> > >> > >> > > > > https://codereview.chromium.**org/21777003/<https://codereview.chromium.org/2... >
On 2013/08/07 19:17:45, piman wrote: > On Wed, Aug 7, 2013 at 12:11 PM, <mailto:sievers@chromium.org> wrote: > > > On 2013/08/07 19:06:01, piman wrote: > > > > On Wed, Aug 7, 2013 at 11:41 AM, <mailto:sievers@chromium.org> wrote: > >> > > > > > +dana for some questions about cc::Layer copy requests: > >> > > >> > Looks like this does not support GPU scaling. Are there any plans for > >> it? > >> > > >> > > > > There's a path to return a texture. We can scale/crop/... after that with > >> the GLHelper. That's what we do on Aura. > >> > > > > Yes, but then we also have an extra copy (could be expensive on a high-res > > device). > > > Does it matter? Is this used in the critical path? The glReadPixels is > likely going to be slower than anything else anyway... > Not sure if it matters. Considering that the current readback is synchronous on the UI thread, the (clients-side) async readback is likely an improvement already, even with the extra gpu copy. Also, it makes synchronizing textures easier. However, we might want to make sure that we don't accidentally OOM with the multiple readbacks in flight (for example fast tab switching), each operation allocating an extra 1-2 textures plus bitmap. Since there are two threads involved in creating textures (cc on impl thread, helper on ui thread), maybe it makes sense to defer kicking off a new readback request through cc until the last helper request has finished. > > > But for ubercomp we'd have to go through the cc layer API first anyhow. > > We might want to avoid the GPU copy for non-ubercomp though. > > > > > > > > > > >> > On Android, I think we want to avoid scaling on the CPU. > >> > Therefore we probably need to route everything through the GLHelper path > >> > for now > >> > (which can scale on the GPU into a new texture and then readback a > >> > potentially > >> > smaller copy). > >> > > >> > Therefore the only motivation to go through the layer's copy request > >> might > >> > be to > >> > have a codepath that's compatible with ubercomp (layer_ is a > >> > cc::DelegatedRendererLayer). But that's also nothing you should > >> > necessarily have > >> > to worry about in this patch if it makes things harder. > >> > > >> > > >> > > >> > > >> > > >> > > >> > https://codereview.chromium.****org/21777003/diff/75001/** > >> > > >> > > > > content/browser/renderer_host/****render_widget_host_view_****android.cc< > > https://codereview.**chromium.org/21777003/diff/**75001/content/browser/** > > > renderer_host/render_widget_**host_view_android.cc<https://codereview.chromium.org/21777003/diff/75001/content/browser/renderer_host/render_widget_host_view_android.cc> > > > > > > >> > File content/browser/renderer_host/****render_widget_host_view_**** > >> android.cc > >> > (right): > >> > > >> > https://codereview.chromium.****org/21777003/diff/75001/** > >> > content/browser/renderer_host/****render_widget_host_view_** > >> > > >> > > > > android.cc#newcode283<https://**codereview.chromium.org/** > > 21777003/diff/75001/content/**browser/renderer_host/render_** > > > widget_host_view_android.cc#**newcode283<https://codereview.chromium.org/21777003/diff/75001/content/browser/renderer_host/render_widget_host_view_android.cc#newcode283> > > > > > > >> > content/browser/renderer_host/****render_widget_host_view_**** > >> android.cc:283: > >> > bool RenderWidgetHostViewAndroid::****PopulateBitmapWithContents( > >> > >> > So this is unneeded then? > >> > > >> > > >> > > > > https://codereview.chromium.****org/21777003/%253Chttps://codere** > > view.chromium.org/21777003/ <http://codereview.chromium.org/21777003/>> > > > >> > > >> > > > > > > > > > https://codereview.chromium.**org/21777003/%3Chttps://codereview.chromium.org...> > >
general idea looks good. thanks for doing this! https://codereview.chromium.org/21777003/diff/75001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/21777003/diff/75001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:366: NOTIMPLEMENTED(); return HasValidFrame() https://codereview.chromium.org/21777003/diff/75001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:584: cc::CopyOutputRequest::CreateRequest(base::Bind( Should this create a bitmap request if we don't have to scale? https://codereview.chromium.org/21777003/diff/75001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:1305: SkBitmap bitmap = skia::ImageOperations::Resize( Let's remove this and dcheck() that the dimensions are correct here.
I think this looks great Pavel. https://codereview.chromium.org/21777003/diff/75001/content/browser/devtools/... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/21777003/diff/75001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:43: static int kDefaultQuality = 90; nit: kDefaultScreenshotQuality? https://codereview.chromium.org/21777003/diff/75001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:214: reinterpret_cast<unsigned char*>(bitmap.getAddr32(0, 0)), nit: should be indented by two more spaces. https://codereview.chromium.org/21777003/diff/75001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:221: reinterpret_cast<unsigned char*>(bitmap.getAddr32(0, 0)), nit: ditto https://codereview.chromium.org/21777003/diff/75001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:238: std::string base_64_data; It would be great if we could free the bitmap at this point since we don't need it anymore. That would mean this callback should take a non-const reference to the bitmap. https://codereview.chromium.org/21777003/diff/75001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/21777003/diff/75001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:584: cc::CopyOutputRequest::CreateRequest(base::Bind( On 2013/08/07 19:51:28, sievers wrote: > Should this create a bitmap request if we don't have to scale? Yeah, no need to make a redundant copy in that case.
https://codereview.chromium.org/21777003/diff/75001/content/browser/devtools/... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/21777003/diff/75001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:43: static int kDefaultQuality = 90; On 2013/08/08 11:35:00, Sami wrote: > nit: kDefaultScreenshotQuality? Done. https://codereview.chromium.org/21777003/diff/75001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:214: reinterpret_cast<unsigned char*>(bitmap.getAddr32(0, 0)), On 2013/08/08 11:35:00, Sami wrote: > nit: should be indented by two more spaces. Done. https://codereview.chromium.org/21777003/diff/75001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:221: reinterpret_cast<unsigned char*>(bitmap.getAddr32(0, 0)), On 2013/08/08 11:35:00, Sami wrote: > nit: ditto Done. https://codereview.chromium.org/21777003/diff/75001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:238: std::string base_64_data; On 2013/08/08 11:35:00, Sami wrote: > It would be great if we could free the bitmap at this point since we don't need > it anymore. That would mean this callback should take a non-const reference to > the bitmap. It is declared in RenderWidgetHostViewPort and could have other clients. I guess it is safer existing way. https://codereview.chromium.org/21777003/diff/75001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/21777003/diff/75001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:283: bool RenderWidgetHostViewAndroid::PopulateBitmapWithContents( Correct. Removed. https://codereview.chromium.org/21777003/diff/75001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:366: NOTIMPLEMENTED(); On 2013/08/07 19:51:28, sievers wrote: > return HasValidFrame() Done. https://codereview.chromium.org/21777003/diff/75001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:584: cc::CopyOutputRequest::CreateRequest(base::Bind( On 2013/08/07 19:51:28, sievers wrote: > Should this create a bitmap request if we don't have to scale? Done. https://codereview.chromium.org/21777003/diff/75001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:1305: SkBitmap bitmap = skia::ImageOperations::Resize( On 2013/08/07 19:51:28, sievers wrote: > Let's remove this and dcheck() that the dimensions are correct here. Done.
PTAL
lgtm modulo nits and the question about IsSurfaceAvailableForCopy(). https://codereview.chromium.org/21777003/diff/88001/content/browser/devtools/... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/21777003/diff/88001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:44: static int kDefaultQuality = 80; Forgot to make this kDefaultScreenshotQuality? https://codereview.chromium.org/21777003/diff/88001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:257: std::string base_64_data; nit: "base64_data" https://codereview.chromium.org/21777003/diff/88001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/21777003/diff/88001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:546: if (!CanCopyToBitmap()) { Seems like we could just call IsSurfaceAvailableForCopy() here. https://codereview.chromium.org/21777003/diff/88001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:584: return true; Do we need this method? Please add a TODO if something needs to be done here.
Thanks for the review and guidance! https://codereview.chromium.org/21777003/diff/88001/content/browser/devtools/... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/21777003/diff/88001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:44: static int kDefaultQuality = 80; Sorry, was switching between two clients. Done. https://codereview.chromium.org/21777003/diff/88001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:257: std::string base_64_data; On 2013/08/09 16:08:48, Sami wrote: > nit: "base64_data" Done. https://codereview.chromium.org/21777003/diff/88001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/21777003/diff/88001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:546: if (!CanCopyToBitmap()) { On 2013/08/09 16:08:48, Sami wrote: > Seems like we could just call IsSurfaceAvailableForCopy() here. Done. https://codereview.chromium.org/21777003/diff/88001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:584: return true; Removed
@sievers: waiting for your owners review now.
lgtm with nits for content/browser/renderer_host/. thanks! https://codereview.chromium.org/21777003/diff/106002/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/21777003/diff/106002/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:555: const gfx::Size& dst_size_in_pixel = ConvertViewSizeToPixel(this, dst_size); Ok, I guess it makes sense that we use DIP here, since that is what RWHVAndroid::GetViewBounds returns. But I hope ConvertViewSizeToPixel() works correctly on Android, esp. since there seems to be a table of supported scale factors it tries to match it to. Do you mind adding a DCHECK() along the lines of DCHECK_EQ(device_scale_factor, ui::GetScaleFactorScale(GetScaleFactorForView(view))? https://codereview.chromium.org/21777003/diff/106002/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1184: void RenderWidgetHostViewAndroid::CopyFromCompositingSurfaceHasResult( nit: Do we even need this function now that we have the if-else in CopyFromCompositingSurface() above and could just directly bind the respective target function? https://codereview.chromium.org/21777003/diff/106002/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1203: const base::Callback<void(bool, const SkBitmap&)>& callback, can you put this function inside the anonymous namespace?
https://codereview.chromium.org/21777003/diff/106002/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/21777003/diff/106002/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:555: const gfx::Size& dst_size_in_pixel = ConvertViewSizeToPixel(this, dst_size); On 2013/08/09 20:54:03, sievers wrote: > Ok, I guess it makes sense that we use DIP here, since that is what > RWHVAndroid::GetViewBounds returns. > > But I hope ConvertViewSizeToPixel() works correctly on Android, esp. since there > seems to be a table of supported scale factors it tries to match it to. > > Do you mind adding a DCHECK() along the lines of > DCHECK_EQ(device_scale_factor, > ui::GetScaleFactorScale(GetScaleFactorForView(view))? Done. https://codereview.chromium.org/21777003/diff/106002/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1184: void RenderWidgetHostViewAndroid::CopyFromCompositingSurfaceHasResult( Done. Branches now call respective callbacks. https://codereview.chromium.org/21777003/diff/106002/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1203: const base::Callback<void(bool, const SkBitmap&)>& callback, On 2013/08/09 20:54:03, sievers wrote: > can you put this function inside the anonymous namespace? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pfeldman@chromium.org/21777003/124001
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...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pfeldman@chromium.org/21777003/124001
Message was sent while issue was closed.
Change committed as 217073 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
