Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
384453002:
wip: Improving GestureNav screenshotting performance
|
Created:
6 years, 5 months ago by mfomitchev Modified:
6 years, 5 months ago CC:
chromium-reviews, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, sievers+watch_chromium.org, jbauman+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Descriptionwip: Improving GestureNav screenshotting performance
Pixel readback in GL_RGBA format can be a very expensive operation on
some hardware configurations.
When taking a grayscale screenshot, convert to grayscale on the GPU and
then do a readback using GL_BGRA_EXT format.
This improves the readback performance because we read four times less
data and we don't have to do any format conversions as we are reading.
BUG=379983, 380779
Patch Set 1 #
Total comments: 44
Messages
Total messages: 15 (0 generated)
This is wip - can please take a look and comment on the approach? If the approach is fine, I will clean it up and publish a new review. I've put a couple of questions in the comments. Thanks! https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... content/browser/compositor/delegated_frame_host.cc:538: if (config != SkBitmap::kARGB_8888_Config && config != SkBitmap::kA8_Config) { Any tips for testing this code path? How do I make RequestCopyOfOutput return a bitmap instead of a texture? https://codereview.chromium.org/384453002/diff/1/content/common/gpu/client/gl... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/384453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:602: kSwizzleBGRA, The config and swizzle for the A8 case are picked such that we end up using GL_BGRA_EXT format in ReadbackAsync(). Is this too confusing/hacky or is it okay?
The tests fail because I need to rebase.
https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... content/browser/compositor/delegated_frame_host.cc:538: if (config != SkBitmap::kARGB_8888_Config && config != SkBitmap::kA8_Config) { On 2014/07/09 18:02:10, mfomitchev wrote: > Any tips for testing this code path? How do I make RequestCopyOfOutput return a > bitmap instead of a texture? Software compositing. https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... content/browser/compositor/delegated_frame_host.cc:546: SkBitmap scaledBitmap; scaled_bitmap https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... content/browser/compositor/delegated_frame_host.cc:548: source->height() != dst_size_in_pixel.height()) { if they are == you want to return the original bitmap instead of an empty one? https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... content/browser/compositor/delegated_frame_host.cc:559: // Else - |config| is kA8_Config. how about DCHECK_EQ(config, kA8_Config) instead of a comment. https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... content/browser/compositor/delegated_frame_host.cc:561: SkBitmap a8Bitmap; grayscale_bitmap https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... content/browser/compositor/delegated_frame_host.cc:562: a8Bitmap.setConfig(SkBitmap::kA8_Config, setConfig is deprecated. Use an SkImageInfo that you pass to allocPixels https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... content/browser/compositor/delegated_frame_host.cc:571: filter->unref(); no manual refcounting. use skia::RefPtr instead https://codereview.chromium.org/384453002/diff/1/content/browser/frame_host/n... File content/browser/frame_host/navigation_entry_screenshot_manager.cc (right): https://codereview.chromium.org/384453002/diff/1/content/browser/frame_host/n... content/browser/frame_host/navigation_entry_screenshot_manager.cc:53: // Encode the A8 bitmap to grayscale PNG treating alpha as color intensity. you can dcheck the bitmap config here if you like? might be nice https://codereview.chromium.org/384453002/diff/1/content/browser/frame_host/n... content/browser/frame_host/navigation_entry_screenshot_manager.cc:162: CHECK(bitmap.getConfig() == SkBitmap::kA8_Config) DCHECK_EQ, then you don't need the extra << either as it'll print out both https://codereview.chromium.org/384453002/diff/1/content/common/gpu/client/gl... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/384453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:327: // original, but when read back the pixel data can be used to construct the just drop the reference to bitmaps here, this function itself has nothing to do with bitmaps https://codereview.chromium.org/384453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:330: GLuint TransformTextureToA8(GLuint src_texture, TransformTextureToGrayscale. There's no A8 bitmaps here. https://codereview.chromium.org/384453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:344: static const float kRGBtoA8ColorWeights[]; toGrayscale https://codereview.chromium.org/384453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:450: *result_size = gfx::Size((src_size.width() + 3) / 4, src_size.height()); hm, doing this here feels awkward, cuz there's some channels in the last pixel that are not valid. if we can move this later (inside ReadbackAsync maybe?) that might be better? https://codereview.chromium.org/384453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:532: if (bitmap_config != SkBitmap::kA8_Config && why is this here instead of deciding A8 is ok inside IsReadbackConfigSupported? https://codereview.chromium.org/384453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:540: // For readback purposes, treat the resulting texture as ARGB of smaller What does it look like if you teach ReadbackAsync to deal with A8 type? https://codereview.chromium.org/384453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:564: src_size, git cl format https://codereview.chromium.org/384453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:602: kSwizzleBGRA, This None->BGRA will affect more than the A8 path. This seems like a separate CL first with tests to show it's the right thing to do. Also is BGRA always the right thing to do on all hardware?
I was thinking about declaring A8 as the "supported" color type in gl helper. I can probably take care of all the readback-related methods in GLHelper (by reading the x4 smaller texture in GL_BGRA), but I don't think ScaleTexture() will work for A8, at least not without a bunch of changes in GLHelperScaling, which seems like too much work. Also, GLHelper::IsReadbackConfigSupported currently just calls into GLHelperReadbackSupport::IsReadbackConfigSupported. We can't change the latter, so we'd have to special-case A8 in GLHelper::IsReadbackConfigSupported and the semantics of these two methods would diverge which could be confusing. Perhaps we could split GLHelper::IsReadbackConfigSupported into two - one for native support and one for readback (not scaling) support in GLHelper. It would be useful - e.g. RenderWidgetHostViewAndroid uses it to determine if it can take a screenshot in a given format. However the semantics would be confusing/hard to explain. E.g. CropScaleReadbackAndCleanTexture() can accept A8, whereas ScaleTexture() can't.. What do you think?
I've split up the review into two. Responding to comments here, going to publish new reviews in a sec. https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... content/browser/compositor/delegated_frame_host.cc:538: if (config != SkBitmap::kARGB_8888_Config && config != SkBitmap::kA8_Config) { It uses --disable-gpu, but when I try to launch chrome binary with this flag it crashes on startup: [6767:6767:0711/150707:FATAL:gpu_process_transport_factory.cc(202)] Failed to create UI context, but can't use software compositing with browser threaded compositing. Aborting. I tried adding --disable-threaded-compositing, but it still crashes with the same error. Tried using --disable-gpu-compositing instead, and it launches, but looks pretty messed up. https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... content/browser/compositor/delegated_frame_host.cc:546: SkBitmap scaledBitmap; On 2014/07/10 19:13:02, danakj wrote: > scaled_bitmap Done. https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... content/browser/compositor/delegated_frame_host.cc:548: source->height() != dst_size_in_pixel.height()) { On 2014/07/10 19:13:02, danakj wrote: > if they are == you want to return the original bitmap instead of an empty one? Oops. Done. https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... content/browser/compositor/delegated_frame_host.cc:559: // Else - |config| is kA8_Config. On 2014/07/10 19:13:02, danakj wrote: > how about DCHECK_EQ(config, kA8_Config) instead of a comment. Done. https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... content/browser/compositor/delegated_frame_host.cc:562: a8Bitmap.setConfig(SkBitmap::kA8_Config, On 2014/07/10 19:13:02, danakj wrote: > setConfig is deprecated. Use an SkImageInfo that you pass to allocPixels Done. https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... content/browser/compositor/delegated_frame_host.cc:571: filter->unref(); Neat! Done. https://codereview.chromium.org/384453002/diff/1/content/browser/frame_host/n... File content/browser/frame_host/navigation_entry_screenshot_manager.cc (right): https://codereview.chromium.org/384453002/diff/1/content/browser/frame_host/n... content/browser/frame_host/navigation_entry_screenshot_manager.cc:53: // Encode the A8 bitmap to grayscale PNG treating alpha as color intensity. Done. I did it in the other place because the stack trace from that error would be more useful (since this one is run on WorkerPool), but yeah, I guess that's not a great reason. https://codereview.chromium.org/384453002/diff/1/content/browser/frame_host/n... content/browser/frame_host/navigation_entry_screenshot_manager.cc:162: CHECK(bitmap.getConfig() == SkBitmap::kA8_Config) On 2014/07/10 19:13:02, danakj wrote: > DCHECK_EQ, then you don't need the extra << either as it'll print out both Done. https://codereview.chromium.org/384453002/diff/1/content/common/gpu/client/gl... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/384453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:327: // original, but when read back the pixel data can be used to construct the On 2014/07/10 19:13:02, danakj wrote: > just drop the reference to bitmaps here, this function itself has nothing to do > with bitmaps Done. See if you like the new version. https://codereview.chromium.org/384453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:330: GLuint TransformTextureToA8(GLuint src_texture, On 2014/07/10 19:13:03, danakj wrote: > TransformTextureToGrayscale. There's no A8 bitmaps here. Done. https://codereview.chromium.org/384453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:344: static const float kRGBtoA8ColorWeights[]; On 2014/07/10 19:13:03, danakj wrote: > toGrayscale Done. https://codereview.chromium.org/384453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:450: *result_size = gfx::Size((src_size.width() + 3) / 4, src_size.height()); On 2014/07/10 19:13:02, danakj wrote: > hm, doing this here feels awkward, cuz there's some channels in the last pixel > that are not valid. if we can move this later (inside ReadbackAsync maybe?) that > might be better? Got rid of result_size. https://codereview.chromium.org/384453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:532: if (bitmap_config != SkBitmap::kA8_Config && Responded separately in the reply. https://codereview.chromium.org/384453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:540: // For readback purposes, treat the resulting texture as ARGB of smaller Done. If the new version looks ok, I will make similar changes in ReadbackTextureSync https://codereview.chromium.org/384453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:564: src_size, On 2014/07/10 19:13:03, danakj wrote: > git cl format Done. https://codereview.chromium.org/384453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:602: kSwizzleBGRA, Good point, changed it back. I will separate this into a separate CL and add tests before committing. >Also is BGRA always the right thing to do on all hardware? It sounded like that from @hubbe's comments... Should I add one of the owners of gl_helper to the review?
https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... content/browser/compositor/delegated_frame_host.cc:538: if (config != SkBitmap::kARGB_8888_Config && config != SkBitmap::kA8_Config) { Ah, looks like I can call force_bitmap_result() on the request. I'll try that.
https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... content/browser/compositor/delegated_frame_host.cc:538: if (config != SkBitmap::kARGB_8888_Config && config != SkBitmap::kA8_Config) { On 2014/07/11 20:47:50, mfomitchev wrote: > Ah, looks like I can call force_bitmap_result() on the request. I'll try that. I'm planning to remove that. You want --ui-disable-threaded-compositing. What you really want is to call UseSoftwareCompositing() in your test which sets both these flags.
https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... content/browser/compositor/delegated_frame_host.cc:538: if (config != SkBitmap::kARGB_8888_Config && config != SkBitmap::kA8_Config) { Ok, thanks. Does this path actually have value? Looking at the GPU feature dashboard, we use GPU accelerated compositing in all configurations now. So will this code ever be executed outside of tests?
https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... content/browser/compositor/delegated_frame_host.cc:538: if (config != SkBitmap::kARGB_8888_Config && config != SkBitmap::kA8_Config) { On 2014/07/11 21:06:51, mfomitchev wrote: > Ok, thanks. > > Does this path actually have value? Looking at the GPU feature dashboard, we use > GPU accelerated compositing in all configurations now. So will this code ever be > executed outside of tests? We don't use software compositing on chromeos. We do use it on every other platform when gpu is blacklisted, when gpu process is unstable, or when users turn it off in their prefs.
https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... content/browser/compositor/delegated_frame_host.cc:538: if (config != SkBitmap::kARGB_8888_Config && config != SkBitmap::kA8_Config) { Got it, thanks. On a related note, do you think it's still worth creating a separate issue for Layer::RequestCopyOfOutput() (https://code.google.com/p/chromium/issues/detail?id=379983#c26)? The only thing not covered by this CL is optimizing the software path. We'd also have to add a way to specify the bitmap color format to CopyOutputRequest. I am not sure if it's worth it.
https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... content/browser/compositor/delegated_frame_host.cc:538: if (config != SkBitmap::kARGB_8888_Config && config != SkBitmap::kA8_Config) { On 2014/07/14 14:30:28, mfomitchev wrote: > Got it, thanks. On a related note, do you think it's still worth creating a > separate issue for Layer::RequestCopyOfOutput() > (https://code.google.com/p/chromium/issues/detail?id=379983#c26)? > > The only thing not covered by this CL is optimizing the software path. We'd also > have to add a way to specify the bitmap color format to CopyOutputRequest. I am > not sure if it's worth it. I was thinking about removing it actually, since the only thing that needs it is tests and they can use GLHelper instead.
https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... content/browser/compositor/delegated_frame_host.cc:538: if (config != SkBitmap::kARGB_8888_Config && config != SkBitmap::kA8_Config) { Are you talking about force_bitmap_result()? Specifying the color format might still help performance wise (for the case where it falls back on the software path and does return a bitmap), I am just not sure it's worth it. So no new issue for Layer::RequestCopyOfOutput()?
https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... content/browser/compositor/delegated_frame_host.cc:538: if (config != SkBitmap::kARGB_8888_Config && config != SkBitmap::kA8_Config) { On 2014/07/14 15:00:40, mfomitchev wrote: > Are you talking about force_bitmap_result()? Specifying the color format might > still help performance wise (for the case where it falls back on the software > path and does return a bitmap), I am just not sure it's worth it. So no new > issue for Layer::RequestCopyOfOutput()? Yep I'm talking about that. The bitmap output would only happen for software compositing. The texture output would only happen for GL compositing.
https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/384453002/diff/1/content/browser/compositor/d... content/browser/compositor/delegated_frame_host.cc:538: if (config != SkBitmap::kARGB_8888_Config && config != SkBitmap::kA8_Config) { Right, and as you said software compositing may be used as a fallback when gpu is blacklisted, etc... I was just looking at the discussion we had about creating a new issue for Layer::RequestCopyOfOutput() ( https://code.google.com/p/chromium/issues/detail?id=379983#c26 ). This fix covers the GPU path, so the only thing left is the software path. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
