Add support for composite-to-texture to chromium-gpu platform for new-run-webkit-tests.
Created attachment 89812 [details] Patch
Not sure who should review this, but have CC'd some people who are knowledgeable about the subject of the patch. Suggestions? I'm not sure if adding "chromium-gpu-ctt" as a new platform name is the best way to go, but it does lead to simple plumbing. I'm open to other ideas.
Created attachment 89818 [details] Patch
(In reply to comment #3) > Created an attachment (id=89818) [details] > Patch Fixed minor mistake.
Comment on attachment 89818 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89818&action=review Are we going to keep growing arguments to DRT? Can we remove some of the existing ones yet and just assume they are enabled (or at least combine flags)? > Tools/Scripts/webkitpy/layout_tests/port/chromium_gpu.py:42 > + if port_name == 'chromium-gpu-ctt': Unless we need to spin up new bots that use this config, we shouldn't introduce new port names, so this probably isn't the best way to add this flag. You should add a new command line argument to run_webkit_tests.py (see the examples around line 241). > Tools/Scripts/webkitpy/layout_tests/port/chromium_gpu.py:72 > + if composite_to_texture: This should be using port.get_option() like the others, and not a new method parameter.
(In reply to comment #5) > (From update of attachment 89818 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=89818&action=review > > Are we going to keep growing arguments to DRT? Can we remove some of the existing ones yet and just assume they are enabled (or at least combine flags)? > > > Tools/Scripts/webkitpy/layout_tests/port/chromium_gpu.py:42 > > + if port_name == 'chromium-gpu-ctt': > > Unless we need to spin up new bots that use this config, we shouldn't introduce new port names, so this probably isn't the best way to add this flag. > > You should add a new command line argument to run_webkit_tests.py (see the examples around line 241). > > > Tools/Scripts/webkitpy/layout_tests/port/chromium_gpu.py:72 > > + if composite_to_texture: > > This should be using port.get_option() like the others, and not a new method parameter. Thanks Dirk, I'll look at that and re-submit.
Created attachment 89826 [details] Patch
(In reply to comment #7) > Created an attachment (id=89826) [details] > Patch Revised ... I missed the option parsing stuff in run_webkit_tests.py first time around, much easier.
In fact, I just posted a patch that will allow you to pass through flags to DRT. If you can wait for that to land, maybe that would allow you to not have to change any python code at all? (I don't know why we're adding this flag or where it will be used).
(In reply to comment #9) > In fact, I just posted a patch that will allow you to pass through flags to DRT. If you can wait for that to land, maybe that would allow you to not have to change any python code at all? I'm fine with that if no one has objections. > (I don't know why we're adding this flag or where it will be used). It invokes a modified rendering path in the GPU compositor architecture that needs to be tested so regressions don't creep in. This pathway composites to an intermediate texture instead of directly to the screen, so the texture can be used/re-used elsewhere as needed. Ultimately, it will be nice if automated testing runs two passes, one with and one without this flag.
(In reply to comment #10) > (In reply to comment #9) > > In fact, I just posted a patch that will allow you to pass through flags to DRT. If you can wait for that to land, maybe that would allow you to not have to change any python code at all? > > I'm fine with that if no one has objections. > > > (I don't know why we're adding this flag or where it will be used). > > It invokes a modified rendering path in the GPU compositor architecture that needs to be tested so regressions don't creep in. This pathway composites to an intermediate texture instead of directly to the screen, so the texture can be used/re-used elsewhere as needed. Ultimately, it will be nice if automated testing runs two passes, one with and one without this flag. The two code paths will exist indefinitely into the future? How does the renderer normal figure out which to use? Some sort of runtime feature detection from the video card? Can we test this using an additional machine in the hardware lab (as if it is a different card), or do we really need to do an additional run on ever bot config?
(In reply to comment #11) > > The two code paths will exist indefinitely into the future? Yes. > How does the renderer normal figure out which to use? Some sort of runtime feature detection from the video card? Probably a build flag and/or command line switch. It's intended for use in ChromeOS eventually. > > Can we test this using an additional machine in the hardware lab (as if it is a different card), or do we really need to do an additional run on ever bot config? In order to avoid breakage, I think regular runs on a bot will be needed. Much to my dismay, a non-trivial amount of pixel regression has crept in since the code path was first established.
(In reply to comment #12) > (In reply to comment #11) > > > > The two code paths will exist indefinitely into the future? > > Yes. > > > How does the renderer normal figure out which to use? Some sort of runtime feature detection from the video card? > > Probably a build flag and/or command line switch. It's intended for use in ChromeOS eventually. > > > > > Can we test this using an additional machine in the hardware lab (as if it is a different card), or do we really need to do an additional run on ever bot config? > > In order to avoid breakage, I think regular runs on a bot will be needed. Much to my dismay, a non-trivial amount of pixel regression has crept in since the code path was first established. Okay, but this could just be one actively monitored bot, and doesn't need to be all of them?
(In reply to comment #13) > > Okay, but this could just be one actively monitored bot, and doesn't need to be all of them? I'd like to defer to Vangelis, Ken and James on that one. Composite-to-texture has sandboxing potential also, so it may need broader testing.
(In reply to comment #12) > > Can we test this using an additional machine in the hardware lab (as if it is a different card), or do we really need to do an additional run on ever bot config? > > In order to avoid breakage, I think regular runs on a bot will be needed. Much to my dismay, a non-trivial amount of pixel regression has crept in since the code path was first established. What particular breakages have been introduced? Are they the sort of breakages that would break all compositing tests, or just some of them? The cost of maintaining another set of baselines is fairly high.
(In reply to comment #15) > (In reply to comment #12) > > > Can we test this using an additional machine in the hardware lab (as if it is a different card), or do we really need to do an additional run on ever bot config? > > > > In order to avoid breakage, I think regular runs on a bot will be needed. Much to my dismay, a non-trivial amount of pixel regression has crept in since the code path was first established. > > What particular breakages have been introduced? Are they the sort of breakages that would break all compositing tests, or just some of them? > > The cost of maintaining another set of baselines is fairly high. Mostly minor pixel differences at high-contrast edges (although I haven't checked every failing test manually to see if there's anything more severe). There are (at present) 54 such tests, up from around 32 in February. I don't know if we need to add a bot for it, but it would be nice to have it available as an option for people running new-run-webkit-tests so they can try their changes against before submitting CLs (if only to make sure that something more major doesn't slip through ...).
Created attachment 90197 [details] Patch
(In reply to comment #17) > Created an attachment (id=90197) [details] > Patch I've updated the patch to use Dirk's --additional-drt-flag switch, so now no changes are required to any of the Python scripts. A quick survey of the test performance between the two compositing paths gives Composite-to-texture enabled: 64 image mismatch (all are minor pixel mismatches at high contrast edges), 1 text mismatch vs. Composite-to-screen: 8 image mismatch 1 text mismatch I personally suspect that most (if not all) of the additional image errors in composite-to-texture are the result of interpolation differences at high-contrast edges, and that this will diminish as tiling allows us to use smaller texture sizes (should we so choose).
Comment on attachment 90197 [details] Patch Looks fine.
Comment on attachment 90197 [details] Patch Clearing flags on attachment: 90197 Committed r84361: <http://trac.webkit.org/changeset/84361>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/84361 might have broken GTK Linux 32-bit Debug