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

[Rob Clark]

On Tue, Jan 24, 2017 at 11:54 AM, José Fonseca <jose.r.fonseca at gmail.com> wrote:
> 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.
>

hmm, reading back color buffer(s) is already a pretty heavy operation
(ie. stalling the gpu)..

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

well, if I'm going to add a cmdline option, perhaps old file name if
cmdline flag not set, new otherwise?  Maybe it's my OCD but the sort
order is funny otherwise, ie.

[robclark at thunkpad:~/diff-images/vdrift/tmp]$ ls -l
total 792
-rw-rw-r--. 1 robclark robclark 171718 Jan 24 19:19 0000154202-mrt1.png
-rw-rw-r--. 1 robclark robclark 616406 Jan 24 19:19 0000154202-mrt2.png
-rw-rw-r--. 1 robclark robclark   6362 Jan 24 19:19 0000154202.png
-rw-rw-r--. 1 robclark robclark  11917 Jan 24 19:19 0000154202-z.png

;-)

BR,
-R

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