[graphite] Add RecorderOptions.fRequireOrderedRecordings

Recorders already were tracking their ordered sequence of Recordings
separately from each other. This adds the ability for the client to
require (or not) that Recordings be inserted in order on a per-Recorder
basis.

This allows compositing-like Recorders more flexibility when it's
very likely they won't be relying on any internal atlasing (that takes
incurs overhead when there aren't ordering assumptions). But as long
as these unordered Recorders don't render text, they won't hit that
slow path.

Bug: b/406292843
Change-Id: I72c907003b07bd79177a81c88dcd6117c216f093
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/973737
Reviewed-by: Jim Van Verth <[email protected]>
Auto-Submit: Michael Ludwig <[email protected]>
Reviewed-by: Greg Daniel <[email protected]>
Commit-Queue: Michael Ludwig <[email protected]>

Author: lhkbob

include/gpu/graphite/ContextOptions.h

@@ -89,10 +89,18 @@ struct SK_API ContextOptions {
     bool fSupportBilerpFromGlyphAtlas = false;
 
     /**
-     * For the moment, if Recordings are replayed in the order they are recorded, then
-     * Graphite can make certain assumptions that allow for better performance. Otherwise
-     * we have to flush some caches at the start of each Recording to ensure that they can
+     * For the moment, if Recordings from the same Recorder are replayed in the order they are
+     * recorded, then Graphite can make certain assumptions that allow for better performance.
+     * Otherwise we have to flush some caches at the start of each Recording to ensure that they can
      * be played back properly.
+     *
+     * This is the default ordering requirement for a Recorder. It can be overridden by
+     * setting the same field on the RecorderOptions passed to makeRecorder.
+     *
+     * Regardless of this value or a per-Recorder's setting, Recordings from separate Recorders can
+     * always be inserted in any order but it is the application's responsible to ensure that any
+     * implicit dependencies between the Recorders are respected (e.g. rendering to an SkSurface
+     * in one Recorder and sampling from that SkSurface's SkImage view on another Recorder).
      */
     bool fRequireOrderedRecordings = false;
 

include/gpu/graphite/Recorder.h

@@ -16,6 +16,7 @@
 #include "include/private/base/SkTArray.h"
 
 #include <chrono>
+#include <optional>
 
 struct AHardwareBuffer;
 class SkCanvas;
@@ -70,6 +71,10 @@ struct SK_API RecorderOptions final {
     static constexpr size_t kDefaultRecorderBudget = 256 * (1 << 20);
     // What is the budget for GPU resources allocated and held by this Recorder.
     size_t fGpuBudgetInBytes = kDefaultRecorderBudget;
+    // If Recordings are known to be played back in the order they are recorded, then Graphite
+    // may be able to make certain assuptions that improve performance. This is often the case
+    // if the content being drawn triggers the use of internal atlasing in Graphite (e.g. text).
+    std::optional<bool> fRequireOrderedRecordings;
 };
 
 class SK_API Recorder final {
@@ -281,6 +286,8 @@ class SK_API Recorder final {
 
     uint32_t fUniqueID;  // Needed for MessageBox handling for text
     uint32_t fNextRecordingID = 1;
+    const bool fRequireOrderedRecordings;
+
     std::unique_ptr<AtlasProvider> fAtlasProvider;
     std::unique_ptr<TokenTracker> fTokenTracker;
     std::unique_ptr<sktext::gpu::StrikeCache> fStrikeCache;

include/gpu/graphite/Recording.h

@@ -72,7 +72,7 @@ class SK_API Recording final {
     bool addCommands(CommandBuffer*, ResourceProvider*);
     void addResourceRef(sk_sp<Resource>);
 
-    // Used to verify ordering
+    // Used to verify ordering if recorder ID is not SK_InvalidGenID
     uint32_t fUniqueID;
     uint32_t fRecorderID;
 

relnotes/recorder-ordering-override.md

@@ -0,0 +1,3 @@
+`RecorderOptions.fRequireOrderedRecordings` can now be used to specify a per-`Recorder` ordering
+policy for how its `Recordings` must be inserted into a `Context`. If not provided, the `Recorder`
+will default to the value in `ContextOptions`.

src/gpu/graphite/QueueManager.cpp

@@ -106,10 +106,12 @@ bool QueueManager::addRecording(const InsertRecordingInfo& info, Context* contex
         return false;
     }
 
-    if (fSharedContext->caps()->requireOrderedRecordings()) {
-        uint32_t* recordingID = fLastAddedRecordingIDs.find(info.fRecording->priv().recorderID());
-        if (recordingID &&
-            info.fRecording->priv().uniqueID() != *recordingID+1) {
+    // Recordings from a Recorder that requires ordered recordings will have a valid recorder ID.
+    // Recordings that don't have any required order are assigned SK_InvalidID.
+    uint32_t recorderID = info.fRecording->priv().recorderID();
+    if (recorderID != SK_InvalidGenID) {
+        uint32_t* recordingID = fLastAddedRecordingIDs.find(recorderID);
+        if (recordingID && info.fRecording->priv().uniqueID() != *recordingID + 1) {
             if (callback) {
                 callback->setFailureResult();
             }
@@ -118,8 +120,7 @@ bool QueueManager::addRecording(const InsertRecordingInfo& info, Context* contex
         }
 
         // Note the new Recording ID.
-        fLastAddedRecordingIDs.set(info.fRecording->priv().recorderID(),
-                                   info.fRecording->priv().uniqueID());
+        fLastAddedRecordingIDs.set(recorderID, info.fRecording->priv().uniqueID());
     }
 
     if (info.fTargetSurface &&

src/gpu/graphite/Recorder.cpp

@@ -102,6 +102,9 @@ Recorder::Recorder(sk_sp<SharedContext> sharedContext,
         , fTextureDataCache(new TextureDataCache)
         , fProxyReadCounts(new ProxyReadCountMap)
         , fUniqueID(next_id())
+        , fRequireOrderedRecordings(options.fRequireOrderedRecordings.has_value()
+                                            ? *options.fRequireOrderedRecordings
+                                            : fSharedContext->caps()->requireOrderedRecordings())
         , fAtlasProvider(std::make_unique<AtlasProvider>(this))
         , fTokenTracker(std::make_unique<TokenTracker>())
         , fStrikeCache(std::make_unique<sktext::gpu::StrikeCache>())
@@ -189,7 +192,8 @@ std::unique_ptr<Recording> Recorder::snap() {
     // Recorder doesn't hold a persistent manager and it can be deleted when snap() returns.
     ScratchResourceManager scratchManager{fResourceProvider, std::move(fProxyReadCounts)};
     std::unique_ptr<Recording> recording(new Recording(fNextRecordingID++,
-                                                       fUniqueID,
+                                                       fRequireOrderedRecordings ? fUniqueID
+                                                                                 : SK_InvalidGenID,
                                                        std::move(nonVolatileLazyProxies),
                                                        std::move(volatileLazyProxies),
                                                        std::move(fTargetProxyData),
@@ -227,7 +231,7 @@ std::unique_ptr<Recording> Recorder::snap() {
     fRuntimeEffectDict->reset();
     fProxyReadCounts = std::make_unique<ProxyReadCountMap>();
     fTextureDataCache = std::make_unique<TextureDataCache>();
-    if (!this->priv().caps()->requireOrderedRecordings()) {
+    if (!fRequireOrderedRecordings) {
         fAtlasProvider->invalidateAtlases();
     }
 

tests/graphite/RecorderTest.cpp

@@ -10,6 +10,7 @@
 #include "include/gpu/graphite/Context.h"
 #include "include/gpu/graphite/Recorder.h"
 #include "src/gpu/SkBackingFit.h"
+#include "src/gpu/graphite/ContextPriv.h"
 #include "src/gpu/graphite/Device.h"
 #include "src/gpu/graphite/RecorderPriv.h"
 
@@ -100,3 +101,46 @@ DEF_GRAPHITE_TEST_FOR_ALL_CONTEXTS(RecorderDevicePtrTest, reporter, context,
     device1.reset();
     device3.reset();
 }
+
+// Tests to make sure the Recorders can override the ordering requirement.
+DEF_GRAPHITE_TEST_FOR_ALL_CONTEXTS(RecorderOrderingTest, reporter, context,
+                                   CtsEnforcement::kNever) {
+    std::unique_ptr<Recorder> orderedRecorder, unorderedRecorder;
+
+    if (context->priv().caps()->requireOrderedRecordings()) {
+        orderedRecorder = context->makeRecorder();
+        RecorderOptions opts;
+        opts.fRequireOrderedRecordings = false;
+        unorderedRecorder = context->makeRecorder(opts);
+    } else {
+        RecorderOptions opts;
+        opts.fRequireOrderedRecordings = true;
+        orderedRecorder = context->makeRecorder(opts);
+        unorderedRecorder = context->makeRecorder();
+    }
+
+    auto insert = [context](Recording* r) {
+        InsertRecordingInfo info;
+        info.fRecording = r;
+        return context->insertRecording(info);
+    };
+
+    std::unique_ptr<Recording> o1 = orderedRecorder->snap();
+    std::unique_ptr<Recording> o2 = orderedRecorder->snap();
+    std::unique_ptr<Recording> o3 = orderedRecorder->snap();
+
+    std::unique_ptr<Recording> u1 = unorderedRecorder->snap();
+    std::unique_ptr<Recording> u2 = unorderedRecorder->snap();
+
+    // Unordered insertion of an unordered Recorder succeeds.
+    // NOTE: These Recordings are all out-of-order with respect to orderedRecorder, which had been
+    // snapped multiple times before unorderedRecorder. That is always allowed.
+    REPORTER_ASSERT(reporter, insert(u2.get()));
+    REPORTER_ASSERT(reporter, insert(u1.get()));
+
+    // Unordered insertion of an ordered Recorder fails
+    REPORTER_ASSERT(reporter, insert(o1.get())); // succeeds (first insertion)
+    REPORTER_ASSERT(reporter, !insert(o3.get())); // fails for out of order
+    REPORTER_ASSERT(reporter, insert(o2.get())); // succeeds and recovers
+    REPORTER_ASSERT(reporter, insert(o3.get())); // now in order success
+}