Skip to content

Commit

Permalink
Fix unintentional cpu_sync=true and deprecate sk_sp<SkSurface> APIs
Browse files Browse the repository at this point in the history
As per the linked bugs, this was causing a severe performance
regression.

Change-Id: Ibacf842408cee1eb434795437c8bbe0dc5244c8e
Bug: b/291711510, chromium:1475906
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/750403
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Kevin Lubick <kjlubick@google.com>
(cherry picked from commit 02fa147)
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/751516
  • Loading branch information
kjlubick authored and SkCQ committed Sep 5, 2023
1 parent ffbc376 commit 37822b7
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 8 deletions.
21 changes: 17 additions & 4 deletions include/gpu/GrDirectContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -462,12 +462,14 @@ class SK_API GrDirectContext : public GrRecordingContext {
* @param access type of access the call will do on the backend object after flush
* @param info flush options
*/
GrSemaphoresSubmitted flush(sk_sp<SkSurface> surface,
GrSemaphoresSubmitted flush(SkSurface* surface,
SkSurfaces::BackendSurfaceAccess access,
const GrFlushInfo& info);
GrSemaphoresSubmitted flush(SkSurface* surface,
#if !defined(SK_DISABLE_LEGACY_GRDIRECTCONTEXT_FLUSH)
GrSemaphoresSubmitted flush(sk_sp<SkSurface> surface,
SkSurfaces::BackendSurfaceAccess access,
const GrFlushInfo& info);
#endif

/**
* Same as above except:
Expand All @@ -488,12 +490,15 @@ class SK_API GrDirectContext : public GrRecordingContext {
* @param info flush options
* @param newState optional state change request after flush
*/
GrSemaphoresSubmitted flush(sk_sp<SkSurface> surface,
GrSemaphoresSubmitted flush(SkSurface* surface,
const GrFlushInfo& info,
const skgpu::MutableTextureState* newState = nullptr);
GrSemaphoresSubmitted flush(SkSurface* surface,
#if !defined(SK_DISABLE_LEGACY_GRDIRECTCONTEXT_FLUSH)
// TODO(kjlubick) Remove this variant to be consistent with flushAndSubmit
GrSemaphoresSubmitted flush(sk_sp<SkSurface> surface,
const GrFlushInfo& info,
const skgpu::MutableTextureState* newState = nullptr);
#endif

/** Call to ensure all reads/writes of the surface have been issued to the underlying 3D API.
* Skia will correctly order its own draws and pixel operations. This must to be used to ensure
Expand All @@ -503,14 +508,22 @@ class SK_API GrDirectContext : public GrRecordingContext {
*
* Has no effect on a CPU-backed surface.
*/
void flushAndSubmit(SkSurface* surface, bool syncCpu = false);
#if !defined(SK_DISABLE_LEGACY_GRDIRECTCONTEXT_FLUSH)
// TODO(kjlubick) remove this as it is error prone https://crbug.com/1475906
void flushAndSubmit(sk_sp<SkSurface> surface, bool syncCpu = false);
#endif

/**
* Flushes the given surface with the default GrFlushInfo.
*
* Has no effect on a CPU-backed surface.
*/
void flush(SkSurface* surface);
#if !defined(SK_DISABLE_LEGACY_GRDIRECTCONTEXT_FLUSH)
// TODO(kjlubick) Remove this variant to be consistent with flushAndSubmit
void flush(sk_sp<SkSurface> surface);
#endif

/**
* Submit outstanding work to the gpu from all previously un-submitted flushes. The return
Expand Down
2 changes: 2 additions & 0 deletions relnotes/grdirectctx.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
`GrDirectContext::flush` variants now expect a SkSurface pointer only, not
an sk_sp<SkSurface>.
21 changes: 20 additions & 1 deletion src/gpu/ganesh/GrDirectContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -496,11 +496,13 @@ void GrDirectContext::flushAndSubmit(sk_sp<const SkImage> image) {
this->submit();
}

#if !defined(SK_DISABLE_LEGACY_GRDIRECTCONTEXT_FLUSH)
GrSemaphoresSubmitted GrDirectContext::flush(sk_sp<SkSurface> surface,
SkSurfaces::BackendSurfaceAccess access,
const GrFlushInfo& info) {
return this->flush(surface.get(), access, info);
}
#endif

GrSemaphoresSubmitted GrDirectContext::flush(SkSurface* surface,
SkSurfaces::BackendSurfaceAccess access,
Expand All @@ -512,18 +514,21 @@ GrSemaphoresSubmitted GrDirectContext::flush(SkSurface* surface,
if (!sb->isGaneshBacked()) {
return GrSemaphoresSubmitted::kNo;
}

auto gs = static_cast<SkSurface_Ganesh*>(surface);
SkASSERT(this->priv().matches(gs->getDevice()->recordingContext()->asDirectContext()));
GrRenderTargetProxy* rtp = gs->getDevice()->targetProxy();

return this->priv().flushSurface(rtp, access, info, nullptr);
}

#if !defined(SK_DISABLE_LEGACY_GRDIRECTCONTEXT_FLUSH)
GrSemaphoresSubmitted GrDirectContext::flush(sk_sp<SkSurface> surface,
const GrFlushInfo& info,
const skgpu::MutableTextureState* newState) {
return this->flush(surface.get(), info, newState);
}
#endif

GrSemaphoresSubmitted GrDirectContext::flush(SkSurface* surface,
const GrFlushInfo& info,
Expand All @@ -535,6 +540,7 @@ GrSemaphoresSubmitted GrDirectContext::flush(SkSurface* surface,
if (!sb->isGaneshBacked()) {
return GrSemaphoresSubmitted::kNo;
}

auto gs = static_cast<SkSurface_Ganesh*>(surface);
SkASSERT(this->priv().matches(gs->getDevice()->recordingContext()->asDirectContext()));
GrRenderTargetProxy* rtp = gs->getDevice()->targetProxy();
Expand All @@ -543,14 +549,27 @@ GrSemaphoresSubmitted GrDirectContext::flush(SkSurface* surface,
rtp, SkSurfaces::BackendSurfaceAccess::kNoAccess, info, newState);
}

void GrDirectContext::flushAndSubmit(SkSurface* surface, bool syncCpu) {
this->flush(surface, SkSurfaces::BackendSurfaceAccess::kNoAccess, GrFlushInfo());
this->submit(syncCpu);
}

#if !defined(SK_DISABLE_LEGACY_GRDIRECTCONTEXT_FLUSH)
void GrDirectContext::flushAndSubmit(sk_sp<SkSurface> surface, bool syncCpu) {
this->flush(surface.get(), SkSurfaces::BackendSurfaceAccess::kNoAccess, GrFlushInfo());
this->submit(syncCpu);
}
#endif

void GrDirectContext::flush(SkSurface* surface) {
this->flush(surface, GrFlushInfo(), nullptr);
}

#if !defined(SK_DISABLE_LEGACY_GRDIRECTCONTEXT_FLUSH)
void GrDirectContext::flush(sk_sp<SkSurface> surface) {
this->flush(surface.get(), GrFlushInfo(), nullptr);
this->flush(surface, GrFlushInfo(), nullptr);
}
#endif

////////////////////////////////////////////////////////////////////////////////

Expand Down
6 changes: 3 additions & 3 deletions src/gpu/ganesh/surface/SkSurface_Ganesh.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -775,7 +775,7 @@ GrSemaphoresSubmitted Flush(sk_sp<SkSurface> surface) {
return GrSemaphoresSubmitted::kNo;
}
if (auto rContext = surface->recordingContext(); rContext != nullptr) {
return rContext->asDirectContext()->flush(surface, {});
return rContext->asDirectContext()->flush(surface.get(), {});
}
return GrSemaphoresSubmitted::kNo;
}
Expand All @@ -785,7 +785,7 @@ void FlushAndSubmit(SkSurface* surface) {
return;
}
if (auto rContext = surface->recordingContext(); rContext != nullptr) {
rContext->asDirectContext()->flushAndSubmit(surface);
rContext->asDirectContext()->flushAndSubmit(surface, false);
}
}

Expand All @@ -794,7 +794,7 @@ void FlushAndSubmit(sk_sp<SkSurface> surface) {
return;
}
if (auto rContext = surface->recordingContext(); rContext != nullptr) {
rContext->asDirectContext()->flushAndSubmit(surface);
rContext->asDirectContext()->flushAndSubmit(surface.get(), false);
}
}

Expand Down

0 comments on commit 37822b7

Please sign in to comment.