[DO NOT SQUASH] Fast-forward third_party/gfxreconstruct to 321e90045

.gitmodules

@@ -7,15 +7,19 @@
 [submodule "third_party/gfxreconstruct/external/SPIRV-Headers"]
 	path = third_party/gfxreconstruct/external/SPIRV-Headers
 	url = https://github.com/KhronosGroup/SPIRV-Headers.git
+	shallow = true
 [submodule "third_party/gfxreconstruct/external/SPIRV-Reflect"]
 	path = third_party/gfxreconstruct/external/SPIRV-Reflect
 	url = https://github.com/KhronosGroup/SPIRV-Reflect.git
+	shallow = true
 [submodule "third_party/gfxreconstruct/external/Vulkan-Headers"]
 	path = third_party/gfxreconstruct/external/Vulkan-Headers
 	url = https://github.com/KhronosGroup/Vulkan-Headers.git
+	shallow = true
 [submodule "third_party/gfxreconstruct/external/OpenXR-SDK"]
 	path = third_party/gfxreconstruct/external/OpenXR-SDK
 	url = https://github.com/KhronosGroup/OpenXR-SDK.git
+	shallow = true
 [submodule "third_party/googletest"]
 	path = third_party/googletest
 	url = https://github.com/google/googletest.git

CONTRIBUTING.md

@@ -85,7 +85,7 @@ Make sure to have built everything according to BUILD.md (don't forget the insta
     ```sh
     git subtree pull --prefix=third_party/gfxreconstruct https://github.com/LunarG/gfxreconstruct.git dev --squash
     ```
-1. Copy missing submodule entries from `//third_party/gfxreconstruct/.gitmodules` into `//.gitmodules`
+1. Run `//scripts/incorporate_gfxr_submodules.py` so that the required submodules are cloned.
 1. Update the submodules so that the merge commit includes the correct SHA:
     ```sh
     git submodule update --init --recursive

dive_core/CMakeLists.txt

@@ -143,7 +143,6 @@ target_include_directories(
         ${FREEDRENO_ISA_DIRECTORY}
         ${THIRDPARTY_DIRECTORY}/mesa/src/freedreno/common
         ${THIRDPARTY_DIRECTORY}/mesa/src/
-        ${THIRDPARTY_DIRECTORY}/mesa/include/
         ${THIRDPARTY_DIRECTORY}/libarchive/libarchive/
 )
 

gfxr_dump_resources/gfxr_dump_resources.cpp

@@ -63,6 +63,10 @@ bool SaveAsJsonFile(const std::vector<DumpEntry>& dumpables, const char* filenam
 
     out << "{\n";
 
+    out << "  \"DumpResourcesOptions\": {\n";
+    out << "    \"DumpAllImageSubresources\": true\n";
+    out << "  },\n";
+
     out << "  \"BeginCommandBuffer\": [";
     for (int i = 0; i < dumpables.size(); ++i)
     {
@@ -144,4 +148,4 @@ bool SaveAsJsonFile(const std::vector<DumpEntry>& dumpables, const char* filenam
     return true;
 }
 
-}  // namespace Dive::gfxr
+}  // namespace Dive::gfxr

gfxr_dump_resources/gfxr_dump_resources.h

@@ -34,4 +34,4 @@ std::optional<std::vector<DumpEntry>> FindDumpableResources(const char* filename
 // Returns false on error.
 bool SaveAsJsonFile(const std::vector<DumpEntry>& dumpables, const char* filename);
 
-}  // namespace Dive::gfxr
+}  // namespace Dive::gfxr

gfxr_ext/decode/dive_file_processor.cpp

@@ -44,9 +44,6 @@ bool ShouldCreateRenderDocCapture()
 
 }  // namespace
 
-// TODO GH #1195: frame numbering should be 1-based.
-const uint32_t kFirstFrame = 0;
-
 void DiveFileProcessor::SetLoopSingleFrameCount(uint64_t loop_single_frame_count)
 {
     loop_single_frame_count_ = loop_single_frame_count;
@@ -92,164 +89,144 @@ bool DiveFileProcessor::WriteFile(const std::string& name, const std::string& co
     return true;
 }
 
-bool DiveFileProcessor::ProcessFrameMarker(const format::BlockHeader& block_header,
-                                           format::MarkerType marker_type, bool& should_break)
+bool DiveFileProcessor::ProcessFrameDelimiter(const FrameEndMarkerArgs& end_frame)
 {
-    // Read the rest of the frame marker data. Currently frame markers are not dispatched to
-    // decoders.
-    uint64_t frame_number = 0;
-    bool success = ReadBytes(&frame_number, sizeof(frame_number));
-
-    if (success)
-    {
-        // Validate frame end marker's frame number matches first_frame_ when
-        // capture_uses_frame_markers_ is true.
-        GFXRECON_ASSERT((marker_type != format::kEndMarker) || (!UsesFrameMarkers()) ||
-                        (frame_number == GetFirstFrame()));
+    // Single frame looping, despite the frame number never technically changing, increments
+    // current_frame_number_. However, this triggers an assert in
+    // FileProcessor::ProcessFrameDelimiter which detects when the current frame doesn't match
+    // what's in the file. (This likely detects bad GFXR files or out-of-order execution). Outside
+    // of modifying the assert, we can workaround it by manipulating current_frame_number_.
+    //
+    // Realistically, we could probably keep current_frame_number_ as 0 to improve interactions with
+    // other features like --skip-get-fence-ranges, FpsInfo, etc. However, the previous
+    // implementation allowed current_frame_number_ to increase so I'd rather not deviate at this
+    // time.
+    auto loop_current_frame_number = current_frame_number_;
+    current_frame_number_ = end_frame.frame_number - GetFirstFrame();
+    bool is_frame_delimiter = FileProcessor::ProcessFrameDelimiter(end_frame);
+    current_frame_number_ = loop_current_frame_number;
 
-        for (auto decoder : decoders_)
-        {
-            if (marker_type == format::kEndMarker)
-            {
-                decoder->DispatchFrameEndMarker(frame_number);
-            }
-            else
-            {
-                GFXRECON_LOG_WARNING("Skipping unrecognized frame marker with type %u",
-                                     marker_type);
-            }
-        }
-    }
-    else
+#if defined(__ANDROID__)
+    if (DivePM4Capture::GetInstance().IsPM4CaptureEnabled())
     {
-        HandleBlockReadError(kErrorReadingBlockData, "Failed to read frame marker data");
+        DivePM4Capture::GetInstance().TryStopCapture();
     }
+#endif
 
-    // Break from loop on frame delimiter.
-    if (IsFrameDelimiter(block_header.type, marker_type))
+    // At the last frame in the capture file, determine whether to jump back to the state end
+    // marker, or terminate replay if the loop count has been reached
+    bool finite_looping = loop_single_frame_count_ > 0;
+    // We expect current_frame_number_ to increment by 1 each loop. Ensure it doesn't jump around.
+    GFXRECON_ASSERT(current_frame_number_ < loop_single_frame_count_);
+    // Reaching the Frame End marker means that we've completed one loop. Thus, we have one fewer
+    // loops remaining. current_frame_number_ isn't incremented until after we return.
+    uint64_t loops_left = loop_single_frame_count_ - current_frame_number_ - 1;
+    if (finite_looping && loops_left == 0)
     {
-        // If the capture file contains frame markers, it will have a frame marker for every
-        // frame-ending API call such as vkQueuePresentKHR. If this is the first frame marker
-        // encountered, reset the frame count and ignore frame-ending API calls in
-        // IsFrameDelimiter(format::ApiCallId call_id).
-        if (!UsesFrameMarkers())
-        {
-            SetUsesFrameMarkers(true);
-            current_frame_number_ = kFirstFrame;
-        }
-#if defined(__ANDROID__)
-        if (DivePM4Capture::GetInstance().IsPM4CaptureEnabled())
-        {
-            DivePM4Capture::GetInstance().TryStopCapture();
-        }
-#endif
-        // Make sure to increment the frame number on the way out.
-        ++current_frame_number_;
-        ++block_index_;
-        should_break = true;
-
-        // At the last frame in the capture file, determine whether to jump back to the state end
-        // marker, or terminate replay if the loop count has been reached
-        if ((loop_single_frame_count_ > 0) && (current_frame_number_ >= loop_single_frame_count_))
+        if (ShouldCreateRenderDocCapture())
         {
-            if (ShouldCreateRenderDocCapture())
+            // Finalize the RenderDoc capture after all loops have completed. While the intended
+            // use case is to capture only 1 frame (which can be accomplished by setting
+            // loop_single_frame_count_), I don't see any reason to prevent the user from
+            // capturing all loops if they really want to.
+            if (const RENDERDOC_API_1_0_0* renderdoc = GetRenderDocApi(); renderdoc != nullptr)
             {
-                // Finalize the RenderDoc capture after all loops have completed. While the intended
-                // use case is to capture only 1 frame (which can be accomplished by setting
-                // loop_single_frame_count_), I don't see any reason to prevent the user from
-                // capturing all loops if they really want to.
-                if (const RENDERDOC_API_1_0_0* renderdoc = GetRenderDocApi(); renderdoc != nullptr)
-                {
-                    if (renderdoc->EndFrameCapture(/*device=*/nullptr, /*wndHandle=*/nullptr) != 1)
-                    {
-                        GFXRECON_LOG_WARNING(
-                            "EndFrameCapture failed, RenderDoc .rdc capture likely not created!");
-                    }
-                }
-                else
+                if (renderdoc->EndFrameCapture(/*device=*/nullptr, /*wndHandle=*/nullptr) != 1)
                 {
                     GFXRECON_LOG_WARNING(
-                        "GetRenderDocApi failed. Could not end RenderDoc capture!");
+                        "EndFrameCapture failed, RenderDoc .rdc capture likely not created!");
                 }
             }
-
-            GFXRECON_LOG_INFO("Looped %d frames, terminating replay asap", current_frame_number_);
-            return success;
+            else
+            {
+                GFXRECON_LOG_WARNING("GetRenderDocApi failed. Could not end RenderDoc capture!");
+            }
         }
-        GFXRECON_ASSERT(!gfxr_file_name_.empty());
-        block_index_ = state_end_marker_block_index_;
-        SeekActiveFile(gfxr_file_name_, state_end_marker_file_offset_, util::platform::FileSeekSet);
-        should_break = false;
+
+        GFXRECON_LOG_INFO("Looped %d frames, terminating replay asap", loop_single_frame_count_);
+        // The act of not seeking should cause replay to hit EOF and stop (assuming there is only
+        // one frame in the capture file)
+        return is_frame_delimiter;
     }
-    return success;
+
+    // The block index is printed by --pbi-all. It helps correlate problematic blocks with manual
+    // inspection of capture file. Reset it to the loop point to make debugging easier.
+    block_index_ = state_end_marker_block_index_;
+
+    std::shared_ptr<FileInputStream> gfxr_file = gfxr_file_.lock();
+    GFXRECON_ASSERT(gfxr_file);
+    SeekActiveFile(gfxr_file, state_end_marker_file_offset_, util::platform::FileSeekSet);
+
+    return is_frame_delimiter;
 }
 
-bool DiveFileProcessor::ProcessStateMarker(const format::BlockHeader& block_header,
-                                           format::MarkerType marker_type)
+void DiveFileProcessor::ProcessStateEndMarker(const StateEndMarkerArgs& state_end)
 {
-    bool success = FileProcessor::ProcessStateMarker(block_header, marker_type);
-
-    if ((success) && (marker_type == format::kEndMarker))
-    {
-        // Store state end marker offset
-        GFXRECON_ASSERT(!gfxr_file_name_.empty());
-        state_end_marker_file_offset_ = TellFile(gfxr_file_name_);
-        state_end_marker_block_index_ = block_index_;
-        GFXRECON_LOG_INFO("Stored state end marker offset %d", state_end_marker_file_offset_);
-        GFXRECON_LOG_INFO("Single frame number %d", GetFirstFrame());
+    FileProcessor::ProcessStateEndMarker(state_end);
+
+    // Store state end marker offset
+    std::shared_ptr<FileInputStream> gfxr_file = gfxr_file_.lock();
+    GFXRECON_ASSERT(gfxr_file);
+    state_end_marker_file_offset_ = gfxr_file->FileTell();
+    // The block index is useful while debugging to match the call stack or --pbi output with the
+    // capture file. If it's not reset to the loop point then it just keeps counting up.
+    state_end_marker_block_index_ = block_index_;
+    GFXRECON_LOG_INFO("Stored state end marker offset %d", state_end_marker_file_offset_);
+    GFXRECON_LOG_INFO("Single frame number %d", GetFirstFrame());
 #if defined(__ANDROID__)
-        if (DivePM4Capture::GetInstance().IsPM4CaptureEnabled())
-        {
-            DivePM4Capture::GetInstance().TryStartCapture();
-        }
-        // Tell other processes that replay has finished trim state loading.
-        // TODO: b/444647876 - Implementation that doesn't use global state (filesystem)
-        if (!std::ofstream(Dive::kReplayStateLoadedSignalFile))
+    if (DivePM4Capture::GetInstance().IsPM4CaptureEnabled())
+    {
+        DivePM4Capture::GetInstance().TryStartCapture();
+    }
+    // Tell other processes that replay has finished trim state loading.
+    // TODO: b/444647876 - Implementation that doesn't use global state (filesystem)
+    if (!std::ofstream(Dive::kReplayStateLoadedSignalFile))
+    {
+        GFXRECON_LOG_INFO(
+            "Failed to create a file signaling that trim state loading is "
+            "complete. This will impact our ability to gather metrics.");
+    }
+#endif
+    // Don't bother trying to start a capture for infinite replay since it will never finish!
+    if (loop_single_frame_count_ > 0 && ShouldCreateRenderDocCapture())
+    {
+        if (const RENDERDOC_API_1_0_0* renderdoc = GetRenderDocApi(); renderdoc != nullptr)
         {
-            GFXRECON_LOG_INFO(
-                "Failed to create a file signaling that trim state loading is "
-                "complete. This will impact our ability to gather metrics.");
+            renderdoc->SetCaptureFilePathTemplate(
+                Dive::GetRenderDocCaptureFilePathTemplate(gfxr_file->GetFilename())
+                    .string()
+                    .c_str());
+            // Let RenderDoc choose the Vulkan context and window handle since we typically only
+            // expect one of each.
+            renderdoc->StartFrameCapture(/*device=*/nullptr, /*wndHandle=*/nullptr);
         }
-#endif
-        // Don't bother trying to start a capture for infinite replay since it will never finish!
-        if (loop_single_frame_count_ > 0 && ShouldCreateRenderDocCapture())
+        else
         {
-            if (const RENDERDOC_API_1_0_0* renderdoc = GetRenderDocApi(); renderdoc != nullptr)
-            {
-                renderdoc->SetCaptureFilePathTemplate(
-                    Dive::GetRenderDocCaptureFilePathTemplate(gfxr_file_name_).string().c_str());
-                // Let RenderDoc choose the Vulkan context and window handle since we typically only
-                // expect one of each.
-                renderdoc->StartFrameCapture(/*device=*/nullptr, /*wndHandle=*/nullptr);
-            }
-            else
-            {
-                GFXRECON_LOG_DEBUG("GetRenderDocApi failed! Could not start RenderDoc capture.");
-            }
+            GFXRECON_LOG_DEBUG("GetRenderDocApi failed! Could not start RenderDoc capture.");
         }
     }
-
-    return success;
 }
 
 void DiveFileProcessor::StoreBlockInfo()
 {
-    if (gfxr_file_name_.empty())
+    std::shared_ptr<FileInputStream> gfxr_file = gfxr_file_.lock();
+    if (!gfxr_file)
     {
         // Assuming that the first time StoreBlockInfo() is called, the active file is .gfxr file
-        gfxr_file_name_ = GetActiveFilename();
-        GFXRECON_LOG_INFO("Storing active filename %s", gfxr_file_name_.c_str());
+        gfxr_file = file_stack_.back().active_file;
+        gfxr_file_ = gfxr_file;
+        GFXRECON_LOG_INFO("Storing active filename %s", gfxr_file->GetFilename().c_str());
     }
 
     if (!dive_block_data_)
     {
         return;
     }
 
-    int64_t offset = TellFile(gfxr_file_name_);
+    int64_t offset = gfxr_file->FileTell();
     GFXRECON_ASSERT(offset > 0);
     dive_block_data_->AddOriginalBlock(block_index_, static_cast<uint64_t>(offset));
 }
 
 GFXRECON_END_NAMESPACE(decode)
-GFXRECON_END_NAMESPACE(gfxrecon)
+GFXRECON_END_NAMESPACE(gfxrecon)

gfxr_ext/decode/dive_file_processor.h

@@ -23,6 +23,7 @@ limitations under the License.
 
 #include <memory>
 
+#include "decode/block_parser.h"
 #include "decode/file_processor.h"
 #include "dive_block_data.h"
 
@@ -41,11 +42,9 @@ class DiveFileProcessor : public FileProcessor
     bool WriteFile(const std::string& name, const std::string& content);
 
  protected:
-    bool ProcessFrameMarker(const format::BlockHeader& block_header, format::MarkerType marker_type,
-                            bool& should_break) override;
+    bool ProcessFrameDelimiter(const FrameEndMarkerArgs& end_frame) override;
 
-    bool ProcessStateMarker(const format::BlockHeader& block_header,
-                            format::MarkerType marker_type) override;
+    void ProcessStateEndMarker(const StateEndMarkerArgs& state_end) override;
 
     void StoreBlockInfo() override;
 
@@ -63,8 +62,9 @@ class DiveFileProcessor : public FileProcessor
     // modifications
     std::shared_ptr<DiveBlockData> dive_block_data_ = nullptr;
 
-    // Need to store this because the active file is sometimes the .gfxa one
-    std::string gfxr_file_name_ = "";
+    // Need to store this because the active file is sometimes the .gfxa one. Since the parent class
+    // "owns" this value, avoid sharing ownership and accidentally extending lifetime beyond use.
+    std::weak_ptr<FileInputStream> gfxr_file_;
 };
 
 GFXRECON_END_NAMESPACE(decode)