Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
262263003:
Always use AwesomeResampling for lazy decoded images. (Closed)
|
Created:
6 years, 7 months ago by reveman Modified:
6 years, 7 months ago CC:
blink-reviews, jamesr, krit, jbroman, danakj, Rik, Stephen Chennney, pdr., rwlbuis Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionAlways use AwesomeResampling for lazy decoded images.
We don't know the real destination scale of lazy decoded images
and should not be making filter choices based on the
transformation matrix here.
Instead use AwesomeResampling for all lazy decoded images and
leaves it up to skia to decided if a more efficient filter can
be used at rasterization time.
BUG=368078
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173660
Patch Set 1 #
Total comments: 14
Patch Set 2 : address review feedback #
Total comments: 2
Patch Set 3 : revert changes in last patch set #Patch Set 4 : Add required rebaselines to TestExpectations #Patch Set 5 : move TestExpectations change to avoid conflicts #Messages
Total messages: 28 (0 generated)
On 2014/05/05 22:26:12, reveman wrote: I don't think this is right. This change will force Android to use Awsome resampling.
Sorry reading the code again it is right but quite confusing. Comments follow. https://codereview.chromium.org/262263003/diff/1/Source/platform/graphics/ski... File Source/platform/graphics/skia/NativeImageSkia.cpp (right): https://codereview.chromium.org/262263003/diff/1/Source/platform/graphics/ski... Source/platform/graphics/skia/NativeImageSkia.cpp:380: ResamplingMode resampling; I suggest this be changed to default to AwsomeResampling. https://codereview.chromium.org/262263003/diff/1/Source/platform/graphics/ski... Source/platform/graphics/skia/NativeImageSkia.cpp:386: resampling = AwesomeResampling; Reading this code again. I think this code is correct but confusing. I suggest we do this at line 387: } else if (!isLazyDecoded) { // Take into account .... } https://codereview.chromium.org/262263003/diff/1/Source/platform/graphics/ski... Source/platform/graphics/skia/NativeImageSkia.cpp:410: bool useBicubicFilter = resampling == AwesomeResampling && isLazyDecoded; Because now you want to use skia for all resampling there's no need for this variable. Please just delete this. https://codereview.chromium.org/262263003/diff/1/Source/platform/graphics/ski... Source/platform/graphics/skia/NativeImageSkia.cpp:414: if (resampling == AwesomeResampling && !useBicubicFilter) { Use !isLasyDecoded instead. https://codereview.chromium.org/262263003/diff/1/Source/platform/graphics/ski... Source/platform/graphics/skia/NativeImageSkia.cpp:478: ResamplingMode resampling; Same here. Default to AwsomeResampling please. https://codereview.chromium.org/262263003/diff/1/Source/platform/graphics/ski... Source/platform/graphics/skia/NativeImageSkia.cpp:483: else I suggest this be else if (!isLazyDecoded).
https://codereview.chromium.org/262263003/diff/1/Source/platform/graphics/ski... File Source/platform/graphics/skia/NativeImageSkia.cpp (right): https://codereview.chromium.org/262263003/diff/1/Source/platform/graphics/ski... Source/platform/graphics/skia/NativeImageSkia.cpp:410: bool useBicubicFilter = resampling == AwesomeResampling && isLazyDecoded; On 2014/05/06 19:05:28, Alpha wrote: > Because now you want to use skia for all resampling there's no need for this > variable. Please just delete this. Whooops! This particular comment is wrong. Please ignore it. https://codereview.chromium.org/262263003/diff/1/Source/platform/graphics/ski... Source/platform/graphics/skia/NativeImageSkia.cpp:414: if (resampling == AwesomeResampling && !useBicubicFilter) { On 2014/05/06 19:05:28, Alpha wrote: > Use !isLasyDecoded instead. Whooops! This particular comment is wrong too. Please ignore it.
PTAL https://codereview.chromium.org/262263003/diff/1/Source/platform/graphics/ski... File Source/platform/graphics/skia/NativeImageSkia.cpp (right): https://codereview.chromium.org/262263003/diff/1/Source/platform/graphics/ski... Source/platform/graphics/skia/NativeImageSkia.cpp:380: ResamplingMode resampling; On 2014/05/06 19:05:28, Alpha wrote: > I suggest this be changed to default to AwsomeResampling. Done. https://codereview.chromium.org/262263003/diff/1/Source/platform/graphics/ski... Source/platform/graphics/skia/NativeImageSkia.cpp:386: resampling = AwesomeResampling; On 2014/05/06 19:05:28, Alpha wrote: > Reading this code again. I think this code is correct but confusing. > > I suggest we do this at line 387: > > } else if (!isLazyDecoded) { > // Take into account .... > } Done. https://codereview.chromium.org/262263003/diff/1/Source/platform/graphics/ski... Source/platform/graphics/skia/NativeImageSkia.cpp:478: ResamplingMode resampling; On 2014/05/06 19:05:28, Alpha wrote: > Same here. Default to AwsomeResampling please. Done. https://codereview.chromium.org/262263003/diff/1/Source/platform/graphics/ski... Source/platform/graphics/skia/NativeImageSkia.cpp:483: else On 2014/05/06 19:05:28, Alpha wrote: > I suggest this be else if (!isLazyDecoded). Done.
lgtm
+senorblanco for platform/ owner
Can you please check that this doesn't defeat the timer-based resize (e.g., http://ie.microsoft.com/TestDrive/Performance/FlyingImages/Default.html : performance should be fast but bilinear, then center the mouse and stop the images: should be slow but Lanczos). Thanks! https://codereview.chromium.org/262263003/diff/1/Source/platform/graphics/ski... File Source/platform/graphics/skia/NativeImageSkia.cpp (right): https://codereview.chromium.org/262263003/diff/1/Source/platform/graphics/ski... Source/platform/graphics/skia/NativeImageSkia.cpp:386: resampling = AwesomeResampling; On 2014/05/06 19:05:28, Alpha wrote: > Reading this code again. I think this code is correct but confusing. > > I suggest we do this at line 387: > > } else if (!isLazyDecoded) { > // Take into account .... > } Sorry, but I disagree. I much prefer the way David has written it here, since all code paths explicitly assign a value, rather than relying on the default (which is only used in the isLazyDecoded case). https://codereview.chromium.org/262263003/diff/20001/Source/platform/graphics... File Source/platform/graphics/skia/NativeImageSkia.cpp (right): https://codereview.chromium.org/262263003/diff/20001/Source/platform/graphics... Source/platform/graphics/skia/NativeImageSkia.cpp:476: ResamplingMode resampling = AwesomeResampling; Same here.
Tried the flying images demo but didn't notice a difference between with or without this patch. How is the timer-based resize code supposed to interact with this code? I don't see anything in computeResamplingMode that could be affected by some timer based logic. https://codereview.chromium.org/262263003/diff/1/Source/platform/graphics/ski... File Source/platform/graphics/skia/NativeImageSkia.cpp (right): https://codereview.chromium.org/262263003/diff/1/Source/platform/graphics/ski... Source/platform/graphics/skia/NativeImageSkia.cpp:386: resampling = AwesomeResampling; On 2014/05/07 18:16:31, Stephen White wrote: > On 2014/05/06 19:05:28, Alpha wrote: > > Reading this code again. I think this code is correct but confusing. > > > > I suggest we do this at line 387: > > > > } else if (!isLazyDecoded) { > > // Take into account .... > > } > > Sorry, but I disagree. I much prefer the way David has written it here, since > all code paths explicitly assign a value, rather than relying on the default > (which is only used in the isLazyDecoded case). Done. https://codereview.chromium.org/262263003/diff/20001/Source/platform/graphics... File Source/platform/graphics/skia/NativeImageSkia.cpp (right): https://codereview.chromium.org/262263003/diff/20001/Source/platform/graphics... Source/platform/graphics/skia/NativeImageSkia.cpp:476: ResamplingMode resampling = AwesomeResampling; On 2014/05/07 18:16:31, Stephen White wrote: > Same here. Done.
On 2014/05/07 19:00:45, reveman wrote: > Tried the flying images demo but didn't notice a difference between with or > without this patch. How is the timer-based resize code supposed to interact with > this code? I don't see anything in computeResamplingMode that could be affected > by some timer based logic. Thanks. The timer is up in core (ImageQualityController, called by RenderBoxModelObject::chooseInterpolationQuality()), and it sets the GraphicsContext's InterpolationQuality. It looks like this is applied subsequently in limitResmaplingMode(), so we should be fine; I just forgot where it was applied in the end. https://codereview.chromium.org/262263003/diff/1/Source/platform/graphics/ski... > File Source/platform/graphics/skia/NativeImageSkia.cpp (right): > > https://codereview.chromium.org/262263003/diff/1/Source/platform/graphics/ski... > Source/platform/graphics/skia/NativeImageSkia.cpp:386: resampling = > AwesomeResampling; > On 2014/05/07 18:16:31, Stephen White wrote: > > On 2014/05/06 19:05:28, Alpha wrote: > > > Reading this code again. I think this code is correct but confusing. > > > > > > I suggest we do this at line 387: > > > > > > } else if (!isLazyDecoded) { > > > // Take into account .... > > > } > > > > Sorry, but I disagree. I much prefer the way David has written it here, since > > all code paths explicitly assign a value, rather than relying on the default > > (which is only used in the isLazyDecoded case). > > Done. > > https://codereview.chromium.org/262263003/diff/20001/Source/platform/graphics... > File Source/platform/graphics/skia/NativeImageSkia.cpp (right): > > https://codereview.chromium.org/262263003/diff/20001/Source/platform/graphics... > Source/platform/graphics/skia/NativeImageSkia.cpp:476: ResamplingMode resampling > = AwesomeResampling; > On 2014/05/07 18:16:31, Stephen White wrote: > > Same here. > > Done. LGTM
The CQ bit was checked by reveman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/reveman@chromium.org/262263003/40001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: linux_blink_dbg on tryserver.blink linux_blink_rel on tryserver.blink mac_blink_rel on tryserver.blink
FYI, CQ is re-trying this CL (attempt #2). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: linux_blink_dbg on tryserver.blink linux_blink_rel on tryserver.blink mac_blink_rel on tryserver.blink win_blink_rel on tryserver.blink
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink win_blink_rel on tryserver.blink
The CQ bit was checked by reveman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/reveman@chromium.org/262263003/40001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/6947) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6689) mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/6189) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/6657)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6700)
The CQ bit was checked by reveman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/reveman@chromium.org/262263003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for LayoutTests/TestExpectations: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file LayoutTests/TestExpectations Hunk #1 FAILED at 1144. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/TestExpectations.rej Patch: LayoutTests/TestExpectations Index: LayoutTests/TestExpectations diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations index a040c3b73a7f1151b091db60e8fce6e60be858ce..b7a0ce43b4c9168f5362b32dfe9d974a57e2592f 100644 --- a/LayoutTests/TestExpectations +++ b/LayoutTests/TestExpectations @@ -1144,3 +1144,7 @@ crbug.com/370789 fast/dom/document-contentType-data-uri.html [ Failure ] crbug.com/370814 fast/html/imports/import-custom-element-in-grandchild.html [ Pass Failure ] crbug.com/370906 fast/dom/StyleSheet/gc-declaration-parent-rule.html [ Failure ] + +# Deferred image decoding tests needs rebaseline after adjusting filter choice. +crbug.com/368078 virtual/deferred/fast/images/exif-orientation-height-image-document.html [ NeedsRebaseline ] +crbug.com/368078 virtual/deferred/fast/images/exif-orientation-image-document.html [ NeedsRebaseline ]
The CQ bit was checked by reveman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/reveman@chromium.org/262263003/80001
Message was sent while issue was closed.
Change committed as 173660 |
