<div dir="ltr">Hi Rob,<div> </div><div>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.</div><div> </div><div>Jose</div><div> </div></div><div class="gmail_extra"> <div class="gmail_quote">On Sun, Feb 12, 2017 at 11:21 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">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 <div class="HOEnZb"><div class="h5"> On Sun, Feb 12, 2017 at 7:30 AM, José Fonseca <<a href="mailto:jose.r.fonseca@gmail.com">jose.r.fonseca@gmail.com</a>> 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 <<a href="mailto:robdclark@gmail.com">robdclark@gmail.com</a>> wrote: >> >> 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 >> >> > --- >> > 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>