Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
389683002:
Improving GestureNav screenshotting performance - part 2 - GestureNav (Closed)
|
Created:
6 years, 5 months ago by mfomitchev Modified:
6 years, 3 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@slow_readback_final_GL_HELPER Project:
chromium Visibility:
Public. |
DescriptionImproving GestureNav screenshotting performance - part 2 - GestureNav
Utilize the new gl_helper functionality to capture GestureNav screenshots
in grayscale, which drastically improves screencapture performance,
especially on hardware configurations with expensive RGBA readback.
Part 1: https://codereview.chromium.org/388953002/
Original review: https://codereview.chromium.org/384453002/
BUG=379983, 380779
Committed: https://crrev.com/a6d1cf9094a3af3011dbb8233d5d7469d7d8fddc
Cr-Commit-Position: refs/heads/master@{#291992}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Rebase and cleanup #Patch Set 3 : Comment #
Total comments: 10
Patch Set 4 : Another comment #
Total comments: 6
Patch Set 5 : Addressing review feedback. #Patch Set 6 : Fixing NavigationControllerTest.PurgeScreenshot in content_unittests #
Messages
Total messages: 20 (0 generated)
https://codereview.chromium.org/389683002/diff/1/content/browser/compositor/d... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/389683002/diff/1/content/browser/compositor/d... content/browser/compositor/delegated_frame_host.cc:127: // Only ARGB888 and RGB565 supported as of now. Comment needs updating. Would be nice to use a gl_helper method to determine if the format supported - goes back to IsReadbackConfigSupported discussion in part 1.
https://codereview.chromium.org/389683002/diff/1/content/browser/compositor/d... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/389683002/diff/1/content/browser/compositor/d... content/browser/compositor/delegated_frame_host.cc:563: grayscale_bitmap.allocPixels( We should first check the color type of the returned bitmap here. If it's A8, we can just return it.
@sievers: Please review changes in delegated_frame_host.cc @sky: please review changes in navigation_entry_screenshot_manager.cc https://codereview.chromium.org/389683002/diff/1/content/browser/compositor/d... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/389683002/diff/1/content/browser/compositor/d... content/browser/compositor/delegated_frame_host.cc:563: grayscale_bitmap.allocPixels( On 2014/07/11 21:20:13, mfomitchev_OOO_Aug16-24 wrote: > We should first check the color type of the returned bitmap here. If it's A8, we > can just return it. Done.
https://codereview.chromium.org/389683002/diff/40001/content/browser/composit... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/389683002/diff/40001/content/browser/composit... content/browser/compositor/delegated_frame_host.cc:593: void DelegatedFrameHost::PrepareBitmapCopyOutputResult( This is only for the software renderer, right? Because otherwise for result->HasTexture() we go through PrepareTextureCopyOutputResult() which calls CropScaleReadbackAndCleanMailbox() and I assume already does the right thing after your changes? https://codereview.chromium.org/389683002/diff/40001/content/browser/composit... content/browser/compositor/delegated_frame_host.cc:622: DCHECK_EQ(color_type, kAlpha_8_SkColorType); It's a bit unfortunate that all over the place we treat Alpha8 as a grayscale format now. I guess luminance or something would be better. But seems like it's the only Skia format that works? Or would SkUnknown_SkColorType actually be better and then we define our own enums for GLHelper? https://codereview.chromium.org/389683002/diff/40001/content/browser/composit... content/browser/compositor/delegated_frame_host.cc:624: callback.Run(true, scaled_bitmap); How can we end up here? Who converted the bitmap? https://codereview.chromium.org/389683002/diff/40001/content/browser/composit... content/browser/compositor/delegated_frame_host.cc:637: skia::RefPtr<SkColorFilter> filter = So if we wanted to scale and filter in one pass, we couldn't use the BEST resizer from ImageOperations?
https://codereview.chromium.org/389683002/diff/40001/content/browser/composit... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/389683002/diff/40001/content/browser/composit... content/browser/compositor/delegated_frame_host.cc:593: void DelegatedFrameHost::PrepareBitmapCopyOutputResult( Yup. We used to paint the bitmap to A8 bitmap in ScreenshotData::EncodeOnWorker(), and now we don't since CopyFromCompositingSurfaceFinished/CropScaleReadbackAndCleanMailbox now creates a grayscale bitmap. https://codereview.chromium.org/389683002/diff/40001/content/browser/composit... content/browser/compositor/delegated_frame_host.cc:622: DCHECK_EQ(color_type, kAlpha_8_SkColorType); Using kUnknown_SkColorType seems pretty hacky to me.. I guess we could get a Skia owner to comment on this. It's worth noting, we don't actually use this A8 bitmap to paint. We use its pixel data to encode to grayscale PNG... https://codereview.chromium.org/389683002/diff/40001/content/browser/composit... content/browser/compositor/delegated_frame_host.cc:624: callback.Run(true, scaled_bitmap); I don't think we can currently. However kAlpha_8_SkColorType is the color type we put into cc::CopyOutputRequest object. Currently it's ignored by the software renderer, but I thought it's worth adding this check in case software renderer decides to respect the color type in the request in the future (and perhaps implements some sort of an optimized conversion). https://codereview.chromium.org/389683002/diff/40001/content/browser/composit... content/browser/compositor/delegated_frame_host.cc:637: skia::RefPtr<SkColorFilter> filter = I am not sure. Do you know how would one resize and color-filter in one path with Skia? FWIW, currently we only use this for grayscale conversion and never scale.
lgtm https://codereview.chromium.org/389683002/diff/40001/content/browser/composit... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/389683002/diff/40001/content/browser/composit... content/browser/compositor/delegated_frame_host.cc:624: callback.Run(true, scaled_bitmap); On 2014/08/25 22:38:35, mfomitchev_OOO_Aug16-24 wrote: > I don't think we can currently. However kAlpha_8_SkColorType is the color type > we put into cc::CopyOutputRequest object. Currently it's ignored by the software > renderer, but I thought it's worth adding this check in case software renderer > decides to respect the color type in the request in the future (and perhaps > implements some sort of an optimized conversion). Or maybe put a DCHECK(scaled_bitmap.colorType() != kAlpha_8_SkColorType) here for the time being. https://codereview.chromium.org/389683002/diff/40001/content/browser/composit... content/browser/compositor/delegated_frame_host.cc:637: skia::RefPtr<SkColorFilter> filter = On 2014/08/25 22:38:35, mfomitchev_OOO_Aug16-24 wrote: > I am not sure. Do you know how would one resize and color-filter in one path > with Skia? FWIW, currently we only use this for grayscale conversion and never > scale. I *think* if you make the canvas smaller/bigger it will do scaling, but probably not the filtering you want. But not sure how much you care about the sw compositor here. I guess you are not really making it slower for your use case compared to before, since you are just moving the encoding pass from caller to callee.
https://codereview.chromium.org/389683002/diff/60001/content/browser/composit... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/389683002/diff/60001/content/browser/composit... content/browser/compositor/delegated_frame_host.cc:598: if (color_type != kN32_SkColorType && color_type != kAlpha_8_SkColorType) { Why do we support 565 on 143, but not here? https://codereview.chromium.org/389683002/diff/60001/content/browser/composit... content/browser/compositor/delegated_frame_host.cc:618: DCHECK(scaled_bitmap.colorType() == kN32_SkColorType); DCHECK_EQ https://codereview.chromium.org/389683002/diff/60001/content/browser/composit... content/browser/compositor/delegated_frame_host.cc:622: DCHECK_EQ(color_type, kAlpha_8_SkColorType); Is this because ImageOperations::Resize is always converting to kN32? Shouldn't it create in the same format as passed?
https://codereview.chromium.org/389683002/diff/60001/content/browser/composit... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/389683002/diff/60001/content/browser/composit... content/browser/compositor/delegated_frame_host.cc:598: if (color_type != kN32_SkColorType && color_type != kAlpha_8_SkColorType) { 565 is supported by the hardware path (PrepareTextureCopyOutputResult) but not the software path. I guess this is inconsistent, but it's been this way before my change, I am just adding A8 support. I can file a separate bug for this if you think it's worth it. https://codereview.chromium.org/389683002/diff/60001/content/browser/composit... content/browser/compositor/delegated_frame_host.cc:618: DCHECK(scaled_bitmap.colorType() == kN32_SkColorType); On 2014/08/25 23:28:19, sky wrote: > DCHECK_EQ Done. https://codereview.chromium.org/389683002/diff/60001/content/browser/composit... content/browser/compositor/delegated_frame_host.cc:622: DCHECK_EQ(color_type, kAlpha_8_SkColorType); The source bitmap is N32 to begin with. The software path currently always returns N32 bitmap regardless of the color_type we ask for.
Ok, LGTM
The CQ bit was checked by mfomitchev@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/389683002/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mfomitchev@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/389683002/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Message was sent while issue was closed.
Committed patchset #6 (100001) as bc5b393b8d36082c09ca8333ced2b0d92eccf762
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a6d1cf9094a3af3011dbb8233d5d7469d7d8fddc Cr-Commit-Position: refs/heads/master@{#291992} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
