[PATCH] retrace: support for dumping multiple snapshots (v3)
José Fonseca
jose.r.fonseca at gmail.com
Mon Mar 13 15:15:41 UTC 2017
Hi Rob,
Sorry for the delay. I'm finally looking into this. I'm going to rename
the options from -f/--full to -m/--mrt, because I believe mrt is a more
self describing option and the initial `m` is less common compared with `f`
(e.g, full-xxx, force-xxx, etc.) Otherwise I'll commit as is. Thanks.
Jose
On Sun, Feb 12, 2017 at 11:21 PM, Rob Clark <robdclark at gmail.com> wrote:
> Thanks.. not that urgent, just want to make sure that it wasn't
> forgotten (or assumed that I could push myself)..
>
> And congrats :-)
>
> BR,
> -R
>
> On Sun, Feb 12, 2017 at 7:30 AM, José Fonseca <jose.r.fonseca at gmail.com>
> wrote:
> > Sorry for the silence, Rob.
> >
> > I'm on a 2 week paternity leave. I'll take a look at the patches when
> I'm
> > back to work, on the week of Feb 20th.
> >
> > Jose
> >
> >
> > On Sat, Feb 11, 2017 at 2:59 PM, Rob Clark <robdclark at gmail.com> wrote:
> >>
> >> On Mon, Jan 30, 2017 at 1:10 PM, Rob Clark <robdclark at gmail.com> wrote:
> >> > 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.
> >> >
> >> > When the --full (or -f) argument is specified, all render targets plus
> >> > depth and/or stencil are dumped, with the suffix -mrtN/-z/-s.
> Otherwise
> >> > the behavior is as before, only mrt0 is dumped with no suffix.
> >> >
> >> > Only implemented for GLDumper, since I don't know anything about D3D.
> >>
> >>
> >> btw, is everyone happy with this version? If so, could someone push it?
> >>
> >> BR,
> >> -R
> >>
> >> > ---
> >> > cli/cli_dump_images.cpp | 10 +++++++-
> >> > retrace/d3dretrace.hpp | 9 +++++--
> >> > retrace/glretrace_main.cpp | 12 ++++++++--
> >> > retrace/glstate.hpp | 5 +++-
> >> > retrace/glstate_images.cpp | 51 ++++++++++++++++++++++++++++++
> +--------
> >> > retrace/retrace.hpp | 10 +++++++-
> >> > retrace/retrace_main.cpp | 59
> >> > +++++++++++++++++++++++++++++++++++++---------
> >> > 7 files changed, 128 insertions(+), 28 deletions(-)
> >> >
> >> > diff --git a/cli/cli_dump_images.cpp b/cli/cli_dump_images.cpp
> >> > index bf3dc56..ee9f416 100644
> >> > --- a/cli/cli_dump_images.cpp
> >> > +++ b/cli/cli_dump_images.cpp
> >> > @@ -54,6 +54,7 @@ usage(void)
> >> > " otherwise use sequental numbers
> >> > (default=yes)\n"
> >> > " -o, --output=PREFIX prefix to use in naming output
> >> > files\n"
> >> > " (default is trace filename
> without
> >> > extension)\n"
> >> > + " -f, --full dump all MRTs and
> depth/stencil\n"
> >> > "\n";
> >> > }
> >> >
> >> > @@ -63,7 +64,7 @@ enum {
> >> > };
> >> >
> >> > const static char *
> >> > -shortOptions = "ho:";
> >> > +shortOptions = "ho:f";
> >> >
> >> > const static struct option
> >> > longOptions[] = {
> >> > @@ -71,6 +72,7 @@ longOptions[] = {
> >> > {"calls", required_argument, 0, CALLS_OPT},
> >> > {"call-nos", optional_argument, 0, CALL_NOS_OPT},
> >> > {"output", required_argument, 0, 'o'},
> >> > + {"full", no_argument, 0, 'f'},
> >> > {0, 0, 0, 0}
> >> > };
> >> >
> >> > @@ -82,6 +84,7 @@ command(int argc, char *argv[])
> >> > const char *traceName = NULL;
> >> > const char *output = NULL;
> >> > std::string call_nos;
> >> > + bool full = false;
> >> >
> >> > int opt;
> >> > while ((opt = getopt_long(argc, argv, shortOptions, longOptions,
> >> > NULL)) != -1) {
> >> > @@ -99,6 +102,9 @@ command(int argc, char *argv[])
> >> > case 'o':
> >> > output = optarg;
> >> > break;
> >> > + case 'f':
> >> > + full = true;
> >> > + break;
> >> > default:
> >> > std::cerr << "error: unexpected option `" << (char)opt <<
> >> > "`\n";
> >> > usage();
> >> > @@ -140,6 +146,8 @@ command(int argc, char *argv[])
> >> > if (!call_nos.empty()) {
> >> > opts.push_back(call_nos.c_str());
> >> > }
> >> > + if (full)
> >> > + opts.push_back("-f");
> >> >
> >> > return executeRetrace(opts, traceName);
> >> > }
> >> > diff --git a/retrace/d3dretrace.hpp b/retrace/d3dretrace.hpp
> >> > index d0be577..d5fc848 100644
> >> > --- a/retrace/d3dretrace.hpp
> >> > +++ b/retrace/d3dretrace.hpp
> >> > @@ -50,9 +50,14 @@ public:
> >> > pLastDevice(NULL)
> >> > {}
> >> >
> >> > + int
> >> > + getSnapshotCount(void) override {
> >> > + return 1;
> >> > + }
> >> > +
> >> > image::Image *
> >> > - getSnapshot(void) {
> >> > - if (!pLastDevice) {
> >> > + getSnapshot(int n) {
> >> > + if ((n != 0) || !pLastDevice) {
> >> > return NULL;
> >> > }
> >> > return d3dstate::getRenderTargetImage(pLastDevice);
> >> > diff --git a/retrace/glretrace_main.cpp b/retrace/glretrace_main.cpp
> >> > index db94ce6..681a268 100755
> >> > --- a/retrace/glretrace_main.cpp
> >> > +++ b/retrace/glretrace_main.cpp
> >> > @@ -817,12 +817,20 @@ debugOutputCallback(GLenum source, GLenum type,
> >> > GLuint id, GLenum severity,
> >> >
> >> > class GLDumper : public retrace::Dumper {
> >> > public:
> >> > + int
> >> > + getSnapshotCount(void) override {
> >> > + if (!glretrace::getCurrentContext()) {
> >> > + return 0;
> >> > + }
> >> > + return glstate::getDrawBufferImageCount();
> >> > + }
> >> > +
> >> > image::Image *
> >> > - getSnapshot(void) override {
> >> > + getSnapshot(int n) override {
> >> > if (!glretrace::getCurrentContext()) {
> >> > return NULL;
> >> > }
> >> > - return glstate::getDrawBufferImage();
> >> > + return glstate::getDrawBufferImage(n);
> >> > }
> >> >
> >> > bool
> >> > diff --git a/retrace/glstate.hpp b/retrace/glstate.hpp
> >> > index 417a032..b6eb73a 100644
> >> > --- a/retrace/glstate.hpp
> >> > +++ b/retrace/glstate.hpp
> >> > @@ -64,8 +64,11 @@ dumpCurrentContext(StateWriter &writer);
> >> > bool
> >> > getDrawableBounds(GLint *width, GLint *height);
> >> >
> >> > +int
> >> > +getDrawBufferImageCount(void);
> >> > +
> >> > image::Image *
> >> > -getDrawBufferImage(void);
> >> > +getDrawBufferImage(int n);
> >> >
> >> >
> >> > } /* namespace glstate */
> >> > diff --git a/retrace/glstate_images.cpp b/retrace/glstate_images.cpp
> >> > index 13cddb1..a74a5b8 100644
> >> > --- a/retrace/glstate_images.cpp
> >> > +++ b/retrace/glstate_images.cpp
> >> > @@ -959,9 +959,25 @@ getFramebufferAttachmentDesc(Context &context,
> >> > GLenum target, GLenum attachment,
> >> > }
> >> >
> >> >
> >> > +int
> >> > +getDrawBufferImageCount()
> >> > +{
> >> > + Context context;
> >> > + GLint count;
> >> > +
> >> > + if (context.framebuffer_object) {
> >> > + glGetIntegerv(GL_MAX_DRAW_BUFFERS, &count);
> >> > + flushErrors();
> >> > + } else {
> >> > + return 0;
> >> > + }
> >> > +
> >> > + return count;
> >> > +}
> >> > +
> >> >
> >> > image::Image *
> >> > -getDrawBufferImage()
> >> > +getDrawBufferImage(int n)
> >> > {
> >> > Context context;
> >> >
> >> > @@ -979,23 +995,42 @@ getDrawBufferImage()
> >> > glGetIntegerv(framebuffer_binding, &draw_framebuffer);
> >> > }
> >> >
> >> > + GLenum format = GL_RGB;
> >> > + GLenum type = GL_UNSIGNED_BYTE;
> >> > + if (context.ES) {
> >> > + format = GL_RGBA;
> >> > + if ((n < 0) && !context.NV_read_depth_stencil) {
> >> > + return NULL;
> >> > + }
> >> > + }
> >> > +
> >> > + if (n == -2) {
> >> > + /* read stencil */
> >> > + format = GL_STENCIL_INDEX;
> >> > + n = 0;
> >> > + } else if (n == -1) {
> >> > + /* read depth */
> >> > + format = GL_DEPTH_COMPONENT;
> >> > + n = 0;
> >> > + }
> >> > +
> >> > GLint draw_buffer = GL_NONE;
> >> > ImageDesc desc;
> >> > if (draw_framebuffer) {
> >> > if (context.ARB_draw_buffers) {
> >> > - glGetIntegerv(GL_DRAW_BUFFER0, &draw_buffer);
> >> > + glGetIntegerv(GL_DRAW_BUFFER0 + n, &draw_buffer);
> >> > if (draw_buffer == GL_NONE) {
> >> > return NULL;
> >> > }
> >> > } else {
> >> > // GL_COLOR_ATTACHMENT0 is implied
> >> > - draw_buffer = GL_COLOR_ATTACHMENT0;
> >> > + draw_buffer = GL_COLOR_ATTACHMENT0 + n;
> >> > }
> >> >
> >> > if (!getFramebufferAttachmentDesc(context,
> framebuffer_target,
> >> > draw_buffer, desc)) {
> >> > return NULL;
> >> > }
> >> > - } else {
> >> > + } else if (n == 0) {
> >> > if (context.ES) {
> >> > // XXX: Draw buffer is always FRONT for single buffer
> >> > context, BACK
> >> > // for double buffered contexts. There is no way to know
> >> > which (as
> >> > @@ -1014,12 +1049,8 @@ getDrawBufferImage()
> >> > }
> >> >
> >> > desc.depth = 1;
> >> > - }
> >> > -
> >> > - GLenum format = GL_RGB;
> >> > - GLenum type = GL_UNSIGNED_BYTE;
> >> > - if (context.ES) {
> >> > - format = GL_RGBA;
> >> > + } else {
> >> > + return NULL;
> >> > }
> >> >
> >> > GLint channels = _gl_format_channels(format);
> >> > diff --git a/retrace/retrace.hpp b/retrace/retrace.hpp
> >> > index 356ad31..a0f3442 100644
> >> > --- a/retrace/retrace.hpp
> >> > +++ b/retrace/retrace.hpp
> >> > @@ -119,6 +119,11 @@ extern unsigned debug;
> >> > extern bool markers;
> >> >
> >> > /**
> >> > + * Snapshot all render targets plus depth/stencil.
> >> > + */
> >> > +extern bool snapshotFull;
> >> > +
> >> > +/**
> >> > * Whether to force windowed. Recommeded, as there is no guarantee
> that
> >> > the
> >> > * original display mode is available.
> >> > */
> >> > @@ -223,8 +228,11 @@ public:
> >> > class Dumper
> >> > {
> >> > public:
> >> > + virtual int
> >> > + getSnapshotCount(void) = 0;
> >> > +
> >> > virtual image::Image *
> >> > - getSnapshot(void) = 0;
> >> > + getSnapshot(int n) = 0;
> >> >
> >> > virtual bool
> >> > canDump(void) = 0;
> >> > diff --git a/retrace/retrace_main.cpp b/retrace/retrace_main.cpp
> >> > index 83b43ca..7c91a6b 100644
> >> > --- a/retrace/retrace_main.cpp
> >> > +++ b/retrace/retrace_main.cpp
> >> > @@ -76,6 +76,7 @@ trace::Profiler profiler;
> >> > int verbosity = 0;
> >> > unsigned debug = 1;
> >> > bool markers = false;
> >> > +bool snapshotFull = false;
> >> > bool forceWindowed = true;
> >> > bool dumpingState = false;
> >> > bool dumpingSnapshots = false;
> >> > @@ -133,8 +134,13 @@ frameComplete(trace::Call &call) {
> >> > class DefaultDumper: public Dumper
> >> > {
> >> > public:
> >> > + int
> >> > + getSnapshotCount(void) override {
> >> > + return 0;
> >> > + }
> >> > +
> >> > image::Image *
> >> > - getSnapshot(void) override {
> >> > + getSnapshot(int n) override {
> >> > return NULL;
> >> > }
> >> >
> >> > @@ -166,15 +172,16 @@ static Snapshotter *snapshotter;
> >> > * Take snapshots.
> >> > */
> >> > static void
> >> > -takeSnapshot(unsigned call_no) {
> >> > - static unsigned snapshot_no = 0;
> >> > +takeSnapshot(unsigned call_no, int mrt, unsigned snapshot_no) {
> >> >
> >> > assert(dumpingSnapshots);
> >> > assert(snapshotPrefix);
> >> >
> >> > - std::unique_ptr<image::Image> src(dumper->getSnapshot());
> >> > + std::unique_ptr<image::Image> src(dumper->getSnapshot(mrt));
> >> > if (!src) {
> >> > - std::cerr << call_no << ": warning: failed to get
> snapshot\n";
> >> > + /* TODO for mrt>0 we probably don't want to treat this as an
> >> > error: */
> >> > + if (mrt == 0)
> >> > + std::cerr << call_no << ": warning: failed to get
> >> > snapshot\n";
> >> > return;
> >> > }
> >> >
> >> > @@ -200,9 +207,21 @@ takeSnapshot(unsigned call_no) {
> >> > break;
> >> > }
> >> > } else {
> >> > - os::String filename = os::String::format("%s%010u.png",
> >> > - snapshotPrefix,
> >> > - useCallNos ?
> >> > call_no : snapshot_no);
> >> > + os::String filename;
> >> > + unsigned no = useCallNos ? call_no : snapshot_no;
> >> > +
> >> > + if (!retrace::snapshotFull) {
> >> > + assert(mrt == 0);
> >> > + filename = os::String::format("%s%010u.png",
> >> > snapshotPrefix, no);
> >> > + } else if (mrt == -2) {
> >> > + /* stencil */
> >> > + filename = os::String::format("%s%010u-s.png",
> >> > snapshotPrefix, no);
> >> > + } else if (mrt == -1) {
> >> > + /* depth */
> >> > + filename = os::String::format("%s%010u-z.png",
> >> > snapshotPrefix, no);
> >> > + } else {
> >> > + filename = os::String::format("%s%010u-mrt%u.png",
> >> > snapshotPrefix, no, mrt);
> >> > + }
> >> >
> >> > // Here we release our ownership on the Image, it is now
> >> > the
> >> > // responsibility of the snapshotter to delete it.
> >> > @@ -210,11 +229,24 @@ takeSnapshot(unsigned call_no) {
> >> > }
> >> > }
> >> >
> >> > - snapshot_no++;
> >> > -
> >> > return;
> >> > }
> >> >
> >> > +static void
> >> > +takeSnapshot(unsigned call_no) {
> >> > + static unsigned snapshot_no = 0;
> >> > + int cnt = dumper->getSnapshotCount();
> >> > +
> >> > + if (retrace::snapshotFull) {
> >> > + for (int mrt = -2; mrt < cnt; mrt++) {
> >> > + takeSnapshot(call_no, mrt, snapshot_no);
> >> > + }
> >> > + } else {
> >> > + takeSnapshot(call_no, 0, snapshot_no);
> >> > + }
> >> > +
> >> > + snapshot_no++;
> >> > +}
> >> >
> >> > /**
> >> > * Retrace one call.
> >> > @@ -632,6 +664,7 @@ usage(const char *argv0) {
> >> > " -S, --snapshot=CALLSET calls to snapshot (default is
> every
> >> > frame)\n"
> >> > " --snapshot-interval=N specify a frame interval when
> >> > generating snaphots (default is 0)\n"
> >> > " -t, --snapshot-threaded encode screenshots on multiple
> >> > threads\n"
> >> > + " -f, --full dump all MRTs and depth/stencil\n"
> >> > " -v, --verbose increase output verbosity\n"
> >> > " -D, --dump-state=CALL dump state at specific call no\n"
> >> > " --dump-format=FORMAT dump state format (`json` or
> >> > `ubjson`)\n"
> >> > @@ -668,7 +701,7 @@ enum {
> >> > };
> >> >
> >> > const static char *
> >> > -shortOptions = "bdD:hs:S:vwt";
> >> > +shortOptions = "bdD:hs:S:fvwt";
> >> >
> >> > const static struct option
> >> > longOptions[] = {
> >> > @@ -701,6 +734,7 @@ longOptions[] = {
> >> > {"snapshot", required_argument, 0, 'S'},
> >> > {"snapshot-interval", required_argument, 0,
> SNAPSHOT_INTERVAL_OPT},
> >> > {"snapshot-threaded", no_argument, 0, 't'},
> >> > + {"full", no_argument, 0, 'f'},
> >> > {"verbose", no_argument, 0, 'v'},
> >> > {"wait", no_argument, 0, 'w'},
> >> > {"loop", optional_argument, 0, LOOP_OPT},
> >> > @@ -843,6 +877,9 @@ int main(int argc, char **argv)
> >> > case 't':
> >> > snapshotThreaded = true;
> >> > break;
> >> > + case 'f':
> >> > + retrace::snapshotFull = true;
> >> > + break;
> >> > case 'v':
> >> > ++retrace::verbosity;
> >> > break;
> >> > --
> >> > 2.9.3
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/apitrace/attachments/20170313/7ee6123e/attachment-0001.html>
More information about the apitrace
mailing list