Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12393053:
Re-land: cc: Added antialiasing support for solid color layers (Closed)
|
Created:
7 years, 9 months ago by ernstm Modified:
7 years, 9 months ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionRe-land: cc: Added antialiasing support for solid color layers
BUG=166570
Previously committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186130
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188229
Patch Set 1 #
Total comments: 4
Patch Set 2 : setupQuadForAntialiasing modifies localQuad only if useAA is true #
Total comments: 6
Patch Set 3 : Rebased to tip of tree. Fixed comments. Changed type of edge parameter. #Patch Set 4 : Re-land: cc: Added antialiasing support for solid color layers #
Total comments: 1
Patch Set 5 : Re-enabled GLRendererPixelTest.AnitAliasing on Windows. #
Total comments: 2
Patch Set 6 : Changed formatting in GLRenderer to Chrome style. #
Total comments: 14
Patch Set 7 : More Chrome style formatting changes. #
Messages
Total messages: 33 (0 generated)
https://codereview.chromium.org/12393053/diff/1/cc/gl_renderer.cc File cc/gl_renderer.cc (right): https://codereview.chromium.org/12393053/diff/1/cc/gl_renderer.cc#newcode760 cc/gl_renderer.cc:760: if (useAA) { you can do "if (!useAA) return false;" here if you make the change I suggested in gl_renderer.h. https://codereview.chromium.org/12393053/diff/1/cc/gl_renderer.h File cc/gl_renderer.h (right): https://codereview.chromium.org/12393053/diff/1/cc/gl_renderer.h#newcode124 cc/gl_renderer.h:124: bool setupQuadForAntialiasing(const gfx::Transform& deviceTransform, const DrawQuad* quad, gfx::QuadF* localQuad, float* edge) const; Lets have this only modify input parameters when quad needs anti-aliasing. I think that might be a bit cleaner and it's the behavior I would expect if I didn't read your comment above.
FYI, the trybots and commit queue do not support binary files (like pngs) so to land this you'll need to ask somebody to manually commit the whole patch or (even better) manually check in the .png and then run tryjobs + CQ
jamesr, thanks for the heads up. ernstm, you can remove the png from the patch. I'll take care of landing it ahead of this patch.
Made changes requested by reveman in #2: setupQuadForAntialiasing modifies localQuad only if useAA is true. https://codereview.chromium.org/12393053/diff/1/cc/gl_renderer.cc File cc/gl_renderer.cc (right): https://codereview.chromium.org/12393053/diff/1/cc/gl_renderer.cc#newcode760 cc/gl_renderer.cc:760: if (useAA) { On 2013/03/04 20:28:30, David Reveman wrote: > you can do "if (!useAA) return false;" here if you make the change I suggested > in gl_renderer.h. Done. https://codereview.chromium.org/12393053/diff/1/cc/gl_renderer.h File cc/gl_renderer.h (right): https://codereview.chromium.org/12393053/diff/1/cc/gl_renderer.h#newcode124 cc/gl_renderer.h:124: bool setupQuadForAntialiasing(const gfx::Transform& deviceTransform, const DrawQuad* quad, gfx::QuadF* localQuad, float* edge) const; On 2013/03/04 20:28:30, David Reveman wrote: > Lets have this only modify input parameters when quad needs anti-aliasing. I > think that might be a bit cleaner and it's the behavior I would expect if I > didn't read your comment above. Done.
Thanks for the refactoring and code reuse; that antialiasing code is hairy enough as it is. Other than one comment below, lgtm, but please get reveman's approval too. https://codereview.chromium.org/12393053/diff/2002/cc/gl_renderer.h File cc/gl_renderer.h (right): https://codereview.chromium.org/12393053/diff/2002/cc/gl_renderer.h#newcode124 cc/gl_renderer.h:124: bool setupQuadForAntialiasing(const gfx::Transform& deviceTransform, const DrawQuad* quad, gfx::QuadF* localQuad, float* edge) const; Can you make this parameter float edge[24] here rather than just a raw pointer to document that's what's expected?
lgtm, just adjust some of the comments and address enne's feedback before landing. fyi, the pixel test png was committed in r186007. this will also need a rebase before it can land. https://codereview.chromium.org/12393053/diff/2002/cc/gl_renderer.cc File cc/gl_renderer.cc (right): https://codereview.chromium.org/12393053/diff/2002/cc/gl_renderer.cc#newcode868 cc/gl_renderer.cc:868: // The tile quad shader behaves differently compared to all other shaders. we shouldn't be referring to the tile quad shader here. this comment also happens to be false after this change. we now have two shaders that behaves differently :) please adjust this comment. https://codereview.chromium.org/12393053/diff/2002/cc/gl_renderer.cc#newcode1016 cc/gl_renderer.cc:1016: // The tile quad shader behaves differently compared to all other shaders. adjust this comment too.
Made changes requested by enne (#6) and reveman (#7). Rebased to tip of tree. https://codereview.chromium.org/12393053/diff/2002/cc/gl_renderer.cc File cc/gl_renderer.cc (right): https://codereview.chromium.org/12393053/diff/2002/cc/gl_renderer.cc#newcode868 cc/gl_renderer.cc:868: // The tile quad shader behaves differently compared to all other shaders. On 2013/03/04 22:59:06, David Reveman wrote: > we shouldn't be referring to the tile quad shader here. this comment also > happens to be false after this change. we now have two shaders that behaves > differently :) please adjust this comment. Done. https://codereview.chromium.org/12393053/diff/2002/cc/gl_renderer.cc#newcode1016 cc/gl_renderer.cc:1016: // The tile quad shader behaves differently compared to all other shaders. On 2013/03/04 22:59:06, David Reveman wrote: > adjust this comment too. Done. https://codereview.chromium.org/12393053/diff/2002/cc/gl_renderer.h File cc/gl_renderer.h (right): https://codereview.chromium.org/12393053/diff/2002/cc/gl_renderer.h#newcode124 cc/gl_renderer.h:124: bool setupQuadForAntialiasing(const gfx::Transform& deviceTransform, const DrawQuad* quad, gfx::QuadF* localQuad, float* edge) const; On 2013/03/04 22:44:59, enne wrote: > Can you make this parameter float edge[24] here rather than just a raw pointer > to document that's what's expected? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ernstm@chromium.org/12393053/6002
Message was sent while issue was closed.
Change committed as 186130
Re-land: Disabled flaky GLRendererPixelTest.AntiAliasing on Windows.
https://chromiumcodereview.appspot.com/12393053/diff/16001/cc/gl_renderer_pix... File cc/gl_renderer_pixeltest.cc (right): https://chromiumcodereview.appspot.com/12393053/diff/16001/cc/gl_renderer_pix... cc/gl_renderer_pixeltest.cc:201: // This test is flaky on Windows. We only enable it on Linux for now. Do we know why?
On 2013/03/05 18:50:00, jamesr wrote: > https://chromiumcodereview.appspot.com/12393053/diff/16001/cc/gl_renderer_pix... > File cc/gl_renderer_pixeltest.cc (right): > > https://chromiumcodereview.appspot.com/12393053/diff/16001/cc/gl_renderer_pix... > cc/gl_renderer_pixeltest.cc:201: // This test is flaky on Windows. We only > enable it on Linux for now. > Do we know why? One pixel is different.
On 2013/03/05 18:50:53, ernstm wrote: > On 2013/03/05 18:50:00, jamesr wrote: > > > https://chromiumcodereview.appspot.com/12393053/diff/16001/cc/gl_renderer_pix... > > File cc/gl_renderer_pixeltest.cc (right): > > > > > https://chromiumcodereview.appspot.com/12393053/diff/16001/cc/gl_renderer_pix... > > cc/gl_renderer_pixeltest.cc:201: // This test is flaky on Windows. We only > > enable it on Linux for now. > > Do we know why? > > One pixel is different. Yes, and why? We don't have any defined(OS_WINDOWS) code in the AA path that I know of. Just disabling a test because we don't understand it is really bad practice.
this one is on me. I asked ernstm to put this up as a possible temporary solution. I think a few things need to be done before we can land this or anything else: - determine if this is failing consistently or is just flaky on windows - determine if it's at all possible to have exact aa pixel tests that produce the same result on all platforms, and if not: * do we provide different results for each platform * or do we add some form of fuzzyness support to the pixel test system if we decide to land this current cl then it will have to include a TODO with a link to the appropriate long term solution.
On 2013/03/05 18:53:01, jamesr wrote: > On 2013/03/05 18:50:53, ernstm wrote: > > On 2013/03/05 18:50:00, jamesr wrote: > > > > > > https://chromiumcodereview.appspot.com/12393053/diff/16001/cc/gl_renderer_pix... > > > File cc/gl_renderer_pixeltest.cc (right): > > > > > > > > > https://chromiumcodereview.appspot.com/12393053/diff/16001/cc/gl_renderer_pix... > > > cc/gl_renderer_pixeltest.cc:201: // This test is flaky on Windows. We only > > > enable it on Linux for now. > > > Do we know why? > > > > One pixel is different. > > Yes, and why? We don't have any defined(OS_WINDOWS) code in the AA path that I > know of. Just disabling a test because we don't understand it is really bad > practice. You are right. I will investigate the issue. My guess is that it is caused by different OpenGL drivers. Btw, I came across a variety of these color differences in Chrome caused either by OpenGL drivers or by differences in fixed-point color computations in different code paths. We should either consider making our tests tolerate these differences, or make an attempt to eliminate the differences.
On 2013/03/05 19:25:48, ernstm wrote: > On 2013/03/05 18:53:01, jamesr wrote: > > On 2013/03/05 18:50:53, ernstm wrote: > > > On 2013/03/05 18:50:00, jamesr wrote: > > > > > > > > > > https://chromiumcodereview.appspot.com/12393053/diff/16001/cc/gl_renderer_pix... > > > > File cc/gl_renderer_pixeltest.cc (right): > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/12393053/diff/16001/cc/gl_renderer_pix... > > > > cc/gl_renderer_pixeltest.cc:201: // This test is flaky on Windows. We only > > > > enable it on Linux for now. > > > > Do we know why? > > > > > > One pixel is different. > > > > Yes, and why? We don't have any defined(OS_WINDOWS) code in the AA path that I > > know of. Just disabling a test because we don't understand it is really bad > > practice. > > You are right. I will investigate the issue. My guess is that it is caused by > different OpenGL drivers. Btw, I came across a variety of these color > differences in Chrome caused either by OpenGL drivers or by differences in > fixed-point color computations in different code paths. We should either > consider making our tests tolerate these differences, or make an attempt to > eliminate the differences. We don't use real OpenGL drivers for this, we use OSMESA which should be the same on windows as on every other platform.
> We don't use real OpenGL drivers for this, we use OSMESA which should be the > same on windows as on every other platform. same situation as our layout tests, right? how is our ability to produce the exact same results across platforms there? if that works, then there's no reason for this to fail.
On 2013/03/05 19:25:48, ernstm wrote: > On 2013/03/05 18:53:01, jamesr wrote: > > On 2013/03/05 18:50:53, ernstm wrote: > > > On 2013/03/05 18:50:00, jamesr wrote: > > > > > > > > > > https://chromiumcodereview.appspot.com/12393053/diff/16001/cc/gl_renderer_pix... > > > > File cc/gl_renderer_pixeltest.cc (right): > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/12393053/diff/16001/cc/gl_renderer_pix... > > > > cc/gl_renderer_pixeltest.cc:201: // This test is flaky on Windows. We only > > > > enable it on Linux for now. > > > > Do we know why? > > > > > > One pixel is different. > > > > Yes, and why? We don't have any defined(OS_WINDOWS) code in the AA path that I > > know of. Just disabling a test because we don't understand it is really bad > > practice. > > You are right. I will investigate the issue. My guess is that it is caused by > different OpenGL drivers. Btw, I came across a variety of these color > differences in Chrome caused either by OpenGL drivers or by differences in > fixed-point color computations in different code paths. We should either > consider making our tests tolerate these differences, or make an attempt to > eliminate the differences. It turns out that the pixel at x=193, y=51 is (255,1,0) instead of (255,0,0). I do not have an explanation where this difference comes from. reveman has run a script to compare WebKit layout tests on different platforms and the results show that there are differences in many tests. WebKit seems to use platform dependent reference files to deal with the variation. Instead of doing this for the pixel tests, we might want to add an option to the tests to tolerate a certain amount of error, because pixel accurate results cannot be guaranteed across platforms and even within a platform when different code paths are taken. Reveman is going to kick off a separate discussion on this idea.
message: On 2013/03/06 02:05:27, ernstm wrote: > It turns out that the pixel at x=193, y=51 is (255,1,0) instead of (255,0,0). I > do not have an explanation where this difference comes from. The different pixel color comes from Mesa. In third_party/mesa/MesaLib/src/mesa/main/macros.h the macro UNCLAMPED_FLOAT_TO_UBYTE is defined differently depending on whether DEBUG is defined or not. This, in combination with DEBUG being defined in our Mesa build on Windows but not on Linux independently of Debug or Release build, causes the difference. This means that two things should be fixed: A) define DEBUG in Mesa build consistently on Linux and Windows, B) use a single definition of UNCLAMPED_FLOAT_TO_UBYTE in all cases (fix in Mesa)
Re-enabled GLRendererPixelTest.AntiAliasing after fixing Mesa bug that caused different behavior on Windows (CL 12764015). Re-based to tip of tree.
lgtm https://codereview.chromium.org/12393053/diff/33001/cc/gl_renderer_pixeltest.cc File cc/gl_renderer_pixeltest.cc (right): https://codereview.chromium.org/12393053/diff/33001/cc/gl_renderer_pixeltest.... cc/gl_renderer_pixeltest.cc:132: #if !defined(OS_ANDROID) can we remove this too now? lets not change it in this cl but as a follow up if possible.
https://codereview.chromium.org/12393053/diff/33001/cc/gl_renderer_pixeltest.cc File cc/gl_renderer_pixeltest.cc (right): https://codereview.chromium.org/12393053/diff/33001/cc/gl_renderer_pixeltest.... cc/gl_renderer_pixeltest.cc:132: #if !defined(OS_ANDROID) On 2013/03/13 00:33:38, David Reveman wrote: > can we remove this too now? lets not change it in this cl but as a follow up if > possible. Not trivially. We don't have anything set up to build osmesa for android right now and if we did running these tests through osmesa on an android device would probably suck pretty hard. I don't think we should have unit tests depending on actual GL drivers, so I think we're stuck.
+vmpstr
On 2013/03/13 00:35:10, jamesr wrote: > https://codereview.chromium.org/12393053/diff/33001/cc/gl_renderer_pixeltest.cc > File cc/gl_renderer_pixeltest.cc (right): > > https://codereview.chromium.org/12393053/diff/33001/cc/gl_renderer_pixeltest.... > cc/gl_renderer_pixeltest.cc:132: #if !defined(OS_ANDROID) > On 2013/03/13 00:33:38, David Reveman wrote: > > can we remove this too now? lets not change it in this cl but as a follow up > if > > possible. > > Not trivially. We don't have anything set up to build osmesa for android right > now and if we did running these tests through osmesa on an android device would > probably suck pretty hard. I don't think we should have unit tests depending on > actual GL drivers, so I think we're stuck. ok, that's too bad. would be nice if we could test things like anti-aliasing on android too. I guess we can at least run whatever we add to software_renderer_pixeltest.cc on android for now.
Rebased to tip of tree. Changed formatting in GLRenderer to Chrome style.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ernstm@chromium.org/12393053/51001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ernstm@chromium.org/12393053/51001
This change has some style problems with chromium style. https://chromiumcodereview.appspot.com/12393053/diff/51001/cc/gl_renderer.cc File cc/gl_renderer.cc (right): https://chromiumcodereview.appspot.com/12393053/diff/51001/cc/gl_renderer.cc#... cc/gl_renderer.cc:860: SolidColorProgramUniforms& uniforms) { no non-const references. this should be a pointer. https://chromiumcodereview.appspot.com/12393053/diff/51001/cc/gl_renderer.cc#... cc/gl_renderer.cc:862: uniforms.matrix_location = program->vertexShader().matrixLocation(); 2 space indents https://chromiumcodereview.appspot.com/12393053/diff/51001/cc/gl_renderer.cc#... cc/gl_renderer.cc:869: bool GLRenderer::SetupQuadForAntialiasing(const gfx::Transform& device_transform, 80 cols https://chromiumcodereview.appspot.com/12393053/diff/51001/cc/gl_renderer.cc#... cc/gl_renderer.cc:989: if (use_aa) { no {} for 1 line https://chromiumcodereview.appspot.com/12393053/diff/51001/cc/gl_renderer.cc#... cc/gl_renderer.cc:1006: gfx::RectF centered_rect(gfx::PointF(-0.5 * tile_rect.width(), 0.5f https://chromiumcodereview.appspot.com/12393053/diff/51001/cc/gl_renderer.cc#... cc/gl_renderer.cc:1996: solid_color_program_aa_ = make_scoped_ptr(new SolidColorProgramAA(context_)); 80 cols. line break and use {} for the if. https://chromiumcodereview.appspot.com/12393053/diff/51001/cc/gl_renderer.h File cc/gl_renderer.h (right): https://chromiumcodereview.appspot.com/12393053/diff/51001/cc/gl_renderer.h#n... cc/gl_renderer.h:162: const DrawQuad* quad, gfx::QuadF* localQuad, 1 param per line if they don't all fit on a single line.
Made formatting changes requested by danakj. https://codereview.chromium.org/12393053/diff/51001/cc/gl_renderer.cc File cc/gl_renderer.cc (right): https://codereview.chromium.org/12393053/diff/51001/cc/gl_renderer.cc#newcode860 cc/gl_renderer.cc:860: SolidColorProgramUniforms& uniforms) { On 2013/03/14 00:54:25, danakj wrote: > no non-const references. this should be a pointer. Done. https://codereview.chromium.org/12393053/diff/51001/cc/gl_renderer.cc#newcode862 cc/gl_renderer.cc:862: uniforms.matrix_location = program->vertexShader().matrixLocation(); On 2013/03/14 00:54:25, danakj wrote: > 2 space indents Done. https://codereview.chromium.org/12393053/diff/51001/cc/gl_renderer.cc#newcode869 cc/gl_renderer.cc:869: bool GLRenderer::SetupQuadForAntialiasing(const gfx::Transform& device_transform, On 2013/03/14 00:54:25, danakj wrote: > 80 cols Done. https://codereview.chromium.org/12393053/diff/51001/cc/gl_renderer.cc#newcode989 cc/gl_renderer.cc:989: if (use_aa) { On 2013/03/14 00:54:25, danakj wrote: > no {} for 1 line Done. https://codereview.chromium.org/12393053/diff/51001/cc/gl_renderer.cc#newcode... cc/gl_renderer.cc:1006: gfx::RectF centered_rect(gfx::PointF(-0.5 * tile_rect.width(), On 2013/03/14 00:54:25, danakj wrote: > 0.5f Done. https://codereview.chromium.org/12393053/diff/51001/cc/gl_renderer.cc#newcode... cc/gl_renderer.cc:1996: solid_color_program_aa_ = make_scoped_ptr(new SolidColorProgramAA(context_)); On 2013/03/14 00:54:25, danakj wrote: > 80 cols. line break and use {} for the if. Done. https://codereview.chromium.org/12393053/diff/51001/cc/gl_renderer.h File cc/gl_renderer.h (right): https://codereview.chromium.org/12393053/diff/51001/cc/gl_renderer.h#newcode162 cc/gl_renderer.h:162: const DrawQuad* quad, gfx::QuadF* localQuad, On 2013/03/14 00:54:25, danakj wrote: > 1 param per line if they don't all fit on a single line. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ernstm@chromium.org/12393053/76001
Message was sent while issue was closed.
Change committed as 188229 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
