Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1566193002:
Always antialias clips in ClipDisplayItem. (Closed)
|
Created:
4 years, 11 months ago by chrishtr Modified:
4 years, 11 months ago CC:
blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, cc-bugs_chromium.org, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, jbroman, Justin Novosad, kinuko+watch, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney, vmpstr+blinkwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAlways antialias clips in ClipDisplayItem.
BUG=390040
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/ce3dce9ffa916d60c9b6d70f427a244a80358ccb
Cr-Commit-Position: refs/heads/master@{#369246}
Patch Set 1 #
Total comments: 2
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Messages
Total messages: 36 (16 generated)
Description was changed from ========== none none BUG= ========== to ========== none none BUG= CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Description was changed from ========== none none BUG= CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Always antialias clips in LayerClipRecorder. BUG=390040 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
chrishtr@chromium.org changed reviewers: + ajuma@chromium.org, fmalita@chromium.org
Pixel diff results: https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r... https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel... https://storage.googleapis.com/chromium-layout-test-archives/win_chromium_rel... Seems to be a strict improvement, as well as fixing the referenced bug.
Description was changed from ========== Always antialias clips in LayerClipRecorder. BUG=390040 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Always antialias clips in ClipDisplayItem. BUG=390040 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
On 2016/01/08 00:43:51, chrishtr wrote: > Pixel diff results: > > https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r... > > https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel... > > https://storage.googleapis.com/chromium-layout-test-archives/win_chromium_rel... > > Seems to be a strict improvement, as well as fixing the referenced bug. Not saying this is wrong but just pointing out this might change UI behaviour too.
On 2016/01/08 at 00:52:32, danakj wrote: > On 2016/01/08 00:43:51, chrishtr wrote: > > Pixel diff results: > > > > https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r... > > > > https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel... > > > > https://storage.googleapis.com/chromium-layout-test-archives/win_chromium_rel... > > > > Seems to be a strict improvement, as well as fixing the referenced bug. > > Not saying this is wrong but just pointing out this might change UI behaviour too. Right...but is it the case that UI always has axis-aligned and pixel-aligned elements? If so this should have no effect. If not, is it reasonable to commit this and see?
On Thu, Jan 7, 2016 at 5:05 PM, <chrishtr@chromium.org> wrote: > On 2016/01/08 at 00:52:32, danakj wrote: > >> On 2016/01/08 00:43:51, chrishtr wrote: >> > Pixel diff results: >> > >> > >> > > https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r... > >> > >> > >> > > https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel... > >> > >> > >> > > https://storage.googleapis.com/chromium-layout-test-archives/win_chromium_rel... > >> > >> > Seems to be a strict improvement, as well as fixing the referenced bug. >> > > Not saying this is wrong but just pointing out this might change UI >> behaviour >> > too. > > Right...but is it the case that UI always has axis-aligned and > pixel-aligned > elements? If so this should have no effect. If not, > is it reasonable to commit this and see? They're probably axis aligned? I don't know tbh. It should be fine to land this for the UI, it's good to know that when axis aligned this has no effect. -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Thu, Jan 7, 2016 at 5:05 PM, <chrishtr@chromium.org> wrote: > On 2016/01/08 at 00:52:32, danakj wrote: > >> On 2016/01/08 00:43:51, chrishtr wrote: >> > Pixel diff results: >> > >> > >> > > https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r... > >> > >> > >> > > https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel... > >> > >> > >> > > https://storage.googleapis.com/chromium-layout-test-archives/win_chromium_rel... > >> > >> > Seems to be a strict improvement, as well as fixing the referenced bug. >> > > Not saying this is wrong but just pointing out this might change UI >> behaviour >> > too. > > Right...but is it the case that UI always has axis-aligned and > pixel-aligned > elements? If so this should have no effect. If not, > is it reasonable to commit this and see? They're probably axis aligned? I don't know tbh. It should be fine to land this for the UI, it's good to know that when axis aligned this has no effect. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/01/08 at 01:30:18, chromium-reviews wrote: > On Thu, Jan 7, 2016 at 5:05 PM, <chrishtr@chromium.org> wrote: > > > On 2016/01/08 at 00:52:32, danakj wrote: > > > >> On 2016/01/08 00:43:51, chrishtr wrote: > >> > Pixel diff results: > >> > > >> > > >> > > > > https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r... > > > >> > > >> > > >> > > > > https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel... > > > >> > > >> > > >> > > > > https://storage.googleapis.com/chromium-layout-test-archives/win_chromium_rel... > > > >> > > >> > Seems to be a strict improvement, as well as fixing the referenced bug. > >> > > > > Not saying this is wrong but just pointing out this might change UI > >> behaviour > >> > > too. > > > > Right...but is it the case that UI always has axis-aligned and > > pixel-aligned > > elements? If so this should have no effect. If not, > > is it reasonable to commit this and see? > > > They're probably axis aligned? I don't know tbh. It should be fine to land > this for the UI, it's good to know that when axis aligned this has no > effect. Axis and pixel. Florin, please confirm my understanding. :) > > -- > You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. >
https://codereview.chromium.org/1566193002/diff/1/cc/playback/clip_display_it... File cc/playback/clip_display_item.cc (right): https://codereview.chromium.org/1566193002/diff/1/cc/playback/clip_display_it... cc/playback/clip_display_item.cc:66: SkRegion::kIntersect_Op, true); Please use a variable for the bool rather than just a literal, like we were doing before in the 'else' case below.
I think this makes sense, AFAIK we now always use AA for all other draws/clips. LGTM % convincing ourselves we don't mess up UI :) On 2016/01/08 01:32:11, chrishtr wrote: > On 2016/01/08 at 01:30:18, chromium-reviews wrote: > > On Thu, Jan 7, 2016 at 5:05 PM, <mailto:chrishtr@chromium.org> wrote: > > > > > On 2016/01/08 at 00:52:32, danakj wrote: > > > > > >> On 2016/01/08 00:43:51, chrishtr wrote: > > >> > Seems to be a strict improvement, as well as fixing the referenced bug. > > >> > > > > > > Not saying this is wrong but just pointing out this might change UI > > >> behaviour > > >> > > > too. > > > > > > Right...but is it the case that UI always has axis-aligned and > > > pixel-aligned > > > elements? If so this should have no effect. If not, > > > is it reasonable to commit this and see? > > > > > > They're probably axis aligned? I don't know tbh. It should be fine to land > > this for the UI, it's good to know that when axis aligned this has no > > effect. > > Axis and pixel. Florin, please confirm my understanding. :) Right: when the edges are pixel (and axis) aligned in device space, there's no anti-aliasing going on - so hard clipRect == soft clipRect. I would also hope that UI is aligning its clips (or, if not, soft clips produce better results), but if that's not the case we could add a ClipDisplayItem AA param to let clients specify the desired behavior. What's the best way to test the impact on UI? AFAIK we don't have automated tests. Just poke around in a chromeos build?
On Fri, Jan 8, 2016 at 7:55 AM, <fmalita@chromium.org> wrote: > I think this makes sense, AFAIK we now always use AA for all other > draws/clips. > > LGTM % convincing ourselves we don't mess up UI :) > > On 2016/01/08 01:32:11, chrishtr wrote: > >> On 2016/01/08 at 01:30:18, chromium-reviews wrote: >> > On Thu, Jan 7, 2016 at 5:05 PM, <mailto:chrishtr@chromium.org> wrote: >> > >> > > On 2016/01/08 at 00:52:32, danakj wrote: >> > > >> > >> On 2016/01/08 00:43:51, chrishtr wrote: >> > >> > Seems to be a strict improvement, as well as fixing the referenced >> bug. >> > >> >> > > >> > > Not saying this is wrong but just pointing out this might change UI >> > >> behaviour >> > >> >> > > too. >> > > >> > > Right...but is it the case that UI always has axis-aligned and >> > > pixel-aligned >> > > elements? If so this should have no effect. If not, >> > > is it reasonable to commit this and see? >> > >> > >> > They're probably axis aligned? I don't know tbh. It should be fine to >> land >> > this for the UI, it's good to know that when axis aligned this has no >> > effect. >> > > Axis and pixel. Florin, please confirm my understanding. :) >> > > Right: when the edges are pixel (and axis) aligned in device space, > there's no > anti-aliasing going on - so hard clipRect == soft clipRect. > > I would also hope that UI is aligning its clips (or, if not, soft clips > produce > better results), but if that's not the case we could add a ClipDisplayItem > AA > param to let clients specify the desired behavior. > > What's the best way to test the impact on UI? AFAIK we don't have > automated > tests. Just poke around in a chromeos build? > Maybe, or look at what callsites use this and see what UI elements they correspond to. Maybe look at those when DSF=1.5 or 2 or so. > > https://codereview.chromium.org/1566193002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Fri, Jan 8, 2016 at 7:55 AM, <fmalita@chromium.org> wrote: > I think this makes sense, AFAIK we now always use AA for all other > draws/clips. > > LGTM % convincing ourselves we don't mess up UI :) > > On 2016/01/08 01:32:11, chrishtr wrote: > >> On 2016/01/08 at 01:30:18, chromium-reviews wrote: >> > On Thu, Jan 7, 2016 at 5:05 PM, <mailto:chrishtr@chromium.org> wrote: >> > >> > > On 2016/01/08 at 00:52:32, danakj wrote: >> > > >> > >> On 2016/01/08 00:43:51, chrishtr wrote: >> > >> > Seems to be a strict improvement, as well as fixing the referenced >> bug. >> > >> >> > > >> > > Not saying this is wrong but just pointing out this might change UI >> > >> behaviour >> > >> >> > > too. >> > > >> > > Right...but is it the case that UI always has axis-aligned and >> > > pixel-aligned >> > > elements? If so this should have no effect. If not, >> > > is it reasonable to commit this and see? >> > >> > >> > They're probably axis aligned? I don't know tbh. It should be fine to >> land >> > this for the UI, it's good to know that when axis aligned this has no >> > effect. >> > > Axis and pixel. Florin, please confirm my understanding. :) >> > > Right: when the edges are pixel (and axis) aligned in device space, > there's no > anti-aliasing going on - so hard clipRect == soft clipRect. > > I would also hope that UI is aligning its clips (or, if not, soft clips > produce > better results), but if that's not the case we could add a ClipDisplayItem > AA > param to let clients specify the desired behavior. > > What's the best way to test the impact on UI? AFAIK we don't have > automated > tests. Just poke around in a chromeos build? > Maybe, or look at what callsites use this and see what UI elements they correspond to. Maybe look at those when DSF=1.5 or 2 or so. > > https://codereview.chromium.org/1566193002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1566193002/diff/1/cc/playback/clip_display_it... File cc/playback/clip_display_item.cc (right): https://codereview.chromium.org/1566193002/diff/1/cc/playback/clip_display_it... cc/playback/clip_display_item.cc:66: SkRegion::kIntersect_Op, true); On 2016/01/08 at 14:15:04, ajuma wrote: > Please use a variable for the bool rather than just a literal, like we were doing before in the 'else' case below. Done
On 2016/01/12 23:43:14, chrishtr wrote: > https://codereview.chromium.org/1566193002/diff/1/cc/playback/clip_display_it... > File cc/playback/clip_display_item.cc (right): > > https://codereview.chromium.org/1566193002/diff/1/cc/playback/clip_display_it... > cc/playback/clip_display_item.cc:66: SkRegion::kIntersect_Op, true); > On 2016/01/08 at 14:15:04, ajuma wrote: > > Please use a variable for the bool rather than just a literal, like we were > doing before in the 'else' case below. > > Done Thanks, lgtm.
I surveyed the UI code, and found only integers when looking at a lot of the code. This CL looks safe to me.
The CQ bit was checked by chrishtr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org Link to the patchset: https://codereview.chromium.org/1566193002/#ps40001 (title: " ")
The CQ bit was unchecked by chrishtr@chromium.org
The CQ bit was checked by chrishtr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org, fmalita@chromium.org Link to the patchset: https://codereview.chromium.org/1566193002/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1566193002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1566193002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by chrishtr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org, fmalita@chromium.org Link to the patchset: https://codereview.chromium.org/1566193002/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1566193002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1566193002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by chrishtr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org, fmalita@chromium.org Link to the patchset: https://codereview.chromium.org/1566193002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1566193002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1566193002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Always antialias clips in ClipDisplayItem. BUG=390040 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Always antialias clips in ClipDisplayItem. BUG=390040 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/ce3dce9ffa916d60c9b6d70f427a244a80358ccb Cr-Commit-Position: refs/heads/master@{#369246} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/ce3dce9ffa916d60c9b6d70f427a244a80358ccb Cr-Commit-Position: refs/heads/master@{#369246} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
