[PATCH] retrace: support for dumping multiple snapshots (v2)

[José Fonseca]

On Sat, Jan 21, 2017 at 6:30 PM, Eric Anholt <eric at anholt.net> wrote:

> Rob Clark <robdclark at gmail.com> writes:
>
> > Usually if an app is using MRT, we want to dump (and diff) *all* the
> > render targets to track down where things are going wrong.  Also dumps
> > depth and stencil buffers.
> >
> > Only implemented for GLDumper, since I don't know anything about D3D.
> >
> > TODO maybe cmdline arg to control this?

I'm not sure, I kinda tend to
> > think if you are using dump-images + diff-images you probably want to
> > compare all the render targets.
>

Yes, let's add a command line please.  I fear that reading back
depth-stencil might cause instability / and slow down things tremendously,
as it can really be an heavy operation for HW which stores Z buffer in a
non-linear fashion.


> > ---
> >  retrace/d3dretrace.hpp     |  9 ++++++--
> >  retrace/glretrace_main.cpp | 12 +++++++++--
> >  retrace/glstate.hpp        |  5 ++++-
> >  retrace/glstate_images.cpp | 51 ++++++++++++++++++++++++++++++
> +++++++---------
> >  retrace/retrace.hpp        |  5 ++++-
> >  retrace/retrace_main.cpp   | 50 ++++++++++++++++++++++++++++++
> ++++++---------
> >  6 files changed, 106 insertions(+), 26 deletions(-)
> >
>
> > @@ -200,9 +206,24 @@ takeSnapshot(unsigned call_no) {
> >                  break;
> >              }
> >          } else {
> > -            os::String filename = os::String::format("%s%010u.png",
> > -                                                     snapshotPrefix,
> > -                                                     useCallNos ?
> call_no : snapshot_no);
> > +            os::String filename;
> > +
> > +            if (mrt == -2) {
> > +                /* stencil */
> > +                filename = os::String::format("%s%010u-s.png",
> > +                                              snapshotPrefix,
> > +                                              useCallNos ? call_no :
> snapshot_no);
> > +            } else if (mrt == -1) {
> > +                /* depth */
> > +                filename = os::String::format("%s%010u-z.png",
> > +                                              snapshotPrefix,
> > +                                              useCallNos ? call_no :
> snapshot_no);
> > +            } else {
> > +                filename = os::String::format("%s%010u-mrt%u.png",
> > +                                              snapshotPrefix,
> > +                                              useCallNos ? call_no :
> snapshot_no,
> > +                                              mrt);
> > +            }
>
> The rename here is going to break piglit's testing of color rendering
> output.  Could we keep the old filename for the first color buffer, or
> could you fix piglit to look for both?
>

Yes, please let's keep the old filename for first color buffers.  There are
lots of test harnesses (besides piglit) that are built upon apitrace that
would need to change if not.


>
> The patch (other than that) seems really useful, though.
>
> _______________________________________________
> apitrace mailing list
> apitrace at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/apitrace
>
>
Indeed.  Thanks.

Jose
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/apitrace/attachments/20170124/b4ad1bae/attachment.html>