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

Rob Clark robdclark at gmail.com
Sat Feb 11 14:59:14 UTC 2017


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