Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1682803003:
cc: ImageDecodes: handle low quality filters. (Closed)
Descriptioncc: ImageDecodes: handle low quality filters.
This patch pins low quality interpolation filters, which means that
it is now handled by the image decode controller.
R=enne
BUG=558063
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/6372e1669779a7b82bd130c55dca71d39f23643f
Cr-Commit-Position: refs/heads/master@{#378355}
Patch Set 1 #Patch Set 2 : remove old code #
Total comments: 4
Patch Set 3 : #
Total comments: 6
Patch Set 4 : rebase #Patch Set 5 : rebase #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : update #Patch Set 9 : #
Messages
Total messages: 26 (10 generated)
Description was changed from ========== cc: ImageDecodes: handle low quality filters. This patch pins low quality interpolation filters, which means that it is now handled by the image decode controller. R=enne BUG=558063 ========== to ========== cc: ImageDecodes: handle low quality filters. This patch pins low quality interpolation filters, which means that it is now handled by the image decode controller. R=enne BUG=558063 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Please take a look (meant to send it out yesterday, oops)
https://codereview.chromium.org/1682803003/diff/20001/cc/tiles/image_decode_c... File cc/tiles/image_decode_controller.cc (right): https://codereview.chromium.org/1682803003/diff/20001/cc/tiles/image_decode_c... cc/tiles/image_decode_controller.cc:801: hash_ = image_id_; Do you need to hash this to get a good distribution? https://codereview.chromium.org/1682803003/diff/20001/cc/tiles/image_decode_c... File cc/tiles/image_decode_controller.h (right): https://codereview.chromium.org/1682803003/diff/20001/cc/tiles/image_decode_c... cc/tiles/image_decode_controller.h:89: gfx::Size original_size_; I'm not sure I follow why you need two sizes here? If you can use the original decode, isn't target_size == original_size? (Or can it be?)
https://codereview.chromium.org/1682803003/diff/20001/cc/tiles/image_decode_c... File cc/tiles/image_decode_controller.cc (right): https://codereview.chromium.org/1682803003/diff/20001/cc/tiles/image_decode_c... cc/tiles/image_decode_controller.cc:801: hash_ = image_id_; On 2016/02/11 00:06:06, enne wrote: > Do you need to hash this to get a good distribution? Sure :D https://codereview.chromium.org/1682803003/diff/20001/cc/tiles/image_decode_c... File cc/tiles/image_decode_controller.h (right): https://codereview.chromium.org/1682803003/diff/20001/cc/tiles/image_decode_c... cc/tiles/image_decode_controller.h:89: gfx::Size original_size_; On 2016/02/11 00:06:06, enne wrote: > I'm not sure I follow why you need two sizes here? If you can use the original > decode, isn't target_size == original_size? (Or can it be?) Hmm.. I'm going to confirm, but yeah I think I can use target_size (ie I can set target size to be the full image rect in the case where we're using the original decode)
PTAL
ericrk@chromium.org changed reviewers: + ericrk@chromium.org
https://codereview.chromium.org/1682803003/diff/40001/cc/tiles/image_decode_c... File cc/tiles/image_decode_controller.cc (right): https://codereview.chromium.org/1682803003/diff/40001/cc/tiles/image_decode_c... cc/tiles/image_decode_controller.cc:380: DrawWithImageFinished(original_size_draw_image, decoded_draw_image); not really specific to this change - but would it work well to have DecodedDrawImage auto call this when it goes out of scope? Or have a ScopedDrawImage or something? https://codereview.chromium.org/1682803003/diff/40001/cc/tiles/image_decode_c... cc/tiles/image_decode_controller.cc:782: bool can_use_original_decode = nit: does it make more sense to call it "needs_caching" not "scale_needs_caching" - it's weird that "scale_needs_caching" can be true when scale_is_required is false...
lgtm % ericrk
https://codereview.chromium.org/1682803003/diff/40001/cc/tiles/image_decode_c... File cc/tiles/image_decode_controller.cc (right): https://codereview.chromium.org/1682803003/diff/40001/cc/tiles/image_decode_c... cc/tiles/image_decode_controller.cc:380: DrawWithImageFinished(original_size_draw_image, decoded_draw_image); On 2016/02/11 01:21:50, ericrk wrote: > not really specific to this change - but would it work well to have > DecodedDrawImage auto call this when it goes out of scope? Or have a > ScopedDrawImage or something? Yeah I have that in the ImageHijackCanvas. I figured it's not too bad here, since there are only two exits out of the function. For the time being I'm trying to limit the amount of classes that I introduce, but maybe we can revisit this later. https://codereview.chromium.org/1682803003/diff/40001/cc/tiles/image_decode_c... cc/tiles/image_decode_controller.cc:782: bool can_use_original_decode = On 2016/02/11 01:21:50, ericrk wrote: > nit: does it make more sense to call it "needs_caching" not > "scale_needs_caching" - it's weird that "scale_needs_caching" can be true when > scale_is_required is false... Hmm well if scale is not required, then scale_needs_caching is false, because the filter quality would be low. It's a bit weird for me to call it needs_caching, because the low filter quality (ie the original decode) might need caching, but not the scaled version... hence scale_needs_caching.. Wdyt?
lgtm lg https://codereview.chromium.org/1682803003/diff/40001/cc/tiles/image_decode_c... File cc/tiles/image_decode_controller.cc (right): https://codereview.chromium.org/1682803003/diff/40001/cc/tiles/image_decode_c... cc/tiles/image_decode_controller.cc:380: DrawWithImageFinished(original_size_draw_image, decoded_draw_image); On 2016/02/17 21:21:03, vmpstr wrote: > On 2016/02/11 01:21:50, ericrk wrote: > > not really specific to this change - but would it work well to have > > DecodedDrawImage auto call this when it goes out of scope? Or have a > > ScopedDrawImage or something? > > Yeah I have that in the ImageHijackCanvas. I figured it's not too bad here, > since there are only two exits out of the function. > > For the time being I'm trying to limit the amount of classes that I introduce, > but maybe we can revisit this later. sounds good. https://codereview.chromium.org/1682803003/diff/40001/cc/tiles/image_decode_c... cc/tiles/image_decode_controller.cc:782: bool can_use_original_decode = On 2016/02/17 21:21:03, vmpstr wrote: > On 2016/02/11 01:21:50, ericrk wrote: > > nit: does it make more sense to call it "needs_caching" not > > "scale_needs_caching" - it's weird that "scale_needs_caching" can be true when > > scale_is_required is false... > > Hmm well if scale is not required, then scale_needs_caching is false, because > the filter quality would be low. It's a bit weird for me to call it > needs_caching, because the low filter quality (ie the original decode) might > need caching, but not the scaled version... hence scale_needs_caching.. Wdyt? Wouldn't a high quality image without scaling set scale_needs_caching to true and scale_is_required_to false? Might just be missing another factor here. Not a big deal though :D
The CQ bit was checked by vmpstr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org, ericrk@chromium.org Link to the patchset: https://codereview.chromium.org/1682803003/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1682803003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1682803003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...)
On 2016/02/25 01:49:45, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_blink_rel on tryserver.blink (JOB_FAILED, > http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...) lgtm
The CQ bit was checked by vmpstr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1682803003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1682803003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by vmpstr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org Link to the patchset: https://codereview.chromium.org/1682803003/#ps160001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1682803003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1682803003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== cc: ImageDecodes: handle low quality filters. This patch pins low quality interpolation filters, which means that it is now handled by the image decode controller. R=enne BUG=558063 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: ImageDecodes: handle low quality filters. This patch pins low quality interpolation filters, which means that it is now handled by the image decode controller. R=enne BUG=558063 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/6372e1669779a7b82bd130c55dca71d39f23643f Cr-Commit-Position: refs/heads/master@{#378355} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/6372e1669779a7b82bd130c55dca71d39f23643f Cr-Commit-Position: refs/heads/master@{#378355} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
