Skip to content
This repository has been archived by the owner on Feb 25, 2025. It is now read-only.

Commit

Permalink
Revert "Update DDL test harness to use backendTextures to back tiles"
Browse files Browse the repository at this point in the history 
This reverts commit 7ae9d2f.

Reason for revert: Triggering Vulkan Debug layer errors

Original change's description:
> Update DDL test harness to use backendTextures to back tiles
> 
> This better matches Chrome's use of DDLs.
> 
> With path, image, and text draws stripped out, here is the perf impact of this change:
> 
>            before CL   after CL
> w/ DDLs      7.792      1.038
> w/o DDLs     0.800      0.876
> 
> This perf improvement (in the DDL case) is from backend texture wrapping SkSurfaces being created w/o initialization. The prior method of SkSurface creation was resulting in double clearing of all the surfaces.
> 
> This perf improvement won't be seen by Chrome since they've always being using wrapped backend texture SkSurfaces.
> 
> TBR=bsalomon@google.com
> Change-Id: Ice3993ca125fce37804e58c353c265cf659dbe2f
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/283456
> Reviewed-by: Brian Salomon <bsalomon@google.com>
> Commit-Queue: Robert Phillips <robertphillips@google.com>

TBR=egdaniel@google.com,bsalomon@google.com,robertphillips@google.com

Change-Id: Ife023ede0774ec2cce4c0d6e7708c036347ebf54
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/283648
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
  • Loading branch information
@rphilli
rphilli authored and Skia Commit-Bot committed Apr 15, 2020
1 parent 7ae9d2f commit 9ff1d84
Show file tree
Hide file tree
Showing 5 changed files with 25 additions and 138 deletions.
10 changes: 1 addition & 9 deletions dm/DMSrcSink.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1662,8 +1662,6 @@ Result GPUDDLSink::ddlDraw(const Src& src,
constexpr int kNumDivisions = 3;
DDLTileHelper tiles(dstSurface, dstCharacterization, viewport, kNumDivisions);

tiles.createBackendTextures(gpuTaskGroup, gpuThreadCtx);

// Reinflate the compressed picture individually for each thread.
tiles.createSKPPerTile(compressedPictureData.get(), promiseImageHelper);

Expand All @@ -1679,8 +1677,6 @@ Result GPUDDLSink::ddlDraw(const Src& src,
// It is simpler to also delete them at this point on the gpuThread.
promiseImageHelper.deleteAllFromGPU(gpuTaskGroup, gpuThreadCtx);

tiles.deleteBackendTextures(gpuTaskGroup, gpuThreadCtx);

// A flush has already been scheduled on the gpu thread along with the clean up of the backend
// textures so it is safe to schedule making 'mainCtx' not current on the gpuThread.
gpuTaskGroup->add([gpuTestCtx] { gpuTestCtx->makeNotCurrent(); });
Expand Down Expand Up @@ -2102,8 +2098,6 @@ Result ViaDDL::draw(const Src& src, SkBitmap* bitmap, SkWStream* stream, SkStrin
// First, create all the tiles (including their individual dest surfaces)
DDLTileHelper tiles(dstSurface, dstCharacterization, viewport, fNumDivisions);

tiles.createBackendTextures(nullptr, context);

// Second, reinflate the compressed picture individually for each thread
// This recreates the promise SkImages on each replay iteration. We are currently
// relying on this to test using a SkPromiseImageTexture to fulfill different
Expand All @@ -2127,10 +2121,8 @@ Result ViaDDL::draw(const Src& src, SkBitmap* bitmap, SkWStream* stream, SkStrin
// Finally, compose the drawn tiles into the result
// Note: the separation between the tiles and the final composition better
// matches Chrome but costs us a copy
tiles.composeAllTiles(context);
tiles.composeAllTiles();
context->flush();

tiles.deleteBackendTextures(nullptr, context);
}
return Result::Ok();
};
Expand Down
6 changes: 1 addition & 5 deletions include/core/SkDeferredDisplayList.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,7 @@ class SkDeferredDisplayList {
public:
// Upon being replayed - this field will be filled in (by the DrawingManager) with the
// proxy backing the destination SkSurface. Note that, since there is no good place to
// clear it, it can become a dangling pointer. Additionally, since the renderTargetProxy
// doesn't get a ref here, the SkSurface that owns it must remain alive until the DDL
// is flushed.
// TODO: the drawing manager could ref the renderTargetProxy for the DDL and then add
// a renderingTask to unref it after the DDL's ops have been executed.
// clear it, it can become a dangling pointer.
GrRenderTargetProxy* fReplayDest = nullptr;
#endif
};
Expand Down
120 changes: 19 additions & 101 deletions tools/DDLTileHelper.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ void DDLTileHelper::TileData::init(int id,

fCharacterization = dstSurfaceCharacterization.createResized(clip.width(), clip.height());
SkASSERT(fCharacterization.isValid());
SkASSERT(!fBackendTexture.isValid());
}

DDLTileHelper::TileData::~TileData() {}
Expand Down Expand Up @@ -92,100 +91,51 @@ void DDLTileHelper::TileData::precompile(GrContext* context) {
}
}

sk_sp<SkSurface> DDLTileHelper::TileData::makeWrappedTileDest(GrContext* context) {
if (!fBackendTexture.isValid()) {
return nullptr;
}

return SkSurface::MakeFromBackendTexture(context,
fBackendTexture,
fCharacterization.origin(),
fCharacterization.sampleCount(),
fCharacterization.colorType(),
fCharacterization.refColorSpace(),
&fCharacterization.surfaceProps());
}

void DDLTileHelper::TileData::drawSKPDirectly(GrContext* context) {
SkASSERT(!fDisplayList && !fTileSurface && fReconstitutedPicture);
SkASSERT(!fDisplayList && !fImage && fReconstitutedPicture);

fTileSurface = this->makeWrappedTileDest(context);
if (fTileSurface) {
SkCanvas* tileCanvas = fTileSurface->getCanvas();
sk_sp<SkSurface> tileSurface = SkSurface::MakeRenderTarget(context, fCharacterization,
SkBudgeted::kYes);
if (tileSurface) {
SkCanvas* tileCanvas = tileSurface->getCanvas();

tileCanvas->clipRect(SkRect::MakeWH(fClip.width(), fClip.height()));
tileCanvas->translate(-fClip.fLeft, -fClip.fTop);

tileCanvas->drawPicture(fReconstitutedPicture);

// We can't snap an image here bc, since we're using wrapped backend textures for the
// surfaces, that would incur a copy.
fImage = tileSurface->makeImageSnapshot();
}
}

void DDLTileHelper::TileData::draw(GrContext* context) {
SkASSERT(fDisplayList && !fTileSurface);
SkASSERT(fDisplayList && !fImage);

// The tile's surface needs to be held until after the DDL is flushed
fTileSurface = this->makeWrappedTileDest(context);
if (fTileSurface) {
fTileSurface->draw(fDisplayList.get());
sk_sp<SkSurface> tileSurface = SkSurface::MakeRenderTarget(context, fCharacterization,
SkBudgeted::kYes);
if (tileSurface) {
tileSurface->draw(fDisplayList.get());

// We can't snap an image here bc, since we're using wrapped backend textures for the
// surfaces, that would incur a copy.
fImage = tileSurface->makeImageSnapshot();
}
}

// TODO: We should create a single DDL for the composition step and just add replaying it
// as the last GPU task
void DDLTileHelper::TileData::compose(GrContext* context) {
SkASSERT(context->priv().asDirectContext());
SkASSERT(fDstSurface);

if (!fBackendTexture.isValid()) {
return;
}

// Here we are, unfortunately, aliasing 'fBackendTexture'. It is backing both 'fTileSurface'
// and 'tmp'.
sk_sp<SkImage> tmp = SkImage::MakeFromTexture(context,
fBackendTexture,
fCharacterization.origin(),
fCharacterization.colorType(),
kPremul_SkAlphaType,
fCharacterization.refColorSpace());
void DDLTileHelper::TileData::compose() {
SkASSERT(fDstSurface && fImage);

SkCanvas* canvas = fDstSurface->getCanvas();
canvas->save();
canvas->clipRect(SkRect::Make(fClip));
canvas->drawImage(tmp, fClip.fLeft, fClip.fTop);
canvas->drawImage(fImage, fClip.fLeft, fClip.fTop);
canvas->restore();
}

void DDLTileHelper::TileData::reset() {
// TODO: when DDLs are re-renderable we don't need to do this
fDisplayList = nullptr;
fTileSurface = nullptr;
}

void DDLTileHelper::TileData::CreateBackendTexture(GrContext* context, TileData* tile) {
SkASSERT(context->priv().asDirectContext());
SkASSERT(!tile->fBackendTexture.isValid());

tile->fBackendTexture = context->createBackendTexture(tile->fCharacterization);
// TODO: it seems that, on the Linux bots, backend texture creation is failing
// a lot (skbug.com/10142)
//SkASSERT(tile->fBackendTexture.isValid());
}

void DDLTileHelper::TileData::DeleteBackendTexture(GrContext* context, TileData* tile) {
SkASSERT(context->priv().asDirectContext());
// TODO: it seems that, on the Linux bots, backend texture creation is failing
// a lot (skbug.com/10142)
//SkASSERT(tile->fBackendTexture.isValid());

tile->fTileSurface = nullptr;
context->deleteBackendTexture(tile->fBackendTexture);
fImage = nullptr;
}

///////////////////////////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -249,7 +199,7 @@ static void do_gpu_stuff(GrContext* context, DDLTileHelper::TileData* tile) {

// TODO: we should actually have a separate DDL that does
// the final composition draw
tile->compose(context);
tile->compose();
}

// We expect to have more than one recording thread but just one gpu thread
Expand Down Expand Up @@ -296,9 +246,9 @@ void DDLTileHelper::drawAllTilesDirectly(GrContext* context) {
}
}

void DDLTileHelper::composeAllTiles(GrContext* context) {
void DDLTileHelper::composeAllTiles() {
for (int i = 0; i < this->numTiles(); ++i) {
fTiles[i].compose(context);
fTiles[i].compose();
}
}

Expand All @@ -307,35 +257,3 @@ void DDLTileHelper::resetAllTiles() {
fTiles[i].reset();
}
}

void DDLTileHelper::createBackendTextures(SkTaskGroup* taskGroup, GrContext* context) {
SkASSERT(context->priv().asDirectContext());

if (taskGroup) {
for (int i = 0; i < this->numTiles(); ++i) {
TileData* tile = &fTiles[i];

taskGroup->add([context, tile]() { TileData::CreateBackendTexture(context, tile); });
}
} else {
for (int i = 0; i < this->numTiles(); ++i) {
TileData::CreateBackendTexture(context, &fTiles[i]);
}
}
}

void DDLTileHelper::deleteBackendTextures(SkTaskGroup* taskGroup, GrContext* context) {
SkASSERT(context->priv().asDirectContext());

if (taskGroup) {
for (int i = 0; i < this->numTiles(); ++i) {
TileData* tile = &fTiles[i];

taskGroup->add([context, tile]() { TileData::DeleteBackendTexture(context, tile); });
}
} else {
for (int i = 0; i < this->numTiles(); ++i) {
TileData::DeleteBackendTexture(context, &fTiles[i]);
}
}
}
18 changes: 3 additions & 15 deletions tools/DDLTileHelper.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,30 +55,21 @@ class DDLTileHelper {

// Draw the result of replaying the DDL (i.e., 'fImage') into the
// final destination surface ('fDstSurface').
void compose(GrContext*);
void compose();

void reset();

int id() const { return fID; }

SkDeferredDisplayList* ddl() { return fDisplayList.get(); }

static void CreateBackendTexture(GrContext*, TileData*);
static void DeleteBackendTexture(GrContext*, TileData*);

private:
sk_sp<SkSurface> makeWrappedTileDest(GrContext* context);

int fID = -1;
sk_sp<SkSurface> fDstSurface; // the ultimate target for composition

GrBackendTexture fBackendTexture; // destination for this tile's content
SkSurfaceCharacterization fCharacterization; // characterization for the tile's surface
SkIRect fClip; // in the device space of the 'fDstSurface'

// 'fTileSurface' wraps 'fBackendTexture' and must exist until after 'fDisplayList'
// has been flushed.
sk_sp<SkSurface> fTileSurface;
sk_sp<SkImage> fImage; // the result of replaying the DDL
sk_sp<SkPicture> fReconstitutedPicture;
SkTArray<sk_sp<SkImage>> fPromiseImages; // All the promise images in the
// reconstituted picture
Expand Down Expand Up @@ -114,15 +105,12 @@ class DDLTileHelper {
// DDLs first - all on a single thread.
void drawAllTilesDirectly(GrContext*);

void composeAllTiles(GrContext*);
void composeAllTiles();

void resetAllTiles();

int numTiles() const { return fNumDivisions * fNumDivisions; }

void createBackendTextures(SkTaskGroup*, GrContext*);
void deleteBackendTextures(SkTaskGroup*, GrContext*);

private:
int fNumDivisions; // number of tiles along a side
TileData* fTiles; // 'fNumDivisions' x 'fNumDivisions'
Expand Down
9 changes: 1 addition & 8 deletions tools/skpbench/skpbench.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -258,8 +258,6 @@ static void run_ddl_benchmark(GrContext* context, sk_sp<SkSurface> surface,

DDLTileHelper tiles(surface, dstCharacterization, viewport, FLAGS_ddlTilingWidthHeight);

tiles.createBackendTextures(nullptr, context);

tiles.createSKPPerTile(compressedPictureData.get(), promiseImageHelper);

SkTaskGroup::Enabler enabled(FLAGS_ddlNumAdditionalThreads);
Expand All @@ -285,7 +283,7 @@ static void run_ddl_benchmark(GrContext* context, sk_sp<SkSurface> surface,

if (!FLAGS_png.isEmpty()) {
// The user wants to see the final result
tiles.composeAllTiles(context);
tiles.composeAllTiles();
}

tiles.resetAllTiles();
Expand All @@ -295,11 +293,6 @@ static void run_ddl_benchmark(GrContext* context, sk_sp<SkSurface> surface,
GrFlushInfo flushInfo;
flushInfo.fFlags = kSyncCpu_GrFlushFlag;
context->flush(flushInfo);

promiseImageHelper.deleteAllFromGPU(nullptr, context);

tiles.deleteBackendTextures(nullptr, context);

}

static void run_benchmark(GrContext* context, SkSurface* surface, SkpProducer* skpp,
Expand Down

0 comments on commit 9ff1d84

Please sign in to comment.