Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(517)

Issue 482163002: Large wallpaper decoding in utility process (Closed)

Created:
6 years, 4 months ago by Greg Levin
Modified:
6 years, 2 months ago
CC:
chromium-apps-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, davemoore+watch_chromium.org, extensions-reviews_chromium.org, jam, nkostylev+watch_chromium.org, oshima+watch_chromium.org, stevenjb+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Handle large wallpaper decoding in utility process: Images over 32M pixels exceed IPC message limit size. In ChromeContentUtilityClient::DecodeImage(), cut image dimensions in half to minimize quality loss and allow decoded image to be returned via IPC message. BUG=366825 TEST=ChromeContentUtilityClientTest.DecodeImageSizeLimit; Set ChromeOS wallpaper using JPGs attached to bug Committed: https://crrev.com/d9d0e601c4c45ec8f2fb1d148e68458d9baa4e24 Cr-Commit-Position: refs/heads/master@{#296407} Committed: https://crrev.com/b18f4aa7717e44c208588b56f534e5e6ab7cd69e Cr-Commit-Position: refs/heads/master@{#296939}

Patch Set 1 : Resize image in utility process to fit IPC message size limit #

Total comments: 26

Patch Set 2 : Address reviewer comments, rework DecodeImage() #

Total comments: 2

Patch Set 3 : Add comment #

Patch Set 4 : Fix unit test for Android #

Patch Set 5 : Add comment about Issue 416916 #

Patch Set 6 : Customize & reduce max_msg_size for testing #

Total comments: 6

Patch Set 7 : Rename function and variable #

Total comments: 2

Patch Set 8 : Add space #

Unified diffs Side-by-side diffs Delta from patch set Stats (+149 lines, -15 lines) Patch
M chrome/browser/chromeos/extensions/wallpaper_function_base.cc View 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/extensions/webstore_install_helper.cc View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/image_decoder.h View 1 2 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/browser/image_decoder.cc View 2 chunks +4 lines, -2 lines 0 comments Download
M chrome/chrome_tests_unit.gypi View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_utility_messages.h View 1 chunk +3 lines, -2 lines 0 comments Download
M chrome/utility/chrome_content_utility_client.h View 1 2 3 4 5 6 2 chunks +12 lines, -2 lines 0 comments Download
M chrome/utility/chrome_content_utility_client.cc View 1 2 3 4 5 4 chunks +43 lines, -7 lines 0 comments Download
A chrome/utility/chrome_content_utility_client_unittest.cc View 1 2 3 4 5 6 7 1 chunk +79 lines, -0 lines 0 comments Download
M chrome/utility/extensions/extensions_handler.cc View 1 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 64 (18 generated)
Greg Levin
Patchset #1 (id:1) has been deleted
6 years, 3 months ago (2014-08-26 18:05:09 UTC) #1
Greg Levin
Patchset #1 (id:20001) has been deleted
6 years, 3 months ago (2014-08-26 18:05:21 UTC) #2
Greg Levin
Patchset #1 (id:40001) has been deleted
6 years, 3 months ago (2014-08-26 18:15:23 UTC) #3
Greg Levin
glevin@chromium.org changed reviewers: + dcheng@chromium.org, thakis@chromium.org, yoz@chromium.org
6 years, 3 months ago (2014-08-27 14:51:54 UTC) #4
Greg Levin
Noogler's first substantial CL, all feedback appreciated. * Would other callers of ImageDecoder() benefit from ...
6 years, 3 months ago (2014-08-27 15:45:00 UTC) #5
Yoyo Zhou
On 2014/08/27 15:45:00, Greg Levin wrote: > Noogler's first substantial CL, all feedback appreciated. > ...
6 years, 3 months ago (2014-08-27 19:40:01 UTC) #6
Greg Levin
> When sending to multiple reviewers, it helps to specify which files you would > ...
6 years, 3 months ago (2014-08-27 21:26:49 UTC) #7
Nico
The usual way is to get someone who's familiar with the feature you're trying to ...
6 years, 3 months ago (2014-08-27 22:32:15 UTC) #8
Nico
https://codereview.chromium.org/482163002/diff/60001/chrome/browser/image_decoder.cc File chrome/browser/image_decoder.cc (right): https://codereview.chromium.org/482163002/diff/60001/chrome/browser/image_decoder.cc#newcode23 chrome/browser/image_decoder.cc:23: shrink_to_fit_(false) { On 2014/08/27 22:32:15, Nico (hiding) wrote: > ...
6 years, 3 months ago (2014-08-27 22:34:29 UTC) #9
Yoyo Zhou
extensions changes LGTM
6 years, 3 months ago (2014-08-27 23:28:35 UTC) #10
Greg Levin
> The usual way is to get someone who's familiar with the feature you're trying ...
6 years, 3 months ago (2014-08-29 19:28:51 UTC) #12
Tom Sepez
The messages are OK, but if you can avoid multiplications, etc. you can dodge all ...
6 years, 3 months ago (2014-08-29 19:47:13 UTC) #13
Tom Sepez
https://codereview.chromium.org/482163002/diff/60001/chrome/utility/chrome_content_utility_client_unittest.cc File chrome/utility/chrome_content_utility_client_unittest.cc (right): https://codereview.chromium.org/482163002/diff/60001/chrome/utility/chrome_content_utility_client_unittest.cc#newcode57 chrome/utility/chrome_content_utility_client_unittest.cc:57: how about 4x just to run your loop more ...
6 years, 3 months ago (2014-08-29 19:51:09 UTC) #14
bshe
this is great. I just have a few nits and optional suggestions. https://codereview.chromium.org/482163002/diff/60001/chrome/utility/chrome_content_utility_client.cc File chrome/utility/chrome_content_utility_client.cc ...
6 years, 3 months ago (2014-08-29 19:56:27 UTC) #15
Greg Levin
> [...] if you can avoid multiplications, etc. you can dodge > all sorts of ...
6 years, 3 months ago (2014-09-04 16:19:34 UTC) #16
Greg Levin
Addressed comments, please have a look. https://codereview.chromium.org/482163002/diff/60001/chrome/utility/chrome_content_utility_client.cc File chrome/utility/chrome_content_utility_client.cc (right): https://codereview.chromium.org/482163002/diff/60001/chrome/utility/chrome_content_utility_client.cc#newcode191 chrome/utility/chrome_content_utility_client.cc:191: encoded_data.size()); On 2014/08/29 ...
6 years, 3 months ago (2014-09-04 16:39:19 UTC) #17
Tom Sepez
lgtm
6 years, 3 months ago (2014-09-04 17:22:06 UTC) #18
bshe
On 2014/09/04 17:22:06, Tom Sepez wrote: > lgtm lgtm
6 years, 3 months ago (2014-09-04 17:31:12 UTC) #19
sky
LGTM with a description of your new field https://codereview.chromium.org/482163002/diff/80001/chrome/browser/image_decoder.h File chrome/browser/image_decoder.h (right): https://codereview.chromium.org/482163002/diff/80001/chrome/browser/image_decoder.h#newcode75 chrome/browser/image_decoder.h:75: bool ...
6 years, 3 months ago (2014-09-09 20:19:11 UTC) #21
Greg Levin
Added comment, sending to skuhne https://codereview.chromium.org/482163002/diff/80001/chrome/browser/image_decoder.h File chrome/browser/image_decoder.h (right): https://codereview.chromium.org/482163002/diff/80001/chrome/browser/image_decoder.h#newcode75 chrome/browser/image_decoder.h:75: bool shrink_to_fit_; On 2014/09/09 ...
6 years, 3 months ago (2014-09-16 15:46:36 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/482163002/100001
6 years, 3 months ago (2014-09-16 15:48:31 UTC) #26
commit-bot: I haz the power
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tests_recipe/builds/9247)
6 years, 3 months ago (2014-09-16 17:54:09 UTC) #28
dcheng
It seems unfortunate that we have different ways of solving this problem for different parts ...
6 years, 3 months ago (2014-09-17 03:24:15 UTC) #30
Greg Levin
Trybot "android_dbg_tests_recipe" failed. Deep in content::DecodeImage(), Android was shrinking the image (for general memory conservation) ...
6 years, 3 months ago (2014-09-18 18:29:55 UTC) #34
dcheng
On 2014/09/18 at 18:29:55, glevin wrote: > Trybot "android_dbg_tests_recipe" failed. Deep in content::DecodeImage(), Android was ...
6 years, 3 months ago (2014-09-18 18:42:49 UTC) #35
Mr4D (OOO till 08-26)
lgtm (didn't realize that I got on the reviewers list by trying to land the ...
6 years, 3 months ago (2014-09-22 14:41:13 UTC) #36
Mr4D (OOO till 08-26)
I'll discuss with dcheng when he has time what we should do.
6 years, 3 months ago (2014-09-22 15:31:11 UTC) #37
Mr4D (OOO till 08-26)
As discussed with dcheng: Utility functions should do what they are supposed to do: Decode ...
6 years, 3 months ago (2014-09-22 17:31:35 UTC) #38
Tom Sepez
lgtm
6 years, 3 months ago (2014-09-23 18:25:37 UTC) #39
dcheng
lgtm
6 years, 3 months ago (2014-09-23 21:10:52 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/482163002/190001
6 years, 3 months ago (2014-09-23 21:21:55 UTC) #42
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_swarming/builds/16664)
6 years, 3 months ago (2014-09-23 21:29:34 UTC) #44
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/482163002/190001
6 years, 3 months ago (2014-09-24 13:54:22 UTC) #46
commit-bot: I haz the power
Committed patchset #5 (id:190001) as 4735e48a27fc303706197d626d19febd06c0375c
6 years, 3 months ago (2014-09-24 14:17:30 UTC) #47
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/d9d0e601c4c45ec8f2fb1d148e68458d9baa4e24 Cr-Commit-Position: refs/heads/master@{#296407}
6 years, 3 months ago (2014-09-24 14:18:04 UTC) #48
jiayl
A revert of this CL (patchset #5 id:190001) has been created in https://codereview.chromium.org/600913002/ by jiayl@chromium.org. ...
6 years, 3 months ago (2014-09-24 17:52:48 UTC) #49
Greg Levin
An image that needed 4x reduction to fit kMaximumMessageSize was about 14000 x 9400, and ...
6 years, 3 months ago (2014-09-24 23:58:21 UTC) #52
Mr4D (OOO till 08-26)
This should do the trick! Only a few more nits... https://codereview.chromium.org/482163002/diff/250001/chrome/utility/chrome_content_utility_client.h File chrome/utility/chrome_content_utility_client.h (right): https://codereview.chromium.org/482163002/diff/250001/chrome/utility/chrome_content_utility_client.h#newcode40 ...
6 years, 3 months ago (2014-09-25 02:15:03 UTC) #53
Greg Levin
https://codereview.chromium.org/482163002/diff/250001/chrome/utility/chrome_content_utility_client.h File chrome/utility/chrome_content_utility_client.h (right): https://codereview.chromium.org/482163002/diff/250001/chrome/utility/chrome_content_utility_client.h#newcode40 chrome/utility/chrome_content_utility_client.h:40: static void set_max_message_size(int64_t max_message_size) { On 2014/09/25 02:15:03, Mr4D ...
6 years, 2 months ago (2014-09-25 18:51:07 UTC) #54
Mr4D (OOO till 08-26)
lgtm [after fixing the last nit] https://codereview.chromium.org/482163002/diff/270001/chrome/utility/chrome_content_utility_client_unittest.cc File chrome/utility/chrome_content_utility_client_unittest.cc (right): https://codereview.chromium.org/482163002/diff/270001/chrome/utility/chrome_content_utility_client_unittest.cc#newcode49 chrome/utility/chrome_content_utility_client_unittest.cc:49: int max_height_for_msg = ...
6 years, 2 months ago (2014-09-25 19:34:49 UTC) #55
Greg Levin
https://codereview.chromium.org/482163002/diff/270001/chrome/utility/chrome_content_utility_client_unittest.cc File chrome/utility/chrome_content_utility_client_unittest.cc (right): https://codereview.chromium.org/482163002/diff/270001/chrome/utility/chrome_content_utility_client_unittest.cc#newcode49 chrome/utility/chrome_content_utility_client_unittest.cc:49: int max_height_for_msg = sqrt(kTestMessageSize/(1.5*4)); On 2014/09/25 19:34:48, Mr4D wrote: ...
6 years, 2 months ago (2014-09-25 19:45:16 UTC) #56
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/482163002/290001
6 years, 2 months ago (2014-09-25 19:49:00 UTC) #58
commit-bot: I haz the power
Exceeded time limit waiting for builds to trigger.
6 years, 2 months ago (2014-09-26 01:51:05 UTC) #60
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/482163002/290001
6 years, 2 months ago (2014-09-26 14:37:30 UTC) #62
commit-bot: I haz the power
Committed patchset #8 (id:290001) as a68e15738c20bdf6372419270e51462c11ff8341
6 years, 2 months ago (2014-09-26 15:02:42 UTC) #63
commit-bot: I haz the power
6 years, 2 months ago (2014-09-26 15:03:34 UTC) #64
Message was sent while issue was closed.
Patchset 8 (id:??) landed as
https://crrev.com/b18f4aa7717e44c208588b56f534e5e6ab7cd69e
Cr-Commit-Position: refs/heads/master@{#296939}

Powered by Google App Engine
This is Rietveld 408576698