Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
257843006:
Run the expected output for RefTests with virtual test suite arguments. (Closed)
|
Created:
6 years, 8 months ago by danakj Modified:
6 years, 7 months ago CC:
blink-reviews, abarth-chromium, enne (OOO), jamesr, ojan, piman, Dominik Röttsches Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionRun the expected output for RefTests with virtual test suite arguments.
Currently we're always using an empty argument list (oops), because
we look up the arguments with the test name after removing the virtual
test suite part from the test name.
R=dpranke@chromium.org
BUG=367315, 368008
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172820
Patch Set 1 #Patch Set 2 : reftests: scrollbars #Patch Set 3 : reftests: #Patch Set 4 : reftests: wontfix #Patch Set 5 : reftests: skip #
Messages
Total messages: 25 (0 generated)
This does not yet look okay. As I explained in the bug, I have concerns/fears that this might produce weird bugs or potentially hide bugs down the road. Perhaps these fears are unjustified. However, let's achieve consensus on the approach first. (the actual code change does look correct, if this is in fact the behavior we decide to have).
Ya no problem. I think this will go away as a problem for me when FCM is always on. I'll close this and work around it another way.
Okay, after further conversation on the bug ojan has convinced me that we should make this change, so I've re-opened it. lgtm. danakj, do you want to go ahead and try to land this?
The CQ bit was checked by danakj@chromium.org
Sure let's see what the bots say.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/danakj@chromium.org/257843006/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_rel
Looks like virtual/gpu/fast/canvas/canvas-bg.html is the only failure. Looking..
Hmm, it's imperceptible differences in...drumroll... the scrollbar ! This is virtual/gpu/fast/canvas which is run with the --enable-accelerated-2d-canvas flag. - So, before this change: The actual result would use the flag enabling composited mode, and presumably the scrollbars are rendered by the compositor? The expected result would not use the flag, and would use software canvas, and the scrollbars are not rendered by the compositor. I'd expect this to cause a pixel difference.. but it didn't.. - After this change: The expected result is using the flag, which enables composited mode and causes the scrollbars to be rendered differently than the actual result which has the compositor enabled? This all sounds so weird. Time for some printf debugging.
Or.. we can just turn off the scrollbars since this test is not a scrollbar test. +enne for the scrollbar change
Alternatively, here's a patchset to just shrink the canvas so that it should not cause scrollbars.
Oh no, there's a slight blending difference with this patch. After this CL: In the expected, which is a <canvas>, the overlapping areas are rgb:100,0,100. In the actual, which is drawing on the <body>, the overlapping areas are rgb:101,1,101. Before this CL; In the expected, which is not running with --enable-accelerated-2d-canvas, the overlapping areas are rbg:101,1,101. So, turning on --enable-accelerated-2d-canvas for the expected results takes them from 101,1,101 to 100,0,100. What to do?
@senorblanco, it seems like in this test, even with accelerated canvas 2d enabled, the background is not gpu composited? Should I be disabling/WontFixing this test for virtual/gpu suite? What do you suggest?
On 2014/04/28 21:37:17, danakj wrote: > @senorblanco, it seems like in this test, even with accelerated canvas 2d > enabled, the background is not gpu composited? Should I be disabling/WontFixing > this test for virtual/gpu suite? What do you suggest? Not sure I understand the plethora of flags here. Accelerated 2D canvas should be triggering compositing mode, I believe. (Accelerated canvas without compositing has been busted for a long while, IIRC). What is causing the blending difference? Compositing in the cc, or compositing in canvas?
On 2014/04/28 21:43:30, Stephen White wrote: > On 2014/04/28 21:37:17, danakj wrote: > > @senorblanco, it seems like in this test, even with accelerated canvas 2d > > enabled, the background is not gpu composited? Should I be > disabling/WontFixing > > this test for virtual/gpu suite? What do you suggest? > > Not sure I understand the plethora of flags here. > > Accelerated 2D canvas should be triggering compositing mode, I believe. > (Accelerated canvas without compositing has been busted for a long while, IIRC). > > What is causing the blending difference? Compositing in the cc, or compositing > in canvas? Ohh, this is getCSSCanvasContext(). It wouldn't surprise me if this gives you headaches. It's a really weird, non-standard feature that has often caused problems. But people don't want to remove it without a standardized substitute. IIRC we tried to make it always run in software mode, since it caused problems when it was accelerated. Maybe we should also be ignoring the command-line flag for it, as well.
On Mon, Apr 28, 2014 at 5:43 PM, <senorblanco@chromium.org> wrote: > On 2014/04/28 21:37:17, danakj wrote: > >> @senorblanco, it seems like in this test, even with accelerated canvas 2d >> enabled, the background is not gpu composited? Should I be >> > disabling/WontFixing > >> this test for virtual/gpu suite? What do you suggest? >> > > Not sure I understand the plethora of flags here. > > Accelerated 2D canvas should be triggering compositing mode, I believe. > (Accelerated canvas without compositing has been busted for a long while, > IIRC). > > What is causing the blending difference? Compositing in the cc, or > compositing > in canvas? > Ok there's 3 states. The Actual, the Expected Before, and the Expected After. The Actual is running with enable accelerated canvas 2d. It draws with document.getCSSCanvasContext. The Expected Before is without enable accelerated canvas (because we ignored the virtual suite). It uses <canvas>. Since both are software, they blend the same way (and are both off by 1 together). The Expected After runs with acclerated canvas (since this is in virtual/gpu/) so we enable compositing, and blend differently. Now we don't match the Actual which was a software blend. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
+junov back On Mon, Apr 28, 2014 at 5:48 PM, Dana Jansens <danakj@chromium.org> wrote: > On Mon, Apr 28, 2014 at 5:43 PM, <senorblanco@chromium.org> wrote: > >> On 2014/04/28 21:37:17, danakj wrote: >> >>> @senorblanco, it seems like in this test, even with accelerated canvas 2d >>> enabled, the background is not gpu composited? Should I be >>> >> disabling/WontFixing >> >>> this test for virtual/gpu suite? What do you suggest? >>> >> >> Not sure I understand the plethora of flags here. >> >> Accelerated 2D canvas should be triggering compositing mode, I believe. >> (Accelerated canvas without compositing has been busted for a long while, >> IIRC). >> >> What is causing the blending difference? Compositing in the cc, or >> compositing >> in canvas? >> > > Ok there's 3 states. The Actual, the Expected Before, and the Expected > After. > > The Actual is running with enable accelerated canvas 2d. It draws > with document.getCSSCanvasContext. > > The Expected Before is without enable accelerated canvas (because we > ignored the virtual suite). It uses <canvas>. Since both are software, they > blend the same way (and are both off by 1 together). > > The Expected After runs with acclerated canvas (since this is in > virtual/gpu/) so we enable compositing, and blend differently. Now we don't > match the Actual which was a software blend. > > > > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
I think we should be ignoring the command line flag for -webkit-canvas(), and forcing it to software in all cases. Here was the last time I tussled with this thing: http://src.chromium.org/viewvc/blink?view=revision&revision=152789
Failing that, skipping/WontFix'ing it for the virtual/gpu path would be acceptable.
ok after some more discussion offline, skipping the test on virtual/gpu and filed crbug.com/368008 about it.
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/danakj@chromium.org/257843006/70001
Message was sent while issue was closed.
Change committed as 172820 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
