Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
569623002:
Switch ChromeOS Chromium login to use IJG libjpeg (Closed)
DescriptionSwitch ChromeOS Chromium login to use IJG libjpeg
The default libjpeg library is libjpeg_turbo, except for Chrome OS. This
change switches the Chrome OS build to use IJG jpeg for the login screen.
The intended outcome is to have everything use libjpeg_turbo except for
JPEGCodec in ui/gfx.
ChromeUtilityMsg_RobustJPEGDecodeImage:
Send decodes to JPEGCodec which then uses chromium_ijg_ mangled libjpeg to
decode JPEGs.
ChromeUtilityMsg_DecodeImage:
Send decodes to blink WebImage. Any JPEGs would then be decoded with
libjpeg_turbo mangled with chromium_jpeg_.
User login code avatar/user_image_manager_impl.cc and
wallpaper/wallpaper_manager.cc currently setup UserImageLoader with
ImageDecoder::ROBUST_JPEG_DECODE.
Tests were run on a Peppy (Haswell 2955U), using the image_decoding benchmark
in telemetry.
ImageDecoding_avg.image_decoding.html?jpg:
Stock (ms): 166.98 166.80 163.03
Patch (ms): 78.95 73.27 78.23
TBR=cpu@chromium.org
BUG=413712
TEST=
$ cd ./tools/perf
$ ./run_benchmark --remote=${IP} \
--browser=cros-chrome-guest \
run image_decoding.image_decoding_measurement
Committed: https://crrev.com/199f66b202c942421e25ced69471f7d9d1f2c4a7
Cr-Commit-Position: refs/heads/master@{#328812}
Patch Set 1 #
Total comments: 1
Patch Set 2 : ChromeOS never uses USE_SYSTEM_* #
Total comments: 1
Patch Set 3 : Update BUILD.gn and add GYP libjpeg variables #
Total comments: 1
Patch Set 4 : Update BUILD.gn with TODO #
Total comments: 4
Patch Set 5 : First submission, update AUTHORS #Patch Set 6 : Only modify build configuration #Patch Set 7 : Rebase on ToT #
Total comments: 2
Patch Set 8 : Rebase #Patch Set 9 : Wrap third_party/BUILD.gn config inside if #Patch Set 10 : Copy decode portion of JPEGCodec to create JPEGCodecRobust #Patch Set 11 : Update GYP/GN patch set #Patch Set 12 : Update JPEGCodecRobust #
Total comments: 2
Patch Set 13 : JPEGCodecRobustSlow #
Total comments: 13
Patch Set 14 : Fix copyright and add unit test assertions #
Messages
Total messages: 84 (9 generated)
edward.baker@intel.com changed reviewers: + jorgelo@chromium.org
I went through all of the commits from BUG=144296 and stepped through the history to verify ROBUST_JPEG_CODEC usage. I believe all of the plumbing already exists for robustly decoding JPEGs at the login screen with ui/gfx JPEGCodec. However, for Chrome OS JPEGCodec needs to always build with IJG libjpeg.
On 2014/09/12 15:48:45, Ed Baker wrote: > I went through all of the commits from BUG=144296 and stepped through the > history to verify ROBUST_JPEG_CODEC usage. I believe all of the plumbing already > exists for robustly decoding JPEGs at the login screen with ui/gfx JPEGCodec. > However, for Chrome OS JPEGCodec needs to always build with IJG libjpeg. lgtm, but you're gonna need OWNERS approval as well. Look for OWNERS files inside the directories with files you're modifying.
edward.baker@intel.com changed reviewers: + dcheng@chromium.org, erg@chromium.org
On 2014/09/16 15:34:43, Jorge Lucangeli Obes wrote: > On 2014/09/12 15:48:45, Ed Baker wrote: > > I went through all of the commits from BUG=144296 and stepped through the > > history to verify ROBUST_JPEG_CODEC usage. I believe all of the plumbing > already > > exists for robustly decoding JPEGs at the login screen with ui/gfx JPEGCodec. > > However, for Chrome OS JPEGCodec needs to always build with IJG libjpeg. > > lgtm, but you're gonna need OWNERS approval as well. Look for OWNERS files > inside the directories with files you're modifying. Thanks for the review and advice Jorge. Sorry about the rookie mistakes :). Added Daniel and Elliot from the ui/gfx/codec OWNERS.
lgtm++ https://chromiumcodereview.appspot.com/569623002/diff/1/ui/gfx/codec/jpeg_cod... File ui/gfx/codec/jpeg_codec.cc (right): https://chromiumcodereview.appspot.com/569623002/diff/1/ui/gfx/codec/jpeg_cod... ui/gfx/codec/jpeg_codec.cc:15: #if defined(USE_SYSTEM_LIBJPEG) && !defined(OS_CHROMEOS) I don't believe that chromeos ever uses USE_SYSTEM_* since it's a linux desktopism. You can probably drop this and the one below in the library variant method.
On 2014/09/16 18:01:58, Elliot Glaysher wrote: > lgtm++ > > https://chromiumcodereview.appspot.com/569623002/diff/1/ui/gfx/codec/jpeg_cod... > File ui/gfx/codec/jpeg_codec.cc (right): > > https://chromiumcodereview.appspot.com/569623002/diff/1/ui/gfx/codec/jpeg_cod... > ui/gfx/codec/jpeg_codec.cc:15: #if defined(USE_SYSTEM_LIBJPEG) && > !defined(OS_CHROMEOS) > I don't believe that chromeos ever uses USE_SYSTEM_* since it's a linux > desktopism. You can probably drop this and the one below in the library variant > method. Thanks for the feedback, dropped in patch set 2.
On 2014/09/16 19:48:17, Ed Baker wrote: > On 2014/09/16 18:01:58, Elliot Glaysher wrote: > > lgtm++ > > > > > https://chromiumcodereview.appspot.com/569623002/diff/1/ui/gfx/codec/jpeg_cod... > > File ui/gfx/codec/jpeg_codec.cc (right): > > > > > https://chromiumcodereview.appspot.com/569623002/diff/1/ui/gfx/codec/jpeg_cod... > > ui/gfx/codec/jpeg_codec.cc:15: #if defined(USE_SYSTEM_LIBJPEG) && > > !defined(OS_CHROMEOS) > > I don't believe that chromeos ever uses USE_SYSTEM_* since it's a linux > > desktopism. You can probably drop this and the one below in the library > variant > > method. > > Thanks for the feedback, dropped in patch set 2. I think will also need OWNERS for the .gyp changes. 'git cl upload' should be suggesting the right OWNERS.
edward.baker@intel.com changed reviewers: + sky@chromium.org - dcheng@chromium.org
On 2014/09/16 21:11:31, Jorge Lucangeli Obes wrote: > On 2014/09/16 19:48:17, Ed Baker wrote: > > On 2014/09/16 18:01:58, Elliot Glaysher wrote: > > > lgtm++ > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/1/ui/gfx/codec/jpeg_cod... > > > File ui/gfx/codec/jpeg_codec.cc (right): > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/1/ui/gfx/codec/jpeg_cod... > > > ui/gfx/codec/jpeg_codec.cc:15: #if defined(USE_SYSTEM_LIBJPEG) && > > > !defined(OS_CHROMEOS) > > > I don't believe that chromeos ever uses USE_SYSTEM_* since it's a linux > > > desktopism. You can probably drop this and the one below in the library > > variant > > > method. > > > > Thanks for the feedback, dropped in patch set 2. > > I think will also need OWNERS for the .gyp changes. 'git cl upload' should be > suggesting the right OWNERS. I ran `git cl owners -vv` for some additional info. It suggests reviewers for jpeg_codec.cc (erg), but not the gyp/gypi changes... sky: Could you review modifications to gfx.gyp and common.gypi?
https://chromiumcodereview.appspot.com/569623002/diff/20001/ui/gfx/gfx.gyp File ui/gfx/gfx.gyp (right): https://chromiumcodereview.appspot.com/569623002/diff/20001/ui/gfx/gfx.gyp#ne... ui/gfx/gfx.gyp:366: '<(DEPTH)/third_party/libjpeg/libjpeg.gyp:libjpeg', Can you make this: '<(libjpeg_gyp_path):libjpeg', just like on line 321?
Also, do you need to update ui/gfx/BUILD.gn?
On 2014/09/17 14:38:42, sky wrote: > Also, do you need to update ui/gfx/BUILD.gn? I'm guessing yes.
On 2014/09/17 14:38:20, sky wrote: > https://chromiumcodereview.appspot.com/569623002/diff/20001/ui/gfx/gfx.gyp > File ui/gfx/gfx.gyp (right): > > https://chromiumcodereview.appspot.com/569623002/diff/20001/ui/gfx/gfx.gyp#ne... > ui/gfx/gfx.gyp:366: '<(DEPTH)/third_party/libjpeg/libjpeg.gyp:libjpeg', > Can you make this: > '<(libjpeg_gyp_path):libjpeg', > just like on line 321? libjpeg_gyp_path is switched between libjpeg and libjpeg_turbo https://chromium.googlesource.com/chromium/src.git/+/master/build/common.gypi... . I was originally thinking about introducing two additional variables such as libjpeg_ijg_gyp_path and libjpeg_turbo_gyp_path, and set libjpeg_gyp_path to one of those. Then this change could use libjpeg_ijg_gyp_path. I'm not at my desk today, but will hopefully get a patch set submitted tomorrow. Thanks for the feedback.
On 2014/09/17 14:38:42, sky wrote: > Also, do you need to update ui/gfx/BUILD.gn? Updated in patch set 3. I am still determining how to best test the BUILD.gn change in the context of the Chrome OS ebuild.
On 2014/09/17 17:40:11, Ed Baker wrote: > On 2014/09/17 14:38:20, sky wrote: > > https://chromiumcodereview.appspot.com/569623002/diff/20001/ui/gfx/gfx.gyp > > File ui/gfx/gfx.gyp (right): > > > > > https://chromiumcodereview.appspot.com/569623002/diff/20001/ui/gfx/gfx.gyp#ne... > > ui/gfx/gfx.gyp:366: '<(DEPTH)/third_party/libjpeg/libjpeg.gyp:libjpeg', > > Can you make this: > > '<(libjpeg_gyp_path):libjpeg', > > just like on line 321? > > libjpeg_gyp_path is switched between libjpeg and libjpeg_turbo > https://chromium.googlesource.com/chromium/src.git/+/master/build/common.gypi... > . I was originally thinking about introducing two additional variables such as > libjpeg_ijg_gyp_path and libjpeg_turbo_gyp_path, and set libjpeg_gyp_path to one > of those. Then this change could use libjpeg_ijg_gyp_path. I'm not at my desk > today, but will hopefully get a patch set submitted tomorrow. Thanks for the > feedback. Patch set three contains two additional variables for libjpeg paths. Let me know if it seems reasonable to add two variables for a Chrome OS specific situation.
On 2014/09/18 20:25:21, Ed Baker wrote: > On 2014/09/17 14:38:42, sky wrote: > > Also, do you need to update ui/gfx/BUILD.gn? > > Updated in patch set 3. I am still determining how to best test the BUILD.gn > change in the context of the Chrome OS ebuild. You might not be able to. The GN build still doesn't work with the chromeos=1 define. https://code.google.com/p/chromium/wiki/GNQuickStart should allow you to test that the Linux build is not broken, you might have already done that. I'm not quite sure what the best way to test this is. Maybe we just add a "TODO(jorgelo): make sure this works on Chrome OS." to BUILD.gn so that we don't lose track of this when the GN build is fixed for chromeos=1.
https://chromiumcodereview.appspot.com/569623002/diff/40001/ui/gfx/BUILD.gn File ui/gfx/BUILD.gn (right): https://chromiumcodereview.appspot.com/569623002/diff/40001/ui/gfx/BUILD.gn#n... ui/gfx/BUILD.gn:322: # ChromeOS Nit: Chrome OS with a space.
https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn File ui/gfx/BUILD.gn (right): https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn#n... ui/gfx/BUILD.gn:323: # TODO(jorgelo): make sure this works on Chrome OS. Please verify before landing. https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... File ui/gfx/codec/jpeg_codec.cc (right): https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... ui/gfx/codec/jpeg_codec.cc:17: #elif defined(USE_LIBJPEG_TURBO) && !defined(OS_CHROMEOS) Why is SE_LIBJPEG_TURBO set for chromeos if we're not using it?
On 2014/09/18 20:54:31, Jorge Lucangeli Obes wrote: > On 2014/09/18 20:25:21, Ed Baker wrote: > > On 2014/09/17 14:38:42, sky wrote: > > > Also, do you need to update ui/gfx/BUILD.gn? > > > > Updated in patch set 3. I am still determining how to best test the BUILD.gn > > change in the context of the Chrome OS ebuild. > > You might not be able to. The GN build still doesn't work with the chromeos=1 > define. > > https://code.google.com/p/chromium/wiki/GNQuickStart should allow you to test > that the Linux build is not broken, you might have already done that. > > I'm not quite sure what the best way to test this is. Maybe we just add a > "TODO(jorgelo): make sure this works on Chrome OS." to BUILD.gn so that we don't > lose track of this when the GN build is fixed for chromeos=1. I tested BUILD.gn patch set 4 changes with `gn gen out/test` and `ninja -C out/test chrome`. The final LINK fails (warnings as errors) on unrelated hidden symbol warnings from harfbuzz.
https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn File ui/gfx/BUILD.gn (right): https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn#n... ui/gfx/BUILD.gn:323: # TODO(jorgelo): make sure this works on Chrome OS. On 2014/09/18 22:19:37, sky wrote: > Please verify before landing. But the GN build does not work with chromeos=1, how would you verify this?
https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... File ui/gfx/codec/jpeg_codec.cc (right): https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... ui/gfx/codec/jpeg_codec.cc:17: #elif defined(USE_LIBJPEG_TURBO) && !defined(OS_CHROMEOS) On 2014/09/18 22:19:37, sky wrote: > Why is SE_LIBJPEG_TURBO set for chromeos if we're not using it? Chrome OS uses both IJG libjpeg and libjpeg_turbo. The robust decodes go through IJG libjpeg. All other decodes (blink) are handled by libjpeg_turbo.
On 2014/09/18 23:07:36, Ed Baker wrote: > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > File ui/gfx/codec/jpeg_codec.cc (right): > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > ui/gfx/codec/jpeg_codec.cc:17: #elif defined(USE_LIBJPEG_TURBO) && > !defined(OS_CHROMEOS) > On 2014/09/18 22:19:37, sky wrote: > > Why is SE_LIBJPEG_TURBO set for chromeos if we're not using it? > > Chrome OS uses both IJG libjpeg and libjpeg_turbo. The robust decodes go through > IJG libjpeg. All other decodes (blink) are handled by libjpeg_turbo. Sorry, I spoke too quickly. USE_LIBJPEG_TURBO is indeed only used by jpeg_codec.cc. I suppose similar functionality could be achieved by modifying the gyp files to not set the define when chromeos==1.
I guess that means you can't verify it:( On Thu, Sep 18, 2014 at 3:22 PM, <jorgelo@chromium.org> wrote: > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn > File ui/gfx/BUILD.gn (right): > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn#n... > ui/gfx/BUILD.gn:323: # TODO(jorgelo): make sure this works on Chrome OS. > On 2014/09/18 22:19:37, sky wrote: >> >> Please verify before landing. > > > But the GN build does not work with chromeos=1, how would you verify > this? > > https://chromiumcodereview.appspot.com/569623002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/09/18 22:19:37, sky wrote: > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn > File ui/gfx/BUILD.gn (right): > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn#n... > ui/gfx/BUILD.gn:323: # TODO(jorgelo): make sure this works on Chrome OS. > Please verify before landing. > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > File ui/gfx/codec/jpeg_codec.cc (right): > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > ui/gfx/codec/jpeg_codec.cc:17: #elif defined(USE_LIBJPEG_TURBO) && > !defined(OS_CHROMEOS) > Why is SE_LIBJPEG_TURBO set for chromeos if we're not using it? Patch set 6 removes the jpeg_codec.cc changes since USE_LIBJPEG_TURBO already handles the build logic. GYP: I verified the correct libraries are used with additional debugging. UserImageManagerImpl and WallpaperManager decodes were handled by libjpeg. GN: `gn gen out/test --args="os=\"chromeos\""` fails on unrelated undefined identifiers. `gn gen out/test` completes OK.
On 2014/09/22 18:30:40, Ed Baker wrote: > On 2014/09/18 22:19:37, sky wrote: > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn > > File ui/gfx/BUILD.gn (right): > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn#n... > > ui/gfx/BUILD.gn:323: # TODO(jorgelo): make sure this works on Chrome OS. > > Please verify before landing. > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > > File ui/gfx/codec/jpeg_codec.cc (right): > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > > ui/gfx/codec/jpeg_codec.cc:17: #elif defined(USE_LIBJPEG_TURBO) && > > !defined(OS_CHROMEOS) > > Why is SE_LIBJPEG_TURBO set for chromeos if we're not using it? > > Patch set 6 removes the jpeg_codec.cc changes since USE_LIBJPEG_TURBO already > handles the build logic. > > GYP: > I verified the correct libraries are used with additional debugging. > UserImageManagerImpl and WallpaperManager decodes were handled by libjpeg. > > GN: > `gn gen out/test --args="os=\"chromeos\""` fails on unrelated undefined > identifiers. > `gn gen out/test` completes OK. lgtm since I think this is as far as GN will let us debug this so far. Maybe try to actually build the GN build just in case.
On 2014/09/22 18:32:08, Jorge Lucangeli Obes wrote: > On 2014/09/22 18:30:40, Ed Baker wrote: > > On 2014/09/18 22:19:37, sky wrote: > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn > > > File ui/gfx/BUILD.gn (right): > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn#n... > > > ui/gfx/BUILD.gn:323: # TODO(jorgelo): make sure this works on Chrome OS. > > > Please verify before landing. > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > > > File ui/gfx/codec/jpeg_codec.cc (right): > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > > > ui/gfx/codec/jpeg_codec.cc:17: #elif defined(USE_LIBJPEG_TURBO) && > > > !defined(OS_CHROMEOS) > > > Why is SE_LIBJPEG_TURBO set for chromeos if we're not using it? > > > > Patch set 6 removes the jpeg_codec.cc changes since USE_LIBJPEG_TURBO already > > handles the build logic. > > > > GYP: > > I verified the correct libraries are used with additional debugging. > > UserImageManagerImpl and WallpaperManager decodes were handled by libjpeg. > > > > GN: > > `gn gen out/test --args="os=\"chromeos\""` fails on unrelated undefined > > identifiers. > > `gn gen out/test` completes OK. > > lgtm since I think this is as far as GN will let us debug this so far. > > Maybe try to actually build the GN build just in case. Ninja fails on unrelated harfbuzz warnings as errors when linking. $ gn gen out/test Done. Wrote 1419 targets from 520 files in 638ms $ ninja -C out/test chrome ninja: Entering directory `out/test' [15019/15019] LINK ./chrome <snip> ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: warning: hidden symbol 'hb_buffer_create' in obj/third_party/harfbuzz-ng/src/harfbuzz-ng.hb-buffer.o is referenced by DSO /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../x86_64-linux-gnu/libpangoft2-1.0.so ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error: treating warnings as errors <snip>
On 2014/09/22 19:54:47, Ed Baker wrote: > On 2014/09/22 18:32:08, Jorge Lucangeli Obes wrote: > > On 2014/09/22 18:30:40, Ed Baker wrote: > > > On 2014/09/18 22:19:37, sky wrote: > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn > > > > File ui/gfx/BUILD.gn (right): > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn#n... > > > > ui/gfx/BUILD.gn:323: # TODO(jorgelo): make sure this works on Chrome OS. > > > > Please verify before landing. > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > > > > File ui/gfx/codec/jpeg_codec.cc (right): > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > > > > ui/gfx/codec/jpeg_codec.cc:17: #elif defined(USE_LIBJPEG_TURBO) && > > > > !defined(OS_CHROMEOS) > > > > Why is SE_LIBJPEG_TURBO set for chromeos if we're not using it? > > > > > > Patch set 6 removes the jpeg_codec.cc changes since USE_LIBJPEG_TURBO > already > > > handles the build logic. > > > > > > GYP: > > > I verified the correct libraries are used with additional debugging. > > > UserImageManagerImpl and WallpaperManager decodes were handled by libjpeg. > > > > > > GN: > > > `gn gen out/test --args="os=\"chromeos\""` fails on unrelated undefined > > > identifiers. > > > `gn gen out/test` completes OK. > > > > lgtm since I think this is as far as GN will let us debug this so far. > > > > Maybe try to actually build the GN build just in case. > > Ninja fails on unrelated harfbuzz warnings as errors when linking. > $ gn gen out/test > Done. Wrote 1419 targets from 520 files in 638ms > $ ninja -C out/test chrome > ninja: Entering directory `out/test' > [15019/15019] LINK ./chrome > <snip> > ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: warning: hidden symbol > 'hb_buffer_create' in obj/third_party/harfbuzz-ng/src/harfbuzz-ng.hb-buffer.o is > referenced by DSO > /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../x86_64-linux-gnu/libpangoft2-1.0.so > ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error: treating > warnings as errors > <snip> Rebased on master as common.gypi changes were not merging cleanly. I also retested the GN build after reviewing https://code.google.com/p/chromium/issues/detail?id=353127 . $ gn gen out/test --args="use_system_harfbuzz=true" Done. Wrote 1615 targets from 551 files in 660ms $ ninja -C out/test chrome ninja: Entering directory `out/test' [460/460] LINK ./chrome $
On 2014/09/23 19:49:43, Ed Baker wrote: > On 2014/09/22 19:54:47, Ed Baker wrote: > > On 2014/09/22 18:32:08, Jorge Lucangeli Obes wrote: > > > On 2014/09/22 18:30:40, Ed Baker wrote: > > > > On 2014/09/18 22:19:37, sky wrote: > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn > > > > > File ui/gfx/BUILD.gn (right): > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn#n... > > > > > ui/gfx/BUILD.gn:323: # TODO(jorgelo): make sure this works on Chrome OS. > > > > > Please verify before landing. > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > > > > > File ui/gfx/codec/jpeg_codec.cc (right): > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > > > > > ui/gfx/codec/jpeg_codec.cc:17: #elif defined(USE_LIBJPEG_TURBO) && > > > > > !defined(OS_CHROMEOS) > > > > > Why is SE_LIBJPEG_TURBO set for chromeos if we're not using it? > > > > > > > > Patch set 6 removes the jpeg_codec.cc changes since USE_LIBJPEG_TURBO > > already > > > > handles the build logic. > > > > > > > > GYP: > > > > I verified the correct libraries are used with additional debugging. > > > > UserImageManagerImpl and WallpaperManager decodes were handled by libjpeg. > > > > > > > > GN: > > > > `gn gen out/test --args="os=\"chromeos\""` fails on unrelated undefined > > > > identifiers. > > > > `gn gen out/test` completes OK. > > > > > > lgtm since I think this is as far as GN will let us debug this so far. > > > > > > Maybe try to actually build the GN build just in case. > > > > Ninja fails on unrelated harfbuzz warnings as errors when linking. > > $ gn gen out/test > > Done. Wrote 1419 targets from 520 files in 638ms > > $ ninja -C out/test chrome > > ninja: Entering directory `out/test' > > [15019/15019] LINK ./chrome > > <snip> > > ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: warning: hidden > symbol > > 'hb_buffer_create' in obj/third_party/harfbuzz-ng/src/harfbuzz-ng.hb-buffer.o > is > > referenced by DSO > > /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../x86_64-linux-gnu/libpangoft2-1.0.so > > ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error: treating > > warnings as errors > > <snip> > > Rebased on master as common.gypi changes were not merging cleanly. I also > retested the GN build after reviewing > https://code.google.com/p/chromium/issues/detail?id=353127 . > > $ gn gen out/test --args="use_system_harfbuzz=true" > Done. Wrote 1615 targets from 551 files in 660ms > $ ninja -C out/test chrome > ninja: Entering directory `out/test' > [460/460] LINK ./chrome > $ Thanks for testing!
On 2014/09/23 21:42:04, Jorge Lucangeli Obes wrote: > On 2014/09/23 19:49:43, Ed Baker wrote: > > On 2014/09/22 19:54:47, Ed Baker wrote: > > > On 2014/09/22 18:32:08, Jorge Lucangeli Obes wrote: > > > > On 2014/09/22 18:30:40, Ed Baker wrote: > > > > > On 2014/09/18 22:19:37, sky wrote: > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn > > > > > > File ui/gfx/BUILD.gn (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn#n... > > > > > > ui/gfx/BUILD.gn:323: # TODO(jorgelo): make sure this works on Chrome > OS. > > > > > > Please verify before landing. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > > > > > > File ui/gfx/codec/jpeg_codec.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > > > > > > ui/gfx/codec/jpeg_codec.cc:17: #elif defined(USE_LIBJPEG_TURBO) && > > > > > > !defined(OS_CHROMEOS) > > > > > > Why is SE_LIBJPEG_TURBO set for chromeos if we're not using it? > > > > > > > > > > Patch set 6 removes the jpeg_codec.cc changes since USE_LIBJPEG_TURBO > > > already > > > > > handles the build logic. > > > > > > > > > > GYP: > > > > > I verified the correct libraries are used with additional debugging. > > > > > UserImageManagerImpl and WallpaperManager decodes were handled by > libjpeg. > > > > > > > > > > GN: > > > > > `gn gen out/test --args="os=\"chromeos\""` fails on unrelated undefined > > > > > identifiers. > > > > > `gn gen out/test` completes OK. > > > > > > > > lgtm since I think this is as far as GN will let us debug this so far. > > > > > > > > Maybe try to actually build the GN build just in case. > > > > > > Ninja fails on unrelated harfbuzz warnings as errors when linking. > > > $ gn gen out/test > > > Done. Wrote 1419 targets from 520 files in 638ms > > > $ ninja -C out/test chrome > > > ninja: Entering directory `out/test' > > > [15019/15019] LINK ./chrome > > > <snip> > > > ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: warning: hidden > > symbol > > > 'hb_buffer_create' in > obj/third_party/harfbuzz-ng/src/harfbuzz-ng.hb-buffer.o > > is > > > referenced by DSO > > > > /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../x86_64-linux-gnu/libpangoft2-1.0.so > > > ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error: treating > > > warnings as errors > > > <snip> > > > > Rebased on master as common.gypi changes were not merging cleanly. I also > > retested the GN build after reviewing > > https://code.google.com/p/chromium/issues/detail?id=353127 . > > > > $ gn gen out/test --args="use_system_harfbuzz=true" > > Done. Wrote 1615 targets from 551 files in 660ms > > $ ninja -C out/test chrome > > ninja: Entering directory `out/test' > > [460/460] LINK ./chrome > > $ > > Thanks for testing! sky: Is this sufficient testing for the build changes?
sky@chromium.org changed reviewers: + derat@chromium.org
On 2014/10/02 23:29:03, Ed Baker wrote: > On 2014/09/23 21:42:04, Jorge Lucangeli Obes wrote: > > On 2014/09/23 19:49:43, Ed Baker wrote: > > > On 2014/09/22 19:54:47, Ed Baker wrote: > > > > On 2014/09/22 18:32:08, Jorge Lucangeli Obes wrote: > > > > > On 2014/09/22 18:30:40, Ed Baker wrote: > > > > > > On 2014/09/18 22:19:37, sky wrote: > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn > > > > > > > File ui/gfx/BUILD.gn (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn#n... > > > > > > > ui/gfx/BUILD.gn:323: # TODO(jorgelo): make sure this works on Chrome > > OS. > > > > > > > Please verify before landing. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > > > > > > > File ui/gfx/codec/jpeg_codec.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > > > > > > > ui/gfx/codec/jpeg_codec.cc:17: #elif defined(USE_LIBJPEG_TURBO) && > > > > > > > !defined(OS_CHROMEOS) > > > > > > > Why is SE_LIBJPEG_TURBO set for chromeos if we're not using it? > > > > > > > > > > > > Patch set 6 removes the jpeg_codec.cc changes since USE_LIBJPEG_TURBO > > > > already > > > > > > handles the build logic. > > > > > > > > > > > > GYP: > > > > > > I verified the correct libraries are used with additional debugging. > > > > > > UserImageManagerImpl and WallpaperManager decodes were handled by > > libjpeg. > > > > > > > > > > > > GN: > > > > > > `gn gen out/test --args="os=\"chromeos\""` fails on unrelated > undefined > > > > > > identifiers. > > > > > > `gn gen out/test` completes OK. > > > > > > > > > > lgtm since I think this is as far as GN will let us debug this so far. > > > > > > > > > > Maybe try to actually build the GN build just in case. > > > > > > > > Ninja fails on unrelated harfbuzz warnings as errors when linking. > > > > $ gn gen out/test > > > > Done. Wrote 1419 targets from 520 files in 638ms > > > > $ ninja -C out/test chrome > > > > ninja: Entering directory `out/test' > > > > [15019/15019] LINK ./chrome > > > > <snip> > > > > ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: warning: hidden > > > symbol > > > > 'hb_buffer_create' in > > obj/third_party/harfbuzz-ng/src/harfbuzz-ng.hb-buffer.o > > > is > > > > referenced by DSO > > > > > > /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../x86_64-linux-gnu/libpangoft2-1.0.so > > > > ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error: treating > > > > warnings as errors > > > > <snip> > > > > > > Rebased on master as common.gypi changes were not merging cleanly. I also > > > retested the GN build after reviewing > > > https://code.google.com/p/chromium/issues/detail?id=353127 . > > > > > > $ gn gen out/test --args="use_system_harfbuzz=true" > > > Done. Wrote 1615 targets from 551 files in 660ms > > > $ ninja -C out/test chrome > > > ninja: Entering directory `out/test' > > > [460/460] LINK ./chrome > > > $ > > > > Thanks for testing! > > sky: Is this sufficient testing for the build changes? Can you clarify why we are having two jpeg decoders here? Isn't one enough?
On 2014/10/03 14:45:34, sky wrote: > On 2014/10/02 23:29:03, Ed Baker wrote: > > On 2014/09/23 21:42:04, Jorge Lucangeli Obes wrote: > > > On 2014/09/23 19:49:43, Ed Baker wrote: > > > > On 2014/09/22 19:54:47, Ed Baker wrote: > > > > > On 2014/09/22 18:32:08, Jorge Lucangeli Obes wrote: > > > > > > On 2014/09/22 18:30:40, Ed Baker wrote: > > > > > > > On 2014/09/18 22:19:37, sky wrote: > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn > > > > > > > > File ui/gfx/BUILD.gn (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn#n... > > > > > > > > ui/gfx/BUILD.gn:323: # TODO(jorgelo): make sure this works on > Chrome > > > OS. > > > > > > > > Please verify before landing. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > > > > > > > > File ui/gfx/codec/jpeg_codec.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > > > > > > > > ui/gfx/codec/jpeg_codec.cc:17: #elif defined(USE_LIBJPEG_TURBO) && > > > > > > > > !defined(OS_CHROMEOS) > > > > > > > > Why is SE_LIBJPEG_TURBO set for chromeos if we're not using it? > > > > > > > > > > > > > > Patch set 6 removes the jpeg_codec.cc changes since > USE_LIBJPEG_TURBO > > > > > already > > > > > > > handles the build logic. > > > > > > > > > > > > > > GYP: > > > > > > > I verified the correct libraries are used with additional debugging. > > > > > > > UserImageManagerImpl and WallpaperManager decodes were handled by > > > libjpeg. > > > > > > > > > > > > > > GN: > > > > > > > `gn gen out/test --args="os=\"chromeos\""` fails on unrelated > > undefined > > > > > > > identifiers. > > > > > > > `gn gen out/test` completes OK. > > > > > > > > > > > > lgtm since I think this is as far as GN will let us debug this so far. > > > > > > > > > > > > Maybe try to actually build the GN build just in case. > > > > > > > > > > Ninja fails on unrelated harfbuzz warnings as errors when linking. > > > > > $ gn gen out/test > > > > > Done. Wrote 1419 targets from 520 files in 638ms > > > > > $ ninja -C out/test chrome > > > > > ninja: Entering directory `out/test' > > > > > [15019/15019] LINK ./chrome > > > > > <snip> > > > > > ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: warning: > hidden > > > > symbol > > > > > 'hb_buffer_create' in > > > obj/third_party/harfbuzz-ng/src/harfbuzz-ng.hb-buffer.o > > > > is > > > > > referenced by DSO > > > > > > > > > /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../x86_64-linux-gnu/libpangoft2-1.0.so > > > > > ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error: > treating > > > > > warnings as errors > > > > > <snip> > > > > > > > > Rebased on master as common.gypi changes were not merging cleanly. I also > > > > retested the GN build after reviewing > > > > https://code.google.com/p/chromium/issues/detail?id=353127 . > > > > > > > > $ gn gen out/test --args="use_system_harfbuzz=true" > > > > Done. Wrote 1615 targets from 551 files in 660ms > > > > $ ninja -C out/test chrome > > > > ninja: Entering directory `out/test' > > > > [460/460] LINK ./chrome > > > > $ > > > > > > Thanks for testing! > > > > sky: Is this sufficient testing for the build changes? > > Can you clarify why we are having two jpeg decoders here? Isn't one enough? User avatars and desktop backgrounds are kept outside of each users' home partitions and can therefore be modified by any other user in the system. We use a more robust (but slower) decoder for these images to mitigate the risk of one user being able to exploit Chrome in the pre-login state by modifying somebody else's picture. Edward noticed that we were using the slower decoder in more cases than we needed. This CL fixes that.
Doesn't non-chromeos code also call into the ui/gfx decoder? On Fri, Oct 3, 2014 at 11:35 AM, <jorgelo@chromium.org> wrote: > On 2014/10/03 14:45:34, sky wrote: >> >> On 2014/10/02 23:29:03, Ed Baker wrote: >> > On 2014/09/23 21:42:04, Jorge Lucangeli Obes wrote: >> > > On 2014/09/23 19:49:43, Ed Baker wrote: >> > > > On 2014/09/22 19:54:47, Ed Baker wrote: >> > > > > On 2014/09/22 18:32:08, Jorge Lucangeli Obes wrote: >> > > > > > On 2014/09/22 18:30:40, Ed Baker wrote: >> > > > > > > On 2014/09/18 22:19:37, sky wrote: >> > > > > > > > >> > > > > >> > >> > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn >> > > > > > > > File ui/gfx/BUILD.gn (right): >> > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn#n... >> >> > > > > > > > ui/gfx/BUILD.gn:323: # TODO(jorgelo): make sure this works >> > > > > > > > on >> Chrome >> > > OS. >> > > > > > > > Please verify before landing. >> > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... >> >> > > > > > > > File ui/gfx/codec/jpeg_codec.cc (right): >> > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... >> >> > > > > > > > ui/gfx/codec/jpeg_codec.cc:17: #elif >> > > > > > > > defined(USE_LIBJPEG_TURBO) > > && >> >> > > > > > > > !defined(OS_CHROMEOS) >> > > > > > > > Why is SE_LIBJPEG_TURBO set for chromeos if we're not using >> > > > > > > > it? >> > > > > > > >> > > > > > > Patch set 6 removes the jpeg_codec.cc changes since >> USE_LIBJPEG_TURBO >> > > > > already >> > > > > > > handles the build logic. >> > > > > > > >> > > > > > > GYP: >> > > > > > > I verified the correct libraries are used with additional > > debugging. >> >> > > > > > > UserImageManagerImpl and WallpaperManager decodes were handled >> > > > > > > by >> > > libjpeg. >> > > > > > > >> > > > > > > GN: >> > > > > > > `gn gen out/test --args="os=\"chromeos\""` fails on unrelated >> > undefined >> > > > > > > identifiers. >> > > > > > > `gn gen out/test` completes OK. >> > > > > > >> > > > > > lgtm since I think this is as far as GN will let us debug this >> > > > > > so > > far. >> >> > > > > > >> > > > > > Maybe try to actually build the GN build just in case. >> > > > > >> > > > > Ninja fails on unrelated harfbuzz warnings as errors when linking. >> > > > > $ gn gen out/test >> > > > > Done. Wrote 1419 targets from 520 files in 638ms >> > > > > $ ninja -C out/test chrome >> > > > > ninja: Entering directory `out/test' >> > > > > [15019/15019] LINK ./chrome >> > > > > <snip> >> > > > > ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: warning: >> hidden >> > > > symbol >> > > > > 'hb_buffer_create' in >> > > obj/third_party/harfbuzz-ng/src/harfbuzz-ng.hb-buffer.o >> > > > is >> > > > > referenced by DSO >> > > > > >> > > >> >> /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../x86_64-linux-gnu/libpangoft2-1.0.so >> > > > > ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error: >> treating >> > > > > warnings as errors >> > > > > <snip> >> > > > >> > > > Rebased on master as common.gypi changes were not merging cleanly. I > > also >> >> > > > retested the GN build after reviewing >> > > > https://code.google.com/p/chromium/issues/detail?id=353127 . >> > > > >> > > > $ gn gen out/test --args="use_system_harfbuzz=true" >> > > > Done. Wrote 1615 targets from 551 files in 660ms >> > > > $ ninja -C out/test chrome >> > > > ninja: Entering directory `out/test' >> > > > [460/460] LINK ./chrome >> > > > $ >> > > >> > > Thanks for testing! >> > >> > sky: Is this sufficient testing for the build changes? > > >> Can you clarify why we are having two jpeg decoders here? Isn't one >> enough? > > > User avatars and desktop backgrounds are kept outside of each users' home > partitions and can therefore be modified by any other user in the system. We > use > a more robust (but slower) decoder for these images to mitigate the risk of > one > user being able to exploit Chrome in the pre-login state by modifying > somebody > else's picture. > > Edward noticed that we were using the slower decoder in more cases than we > needed. This CL fixes that. > > https://chromiumcodereview.appspot.com/569623002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
If you want something different for chromeos, then shouldn't it be in a chromeos directory and not a core directory like ui? On Fri, Oct 3, 2014 at 1:05 PM, Scott Violet <sky@chromium.org> wrote: > Doesn't non-chromeos code also call into the ui/gfx decoder? > > On Fri, Oct 3, 2014 at 11:35 AM, <jorgelo@chromium.org> wrote: >> On 2014/10/03 14:45:34, sky wrote: >>> >>> On 2014/10/02 23:29:03, Ed Baker wrote: >>> > On 2014/09/23 21:42:04, Jorge Lucangeli Obes wrote: >>> > > On 2014/09/23 19:49:43, Ed Baker wrote: >>> > > > On 2014/09/22 19:54:47, Ed Baker wrote: >>> > > > > On 2014/09/22 18:32:08, Jorge Lucangeli Obes wrote: >>> > > > > > On 2014/09/22 18:30:40, Ed Baker wrote: >>> > > > > > > On 2014/09/18 22:19:37, sky wrote: >>> > > > > > > > >>> > > > > >>> > >>> > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn >>> > > > > > > > File ui/gfx/BUILD.gn (right): >>> > > > > > > > >>> > > > > > > > >>> > > > > > > >>> > > > > > >>> > > > > >>> > > > >>> > > >>> > >> >> >> https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn#n... >>> >>> > > > > > > > ui/gfx/BUILD.gn:323: # TODO(jorgelo): make sure this works >>> > > > > > > > on >>> Chrome >>> > > OS. >>> > > > > > > > Please verify before landing. >>> > > > > > > > >>> > > > > > > > >>> > > > > > > >>> > > > > > >>> > > > > >>> > > > >>> > > >>> > >> >> >> https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... >>> >>> > > > > > > > File ui/gfx/codec/jpeg_codec.cc (right): >>> > > > > > > > >>> > > > > > > > >>> > > > > > > >>> > > > > > >>> > > > > >>> > > > >>> > > >>> > >> >> >> https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... >>> >>> > > > > > > > ui/gfx/codec/jpeg_codec.cc:17: #elif >>> > > > > > > > defined(USE_LIBJPEG_TURBO) >> >> && >>> >>> > > > > > > > !defined(OS_CHROMEOS) >>> > > > > > > > Why is SE_LIBJPEG_TURBO set for chromeos if we're not using >>> > > > > > > > it? >>> > > > > > > >>> > > > > > > Patch set 6 removes the jpeg_codec.cc changes since >>> USE_LIBJPEG_TURBO >>> > > > > already >>> > > > > > > handles the build logic. >>> > > > > > > >>> > > > > > > GYP: >>> > > > > > > I verified the correct libraries are used with additional >> >> debugging. >>> >>> > > > > > > UserImageManagerImpl and WallpaperManager decodes were handled >>> > > > > > > by >>> > > libjpeg. >>> > > > > > > >>> > > > > > > GN: >>> > > > > > > `gn gen out/test --args="os=\"chromeos\""` fails on unrelated >>> > undefined >>> > > > > > > identifiers. >>> > > > > > > `gn gen out/test` completes OK. >>> > > > > > >>> > > > > > lgtm since I think this is as far as GN will let us debug this >>> > > > > > so >> >> far. >>> >>> > > > > > >>> > > > > > Maybe try to actually build the GN build just in case. >>> > > > > >>> > > > > Ninja fails on unrelated harfbuzz warnings as errors when linking. >>> > > > > $ gn gen out/test >>> > > > > Done. Wrote 1419 targets from 520 files in 638ms >>> > > > > $ ninja -C out/test chrome >>> > > > > ninja: Entering directory `out/test' >>> > > > > [15019/15019] LINK ./chrome >>> > > > > <snip> >>> > > > > ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: warning: >>> hidden >>> > > > symbol >>> > > > > 'hb_buffer_create' in >>> > > obj/third_party/harfbuzz-ng/src/harfbuzz-ng.hb-buffer.o >>> > > > is >>> > > > > referenced by DSO >>> > > > > >>> > > >>> >>> /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../x86_64-linux-gnu/libpangoft2-1.0.so >>> > > > > ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error: >>> treating >>> > > > > warnings as errors >>> > > > > <snip> >>> > > > >>> > > > Rebased on master as common.gypi changes were not merging cleanly. I >> >> also >>> >>> > > > retested the GN build after reviewing >>> > > > https://code.google.com/p/chromium/issues/detail?id=353127 . >>> > > > >>> > > > $ gn gen out/test --args="use_system_harfbuzz=true" >>> > > > Done. Wrote 1615 targets from 551 files in 660ms >>> > > > $ ninja -C out/test chrome >>> > > > ninja: Entering directory `out/test' >>> > > > [460/460] LINK ./chrome >>> > > > $ >>> > > >>> > > Thanks for testing! >>> > >>> > sky: Is this sufficient testing for the build changes? >> >> >>> Can you clarify why we are having two jpeg decoders here? Isn't one >>> enough? >> >> >> User avatars and desktop backgrounds are kept outside of each users' home >> partitions and can therefore be modified by any other user in the system. We >> use >> a more robust (but slower) decoder for these images to mitigate the risk of >> one >> user being able to exploit Chrome in the pre-login state by modifying >> somebody >> else's picture. >> >> Edward noticed that we were using the slower decoder in more cases than we >> needed. This CL fixes that. >> >> https://chromiumcodereview.appspot.com/569623002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/10/03 20:05:17, sky wrote: > Doesn't non-chromeos code also call into the ui/gfx decoder? The JPEG library ui/gfx builds with is unchanged with this patch set. Chrome OS was using IJG libjpeg before, and does with this change. Other OS builds with the default use_libjpeg_turbo=1 would set USE_LIBJPEG_TURBO=1 and build ui/gfx with libjpeg_turbo as before. > > On Fri, Oct 3, 2014 at 11:35 AM, <mailto:jorgelo@chromium.org> wrote: > > On 2014/10/03 14:45:34, sky wrote: > >> > >> On 2014/10/02 23:29:03, Ed Baker wrote: > >> > On 2014/09/23 21:42:04, Jorge Lucangeli Obes wrote: > >> > > On 2014/09/23 19:49:43, Ed Baker wrote: > >> > > > On 2014/09/22 19:54:47, Ed Baker wrote: > >> > > > > On 2014/09/22 18:32:08, Jorge Lucangeli Obes wrote: > >> > > > > > On 2014/09/22 18:30:40, Ed Baker wrote: > >> > > > > > > On 2014/09/18 22:19:37, sky wrote: > >> > > > > > > > > >> > > > > > >> > > >> > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn > >> > > > > > > > File ui/gfx/BUILD.gn (right): > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn#n... > >> > >> > > > > > > > ui/gfx/BUILD.gn:323: # TODO(jorgelo): make sure this works > >> > > > > > > > on > >> Chrome > >> > > OS. > >> > > > > > > > Please verify before landing. > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > >> > >> > > > > > > > File ui/gfx/codec/jpeg_codec.cc (right): > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > >> > >> > > > > > > > ui/gfx/codec/jpeg_codec.cc:17: #elif > >> > > > > > > > defined(USE_LIBJPEG_TURBO) > > > > && > >> > >> > > > > > > > !defined(OS_CHROMEOS) > >> > > > > > > > Why is SE_LIBJPEG_TURBO set for chromeos if we're not using > >> > > > > > > > it? > >> > > > > > > > >> > > > > > > Patch set 6 removes the jpeg_codec.cc changes since > >> USE_LIBJPEG_TURBO > >> > > > > already > >> > > > > > > handles the build logic. > >> > > > > > > > >> > > > > > > GYP: > >> > > > > > > I verified the correct libraries are used with additional > > > > debugging. > >> > >> > > > > > > UserImageManagerImpl and WallpaperManager decodes were handled > >> > > > > > > by > >> > > libjpeg. > >> > > > > > > > >> > > > > > > GN: > >> > > > > > > `gn gen out/test --args="os=\"chromeos\""` fails on unrelated > >> > undefined > >> > > > > > > identifiers. > >> > > > > > > `gn gen out/test` completes OK. > >> > > > > > > >> > > > > > lgtm since I think this is as far as GN will let us debug this > >> > > > > > so > > > > far. > >> > >> > > > > > > >> > > > > > Maybe try to actually build the GN build just in case. > >> > > > > > >> > > > > Ninja fails on unrelated harfbuzz warnings as errors when linking. > >> > > > > $ gn gen out/test > >> > > > > Done. Wrote 1419 targets from 520 files in 638ms > >> > > > > $ ninja -C out/test chrome > >> > > > > ninja: Entering directory `out/test' > >> > > > > [15019/15019] LINK ./chrome > >> > > > > <snip> > >> > > > > ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: warning: > >> hidden > >> > > > symbol > >> > > > > 'hb_buffer_create' in > >> > > obj/third_party/harfbuzz-ng/src/harfbuzz-ng.hb-buffer.o > >> > > > is > >> > > > > referenced by DSO > >> > > > > > >> > > > >> > >> > /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../x86_64-linux-gnu/libpangoft2-1.0.so > >> > > > > ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error: > >> treating > >> > > > > warnings as errors > >> > > > > <snip> > >> > > > > >> > > > Rebased on master as common.gypi changes were not merging cleanly. I > > > > also > >> > >> > > > retested the GN build after reviewing > >> > > > https://code.google.com/p/chromium/issues/detail?id=353127 . > >> > > > > >> > > > $ gn gen out/test --args="use_system_harfbuzz=true" > >> > > > Done. Wrote 1615 targets from 551 files in 660ms > >> > > > $ ninja -C out/test chrome > >> > > > ninja: Entering directory `out/test' > >> > > > [460/460] LINK ./chrome > >> > > > $ > >> > > > >> > > Thanks for testing! > >> > > >> > sky: Is this sufficient testing for the build changes? > > > > > >> Can you clarify why we are having two jpeg decoders here? Isn't one > >> enough? > > > > > > User avatars and desktop backgrounds are kept outside of each users' home > > partitions and can therefore be modified by any other user in the system. We > > use > > a more robust (but slower) decoder for these images to mitigate the risk of > > one > > user being able to exploit Chrome in the pre-login state by modifying > > somebody > > else's picture. > > > > Edward noticed that we were using the slower decoder in more cases than we > > needed. This CL fixes that. > > > > https://chromiumcodereview.appspot.com/569623002/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On 2014/10/03 20:17:47, sky wrote: > If you want something different for chromeos, then shouldn't it be in > a chromeos directory and not a core directory like ui? Chrome OS uses the functionality chrome/browser/image_decoder provides by specifying ROBUST_JPEG_DECODE. It does look like Chrome OS is the first to do so. Other existing uses of ImageDecoder specify DEFAULT_CODEC. > > On Fri, Oct 3, 2014 at 1:05 PM, Scott Violet <mailto:sky@chromium.org> wrote: > > Doesn't non-chromeos code also call into the ui/gfx decoder? > > > > On Fri, Oct 3, 2014 at 11:35 AM, <mailto:jorgelo@chromium.org> wrote: > >> On 2014/10/03 14:45:34, sky wrote: > >>> > >>> On 2014/10/02 23:29:03, Ed Baker wrote: > >>> > On 2014/09/23 21:42:04, Jorge Lucangeli Obes wrote: > >>> > > On 2014/09/23 19:49:43, Ed Baker wrote: > >>> > > > On 2014/09/22 19:54:47, Ed Baker wrote: > >>> > > > > On 2014/09/22 18:32:08, Jorge Lucangeli Obes wrote: > >>> > > > > > On 2014/09/22 18:30:40, Ed Baker wrote: > >>> > > > > > > On 2014/09/18 22:19:37, sky wrote: > >>> > > > > > > > > >>> > > > > > >>> > > >>> > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn > >>> > > > > > > > File ui/gfx/BUILD.gn (right): > >>> > > > > > > > > >>> > > > > > > > > >>> > > > > > > > >>> > > > > > > >>> > > > > > >>> > > > > >>> > > > >>> > > >> > >> > >> > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn#n... > >>> > >>> > > > > > > > ui/gfx/BUILD.gn:323: # TODO(jorgelo): make sure this works > >>> > > > > > > > on > >>> Chrome > >>> > > OS. > >>> > > > > > > > Please verify before landing. > >>> > > > > > > > > >>> > > > > > > > > >>> > > > > > > > >>> > > > > > > >>> > > > > > >>> > > > > >>> > > > >>> > > >> > >> > >> > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > >>> > >>> > > > > > > > File ui/gfx/codec/jpeg_codec.cc (right): > >>> > > > > > > > > >>> > > > > > > > > >>> > > > > > > > >>> > > > > > > >>> > > > > > >>> > > > > >>> > > > >>> > > >> > >> > >> > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > >>> > >>> > > > > > > > ui/gfx/codec/jpeg_codec.cc:17: #elif > >>> > > > > > > > defined(USE_LIBJPEG_TURBO) > >> > >> && > >>> > >>> > > > > > > > !defined(OS_CHROMEOS) > >>> > > > > > > > Why is SE_LIBJPEG_TURBO set for chromeos if we're not using > >>> > > > > > > > it? > >>> > > > > > > > >>> > > > > > > Patch set 6 removes the jpeg_codec.cc changes since > >>> USE_LIBJPEG_TURBO > >>> > > > > already > >>> > > > > > > handles the build logic. > >>> > > > > > > > >>> > > > > > > GYP: > >>> > > > > > > I verified the correct libraries are used with additional > >> > >> debugging. > >>> > >>> > > > > > > UserImageManagerImpl and WallpaperManager decodes were handled > >>> > > > > > > by > >>> > > libjpeg. > >>> > > > > > > > >>> > > > > > > GN: > >>> > > > > > > `gn gen out/test --args="os=\"chromeos\""` fails on unrelated > >>> > undefined > >>> > > > > > > identifiers. > >>> > > > > > > `gn gen out/test` completes OK. > >>> > > > > > > >>> > > > > > lgtm since I think this is as far as GN will let us debug this > >>> > > > > > so > >> > >> far. > >>> > >>> > > > > > > >>> > > > > > Maybe try to actually build the GN build just in case. > >>> > > > > > >>> > > > > Ninja fails on unrelated harfbuzz warnings as errors when linking. > >>> > > > > $ gn gen out/test > >>> > > > > Done. Wrote 1419 targets from 520 files in 638ms > >>> > > > > $ ninja -C out/test chrome > >>> > > > > ninja: Entering directory `out/test' > >>> > > > > [15019/15019] LINK ./chrome > >>> > > > > <snip> > >>> > > > > ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: warning: > >>> hidden > >>> > > > symbol > >>> > > > > 'hb_buffer_create' in > >>> > > obj/third_party/harfbuzz-ng/src/harfbuzz-ng.hb-buffer.o > >>> > > > is > >>> > > > > referenced by DSO > >>> > > > > > >>> > > > >>> > >>> > /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../x86_64-linux-gnu/libpangoft2-1.0.so > >>> > > > > ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error: > >>> treating > >>> > > > > warnings as errors > >>> > > > > <snip> > >>> > > > > >>> > > > Rebased on master as common.gypi changes were not merging cleanly. I > >> > >> also > >>> > >>> > > > retested the GN build after reviewing > >>> > > > https://code.google.com/p/chromium/issues/detail?id=353127 . > >>> > > > > >>> > > > $ gn gen out/test --args="use_system_harfbuzz=true" > >>> > > > Done. Wrote 1615 targets from 551 files in 660ms > >>> > > > $ ninja -C out/test chrome > >>> > > > ninja: Entering directory `out/test' > >>> > > > [460/460] LINK ./chrome > >>> > > > $ > >>> > > > >>> > > Thanks for testing! > >>> > > >>> > sky: Is this sufficient testing for the build changes? > >> > >> > >>> Can you clarify why we are having two jpeg decoders here? Isn't one > >>> enough? > >> > >> > >> User avatars and desktop backgrounds are kept outside of each users' home > >> partitions and can therefore be modified by any other user in the system. We > >> use > >> a more robust (but slower) decoder for these images to mitigate the risk of > >> one > >> user being able to exploit Chrome in the pre-login state by modifying > >> somebody > >> else's picture. > >> > >> Edward noticed that we were using the slower decoder in more cases than we > >> needed. This CL fixes that. > >> > >> https://chromiumcodereview.appspot.com/569623002/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Again, I don't like the assumption that on chromeos ui/gfx needs a more robust decoder. ui/gfx is meant to be a generic decoder. Places that need need more robust decoding should be isolated and not using ui/gfx. https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... File third_party/BUILD.gn (right): https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... third_party/BUILD.gn:26: if (!is_chromeos) { Wouldn't it be better to wrap the config in the ifdef?
On 2014/10/06 18:21:28, sky wrote: > Again, I don't like the assumption that on chromeos ui/gfx needs a more robust > decoder. ui/gfx is meant to be a generic decoder. Places that need need more > robust decoding should be isolated and not using ui/gfx. > I don't think that's the point I was trying to make. On CrOS there's cases in which we're decoding images provided by one *system* user in the context of another user. We want a slower but more robust mechanism in that case. ui/gfx might indeed not be the best place to put that. Should we just put it inside a chromeos/ directory? WDYT? > https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... > File third_party/BUILD.gn (right): > > https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... > third_party/BUILD.gn:26: if (!is_chromeos) { > Wouldn't it be better to wrap the config in the ifdef?
Yes please. On Mon, Oct 6, 2014 at 4:25 PM, <jorgelo@chromium.org> wrote: > On 2014/10/06 18:21:28, sky wrote: >> >> Again, I don't like the assumption that on chromeos ui/gfx needs a more >> robust >> decoder. ui/gfx is meant to be a generic decoder. Places that need need >> more >> robust decoding should be isolated and not using ui/gfx. > > > > I don't think that's the point I was trying to make. On CrOS there's cases > in > which we're decoding images provided by one *system* user in the context of > another user. We want a slower but more robust mechanism in that case. > ui/gfx > might indeed not be the best place to put that. Should we just put it inside > a > chromeos/ directory? WDYT? > > > > https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... >> >> File third_party/BUILD.gn (right): > > > > https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... >> >> third_party/BUILD.gn:26: if (!is_chromeos) { >> Wouldn't it be better to wrap the config in the ifdef? > > > > > https://chromiumcodereview.appspot.com/569623002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/10/06 23:52:33, sky wrote: > Yes please. We can do that. I would still like to land this CL, since it's a big perf improvement and it narrows down the use of IGJ libjpeg to only the ui/gfx case (and only on Chrome OS), instead of all of Chrome OS. That way, we can work on extracting the code to a Chrome OS-specific class without blocking the perf win. > On Mon, Oct 6, 2014 at 4:25 PM, <mailto:jorgelo@chromium.org> wrote: > > On 2014/10/06 18:21:28, sky wrote: > >> > >> Again, I don't like the assumption that on chromeos ui/gfx needs a more > >> robust > >> decoder. ui/gfx is meant to be a generic decoder. Places that need need > >> more > >> robust decoding should be isolated and not using ui/gfx. > > > > > > > > I don't think that's the point I was trying to make. On CrOS there's cases > > in > > which we're decoding images provided by one *system* user in the context of > > another user. We want a slower but more robust mechanism in that case. > > ui/gfx > > might indeed not be the best place to put that. Should we just put it inside > > a > > chromeos/ directory? WDYT? > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... > >> > >> File third_party/BUILD.gn (right): > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... > >> > >> third_party/BUILD.gn:26: if (!is_chromeos) { > >> Wouldn't it be better to wrap the config in the ifdef? > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
I would rather see this done the right way. Using a different decoder in different configurations just because some code happens to need it is confusing. On Mon, Oct 6, 2014 at 5:25 PM, <jorgelo@chromium.org> wrote: > On 2014/10/06 23:52:33, sky wrote: >> >> Yes please. > > > We can do that. I would still like to land this CL, since it's a big perf > improvement and it narrows down the use of IGJ libjpeg to only the ui/gfx > case > (and only on Chrome OS), instead of all of Chrome OS. That way, we can work > on > extracting the code to a Chrome OS-specific class without blocking the perf > win. > > >> On Mon, Oct 6, 2014 at 4:25 PM, <mailto:jorgelo@chromium.org> wrote: >> > On 2014/10/06 18:21:28, sky wrote: >> >> >> >> Again, I don't like the assumption that on chromeos ui/gfx needs a more >> >> robust >> >> decoder. ui/gfx is meant to be a generic decoder. Places that need need >> >> more >> >> robust decoding should be isolated and not using ui/gfx. >> > >> > >> > >> > I don't think that's the point I was trying to make. On CrOS there's >> > cases >> > in >> > which we're decoding images provided by one *system* user in the context >> > of >> > another user. We want a slower but more robust mechanism in that case. >> > ui/gfx >> > might indeed not be the best place to put that. Should we just put it >> > inside >> > a >> > chromeos/ directory? WDYT? >> > >> > >> > >> > > > > https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... >> >> >> >> >> File third_party/BUILD.gn (right): >> > >> > >> > >> > > > > https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... >> >> >> >> >> third_party/BUILD.gn:26: if (!is_chromeos) { >> >> Wouldn't it be better to wrap the config in the ifdef? >> > >> > >> > >> > >> > https://chromiumcodereview.appspot.com/569623002/ > > >> To unsubscribe from this group and stop receiving emails from it, send an > > email >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > https://chromiumcodereview.appspot.com/569623002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/10/07 15:20:22, sky wrote: > I would rather see this done the right way. Using a different decoder > in different configurations just because some code happens to need it > is confusing. > Fair enough, we can add the new functionality under chromeos/ and then switch things over. Edward, are you OK with taking that on? > On Mon, Oct 6, 2014 at 5:25 PM, <mailto:jorgelo@chromium.org> wrote: > > On 2014/10/06 23:52:33, sky wrote: > >> > >> Yes please. > > > > > > We can do that. I would still like to land this CL, since it's a big perf > > improvement and it narrows down the use of IGJ libjpeg to only the ui/gfx > > case > > (and only on Chrome OS), instead of all of Chrome OS. That way, we can work > > on > > extracting the code to a Chrome OS-specific class without blocking the perf > > win. > > > > > >> On Mon, Oct 6, 2014 at 4:25 PM, <mailto:jorgelo@chromium.org> wrote: > >> > On 2014/10/06 18:21:28, sky wrote: > >> >> > >> >> Again, I don't like the assumption that on chromeos ui/gfx needs a more > >> >> robust > >> >> decoder. ui/gfx is meant to be a generic decoder. Places that need need > >> >> more > >> >> robust decoding should be isolated and not using ui/gfx. > >> > > >> > > >> > > >> > I don't think that's the point I was trying to make. On CrOS there's > >> > cases > >> > in > >> > which we're decoding images provided by one *system* user in the context > >> > of > >> > another user. We want a slower but more robust mechanism in that case. > >> > ui/gfx > >> > might indeed not be the best place to put that. Should we just put it > >> > inside > >> > a > >> > chromeos/ directory? WDYT? > >> > > >> > > >> > > >> > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... > >> > >> >> > >> >> File third_party/BUILD.gn (right): > >> > > >> > > >> > > >> > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... > >> > >> >> > >> >> third_party/BUILD.gn:26: if (!is_chromeos) { > >> >> Wouldn't it be better to wrap the config in the ifdef? > >> > > >> > > >> > > >> > > >> > https://chromiumcodereview.appspot.com/569623002/ > > > > > >> To unsubscribe from this group and stop receiving emails from it, send an > > > > email > >> > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > https://chromiumcodereview.appspot.com/569623002/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On 2014/10/07 20:24:14, Jorge Lucangeli Obes wrote: > On 2014/10/07 15:20:22, sky wrote: > > I would rather see this done the right way. Using a different decoder > > in different configurations just because some code happens to need it > > is confusing. > > > > Fair enough, we can add the new functionality under chromeos/ and then switch > things over. Edward, are you OK with taking that on? > I'd be glad to continue working on this. I do have a few questions though. Introducing a JPEG wrapper dedicated to IJG libjpeg would likely duplicate the decode portion of ui/gfx JPEGCodec. The browser sources would then have two wrapper classes, on top of the instances in third_party. It was appealing to reuse existing ui/gfx code by manipulating the build configuration. I agree with sky that it is confusing :). ImageDecoder from chrome/browser/image_decoder is used by webui, app_list, android, bitmap_fetcher, profiles, media_galleries, and drive. All of them specify ImageDecoder::DEFAULT_CODEC, but something might eventually use ROBUST_JPEG_DECODE. Wouldn't we want the robust decode in a common spot similar to ImageDecoder? Thanks for all of the feedback! > > On Mon, Oct 6, 2014 at 5:25 PM, <mailto:jorgelo@chromium.org> wrote: > > > On 2014/10/06 23:52:33, sky wrote: > > >> > > >> Yes please. > > > > > > > > > We can do that. I would still like to land this CL, since it's a big perf > > > improvement and it narrows down the use of IGJ libjpeg to only the ui/gfx > > > case > > > (and only on Chrome OS), instead of all of Chrome OS. That way, we can work > > > on > > > extracting the code to a Chrome OS-specific class without blocking the perf > > > win. > > > > > > > > >> On Mon, Oct 6, 2014 at 4:25 PM, <mailto:jorgelo@chromium.org> wrote: > > >> > On 2014/10/06 18:21:28, sky wrote: > > >> >> > > >> >> Again, I don't like the assumption that on chromeos ui/gfx needs a more > > >> >> robust > > >> >> decoder. ui/gfx is meant to be a generic decoder. Places that need need > > >> >> more > > >> >> robust decoding should be isolated and not using ui/gfx. > > >> > > > >> > > > >> > > > >> > I don't think that's the point I was trying to make. On CrOS there's > > >> > cases > > >> > in > > >> > which we're decoding images provided by one *system* user in the context > > >> > of > > >> > another user. We want a slower but more robust mechanism in that case. > > >> > ui/gfx > > >> > might indeed not be the best place to put that. Should we just put it > > >> > inside > > >> > a > > >> > chromeos/ directory? WDYT? > > >> > > > >> > > > >> > > > >> > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... > > >> > > >> >> > > >> >> File third_party/BUILD.gn (right): > > >> > > > >> > > > >> > > > >> > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... > > >> > > >> >> > > >> >> third_party/BUILD.gn:26: if (!is_chromeos) { > > >> >> Wouldn't it be better to wrap the config in the ifdef? > > >> > > > >> > > > >> > > > >> > > > >> > https://chromiumcodereview.appspot.com/569623002/ > > > > > > > > >> To unsubscribe from this group and stop receiving emails from it, send an > > > > > > email > > >> > > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org.
On Tue, Oct 7, 2014 at 2:11 PM, <edward.baker@intel.com> wrote: > On 2014/10/07 20:24:14, Jorge Lucangeli Obes wrote: >> >> On 2014/10/07 15:20:22, sky wrote: >> > I would rather see this done the right way. Using a different decoder >> > in different configurations just because some code happens to need it >> > is confusing. >> > > > >> Fair enough, we can add the new functionality under chromeos/ and then >> switch >> things over. Edward, are you OK with taking that on? > > > I'd be glad to continue working on this. I do have a few questions though. > > Introducing a JPEG wrapper dedicated to IJG libjpeg would likely duplicate > the > decode portion of ui/gfx JPEGCodec. The browser sources would then have two > wrapper classes, on top of the instances in third_party. It was appealing to > reuse existing ui/gfx code by manipulating the build configuration. I agree > with > sky that it is confusing :). You can always refactor jpeg_codec into some internal bits that can be shared by the two. > ImageDecoder from chrome/browser/image_decoder is used by webui, app_list, > android, bitmap_fetcher, profiles, media_galleries, and drive. All of them > specify ImageDecoder::DEFAULT_CODEC, but something might eventually use > ROBUST_JPEG_DECODE. Wouldn't we want the robust decode in a common spot > similar > to ImageDecoder? We can share if that becomes necessary later on. In the mean time I would rather isolate the chromeos bits as they are the only place that needs them. -Scott > > Thanks for all of the feedback! > > >> > On Mon, Oct 6, 2014 at 5:25 PM, <mailto:jorgelo@chromium.org> wrote: >> > > On 2014/10/06 23:52:33, sky wrote: >> > >> >> > >> Yes please. >> > > >> > > >> > > We can do that. I would still like to land this CL, since it's a big >> > > perf >> > > improvement and it narrows down the use of IGJ libjpeg to only the >> > > ui/gfx >> > > case >> > > (and only on Chrome OS), instead of all of Chrome OS. That way, we can > > work >> >> > > on >> > > extracting the code to a Chrome OS-specific class without blocking the > > perf >> >> > > win. >> > > >> > > >> > >> On Mon, Oct 6, 2014 at 4:25 PM, <mailto:jorgelo@chromium.org> wrote: >> > >> > On 2014/10/06 18:21:28, sky wrote: >> > >> >> >> > >> >> Again, I don't like the assumption that on chromeos ui/gfx needs a > > more >> >> > >> >> robust >> > >> >> decoder. ui/gfx is meant to be a generic decoder. Places that need > > need >> >> > >> >> more >> > >> >> robust decoding should be isolated and not using ui/gfx. >> > >> > >> > >> > >> > >> > >> > >> > I don't think that's the point I was trying to make. On CrOS >> > >> > there's >> > >> > cases >> > >> > in >> > >> > which we're decoding images provided by one *system* user in the > > context >> >> > >> > of >> > >> > another user. We want a slower but more robust mechanism in that >> > >> > case. >> > >> > ui/gfx >> > >> > might indeed not be the best place to put that. Should we just put >> > >> > it >> > >> > inside >> > >> > a >> > >> > chromeos/ directory? WDYT? >> > >> > >> > >> > >> > >> > >> > >> > >> > > >> > > >> > > >> > > > > https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... >> >> > >> >> > >> >> >> > >> >> File third_party/BUILD.gn (right): >> > >> > >> > >> > >> > >> > >> > >> > >> > > >> > > >> > > >> > > > > https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... >> >> > >> >> > >> >> >> > >> >> third_party/BUILD.gn:26: if (!is_chromeos) { >> > >> >> Wouldn't it be better to wrap the config in the ifdef? >> > >> > >> > >> > >> > >> > >> > >> > >> > >> > https://chromiumcodereview.appspot.com/569623002/ >> > > >> > > >> > >> To unsubscribe from this group and stop receiving emails from it, >> > >> send an >> > > >> > > email >> > >> >> > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > >> > > >> > > https://chromiumcodereview.appspot.com/569623002/ >> > >> > To unsubscribe from this group and stop receiving emails from it, send >> > an >> email >> > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > https://chromiumcodereview.appspot.com/569623002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patch set 9 addresses the third_party/BUILD.gn suggestion before submitting a completely different approach. https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... File third_party/BUILD.gn (right): https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... third_party/BUILD.gn:26: if (!is_chromeos) { On 2014/10/06 18:21:28, sky wrote: > Wouldn't it be better to wrap the config in the ifdef? Done.
On 2014/10/30 23:29:48, Ed Baker wrote: > Patch set 9 addresses the third_party/BUILD.gn suggestion before submitting a > completely different approach. > > https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... > File third_party/BUILD.gn (right): > > https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... > third_party/BUILD.gn:26: if (!is_chromeos) { > On 2014/10/06 18:21:28, sky wrote: > > Wouldn't it be better to wrap the config in the ifdef? > > Done. Approach A: Use build configuration (patch set 9) Previously discussed. Approach B: Subclass JPEGCodec to introduce JPEGCodecRobust To try this route I added protected variables for function pointers. In the constructor the pointers were then set to either libjpeg or libjpeg_turbo functions. One downside with this approach was that the code calling static methods first needed a JPEGCodec object. The other issue was that the linker couldn't find chromium_ijg_* libjpeg functions. This is an interesting problem as we want duplicate copies of the same class with different headers. Headers that have the same name and same include guards. I moved on since this wasn't turning out as clean as I hoped. If anyone has suggestions I'd be very interested :). Approach C: Copy decode portions of JPEGCodec (patch set 10) While straightforward this has the downside of blatant code duplication. It is a clean copy though, and would likely be simple to maintain.
On 2014/11/03 23:06:51, Ed Baker wrote: > On 2014/10/30 23:29:48, Ed Baker wrote: > > Patch set 9 addresses the third_party/BUILD.gn suggestion before submitting a > > completely different approach. > > > > > https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... > > File third_party/BUILD.gn (right): > > > > > https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... > > third_party/BUILD.gn:26: if (!is_chromeos) { > > On 2014/10/06 18:21:28, sky wrote: > > > Wouldn't it be better to wrap the config in the ifdef? > > > > Done. > > Approach A: Use build configuration (patch set 9) > Previously discussed. > > Approach B: Subclass JPEGCodec to introduce JPEGCodecRobust > To try this route I added protected variables for function pointers. In the > constructor the pointers were then set to either libjpeg or libjpeg_turbo > functions. One downside with this approach was that the code calling static > methods first needed a JPEGCodec object. The other issue was that the linker > couldn't find chromium_ijg_* libjpeg functions. This is an interesting problem > as we want duplicate copies of the same class with different headers. Headers > that have the same name and same include guards. I moved on since this wasn't > turning out as clean as I hoped. If anyone has suggestions I'd be very > interested :). > > Approach C: Copy decode portions of JPEGCodec (patch set 10) > While straightforward this has the downside of blatant code duplication. It is a > clean copy though, and would likely be simple to maintain. Approach C': extract as much shared code of JPEGCodec into a class that can be composed in, instead of using inheritance?
On 2014/11/06 18:50:46, Jorge Lucangeli Obes wrote: > On 2014/11/03 23:06:51, Ed Baker wrote: > > On 2014/10/30 23:29:48, Ed Baker wrote: > > > Patch set 9 addresses the third_party/BUILD.gn suggestion before submitting > a > > > completely different approach. > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... > > > File third_party/BUILD.gn (right): > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... > > > third_party/BUILD.gn:26: if (!is_chromeos) { > > > On 2014/10/06 18:21:28, sky wrote: > > > > Wouldn't it be better to wrap the config in the ifdef? > > > > > > Done. > > > > Approach A: Use build configuration (patch set 9) > > Previously discussed. > > > > Approach B: Subclass JPEGCodec to introduce JPEGCodecRobust > > To try this route I added protected variables for function pointers. In the > > constructor the pointers were then set to either libjpeg or libjpeg_turbo > > functions. One downside with this approach was that the code calling static > > methods first needed a JPEGCodec object. The other issue was that the linker > > couldn't find chromium_ijg_* libjpeg functions. This is an interesting problem > > as we want duplicate copies of the same class with different headers. Headers > > that have the same name and same include guards. I moved on since this wasn't > > turning out as clean as I hoped. If anyone has suggestions I'd be very > > interested :). > > > > Approach C: Copy decode portions of JPEGCodec (patch set 10) > > While straightforward this has the downside of blatant code duplication. It is > a > > clean copy though, and would likely be simple to maintain. > > Approach C': extract as much shared code of JPEGCodec into a class that can be > composed in, instead of using inheritance? Thanks for the feedback. I retried that approach but found myself going in circles. Code hoisted into a common spot inevitably needs a jpeg header for a type or function. I don't see a way around this issue without involving build logic.
On 2014/11/18 23:45:30, Ed Baker wrote: > On 2014/11/06 18:50:46, Jorge Lucangeli Obes wrote: > > On 2014/11/03 23:06:51, Ed Baker wrote: > > > On 2014/10/30 23:29:48, Ed Baker wrote: > > > > Patch set 9 addresses the third_party/BUILD.gn suggestion before > submitting > > a > > > > completely different approach. > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... > > > > File third_party/BUILD.gn (right): > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... > > > > third_party/BUILD.gn:26: if (!is_chromeos) { > > > > On 2014/10/06 18:21:28, sky wrote: > > > > > Wouldn't it be better to wrap the config in the ifdef? > > > > > > > > Done. > > > > > > Approach A: Use build configuration (patch set 9) > > > Previously discussed. > > > > > > Approach B: Subclass JPEGCodec to introduce JPEGCodecRobust > > > To try this route I added protected variables for function pointers. In the > > > constructor the pointers were then set to either libjpeg or libjpeg_turbo > > > functions. One downside with this approach was that the code calling static > > > methods first needed a JPEGCodec object. The other issue was that the linker > > > couldn't find chromium_ijg_* libjpeg functions. This is an interesting > problem > > > as we want duplicate copies of the same class with different headers. > Headers > > > that have the same name and same include guards. I moved on since this > wasn't > > > turning out as clean as I hoped. If anyone has suggestions I'd be very > > > interested :). > > > > > > Approach C: Copy decode portions of JPEGCodec (patch set 10) > > > While straightforward this has the downside of blatant code duplication. It > is > > a > > > clean copy though, and would likely be simple to maintain. > > > > Approach C': extract as much shared code of JPEGCodec into a class that can be > > composed in, instead of using inheritance? > > Thanks for the feedback. I retried that approach but found myself going in > circles. Code hoisted into a common spot inevitably needs a jpeg header for a > type or function. I don't see a way around this issue without involving build > logic. OK, in that case I guess the best option is option C, trying to duplicate as little code as possible. Thanks for your continued work on this!
On 2014/11/19 00:05:44, Jorge Lucangeli Obes wrote: > On 2014/11/18 23:45:30, Ed Baker wrote: > > On 2014/11/06 18:50:46, Jorge Lucangeli Obes wrote: > > > On 2014/11/03 23:06:51, Ed Baker wrote: > > > > On 2014/10/30 23:29:48, Ed Baker wrote: > > > > > Patch set 9 addresses the third_party/BUILD.gn suggestion before > > submitting > > > a > > > > > completely different approach. > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... > > > > > File third_party/BUILD.gn (right): > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... > > > > > third_party/BUILD.gn:26: if (!is_chromeos) { > > > > > On 2014/10/06 18:21:28, sky wrote: > > > > > > Wouldn't it be better to wrap the config in the ifdef? > > > > > > > > > > Done. > > > > > > > > Approach A: Use build configuration (patch set 9) > > > > Previously discussed. > > > > > > > > Approach B: Subclass JPEGCodec to introduce JPEGCodecRobust > > > > To try this route I added protected variables for function pointers. In > the > > > > constructor the pointers were then set to either libjpeg or libjpeg_turbo > > > > functions. One downside with this approach was that the code calling > static > > > > methods first needed a JPEGCodec object. The other issue was that the > linker > > > > couldn't find chromium_ijg_* libjpeg functions. This is an interesting > > problem > > > > as we want duplicate copies of the same class with different headers. > > Headers > > > > that have the same name and same include guards. I moved on since this > > wasn't > > > > turning out as clean as I hoped. If anyone has suggestions I'd be very > > > > interested :). > > > > > > > > Approach C: Copy decode portions of JPEGCodec (patch set 10) > > > > While straightforward this has the downside of blatant code duplication. > It > > is > > > a > > > > clean copy though, and would likely be simple to maintain. > > > > > > Approach C': extract as much shared code of JPEGCodec into a class that can > be > > > composed in, instead of using inheritance? > > > > Thanks for the feedback. I retried that approach but found myself going in > > circles. Code hoisted into a common spot inevitably needs a jpeg header for a > > type or function. I don't see a way around this issue without involving build > > logic. > > OK, in that case I guess the best option is option C, trying to duplicate as > little code as possible. Thanks for your continued work on this! Would a Chrome OS specific unittest in ui/gfx/chromeos/codec checking gfx::JPEGCodec::JpegLibraryVariant() == gfx::JPEGCodec::IJG_LIBJPEG make the build configuration approach clearer? This would also help check that the logic survives the transition from gyp to gn for Chrome OS.
On 2014/11/20 21:20:58, Ed Baker wrote: > On 2014/11/19 00:05:44, Jorge Lucangeli Obes wrote: > > On 2014/11/18 23:45:30, Ed Baker wrote: > > > On 2014/11/06 18:50:46, Jorge Lucangeli Obes wrote: > > > > On 2014/11/03 23:06:51, Ed Baker wrote: > > > > > On 2014/10/30 23:29:48, Ed Baker wrote: > > > > > > Patch set 9 addresses the third_party/BUILD.gn suggestion before > > > submitting > > > > a > > > > > > completely different approach. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... > > > > > > File third_party/BUILD.gn (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/120001/third_party/BUIL... > > > > > > third_party/BUILD.gn:26: if (!is_chromeos) { > > > > > > On 2014/10/06 18:21:28, sky wrote: > > > > > > > Wouldn't it be better to wrap the config in the ifdef? > > > > > > > > > > > > Done. > > > > > > > > > > Approach A: Use build configuration (patch set 9) > > > > > Previously discussed. > > > > > > > > > > Approach B: Subclass JPEGCodec to introduce JPEGCodecRobust > > > > > To try this route I added protected variables for function pointers. In > > the > > > > > constructor the pointers were then set to either libjpeg or > libjpeg_turbo > > > > > functions. One downside with this approach was that the code calling > > static > > > > > methods first needed a JPEGCodec object. The other issue was that the > > linker > > > > > couldn't find chromium_ijg_* libjpeg functions. This is an interesting > > > problem > > > > > as we want duplicate copies of the same class with different headers. > > > Headers > > > > > that have the same name and same include guards. I moved on since this > > > wasn't > > > > > turning out as clean as I hoped. If anyone has suggestions I'd be very > > > > > interested :). > > > > > > > > > > Approach C: Copy decode portions of JPEGCodec (patch set 10) > > > > > While straightforward this has the downside of blatant code duplication. > > It > > > is > > > > a > > > > > clean copy though, and would likely be simple to maintain. > > > > > > > > Approach C': extract as much shared code of JPEGCodec into a class that > can > > be > > > > composed in, instead of using inheritance? > > > > > > Thanks for the feedback. I retried that approach but found myself going in > > > circles. Code hoisted into a common spot inevitably needs a jpeg header for > a > > > type or function. I don't see a way around this issue without involving > build > > > logic. > > > > OK, in that case I guess the best option is option C, trying to duplicate as > > little code as possible. Thanks for your continued work on this! > > Would a Chrome OS specific unittest in ui/gfx/chromeos/codec checking > gfx::JPEGCodec::JpegLibraryVariant() == gfx::JPEGCodec::IJG_LIBJPEG make the > build configuration approach clearer? This would also help check that the logic > survives the transition from gyp to gn for Chrome OS. For patch set 11 I was able to test the GN build with chromeos=1. This revealed that wrapping config("libjpeg_turbo_config") inside if (!is_chromeos) {} resulted in the following error. "ERROR Unresolved dependencies. //third_party:jpeg(//build/toolchain/linux:clang_x64) needs //third_party:libjpeg_turbo_config(//build/toolchain/linux:clang_x64)" jorgelo: I removed the TODOs from the GN changes as I was able to test functionality with gfx_unittests. Let me know if you would like them still. erg: I ifdef wrapped a simple Chrome OS specific IJG library check in the existing JPEG codec unittests. I assumed that this would be preferred over introducing a separate directory and associated files for the test. Testing: $ export GYP_DEFINES="chromeos=1" $ ./build/gyp_chromium $ ninja -C out/Debug/ gfx_unittests $ ./out/Debug/gfx_unittests --gtest_filter=JPEG* $ gn gen out/test –args="os=\"chromeos\"" $ ninja -C out/test $ ./out/test/gfx_unittests --gtest_filter=JPEG* Debug statements were added to libjpeg, libjpeg_turbo, and associated browser code to verify operation on the actual Chromebook. Login screen elements were decoded with libjpeg. All other images when browsing were decoded with libjpeg_turbo.
Patch set 12 rebases and updates the dedicated robust decoding class. Associated plumbing unique to Chrome OS was explicitly ifdef wrapped. Verified that JPEGCodec used libjpeg and JPEGCodecRobust used libjpeg_turbo. $ gn gen --args="target_os=\"chromeos\"" out/test $ ninja -C out/test gfx_unittests $ ./out/test/gfx_unittests --gtest_filter="JPEGCodec*" Verified that JPEGCodec and JPEGCodecRobust both use libjpeg. $ gn gen --args="target_os=\"chromeos\" use_libjpeg_turbo=false" out/test $ ninja -C out/test gfx_unittests $ ./out/test/gfx_unittests --gtest_filter="JPEGCodec*" Verified that JPEGCodec used libjpeg and JPEGCodecRobust used libjpeg_turbo. $ export GYP_DEFINES="chromeos=1" $ ./build/gyp_chromium $ ninja -C out/Debug/ gfx_unittests $ ./out/Debug/gfx_unittests --gtest_filter=JPEGCodec* The previous tests were also run with Linux as the build target to verify that JPEGCodecRobust did not exist.
Tests have been fixed. Lab is ready. I observe halfing of time on Chromebook Pixel as well. LGTM.
edward.baker@intel.com changed reviewers: + thakis@chromium.org - derat@chromium.org
+thakis for build/common.gypi
common.gypi looks fine, but I'd name the target "robust_and_slow" or something similar that makes the tradeoff a bit clearer. Without this, everyone will just depend on the _robust version, because hey, it says "robust" and the other target doesn't and robust is good right? So on Chrome OS, this now links to both libjpeg libraries? Or is the login screen a different binary? How do you protect about people accidentally introducing ODR violations? https://chromiumcodereview.appspot.com/569623002/diff/220001/build/common.gypi File build/common.gypi (right): https://chromiumcodereview.appspot.com/569623002/diff/220001/build/common.gyp... build/common.gypi:1505: 'libjpeg_ijg_gyp_path': '../third_party/libjpeg/libjpeg.gyp', I think you need to say <(DEPTH) instead of .. in gypi files, so that this works in all targets, not just targets one up
On 2015/05/05 18:05:42, Nico wrote: > common.gypi looks fine, but I'd name the target "robust_and_slow" or something > similar that makes the tradeoff a bit clearer. Without this, everyone will just > depend on the _robust version, because hey, it says "robust" and the other > target doesn't and robust is good right? > > So on Chrome OS, this now links to both libjpeg libraries? Or is the login > screen a different binary? > > How do you protect about people accidentally introducing ODR violations? > > https://chromiumcodereview.appspot.com/569623002/diff/220001/build/common.gypi > File build/common.gypi (right): > > https://chromiumcodereview.appspot.com/569623002/diff/220001/build/common.gyp... > build/common.gypi:1505: 'libjpeg_ijg_gyp_path': > '../third_party/libjpeg/libjpeg.gyp', > I think you need to say <(DEPTH) instead of .. in gypi files, so that this works > in all targets, not just targets one up Thanks for the feedback and have added slow to class and file names in ps13. ODR violations are handled by name mangling in the libjpeg libraries (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjpe... and https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjpe...). Chrome OS was already using both libraries due to libyuv always pulling in libjpeg_turbo.
https://chromiumcodereview.appspot.com/569623002/diff/220001/build/common.gypi File build/common.gypi (right): https://chromiumcodereview.appspot.com/569623002/diff/220001/build/common.gyp... build/common.gypi:1505: 'libjpeg_ijg_gyp_path': '../third_party/libjpeg/libjpeg.gyp', On 2015/05/05 18:05:42, Nico wrote: > I think you need to say <(DEPTH) instead of .. in gypi files, so that this works > in all targets, not just targets one up Done.
On 2015/05/05 18:05:42, Nico wrote: > common.gypi looks fine, but I'd name the target "robust_and_slow" or something > similar that makes the tradeoff a bit clearer. Without this, everyone will just > depend on the _robust version, because hey, it says "robust" and the other > target doesn't and robust is good right? > > So on Chrome OS, this now links to both libjpeg libraries? Or is the login > screen a different binary? > Links to both, there's no separate binary for the login screen. > How do you protect about people accidentally introducing ODR violations? > > https://chromiumcodereview.appspot.com/569623002/diff/220001/build/common.gypi > File build/common.gypi (right): > > https://chromiumcodereview.appspot.com/569623002/diff/220001/build/common.gyp... > build/common.gypi:1505: 'libjpeg_ijg_gyp_path': > '../third_party/libjpeg/libjpeg.gyp', > I think you need to say <(DEPTH) instead of .. in gypi files, so that this works > in all targets, not just targets one up
lgtm, thanks!
sky: When you get a minute could you review the latest patch set?
On 2014/10/03 18:35:11, Jorge Lucangeli Obes wrote: > On 2014/10/03 14:45:34, sky wrote: > > On 2014/10/02 23:29:03, Ed Baker wrote: > > > On 2014/09/23 21:42:04, Jorge Lucangeli Obes wrote: > > > > On 2014/09/23 19:49:43, Ed Baker wrote: > > > > > On 2014/09/22 19:54:47, Ed Baker wrote: > > > > > > On 2014/09/22 18:32:08, Jorge Lucangeli Obes wrote: > > > > > > > On 2014/09/22 18:30:40, Ed Baker wrote: > > > > > > > > On 2014/09/18 22:19:37, sky wrote: > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn > > > > > > > > > File ui/gfx/BUILD.gn (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn#n... > > > > > > > > > ui/gfx/BUILD.gn:323: # TODO(jorgelo): make sure this works on > > Chrome > > > > OS. > > > > > > > > > Please verify before landing. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > > > > > > > > > File ui/gfx/codec/jpeg_codec.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > > > > > > > > > ui/gfx/codec/jpeg_codec.cc:17: #elif defined(USE_LIBJPEG_TURBO) > && > > > > > > > > > !defined(OS_CHROMEOS) > > > > > > > > > Why is SE_LIBJPEG_TURBO set for chromeos if we're not using it? > > > > > > > > > > > > > > > > Patch set 6 removes the jpeg_codec.cc changes since > > USE_LIBJPEG_TURBO > > > > > > already > > > > > > > > handles the build logic. > > > > > > > > > > > > > > > > GYP: > > > > > > > > I verified the correct libraries are used with additional > debugging. > > > > > > > > UserImageManagerImpl and WallpaperManager decodes were handled by > > > > libjpeg. > > > > > > > > > > > > > > > > GN: > > > > > > > > `gn gen out/test --args="os=\"chromeos\""` fails on unrelated > > > undefined > > > > > > > > identifiers. > > > > > > > > `gn gen out/test` completes OK. > > > > > > > > > > > > > > lgtm since I think this is as far as GN will let us debug this so > far. > > > > > > > > > > > > > > Maybe try to actually build the GN build just in case. > > > > > > > > > > > > Ninja fails on unrelated harfbuzz warnings as errors when linking. > > > > > > $ gn gen out/test > > > > > > Done. Wrote 1419 targets from 520 files in 638ms > > > > > > $ ninja -C out/test chrome > > > > > > ninja: Entering directory `out/test' > > > > > > [15019/15019] LINK ./chrome > > > > > > <snip> > > > > > > ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: warning: > > hidden > > > > > symbol > > > > > > 'hb_buffer_create' in > > > > obj/third_party/harfbuzz-ng/src/harfbuzz-ng.hb-buffer.o > > > > > is > > > > > > referenced by DSO > > > > > > > > > > > > /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../x86_64-linux-gnu/libpangoft2-1.0.so > > > > > > ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error: > > treating > > > > > > warnings as errors > > > > > > <snip> > > > > > > > > > > Rebased on master as common.gypi changes were not merging cleanly. I > also > > > > > retested the GN build after reviewing > > > > > https://code.google.com/p/chromium/issues/detail?id=353127 . > > > > > > > > > > $ gn gen out/test --args="use_system_harfbuzz=true" > > > > > Done. Wrote 1615 targets from 551 files in 660ms > > > > > $ ninja -C out/test chrome > > > > > ninja: Entering directory `out/test' > > > > > [460/460] LINK ./chrome > > > > > $ > > > > > > > > Thanks for testing! > > > > > > sky: Is this sufficient testing for the build changes? > > > > Can you clarify why we are having two jpeg decoders here? Isn't one enough? > > User avatars and desktop backgrounds are kept outside of each users' home > partitions and can therefore be modified by any other user in the system. We use > a more robust (but slower) decoder for these images to mitigate the risk of one > user being able to exploit Chrome in the pre-login state by modifying somebody > else's picture. > > Edward noticed that we were using the slower decoder in more cases than we > needed. This CL fixes that. I'm still confused by all this. I guess my question stems from why we need ui/gfx/chromeos at all. If we don't trust the jpgs than we should decode them in a utility process (which we have code for), which we do for other random jpgs downloaded that we can't trust.
On 2015/05/06 20:04:27, sky wrote: > On 2014/10/03 18:35:11, Jorge Lucangeli Obes wrote: > > On 2014/10/03 14:45:34, sky wrote: > > > On 2014/10/02 23:29:03, Ed Baker wrote: > > > > On 2014/09/23 21:42:04, Jorge Lucangeli Obes wrote: > > > > > On 2014/09/23 19:49:43, Ed Baker wrote: > > > > > > On 2014/09/22 19:54:47, Ed Baker wrote: > > > > > > > On 2014/09/22 18:32:08, Jorge Lucangeli Obes wrote: > > > > > > > > On 2014/09/22 18:30:40, Ed Baker wrote: > > > > > > > > > On 2014/09/18 22:19:37, sky wrote: > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn > > > > > > > > > > File ui/gfx/BUILD.gn (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn#n... > > > > > > > > > > ui/gfx/BUILD.gn:323: # TODO(jorgelo): make sure this works on > > > Chrome > > > > > OS. > > > > > > > > > > Please verify before landing. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > > > > > > > > > > File ui/gfx/codec/jpeg_codec.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > > > > > > > > > > ui/gfx/codec/jpeg_codec.cc:17: #elif > defined(USE_LIBJPEG_TURBO) > > && > > > > > > > > > > !defined(OS_CHROMEOS) > > > > > > > > > > Why is SE_LIBJPEG_TURBO set for chromeos if we're not using > it? > > > > > > > > > > > > > > > > > > Patch set 6 removes the jpeg_codec.cc changes since > > > USE_LIBJPEG_TURBO > > > > > > > already > > > > > > > > > handles the build logic. > > > > > > > > > > > > > > > > > > GYP: > > > > > > > > > I verified the correct libraries are used with additional > > debugging. > > > > > > > > > UserImageManagerImpl and WallpaperManager decodes were handled > by > > > > > libjpeg. > > > > > > > > > > > > > > > > > > GN: > > > > > > > > > `gn gen out/test --args="os=\"chromeos\""` fails on unrelated > > > > undefined > > > > > > > > > identifiers. > > > > > > > > > `gn gen out/test` completes OK. > > > > > > > > > > > > > > > > lgtm since I think this is as far as GN will let us debug this so > > far. > > > > > > > > > > > > > > > > Maybe try to actually build the GN build just in case. > > > > > > > > > > > > > > Ninja fails on unrelated harfbuzz warnings as errors when linking. > > > > > > > $ gn gen out/test > > > > > > > Done. Wrote 1419 targets from 520 files in 638ms > > > > > > > $ ninja -C out/test chrome > > > > > > > ninja: Entering directory `out/test' > > > > > > > [15019/15019] LINK ./chrome > > > > > > > <snip> > > > > > > > ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: warning: > > > hidden > > > > > > symbol > > > > > > > 'hb_buffer_create' in > > > > > obj/third_party/harfbuzz-ng/src/harfbuzz-ng.hb-buffer.o > > > > > > is > > > > > > > referenced by DSO > > > > > > > > > > > > > > > > /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../x86_64-linux-gnu/libpangoft2-1.0.so > > > > > > > ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error: > > > treating > > > > > > > warnings as errors > > > > > > > <snip> > > > > > > > > > > > > Rebased on master as common.gypi changes were not merging cleanly. I > > also > > > > > > retested the GN build after reviewing > > > > > > https://code.google.com/p/chromium/issues/detail?id=353127 . > > > > > > > > > > > > $ gn gen out/test --args="use_system_harfbuzz=true" > > > > > > Done. Wrote 1615 targets from 551 files in 660ms > > > > > > $ ninja -C out/test chrome > > > > > > ninja: Entering directory `out/test' > > > > > > [460/460] LINK ./chrome > > > > > > $ > > > > > > > > > > Thanks for testing! > > > > > > > > sky: Is this sufficient testing for the build changes? > > > > > > Can you clarify why we are having two jpeg decoders here? Isn't one enough? > > > > User avatars and desktop backgrounds are kept outside of each users' home > > partitions and can therefore be modified by any other user in the system. We > use > > a more robust (but slower) decoder for these images to mitigate the risk of > one > > user being able to exploit Chrome in the pre-login state by modifying somebody > > else's picture. > > > > Edward noticed that we were using the slower decoder in more cases than we > > needed. This CL fixes that. > > I'm still confused by all this. I guess my question stems from why we need > ui/gfx/chromeos at all. If we don't trust the jpgs than we should decode them in > a utility process (which we have code for), which we do for other random jpgs > downloaded that we can't trust. Both paths (fast-not-robust, robust-but-slow) are going through the utility process. The handling side of the utility process message uses gfx::JPEGCodecRobustSlow in the robust-but-slow case. Since the potential for abuse in the login screen is high, and can result in persistent compromise (defeating verified boot and nullifying one of Chrome OS's main advantages), we concluded processing in a separate process wasn't enough, and we wanted to add a more robust decoder as well.
On Wed, May 6, 2015 at 1:50 PM, <jorgelo@chromium.org> wrote: > On 2015/05/06 20:04:27, sky wrote: >> >> On 2014/10/03 18:35:11, Jorge Lucangeli Obes wrote: >> > On 2014/10/03 14:45:34, sky wrote: >> > > On 2014/10/02 23:29:03, Ed Baker wrote: >> > > > On 2014/09/23 21:42:04, Jorge Lucangeli Obes wrote: >> > > > > On 2014/09/23 19:49:43, Ed Baker wrote: >> > > > > > On 2014/09/22 19:54:47, Ed Baker wrote: >> > > > > > > On 2014/09/22 18:32:08, Jorge Lucangeli Obes wrote: >> > > > > > > > On 2014/09/22 18:30:40, Ed Baker wrote: >> > > > > > > > > On 2014/09/18 22:19:37, sky wrote: >> > > > > > > > > > >> > > > > > > >> > > > >> >> https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn >> > > > > > > > > > File ui/gfx/BUILD.gn (right): >> > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn#n... >> >> > > > > > > > > > ui/gfx/BUILD.gn:323: # TODO(jorgelo): make sure this >> > > > > > > > > > works > > on >> >> > > Chrome >> > > > > OS. >> > > > > > > > > > Please verify before landing. >> > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... >> >> > > > > > > > > > File ui/gfx/codec/jpeg_codec.cc (right): >> > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... >> >> > > > > > > > > > ui/gfx/codec/jpeg_codec.cc:17: #elif >> defined(USE_LIBJPEG_TURBO) >> > && >> > > > > > > > > > !defined(OS_CHROMEOS) >> > > > > > > > > > Why is SE_LIBJPEG_TURBO set for chromeos if we're not >> > > > > > > > > > using >> it? >> > > > > > > > > >> > > > > > > > > Patch set 6 removes the jpeg_codec.cc changes since >> > > USE_LIBJPEG_TURBO >> > > > > > > already >> > > > > > > > > handles the build logic. >> > > > > > > > > >> > > > > > > > > GYP: >> > > > > > > > > I verified the correct libraries are used with additional >> > debugging. >> > > > > > > > > UserImageManagerImpl and WallpaperManager decodes were >> > > > > > > > > handled >> by >> > > > > libjpeg. >> > > > > > > > > >> > > > > > > > > GN: >> > > > > > > > > `gn gen out/test --args="os=\"chromeos\""` fails on >> > > > > > > > > unrelated >> > > > undefined >> > > > > > > > > identifiers. >> > > > > > > > > `gn gen out/test` completes OK. >> > > > > > > > >> > > > > > > > lgtm since I think this is as far as GN will let us debug >> > > > > > > > this > > so >> >> > far. >> > > > > > > > >> > > > > > > > Maybe try to actually build the GN build just in case. >> > > > > > > >> > > > > > > Ninja fails on unrelated harfbuzz warnings as errors when >> > > > > > > linking. >> > > > > > > $ gn gen out/test >> > > > > > > Done. Wrote 1419 targets from 520 files in 638ms >> > > > > > > $ ninja -C out/test chrome >> > > > > > > ninja: Entering directory `out/test' >> > > > > > > [15019/15019] LINK ./chrome >> > > > > > > <snip> >> > > > > > > ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: >> > > > > > > warning: >> > > hidden >> > > > > > symbol >> > > > > > > 'hb_buffer_create' in >> > > > > obj/third_party/harfbuzz-ng/src/harfbuzz-ng.hb-buffer.o >> > > > > > is >> > > > > > > referenced by DSO >> > > > > > > >> > > > > >> > > >> >> /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../x86_64-linux-gnu/libpangoft2-1.0.so >> > > > > > > ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: >> > > > > > > error: >> > > treating >> > > > > > > warnings as errors >> > > > > > > <snip> >> > > > > > >> > > > > > Rebased on master as common.gypi changes were not merging >> > > > > > cleanly. I >> > also >> > > > > > retested the GN build after reviewing >> > > > > > https://code.google.com/p/chromium/issues/detail?id=353127 . >> > > > > > >> > > > > > $ gn gen out/test --args="use_system_harfbuzz=true" >> > > > > > Done. Wrote 1615 targets from 551 files in 660ms >> > > > > > $ ninja -C out/test chrome >> > > > > > ninja: Entering directory `out/test' >> > > > > > [460/460] LINK ./chrome >> > > > > > $ >> > > > > >> > > > > Thanks for testing! >> > > > >> > > > sky: Is this sufficient testing for the build changes? >> > > >> > > Can you clarify why we are having two jpeg decoders here? Isn't one > > enough? >> >> > >> > User avatars and desktop backgrounds are kept outside of each users' >> > home >> > partitions and can therefore be modified by any other user in the >> > system. We >> use >> > a more robust (but slower) decoder for these images to mitigate the risk >> > of >> one >> > user being able to exploit Chrome in the pre-login state by modifying > > somebody >> >> > else's picture. >> > >> > Edward noticed that we were using the slower decoder in more cases than >> > we >> > needed. This CL fixes that. > > >> I'm still confused by all this. I guess my question stems from why we need >> ui/gfx/chromeos at all. If we don't trust the jpgs than we should decode >> them > > in >> >> a utility process (which we have code for), which we do for other random >> jpgs >> downloaded that we can't trust. > > > Both paths (fast-not-robust, robust-but-slow) are going through the utility > process. The handling side of the utility process message uses > gfx::JPEGCodecRobustSlow in the robust-but-slow case. Since the potential > for > abuse in the login screen is high, and can result in persistent compromise > (defeating verified boot and nullifying one of Chrome OS's main advantages), > we > concluded processing in a separate process wasn't enough, and we wanted to > add a > more robust decoder as well. > > https://chromiumcodereview.appspot.com/569623002/ What is different about chromeos than chrome downloading random jpgs from the web that warrants the extra level? -Scott To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/05/06 21:01:40, sky wrote: > On Wed, May 6, 2015 at 1:50 PM, <mailto:jorgelo@chromium.org> wrote: > > On 2015/05/06 20:04:27, sky wrote: > >> > >> On 2014/10/03 18:35:11, Jorge Lucangeli Obes wrote: > >> > On 2014/10/03 14:45:34, sky wrote: > >> > > On 2014/10/02 23:29:03, Ed Baker wrote: > >> > > > On 2014/09/23 21:42:04, Jorge Lucangeli Obes wrote: > >> > > > > On 2014/09/23 19:49:43, Ed Baker wrote: > >> > > > > > On 2014/09/22 19:54:47, Ed Baker wrote: > >> > > > > > > On 2014/09/22 18:32:08, Jorge Lucangeli Obes wrote: > >> > > > > > > > On 2014/09/22 18:30:40, Ed Baker wrote: > >> > > > > > > > > On 2014/09/18 22:19:37, sky wrote: > >> > > > > > > > > > > >> > > > > > > > >> > > > > >> > >> https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn > >> > > > > > > > > > File ui/gfx/BUILD.gn (right): > >> > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/BUILD.gn#n... > >> > >> > > > > > > > > > ui/gfx/BUILD.gn:323: # TODO(jorgelo): make sure this > >> > > > > > > > > > works > > > > on > >> > >> > > Chrome > >> > > > > OS. > >> > > > > > > > > > Please verify before landing. > >> > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > >> > >> > > > > > > > > > File ui/gfx/codec/jpeg_codec.cc (right): > >> > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > > > > > > https://chromiumcodereview.appspot.com/569623002/diff/60001/ui/gfx/codec/jpeg... > >> > >> > > > > > > > > > ui/gfx/codec/jpeg_codec.cc:17: #elif > >> defined(USE_LIBJPEG_TURBO) > >> > && > >> > > > > > > > > > !defined(OS_CHROMEOS) > >> > > > > > > > > > Why is SE_LIBJPEG_TURBO set for chromeos if we're not > >> > > > > > > > > > using > >> it? > >> > > > > > > > > > >> > > > > > > > > Patch set 6 removes the jpeg_codec.cc changes since > >> > > USE_LIBJPEG_TURBO > >> > > > > > > already > >> > > > > > > > > handles the build logic. > >> > > > > > > > > > >> > > > > > > > > GYP: > >> > > > > > > > > I verified the correct libraries are used with additional > >> > debugging. > >> > > > > > > > > UserImageManagerImpl and WallpaperManager decodes were > >> > > > > > > > > handled > >> by > >> > > > > libjpeg. > >> > > > > > > > > > >> > > > > > > > > GN: > >> > > > > > > > > `gn gen out/test --args="os=\"chromeos\""` fails on > >> > > > > > > > > unrelated > >> > > > undefined > >> > > > > > > > > identifiers. > >> > > > > > > > > `gn gen out/test` completes OK. > >> > > > > > > > > >> > > > > > > > lgtm since I think this is as far as GN will let us debug > >> > > > > > > > this > > > > so > >> > >> > far. > >> > > > > > > > > >> > > > > > > > Maybe try to actually build the GN build just in case. > >> > > > > > > > >> > > > > > > Ninja fails on unrelated harfbuzz warnings as errors when > >> > > > > > > linking. > >> > > > > > > $ gn gen out/test > >> > > > > > > Done. Wrote 1419 targets from 520 files in 638ms > >> > > > > > > $ ninja -C out/test chrome > >> > > > > > > ninja: Entering directory `out/test' > >> > > > > > > [15019/15019] LINK ./chrome > >> > > > > > > <snip> > >> > > > > > > ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: > >> > > > > > > warning: > >> > > hidden > >> > > > > > symbol > >> > > > > > > 'hb_buffer_create' in > >> > > > > obj/third_party/harfbuzz-ng/src/harfbuzz-ng.hb-buffer.o > >> > > > > > is > >> > > > > > > referenced by DSO > >> > > > > > > > >> > > > > > >> > > > >> > >> > /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../x86_64-linux-gnu/libpangoft2-1.0.so > >> > > > > > > ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: > >> > > > > > > error: > >> > > treating > >> > > > > > > warnings as errors > >> > > > > > > <snip> > >> > > > > > > >> > > > > > Rebased on master as common.gypi changes were not merging > >> > > > > > cleanly. I > >> > also > >> > > > > > retested the GN build after reviewing > >> > > > > > https://code.google.com/p/chromium/issues/detail?id=353127 . > >> > > > > > > >> > > > > > $ gn gen out/test --args="use_system_harfbuzz=true" > >> > > > > > Done. Wrote 1615 targets from 551 files in 660ms > >> > > > > > $ ninja -C out/test chrome > >> > > > > > ninja: Entering directory `out/test' > >> > > > > > [460/460] LINK ./chrome > >> > > > > > $ > >> > > > > > >> > > > > Thanks for testing! > >> > > > > >> > > > sky: Is this sufficient testing for the build changes? > >> > > > >> > > Can you clarify why we are having two jpeg decoders here? Isn't one > > > > enough? > >> > >> > > >> > User avatars and desktop backgrounds are kept outside of each users' > >> > home > >> > partitions and can therefore be modified by any other user in the > >> > system. We > >> use > >> > a more robust (but slower) decoder for these images to mitigate the risk > >> > of > >> one > >> > user being able to exploit Chrome in the pre-login state by modifying > > > > somebody > >> > >> > else's picture. > >> > > >> > Edward noticed that we were using the slower decoder in more cases than > >> > we > >> > needed. This CL fixes that. > > > > > >> I'm still confused by all this. I guess my question stems from why we need > >> ui/gfx/chromeos at all. If we don't trust the jpgs than we should decode > >> them > > > > in > >> > >> a utility process (which we have code for), which we do for other random > >> jpgs > >> downloaded that we can't trust. > > > > > > Both paths (fast-not-robust, robust-but-slow) are going through the utility > > process. The handling side of the utility process message uses > > gfx::JPEGCodecRobustSlow in the robust-but-slow case. Since the potential > > for > > abuse in the login screen is high, and can result in persistent compromise > > (defeating verified boot and nullifying one of Chrome OS's main advantages), > > we > > concluded processing in a separate process wasn't enough, and we wanted to > > add a > > more robust decoder as well. > > > > https://chromiumcodereview.appspot.com/569623002/ > > What is different about chromeos than chrome downloading random jpgs > from the web that warrants the extra level? > Chrome is a browser, Chrome OS is an operating system. Chrome does not make any guarantees about the integrity of its binaries, Chrome OS does. Chrome OS makes a very strong integrity guarantee: if the device boots to the login screen, you're executing code that's been signed by Google and verified by the device. That's why we have a cryptographically-verified boot chain and root partition. We also guarantee very strong per-user boundaries. Therefore, anything that compromises these guarantee is a big deal. For example, the possibility of using a bug in libjpeg-turbo to exploit Chrome at the login screen just by changing your profile picture inside your Chrome OS session. > -Scott > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/569623002/diff/240001/chrome/browser/image_de... File chrome/browser/image_decoder.h (right): https://codereview.chromium.org/569623002/diff/240001/chrome/browser/image_de... chrome/browser/image_decoder.h:62: ROBUST_JPEG_CODEC, // Restrict decoding to robust jpeg codec. Could we (separately) get a more descriptive name here. I'm thinking something like THOROUGH_BUT_SLOWER_JPEG_CODEC. Robust by itself doesn't give enough information. https://codereview.chromium.org/569623002/diff/240001/ui/gfx/chromeos/codec/O... File ui/gfx/chromeos/codec/OWNERS (right): https://codereview.chromium.org/569623002/diff/240001/ui/gfx/chromeos/codec/O... ui/gfx/chromeos/codec/OWNERS:1: jorgelo@chromium.org Are you a committer? https://codereview.chromium.org/569623002/diff/240001/ui/gfx/chromeos/codec/j... File ui/gfx/chromeos/codec/jpeg_codec_robust_slow.h (right): https://codereview.chromium.org/569623002/diff/240001/ui/gfx/chromeos/codec/j... ui/gfx/chromeos/codec/jpeg_codec_robust_slow.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. 2015 and no c. https://codereview.chromium.org/569623002/diff/240001/ui/gfx/chromeos/codec/j... ui/gfx/chromeos/codec/jpeg_codec_robust_slow.h:21: class GFX_EXPORT JPEGCodecRobustSlow { Similar comment about name. https://codereview.chromium.org/569623002/diff/240001/ui/gfx/chromeos/codec/j... File ui/gfx/chromeos/codec/jpeg_codec_robust_slow_unittest.cc (right): https://codereview.chromium.org/569623002/diff/240001/ui/gfx/chromeos/codec/j... ui/gfx/chromeos/codec/jpeg_codec_robust_slow_unittest.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. no (c), 2015 https://codereview.chromium.org/569623002/diff/240001/ui/gfx/chromeos/codec/j... ui/gfx/chromeos/codec/jpeg_codec_robust_slow_unittest.cc:95: JPEGCodecRobustSlow::Decode(kTopSitesMigrationTestImage, I don't see any assertions here?
https://codereview.chromium.org/569623002/diff/240001/ui/gfx/chromeos/codec/O... File ui/gfx/chromeos/codec/OWNERS (right): https://codereview.chromium.org/569623002/diff/240001/ui/gfx/chromeos/codec/O... ui/gfx/chromeos/codec/OWNERS:1: jorgelo@chromium.org On 2015/05/06 21:44:27, sky wrote: > Are you a committer? Yes, why? I wouldn't have lgtm'd this if I were not.
https://codereview.chromium.org/569623002/diff/240001/ui/gfx/chromeos/codec/O... File ui/gfx/chromeos/codec/OWNERS (right): https://codereview.chromium.org/569623002/diff/240001/ui/gfx/chromeos/codec/O... ui/gfx/chromeos/codec/OWNERS:1: jorgelo@chromium.org On 2015/05/06 21:46:08, Jorge Lucangeli Obes wrote: > On 2015/05/06 21:44:27, sky wrote: > > Are you a committer? > > Yes, why? I wouldn't have lgtm'd this if I were not. Sorry, I misread the email address.
https://chromiumcodereview.appspot.com/569623002/diff/240001/chrome/browser/i... File chrome/browser/image_decoder.h (right): https://chromiumcodereview.appspot.com/569623002/diff/240001/chrome/browser/i... chrome/browser/image_decoder.h:62: ROBUST_JPEG_CODEC, // Restrict decoding to robust jpeg codec. On 2015/05/06 21:44:27, sky wrote: > Could we (separately) get a more descriptive name here. I'm thinking something > like THOROUGH_BUT_SLOWER_JPEG_CODEC. Robust by itself doesn't give enough > information. Acknowledged. https://chromiumcodereview.appspot.com/569623002/diff/240001/ui/gfx/chromeos/... File ui/gfx/chromeos/codec/jpeg_codec_robust_slow.h (right): https://chromiumcodereview.appspot.com/569623002/diff/240001/ui/gfx/chromeos/... ui/gfx/chromeos/codec/jpeg_codec_robust_slow.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2015/05/06 21:44:27, sky wrote: > 2015 and no c. Done. https://chromiumcodereview.appspot.com/569623002/diff/240001/ui/gfx/chromeos/... ui/gfx/chromeos/codec/jpeg_codec_robust_slow.h:21: class GFX_EXPORT JPEGCodecRobustSlow { On 2015/05/06 21:44:27, sky wrote: > Similar comment about name. Acknowledged. https://chromiumcodereview.appspot.com/569623002/diff/240001/ui/gfx/chromeos/... File ui/gfx/chromeos/codec/jpeg_codec_robust_slow_unittest.cc (right): https://chromiumcodereview.appspot.com/569623002/diff/240001/ui/gfx/chromeos/... ui/gfx/chromeos/codec/jpeg_codec_robust_slow_unittest.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2015/05/06 21:44:28, sky wrote: > no (c), 2015 Done. https://chromiumcodereview.appspot.com/569623002/diff/240001/ui/gfx/chromeos/... ui/gfx/chromeos/codec/jpeg_codec_robust_slow_unittest.cc:95: JPEGCodecRobustSlow::Decode(kTopSitesMigrationTestImage, On 2015/05/06 21:44:28, sky wrote: > I don't see any assertions here? Done.
LGTM
The CQ bit was checked by edward.baker@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org, ihf@chromium.org, jorgelo@chromium.org, thakis@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/569623002/#ps260001 (title: "Fix copyright and add unit test assertions")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/569623002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Looks like we need OWNERS for third_party/libjpeg
On 2015/05/07 17:59:53, Jorge Lucangeli Obes wrote: > Looks like we need OWNERS for third_party/libjpeg According to third_party/OWNERS I was supposed to handle this with a TBR=. Do you know if editing the description would work?
I think editing the description should work.
It should On Thu, May 7, 2015 at 11:11 AM, <edward.baker@intel.com> wrote: > On 2015/05/07 17:59:53, Jorge Lucangeli Obes wrote: >> >> Looks like we need OWNERS for third_party/libjpeg > > > According to third_party/OWNERS I was supposed to handle this with a TBR=. > Do > you know if editing the description would work? > > https://chromiumcodereview.appspot.com/569623002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by edward.baker@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/569623002/260001
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/199f66b202c942421e25ced69471f7d9d1f2c4a7 Cr-Commit-Position: refs/heads/master@{#328812} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
