<div dir="ltr">Sorry for the silence, Rob.<div> </div><div>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.</div><div> </div><div>Jose</div><div> </div></div><div class="gmail_extra"> <div class="gmail_quote">On Sat, Feb 11, 2017 at 2:59 PM, Rob Clark <<a href="mailto:robdclark@gmail.com" target="_blank">robdclark@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Mon, Jan 30, 2017 at 1:10 PM, Rob Clark <<a href="mailto:robdclark@gmail.com">robdclark@gmail.com</a>> 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 <div class="HOEnZb"><div class="h5"> > --- > 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 > </div></div></blockquote></div> </div>