Bug 58675 – Add support for composite-to-texture to DumpRenderTree.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[W. James MacLean]

Add support for composite-to-texture to chromium-gpu platform for new-run-webkit-tests.

[W. James MacLean]

Created attachment 89812 [details]
Patch

[W. James MacLean]

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.

[W. James MacLean]

Created attachment 89818 [details]
Patch

[W. James MacLean]

(In reply to comment #3)
> Created an attachment (id=89818) [details]
> Patch

Fixed minor mistake.

[Dirk Pranke]

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.

[W. James MacLean]

(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.

[W. James MacLean]

Created attachment 89826 [details]
Patch

[W. James MacLean]

(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.

[Dirk Pranke]

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).

[W. James MacLean]

(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.

[Dirk Pranke]

(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?

[W. James MacLean]

(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.

[Dirk Pranke]

(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?

[W. James MacLean]

(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.

[James Robinson]

(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.

[W. James MacLean]

(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 ...).

[W. James MacLean]

Created attachment 90197 [details]
Patch

[W. James MacLean]

(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).

[Kenneth Russell]

Comment on attachment 90197 [details]
Patch

Looks fine.

[W. James MacLean]

Comment on attachment 90197 [details]
Patch

Clearing flags on attachment: 90197

Committed r84361: <http://trac.webkit.org/changeset/84361>

[W. James MacLean]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/84361 might have broken GTK Linux 32-bit Debug