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

Rob Clark robdclark at gmail.com
Sun Feb 12 23:21:28 UTC 2017


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


More information about the apitrace mailing list