Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2895953003:
Use SkJpegEncoder in gfx jpeg_codec (Closed)
|
Created:
3 years, 7 months ago by msarett Modified:
3 years, 6 months ago Reviewers:
Ken Rockot(use gerrit already), jochen (gone - plz use gerrit), sandersd (OOO until July 31), Lei Zhang, dcheng, bshe, Robert Sesek, msarett1, scroggo_chromium, reveman, dgozman, tbarzic CC:
achuith+watch_chromium.org, alemate+watch_chromium.org, cc-bugs_chromium.org, chfremer+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, davemoore+watch_chromium.org, devtools-reviews_chromium.org, extensions-reviews_chromium.org, feature-media-reviews_chromium.org, jam, mfoltz+watch_chromium.org, miu+watch_chromium.org, oshima+watch_chromium.org, pfeldman, piman+watch_chromium.org, posciak+watch_chromium.org, rsesek+watch_chromium.org, xjz+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse SkJpegEncoder in gfx jpeg_codec
TBR=jochen@chromium.org
TBR=reveman@chromium.org
BUG=724616
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2895953003
Cr-Commit-Position: refs/heads/master@{#479694}
Committed: https://chromium.googlesource.com/chromium/src/+/7060f1277a34592bc1f6c16762ccb0345669b073
Patch Set 1 #
Total comments: 16
Patch Set 2 : Rebase on previous CL #Patch Set 3 : Update comment #
Total comments: 4
Patch Set 4 : Remove bad ! #
Total comments: 4
Patch Set 5 : Response to comments #
Total comments: 12
Patch Set 6 : Add API that takes SkBitmap #Patch Set 7 : Remove brackets #
Total comments: 1
Messages
Total messages: 62 (34 generated)
Description was changed from ========== Use SkJpegEncoder in gfx jpeg_codec BUG=724616 ========== to ========== Use SkJpegEncoder in gfx jpeg_codec BUG=724616 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Use SkJpegEncoder in gfx jpeg_codec BUG=724616 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Use SkJpegEncoder in gfx jpeg_codec BUG=724616 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by msarett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
msarett@chromium.org changed reviewers: + dcheng@chromium.org, scroggo@chromium.org
https://codereview.chromium.org/2895953003/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_test_utils.cc (right): https://codereview.chromium.org/2895953003/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_test_utils.cc:93: kN32_SkColorType, width, height, bitmap.rowBytes(), kQuality, This parameter list makes me think we should've taken an SkBitmap (or an SkPixmap?) as a parameter. (Doesn't need to be in this CL, but something to consider?) https://codereview.chromium.org/2895953003/diff/1/ui/gfx/codec/jpeg_codec.cc File ui/gfx/codec/jpeg_codec.cc (left): https://codereview.chromium.org/2895953003/diff/1/ui/gfx/codec/jpeg_codec.cc#... ui/gfx/codec/jpeg_codec.cc:559: if (format == FORMAT_RGB) { So this CL also removes the ability to decode to RGB? I'm guessing that is never used? Please add this to the CL description. (Alternatively, should it be a separate CL?) https://codereview.chromium.org/2895953003/diff/1/ui/gfx/codec/jpeg_codec.cc File ui/gfx/codec/jpeg_codec.cc (right): https://codereview.chromium.org/2895953003/diff/1/ui/gfx/codec/jpeg_codec.cc#... ui/gfx/codec/jpeg_codec.cc:67: DCHECK_EQ(0UL, dst->size()); Not a huge deal, but in the first DCHECK you used dst_, and this time you used dst. Might as well be consistent? https://codereview.chromium.org/2895953003/diff/1/ui/gfx/codec/jpeg_codec.cc#... ui/gfx/codec/jpeg_codec.cc:71: dst_->insert(dst_->end(), reinterpret_cast<const unsigned char*>(buffer), nit: Use a temporary to make this more readable? auto* char_buffer = reinterpret_cast<const unsigned char*>(buffer); dst_->insert(dst_->end(), char_buffer, char_buffer + size); https://codereview.chromium.org/2895953003/diff/1/ui/gfx/codec/jpeg_codec.cc#... ui/gfx/codec/jpeg_codec.cc:290: cinfo.output_components = 4; It looks like this is always 4 if JCS_EXTENSIONS is defined and we're not returning false? Maybe move this down below: #ifdef JCS_EXTENSIONS cinfo.output_components = 4; #else cinfo.output_components = 3; #endif ? https://codereview.chromium.org/2895953003/diff/1/ui/gfx/codec/jpeg_codec.cc#... ui/gfx/codec/jpeg_codec.cc:335: // Rows need conversion to output format: read into a temporary buffer and The diff doesn't clearly show this, but I think this code just got moved out of the else case from FORMAT_RGB?
https://codereview.chromium.org/2895953003/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_test_utils.cc (right): https://codereview.chromium.org/2895953003/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_test_utils.cc:93: kN32_SkColorType, width, height, bitmap.rowBytes(), kQuality, On 2017/05/30 14:27:52, scroggo_chromium wrote: > This parameter list makes me think we should've taken an SkBitmap (or an > SkPixmap?) as a parameter. (Doesn't need to be in this CL, but something to > consider?) I agree this would make sense: PNG codec has some encode helpers that take a SkBitmap parameter. https://codereview.chromium.org/2895953003/diff/1/ui/gfx/codec/jpeg_codec.h File ui/gfx/codec/jpeg_codec.h (right): https://codereview.chromium.org/2895953003/diff/1/ui/gfx/codec/jpeg_codec.h#n... ui/gfx/codec/jpeg_codec.h:59: SkColorType colorType, Nit: color_type From a symmetry perspective, it's a bit odd that Decode() takes a ColorFormat while Encode() takes a SkColorType. Will we have eventual consistency here?
https://codereview.chromium.org/2895953003/diff/1/ui/gfx/codec/jpeg_codec.h File ui/gfx/codec/jpeg_codec.h (right): https://codereview.chromium.org/2895953003/diff/1/ui/gfx/codec/jpeg_codec.h#n... ui/gfx/codec/jpeg_codec.h:59: SkColorType colorType, On 2017/06/02 15:35:00, dcheng wrote: > Nit: color_type > > From a symmetry perspective, it's a bit odd that Decode() takes a ColorFormat > while Encode() takes a SkColorType. Will we have eventual consistency here? Yes. We're in the process of merging Skia's decoders with Blink's, and we'd like to use the same decoders here, too. At that point I think it makes sense to use an SkColorType for Decode.
PTAL, I think this is much cleaner now that I've removed RGB and legacy libjpeg support in a separate CL. I also modified the API to take a SkPixmap. https://codereview.chromium.org/2895953003/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_test_utils.cc (right): https://codereview.chromium.org/2895953003/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_test_utils.cc:93: kN32_SkColorType, width, height, bitmap.rowBytes(), kQuality, On 2017/06/02 15:35:00, dcheng wrote: > On 2017/05/30 14:27:52, scroggo_chromium wrote: > > This parameter list makes me think we should've taken an SkBitmap (or an > > SkPixmap?) as a parameter. (Doesn't need to be in this CL, but something to > > consider?) > > I agree this would make sense: PNG codec has some encode helpers that take a > SkBitmap parameter. Yeah I think it should take an SkPixmap. SkBitmap would be fine as well, but I think Skia in general is trying to move towards SkImage and SkPixmap. https://codereview.chromium.org/2895953003/diff/1/ui/gfx/codec/jpeg_codec.cc File ui/gfx/codec/jpeg_codec.cc (left): https://codereview.chromium.org/2895953003/diff/1/ui/gfx/codec/jpeg_codec.cc#... ui/gfx/codec/jpeg_codec.cc:559: if (format == FORMAT_RGB) { On 2017/05/30 14:27:52, scroggo_chromium wrote: > So this CL also removes the ability to decode to RGB? I'm guessing that is never > used? Please add this to the CL description. (Alternatively, should it be a > separate CL?) Acknowledged. Removed in a previous CL. https://codereview.chromium.org/2895953003/diff/1/ui/gfx/codec/jpeg_codec.cc File ui/gfx/codec/jpeg_codec.cc (right): https://codereview.chromium.org/2895953003/diff/1/ui/gfx/codec/jpeg_codec.cc#... ui/gfx/codec/jpeg_codec.cc:67: DCHECK_EQ(0UL, dst->size()); On 2017/05/30 14:27:52, scroggo_chromium wrote: > Not a huge deal, but in the first DCHECK you used dst_, and this time you used > dst. Might as well be consistent? Done. https://codereview.chromium.org/2895953003/diff/1/ui/gfx/codec/jpeg_codec.cc#... ui/gfx/codec/jpeg_codec.cc:71: dst_->insert(dst_->end(), reinterpret_cast<const unsigned char*>(buffer), On 2017/05/30 14:27:52, scroggo_chromium wrote: > nit: Use a temporary to make this more readable? > > auto* char_buffer = reinterpret_cast<const unsigned char*>(buffer); > dst_->insert(dst_->end(), char_buffer, char_buffer + size); Done. https://codereview.chromium.org/2895953003/diff/1/ui/gfx/codec/jpeg_codec.cc#... ui/gfx/codec/jpeg_codec.cc:290: cinfo.output_components = 4; On 2017/05/30 14:27:52, scroggo_chromium wrote: > It looks like this is always 4 if JCS_EXTENSIONS is defined and we're not > returning false? Maybe move this down below: > > #ifdef JCS_EXTENSIONS > cinfo.output_components = 4; > #else > cinfo.output_components = 3; > #endif > > ? Acknowledged. https://codereview.chromium.org/2895953003/diff/1/ui/gfx/codec/jpeg_codec.cc#... ui/gfx/codec/jpeg_codec.cc:335: // Rows need conversion to output format: read into a temporary buffer and On 2017/05/30 14:27:52, scroggo_chromium wrote: > The diff doesn't clearly show this, but I think this code just got moved out of > the else case from FORMAT_RGB? Yes. Should be clearer now. https://codereview.chromium.org/2895953003/diff/1/ui/gfx/codec/jpeg_codec.h File ui/gfx/codec/jpeg_codec.h (right): https://codereview.chromium.org/2895953003/diff/1/ui/gfx/codec/jpeg_codec.h#n... ui/gfx/codec/jpeg_codec.h:59: SkColorType colorType, On 2017/06/02 17:35:51, scroggo_chromium wrote: > On 2017/06/02 15:35:00, dcheng wrote: > > Nit: color_type > > > > From a symmetry perspective, it's a bit odd that Decode() takes a ColorFormat > > while Encode() takes a SkColorType. Will we have eventual consistency here? > > Yes. We're in the process of merging Skia's decoders with Blink's, and we'd like > to use the same decoders here, too. At that point I think it makes sense to use > an SkColorType for Decode. Yes, I think this is a good future goal.
https://codereview.chromium.org/2895953003/diff/40001/cc/debug/picture_debug_... File cc/debug/picture_debug_util.cc (right): https://codereview.chromium.org/2895953003/diff/40001/cc/debug/picture_debug_... cc/debug/picture_debug_util.cc:51: encoding_succeeded = gfx::PNGCodec::EncodeBGRASkBitmap(bm, false, &data); Make this take an SkPixmap, too? (Or maybe do that in a separate CL?) https://codereview.chromium.org/2895953003/diff/40001/chrome/renderer/chrome_... File chrome/renderer/chrome_render_frame_observer.cc (right): https://codereview.chromium.org/2895953003/diff/40001/chrome/renderer/chrome_... chrome/renderer/chrome_render_frame_observer.cc:217: if (!bitmap.peekPixels(&pixmap)) { I think you don't want the "!" here?
The CQ bit was checked by msarett@chromium.org to run a CQ dry run
https://codereview.chromium.org/2895953003/diff/40001/cc/debug/picture_debug_... File cc/debug/picture_debug_util.cc (right): https://codereview.chromium.org/2895953003/diff/40001/cc/debug/picture_debug_... cc/debug/picture_debug_util.cc:51: encoding_succeeded = gfx::PNGCodec::EncodeBGRASkBitmap(bm, false, &data); On 2017/06/07 18:15:13, scroggo_chromium wrote: > Make this take an SkPixmap, too? (Or maybe do that in a separate CL?) Definitely for a separate CL. This one already has lots of typing :). https://codereview.chromium.org/2895953003/diff/40001/chrome/renderer/chrome_... File chrome/renderer/chrome_render_frame_observer.cc (right): https://codereview.chromium.org/2895953003/diff/40001/chrome/renderer/chrome_... chrome/renderer/chrome_render_frame_observer.cc:217: if (!bitmap.peekPixels(&pixmap)) { On 2017/06/07 18:15:13, scroggo_chromium wrote: > I think you don't want the "!" here? Oops, good catch.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2895953003/diff/60001/components/suggestions/... File components/suggestions/image_encoder.cc (right): https://codereview.chromium.org/2895953003/diff/60001/components/suggestions/... components/suggestions/image_encoder.cc:26: DCHECK(success); It's too bad these three lines get repeated so much. I suppose the best fix is to (eventually) convert callers away from SkBitmap and we won't need them? https://codereview.chromium.org/2895953003/diff/60001/components/user_manager... File components/user_manager/user_image/user_image.cc (right): https://codereview.chromium.org/2895953003/diff/60001/components/user_manager... components/user_manager/user_image/user_image.cc:30: auto* bitmap_data = reinterpret_cast<unsigned char*>(bitmap.getAddr32(0, 0)); This can be moved into the else case (until we update PNGCodec).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2895953003/diff/60001/components/suggestions/... File components/suggestions/image_encoder.cc (right): https://codereview.chromium.org/2895953003/diff/60001/components/suggestions/... components/suggestions/image_encoder.cc:26: DCHECK(success); On 2017/06/07 18:43:56, scroggo_chromium wrote: > It's too bad these three lines get repeated so much. I suppose the best fix is > to (eventually) convert callers away from SkBitmap and we won't need them? That seems right to me. I considered adding an additional API that took an SkBitmap as a convenience, but I didn't want to encourage more uses of SkBitmap. https://codereview.chromium.org/2895953003/diff/60001/components/user_manager... File components/user_manager/user_image/user_image.cc (right): https://codereview.chromium.org/2895953003/diff/60001/components/user_manager... components/user_manager/user_image/user_image.cc:30: auto* bitmap_data = reinterpret_cast<unsigned char*>(bitmap.getAddr32(0, 0)); On 2017/06/07 18:43:56, scroggo_chromium wrote: > This can be moved into the else case (until we update PNGCodec). Done.
LGTM
The CQ bit was checked by msarett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dcheng@ does the new API look good to you? Just want to get your thoughts before I add owners.
On 2017/06/08 16:38:33, msarett1 wrote: > dcheng@ does the new API look good to you? Just want to get your thoughts > before I add owners. Ping, any thoughts on this?
I didn't go through all the files yet; just some high-level comments (sorry for missing this) https://codereview.chromium.org/2895953003/diff/80001/cc/debug/picture_debug_... File cc/debug/picture_debug_util.cc (right): https://codereview.chromium.org/2895953003/diff/80001/cc/debug/picture_debug_... cc/debug/picture_debug_util.cc:37: const int kJpegQuality = 80; Nit: constexpr https://codereview.chromium.org/2895953003/diff/80001/cc/debug/picture_debug_... cc/debug/picture_debug_util.cc:39: static_cast<size_t>(std::numeric_limits<int>::max())); This DCHECK probably isn't needed anymore. https://codereview.chromium.org/2895953003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_test_utils.cc (right): https://codereview.chromium.org/2895953003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_test_utils.cc:91: bool success = bitmap.peekPixels(&pixmap); Realistically, when can we expect failure? What if the bitmap is 0-width/0-height? https://codereview.chromium.org/2895953003/diff/80001/ui/gfx/codec/jpeg_codec.h File ui/gfx/codec/jpeg_codec.h (right): https://codereview.chromium.org/2895953003/diff/80001/ui/gfx/codec/jpeg_codec... ui/gfx/codec/jpeg_codec.h:51: static bool Encode(const SkPixmap& input, Any reason not to use SkBitmap? This avoids all the callers having to call peekBitmap() and (potentially) checking the result.
https://codereview.chromium.org/2895953003/diff/80001/cc/debug/picture_debug_... File cc/debug/picture_debug_util.cc (right): https://codereview.chromium.org/2895953003/diff/80001/cc/debug/picture_debug_... cc/debug/picture_debug_util.cc:37: const int kJpegQuality = 80; On 2017/06/12 20:35:43, dcheng wrote: > Nit: constexpr Will fix. https://codereview.chromium.org/2895953003/diff/80001/cc/debug/picture_debug_... cc/debug/picture_debug_util.cc:39: static_cast<size_t>(std::numeric_limits<int>::max())); On 2017/06/12 20:35:43, dcheng wrote: > This DCHECK probably isn't needed anymore. Will fix. https://codereview.chromium.org/2895953003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_test_utils.cc (right): https://codereview.chromium.org/2895953003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_test_utils.cc:91: bool success = bitmap.peekPixels(&pixmap); On 2017/06/12 20:35:43, dcheng wrote: > Realistically, when can we expect failure? What if the bitmap is > 0-width/0-height? This will never fail if a version of allocPixels() has already been called and succeeds. Note that allocN32Pixels() above will crash if it fails. https://codereview.chromium.org/2895953003/diff/80001/ui/gfx/codec/jpeg_codec.h File ui/gfx/codec/jpeg_codec.h (right): https://codereview.chromium.org/2895953003/diff/80001/ui/gfx/codec/jpeg_codec... ui/gfx/codec/jpeg_codec.h:51: static bool Encode(const SkPixmap& input, On 2017/06/12 20:35:43, dcheng wrote: > Any reason not to use SkBitmap? This avoids all the callers having to call > peekBitmap() and (potentially) checking the result. This is a good question, especially because there are a fair amount of duplicated peekPixels() calls in the clients. SkBitmap is holds a ref to its pixels. SkPixmap is a more lightweight container for a pointer and some info. SkPixmap is the preferred API (from a Skia perspective) and is more versatile. Ex: if we don't start with a SkBitmap, we don't need to install into an SkBitmap. I think for these reasons we definitely want a version that takes SkPixmap. I didn't write a version that takes SkBitmap because Skia is trying to move clients away from using SkBitmap... but I can see it is convenient for a lot of callers. I'm ok with adding another entry point that takes an SkBitmap (and simplifying some of the callers). What do you think?
https://codereview.chromium.org/2895953003/diff/80001/ui/gfx/codec/jpeg_codec.h File ui/gfx/codec/jpeg_codec.h (right): https://codereview.chromium.org/2895953003/diff/80001/ui/gfx/codec/jpeg_codec... ui/gfx/codec/jpeg_codec.h:51: static bool Encode(const SkPixmap& input, On 2017/06/12 20:57:56, msarett1 wrote: > On 2017/06/12 20:35:43, dcheng wrote: > > Any reason not to use SkBitmap? This avoids all the callers having to call > > peekBitmap() and (potentially) checking the result. > > This is a good question, especially because there are a fair amount of > duplicated peekPixels() calls in the clients. > > SkBitmap is holds a ref to its pixels. SkPixmap is a more lightweight container > for a pointer and some info. > > SkPixmap is the preferred API (from a Skia perspective) and is more versatile. > Ex: if we don't start with a SkBitmap, we don't need to install into an > SkBitmap. I think for these reasons we definitely want a version that takes > SkPixmap. > > I didn't write a version that takes SkBitmap because Skia is trying to move > clients away from using SkBitmap... but I can see it is convenient for a lot of > callers. > > I'm ok with adding another entry point that takes an SkBitmap (and simplifying > some of the callers). What do you think? Adding an overload is fine. I'd be ok with SkPixmap being the only version if SkBitmap wasn't so prevalent in Chrome... but it is. So until then, let's have both to make things a bit simpler.
Sounds good to me, I've added a version that takes an SkBitmap. https://codereview.chromium.org/2895953003/diff/80001/cc/debug/picture_debug_... File cc/debug/picture_debug_util.cc (right): https://codereview.chromium.org/2895953003/diff/80001/cc/debug/picture_debug_... cc/debug/picture_debug_util.cc:37: const int kJpegQuality = 80; On 2017/06/12 20:57:56, msarett1 wrote: > On 2017/06/12 20:35:43, dcheng wrote: > > Nit: constexpr > > Will fix. Done. https://codereview.chromium.org/2895953003/diff/80001/cc/debug/picture_debug_... cc/debug/picture_debug_util.cc:39: static_cast<size_t>(std::numeric_limits<int>::max())); On 2017/06/12 20:57:56, msarett1 wrote: > On 2017/06/12 20:35:43, dcheng wrote: > > This DCHECK probably isn't needed anymore. > > Will fix. Done. https://codereview.chromium.org/2895953003/diff/80001/ui/gfx/codec/jpeg_codec.h File ui/gfx/codec/jpeg_codec.h (right): https://codereview.chromium.org/2895953003/diff/80001/ui/gfx/codec/jpeg_codec... ui/gfx/codec/jpeg_codec.h:51: static bool Encode(const SkPixmap& input, On 2017/06/12 21:00:15, dcheng wrote: > On 2017/06/12 20:57:56, msarett1 wrote: > > On 2017/06/12 20:35:43, dcheng wrote: > > > Any reason not to use SkBitmap? This avoids all the callers having to call > > > peekBitmap() and (potentially) checking the result. > > > > This is a good question, especially because there are a fair amount of > > duplicated peekPixels() calls in the clients. > > > > SkBitmap is holds a ref to its pixels. SkPixmap is a more lightweight > container > > for a pointer and some info. > > > > SkPixmap is the preferred API (from a Skia perspective) and is more versatile. > > > Ex: if we don't start with a SkBitmap, we don't need to install into an > > SkBitmap. I think for these reasons we definitely want a version that takes > > SkPixmap. > > > > I didn't write a version that takes SkBitmap because Skia is trying to move > > clients away from using SkBitmap... but I can see it is convenient for a lot > of > > callers. > > > > I'm ok with adding another entry point that takes an SkBitmap (and simplifying > > some of the callers). What do you think? > > Adding an overload is fine. I'd be ok with SkPixmap being the only version if > SkBitmap wasn't so prevalent in Chrome... but it is. So until then, let's have > both to make things a bit simpler. Done.
lgtm
The CQ bit was checked by msarett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Great, adding OWNERS now. Please take a look. reveman@: cc/debug/picture_debug_util.cc bshe@: chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_test_utils.cc jochen@: chrome/renderer/chrome_render_frame_observer.cc jochen@: components/ jochen@: pdf/ dgozman@chromium.org@: content/browser/devtools/devtools_frame_trace_recorder.cc tbarzic@chromium.org@: extensions/browser/api/web_contents_capture_client.cc sandersd@: media/ rockot@: services/data_decoder/image_decoder_impl_unittest.cc rsesek@chromium.org@: ui/gfx/image/image_util.cc
image_util LGTM
On 2017/06/13 14:31:10, Robert Sesek wrote: > image_util LGTM wallpaper lgtm
devtools lgtm
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
media/ lgtm.
lgtm
jochen@, reveman@, can you please take a look? reveman@: cc/debug/picture_debug_util.cc jochen@: chrome/renderer/chrome_render_frame_observer.cc jochen@: components/ jochen@: pdf/
The CQ bit was checked by msarett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Use SkJpegEncoder in gfx jpeg_codec BUG=724616 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Use SkJpegEncoder in gfx jpeg_codec TBR=jochen@chromium.org TBR=reveman@chromium.org BUG=724616 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by msarett@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@chromium.org Link to the patchset: https://codereview.chromium.org/2895953003/#ps120001 (title: "Remove brackets")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1497536981817540,
"parent_rev": "9e8c59957706918a5fbeb97ea4a338271bbc5213", "commit_rev":
"7060f1277a34592bc1f6c16762ccb0345669b073"}
Message was sent while issue was closed.
Description was changed from ========== Use SkJpegEncoder in gfx jpeg_codec TBR=jochen@chromium.org TBR=reveman@chromium.org BUG=724616 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Use SkJpegEncoder in gfx jpeg_codec TBR=jochen@chromium.org TBR=reveman@chromium.org BUG=724616 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2895953003 Cr-Commit-Position: refs/heads/master@{#479694} Committed: https://chromium.googlesource.com/chromium/src/+/7060f1277a34592bc1f6c16762cc... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/7060f1277a34592bc1f6c16762cc...
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
Drive-by chrome/ and pdf/ lgtm https://codereview.chromium.org/2895953003/diff/120001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2895953003/diff/120001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1424: SkImageInfo info = SkImageInfo::Make( We are doing a bit more work than before for a branch that may not be taken. Let me see if I can shuffle the code around a bit. |
