Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
739003002:
cc: Test one copy in mask unit tests (Closed)
|
Created:
6 years, 1 month ago by enne (OOO) Modified:
6 years, 1 month ago CC:
cc-bugs_chromium.org, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Descriptioncc: Test one copy in mask unit tests
R=reveman@chromium.org
Committed: https://crrev.com/1c65b081a4eda9f5bed397cc6d4a59ec14f7e8e1
Cr-Commit-Position: refs/heads/master@{#305284}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Same as before, but no sync query #Patch Set 3 : Turn on one copy rect too #Patch Set 4 : Fix comment #Messages
Total messages: 29 (2 generated)
If I run these tests with GL_ONE_COPY_RECT_STAGING_2D_DRAW, none of the mask textures appear. It could be that somehow GLRenderer is confused about the target and is doing the wrong thing. It still seems worth it to just enable any one copy unit testing with pixels.
On 2014/11/18 22:12:46, enne wrote: > If I run these tests with GL_ONE_COPY_RECT_STAGING_2D_DRAW, none of the mask > textures appear. It could be that somehow GLRenderer is confused about the > target and is doing the wrong thing. It still seems worth it to just enable any > one copy unit testing with pixels. I agree, one test is better than nothing. GLRenderer shouldn't see any difference for GL_ONE_COPY_RECT_STAGING_2D_DRAW. The resources exposed to the GLRenderer should still be using TEXTURE_2D target.
lgtm https://codereview.chromium.org/739003002/diff/1/cc/test/test_in_process_cont... File cc/test/test_in_process_context_provider.cc (right): https://codereview.chromium.org/739003002/diff/1/cc/test/test_in_process_cont... cc/test/test_in_process_context_provider.cc:132: capabilities.gpu.sync_query = true; no need to change this but it works without this too, right?
https://codereview.chromium.org/739003002/diff/1/cc/test/test_in_process_cont... File cc/test/test_in_process_context_provider.cc (right): https://codereview.chromium.org/739003002/diff/1/cc/test/test_in_process_cont... cc/test/test_in_process_context_provider.cc:132: capabilities.gpu.sync_query = true; On 2014/11/18 22:20:41, reveman wrote: > no need to change this but it works without this too, right? No, it does not. ResourceProvider does not end up using sync query and so CanUseOneCopyRasterWorkerPool returns false.
The CQ bit was checked by enne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/739003002/1
https://codereview.chromium.org/739003002/diff/1/cc/test/test_in_process_cont... File cc/test/test_in_process_context_provider.cc (right): https://codereview.chromium.org/739003002/diff/1/cc/test/test_in_process_cont... cc/test/test_in_process_context_provider.cc:132: capabilities.gpu.sync_query = true; On 2014/11/18 22:23:40, enne wrote: > On 2014/11/18 22:20:41, reveman wrote: > > no need to change this but it works without this too, right? > > No, it does not. ResourceProvider does not end up using sync query and so > CanUseOneCopyRasterWorkerPool returns false. I don't think there's a CanUseOneCopyRasterWorkerPool function anymore. One-copy should now work even if this extension is missing.
m https://codereview.chromium.org/739003002/diff/1/cc/test/test_in_process_cont... File cc/test/test_in_process_context_provider.cc (right): https://codereview.chromium.org/739003002/diff/1/cc/test/test_in_process_cont... cc/test/test_in_process_context_provider.cc:132: capabilities.gpu.sync_query = true; On 2014/11/18 22:26:26, reveman wrote: > On 2014/11/18 22:23:40, enne wrote: > > On 2014/11/18 22:20:41, reveman wrote: > > > no need to change this but it works without this too, right? > > > > No, it does not. ResourceProvider does not end up using sync query and so > > CanUseOneCopyRasterWorkerPool returns false. > > I don't think there's a CanUseOneCopyRasterWorkerPool function anymore. One-copy > should now work even if this extension is missing. When did it get removed? https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...
https://codereview.chromium.org/739003002/diff/1/cc/test/test_in_process_cont... File cc/test/test_in_process_context_provider.cc (right): https://codereview.chromium.org/739003002/diff/1/cc/test/test_in_process_cont... cc/test/test_in_process_context_provider.cc:132: capabilities.gpu.sync_query = true; On 2014/11/18 22:29:26, enne wrote: > On 2014/11/18 22:26:26, reveman wrote: > > On 2014/11/18 22:23:40, enne wrote: > > > On 2014/11/18 22:20:41, reveman wrote: > > > > no need to change this but it works without this too, right? > > > > > > No, it does not. ResourceProvider does not end up using sync query and so > > > CanUseOneCopyRasterWorkerPool returns false. > > > > I don't think there's a CanUseOneCopyRasterWorkerPool function anymore. > One-copy > > should now work even if this extension is missing. > > When did it get removed? > https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre... Oh, it was removed by: https://chromium.googlesource.com/chromium/src/+/9bc50696509edce28d25653b19ee... but it looks like it was reintroduced by: https://chromium.googlesource.com/chromium/src/+/03dbe8ae71dbf674627acad91fff... I should have caught it when reviewing that patch, sorry. Do you mind fixing this?
Oops, sorry about that. Do we need to test with and without sync query?
Also, do you want me to fix it in this patch and not set sync_query = true?
On 2014/11/18 23:01:49, enne wrote: > Also, do you want me to fix it in this patch and not set sync_query = true? You can fix this in another patch. sync_query = true is good. false is going to be an exception and a pixel test is not the best way to test this.
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/1f1b197adbd5fdf449229646b26835f0df71e76f Cr-Commit-Position: refs/heads/master@{#304708}
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/735963002/ by machenbach@chromium.org. The reason for reverting is: [Sheriff] Speculative revert for making PixelResourceTest/LayerTreeHostMasksPixelTest.MaskOfClippedLayer/2 flaky. Starting here: http://build.chromium.org/p/chromium.win/builders/Win%207%20Tests%20x64%20%28....
Message was sent while issue was closed.
More specifically, flaky timeouts.
Message was sent while issue was closed.
reveman: It looks like the zero copy was also timing out, which means that maybe turning on sync query broke things? Maybe I should just turn on one copy without sync query and see what that looks like? Is there a reason why this would be more wonky on Windows?
Message was sent while issue was closed.
On Wed, Nov 19, 2014 at 1:28 PM, <enne@chromium.org> wrote: > reveman: It looks like the zero copy was also timing out, which means that > maybe > turning on sync query broke things? Maybe I should just turn on one copy > without > sync query and see what that looks like? Is there a reason why this would > be > more wonky on Windows? > This would be using OSMesa right? To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2014/11/19 at 18:47:11, danakj wrote: > This would be using OSMesa right? Yeah.
Message was sent while issue was closed.
On 2014/11/19 18:28:34, enne wrote: > reveman: It looks like the zero copy was also timing out, which means that maybe > turning on sync query broke things? Maybe I should just turn on one copy without > sync query and see what that looks like? Is there a reason why this would be > more wonky on Windows? How are these tests running? In-process context on top of Mesa? In that case, maybe our in-process support for sync queries is broken somehow. You can turn this test on without sync query support if you like but we should figure out why it's failing asap.
On 2014/11/19 at 19:18:48, reveman wrote: > How are these tests running? In-process context on top of Mesa? In that case, maybe our in-process support for sync queries is broken somehow. Yeah. That seems like the most likely answer. > You can turn this test on without sync query support if you like but we should figure out why it's failing asap. That's the plan. This already confused Dana in that these tests were off, so it'd be nice to at least exercise those code paths and sort out sync query afterwards. I ran a bunch of try jobs, and they're all looking good, so I'll probably commit this soon. I also fixed the one copy rect version which was incorrectly setting the draw texture target to rect as well (oops). I'm not quite sure why the one copy raster worker even lets that be configurable.
For what it's worth, I ran PixelResourceTest/LayerTreeHostMasksPixelTest.MaskOfClippedLayer/2 several thousand times on my Linux machine in Debug with sync query turned on and didn't see any flakes. I wonder if Windows is doing something differently...
On Thu, Nov 20, 2014 at 6:59 PM, <enne@chromium.org> wrote: > For what it's worth, I ran > PixelResourceTest/LayerTreeHostMasksPixelTest.MaskOfClippedLayer/2 several > thousand times on my Linux machine in Debug with sync query turned on and > didn't > see any flakes. I wonder if Windows is doing something differently... > FWIW I've found while ./out/Debug/cc_unittests --test-launcher-jobs=32; do true; done to be a more effective flake-producing mechanism than just running a single test. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I filed https://code.google.com/p/chromium/issues/detail?id=435632 for the Windows timeouts as they appear more general. I will land this patch after https://codereview.chromium.org/751563003 lands.
The CQ bit was checked by enne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/739003002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/1c65b081a4eda9f5bed397cc6d4a59ec14f7e8e1 Cr-Commit-Position: refs/heads/master@{#305284} |
