Commit ba73538c authored by Caleb Raitto's avatar Caleb Raitto Committed by Commit Bot

Don't serialize SkPath::getGenerationID() identifiability digests.

These IDs introduce variance in digest values for the same set of canvas
2D context operations.

Also, add some unit test coverage that we were missing.

Bug: 973801
Change-Id: I12728c5ab4dcd46b6a21bbcfbb18066652ebbf93
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2243731
Commit-Queue: Caleb Raitto <caraitto@chromium.org>
Reviewed-by: default avatarKhushal <khushalsagar@chromium.org>
Reviewed-by: default avatarFernando Serboncini <fserb@chromium.org>
Reviewed-by: default avatarKenneth Russell <kbr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792371}
parent dc5e434f
...@@ -187,6 +187,13 @@ class CC_PAINT_EXPORT PaintOp { ...@@ -187,6 +187,13 @@ class CC_PAINT_EXPORT PaintOp {
// The flags to use when serializing this op. This can be used to override // The flags to use when serializing this op. This can be used to override
// the flags serialized with the op. Valid only for PaintOpWithFlags. // the flags serialized with the op. Valid only for PaintOpWithFlags.
const PaintFlags* flags_to_serialize = nullptr; const PaintFlags* flags_to_serialize = nullptr;
// TODO(crbug.com/1096123): Cleanup after study completion.
//
// If true, perform serializaion in a way that avoids serializing transient
// members, such as IDs, so that a stable digest can be calculated. This
// means that serialized output can't be deserialized correctly.
bool for_identifiability_study = false;
}; };
struct CC_PAINT_EXPORT DeserializeOptions { struct CC_PAINT_EXPORT DeserializeOptions {
......
...@@ -174,7 +174,8 @@ void PaintOpWriter::Write(const SkRRect& rect) { ...@@ -174,7 +174,8 @@ void PaintOpWriter::Write(const SkRRect& rect) {
void PaintOpWriter::Write(const SkPath& path) { void PaintOpWriter::Write(const SkPath& path) {
auto id = path.getGenerationID(); auto id = path.getGenerationID();
Write(id); if (!options_.for_identifiability_study)
Write(id);
if (options_.paint_cache->Get(PaintCacheDataType::kPath, id)) { if (options_.paint_cache->Get(PaintCacheDataType::kPath, id)) {
Write(static_cast<uint32_t>(PaintCacheEntryState::kCached)); Write(static_cast<uint32_t>(PaintCacheEntryState::kCached));
...@@ -511,7 +512,8 @@ void PaintOpWriter::Write(const PaintShader* shader, SkFilterQuality quality) { ...@@ -511,7 +512,8 @@ void PaintOpWriter::Write(const PaintShader* shader, SkFilterQuality quality) {
if (shader->record_) { if (shader->record_) {
Write(true); Write(true);
DCHECK_NE(shader->id_, PaintShader::kInvalidRecordShaderId); DCHECK_NE(shader->id_, PaintShader::kInvalidRecordShaderId);
Write(shader->id_); if (!options_.for_identifiability_study)
Write(shader->id_);
const gfx::Rect playback_rect( const gfx::Rect playback_rect(
gfx::ToEnclosingRect(gfx::SkRectToRectF(shader->tile()))); gfx::ToEnclosingRect(gfx::SkRectToRectF(shader->tile())));
......
...@@ -1928,6 +1928,7 @@ jumbo_source_set("blink_platform_unittests_sources") { ...@@ -1928,6 +1928,7 @@ jumbo_source_set("blink_platform_unittests_sources") {
"graphics/gpu/webgpu_image_bitmap_handler_test.cc", "graphics/gpu/webgpu_image_bitmap_handler_test.cc",
"graphics/gpu/webgpu_swap_buffer_provider_test.cc", "graphics/gpu/webgpu_swap_buffer_provider_test.cc",
"graphics/graphics_context_test.cc", "graphics/graphics_context_test.cc",
"graphics/identifiability_paint_op_digest_unittest.cc",
"graphics/lab_color_space_test.cc", "graphics/lab_color_space_test.cc",
"graphics/paint/cull_rect_test.cc", "graphics/paint/cull_rect_test.cc",
"graphics/paint/display_item_client_test.cc", "graphics/paint/display_item_client_test.cc",
......
...@@ -35,6 +35,7 @@ IdentifiabilityPaintOpDigest::IdentifiabilityPaintOpDigest(IntSize size) ...@@ -35,6 +35,7 @@ IdentifiabilityPaintOpDigest::IdentifiabilityPaintOpDigest(IntSize size)
/*content_supports_distance_field_text=*/false, /*content_supports_distance_field_text=*/false,
/*max_texture_size=*/0, /*max_texture_size=*/0,
/*original_ctm=*/SkMatrix::I()) { /*original_ctm=*/SkMatrix::I()) {
serialize_options_.for_identifiability_study = true;
constexpr size_t kInitialSize = 16 * 1024; constexpr size_t kInitialSize = 16 * 1024;
if (IsUserInIdentifiabilityStudy() && if (IsUserInIdentifiabilityStudy() &&
SerializationBuffer().size() < kInitialSize) SerializationBuffer().size() < kInitialSize)
......
...@@ -11,6 +11,7 @@ ...@@ -11,6 +11,7 @@
#include "cc/paint/image_provider.h" #include "cc/paint/image_provider.h"
#include "cc/paint/paint_cache.h" #include "cc/paint/paint_cache.h"
#include "third_party/blink/renderer/platform/geometry/int_size.h" #include "third_party/blink/renderer/platform/geometry/int_size.h"
#include "third_party/blink/renderer/platform/platform_export.h"
#include "third_party/skia/include/utils/SkNoDrawCanvas.h" #include "third_party/skia/include/utils/SkNoDrawCanvas.h"
namespace blink { namespace blink {
...@@ -19,7 +20,7 @@ namespace blink { ...@@ -19,7 +20,7 @@ namespace blink {
// the serialization of PaintOps generated by those operations. // the serialization of PaintOps generated by those operations.
// //
// TODO(crbug.com/973801): Report results poisoning when dropping PaintOps. // TODO(crbug.com/973801): Report results poisoning when dropping PaintOps.
class IdentifiabilityPaintOpDigest { class PLATFORM_EXPORT IdentifiabilityPaintOpDigest {
public: public:
// Constructs based on the size of the CanvasResourceProvider. // Constructs based on the size of the CanvasResourceProvider.
explicit IdentifiabilityPaintOpDigest(IntSize size); explicit IdentifiabilityPaintOpDigest(IntSize size);
......
// Copyright 2020 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "third_party/blink/renderer/platform/graphics/identifiability_paint_op_digest.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/blink/public/common/privacy_budget/identifiability_study_settings.h"
#include "third_party/blink/public/common/privacy_budget/identifiability_study_settings_provider.h"
#include "third_party/skia/include/core/SkPath.h"
#include "third_party/skia/include/core/SkTileMode.h"
namespace blink {
namespace {
// A IdentifiabilityStudySettingsProvider implementation that opts-into study
// participation.
class ActiveSettingsProvider : public IdentifiabilityStudySettingsProvider {
public:
bool IsActive() const override { return true; }
// The following return values don't matter.
bool IsAnyTypeOrSurfaceBlocked() const override { return true; }
bool IsSurfaceAllowed(IdentifiableSurface surface) const override {
return false;
}
bool IsTypeAllowed(IdentifiableSurface::Type type) const override {
return false;
}
};
// An RAII class that opts into study participation using
// ActiveSettingsProvider.
class StudyParticipationRaii {
public:
StudyParticipationRaii() {
IdentifiabilityStudySettings::SetGlobalProvider(
std::make_unique<ActiveSettingsProvider>());
}
~StudyParticipationRaii() {
IdentifiabilityStudySettings::ResetStateForTesting();
}
};
// Arbitrary non-zero size.
constexpr IntSize kSize(10, 10);
TEST(IdentifiabilityPaintOpDigestTest, Construct) {
IdentifiabilityPaintOpDigest identifiability_paintop_digest(kSize);
}
TEST(IdentifiabilityPaintOpDigestTest, Construct_InStudy) {
StudyParticipationRaii study_participation_raii;
IdentifiabilityPaintOpDigest identifiability_paintop_digest(kSize);
}
TEST(IdentifiabilityPaintOpDigestTest, InitialDigestIsZero) {
StudyParticipationRaii study_participation_raii;
IdentifiabilityPaintOpDigest identifiability_paintop_digest(kSize);
auto paint_record = sk_make_sp<cc::PaintRecord>();
identifiability_paintop_digest.MaybeUpdateDigest(paint_record,
/*num_ops_to_visit=*/1);
EXPECT_EQ(UINT64_C(0), identifiability_paintop_digest.digest());
}
TEST(IdentifiabilityPaintOpDigestTest, SimpleDigest) {
StudyParticipationRaii study_participation_raii;
IdentifiabilityPaintOpDigest identifiability_paintop_digest(kSize);
auto paint_record = sk_make_sp<cc::PaintRecord>();
paint_record->push<cc::ScaleOp>(1.0f, 1.0f);
identifiability_paintop_digest.MaybeUpdateDigest(paint_record,
/*num_ops_to_visit=*/1);
EXPECT_EQ(UINT64_C(5461154373811575393),
identifiability_paintop_digest.digest());
}
TEST(IdentifiabilityPaintOpDigestTest, DigestIsZeroIfNotInStudy) {
IdentifiabilityPaintOpDigest identifiability_paintop_digest(kSize);
auto paint_record = sk_make_sp<cc::PaintRecord>();
paint_record->push<cc::ScaleOp>(1.0f, 1.0f);
identifiability_paintop_digest.MaybeUpdateDigest(paint_record,
/*num_ops_to_visit=*/1);
EXPECT_EQ(UINT64_C(0), identifiability_paintop_digest.digest());
}
TEST(IdentifiabilityPaintOpDigestTest, SkipsTextOps) {
StudyParticipationRaii study_participation_raii;
IdentifiabilityPaintOpDigest identifiability_paintop_digest(kSize);
auto paint_record = sk_make_sp<cc::PaintRecord>();
paint_record->push<cc::DrawTextBlobOp>(
SkTextBlob::MakeFromString("abc", SkFont(SkTypeface::MakeDefault())),
1.0f, 1.0f, cc::PaintFlags());
identifiability_paintop_digest.MaybeUpdateDigest(paint_record,
/*num_ops_to_visit=*/1);
EXPECT_EQ(UINT64_C(0), identifiability_paintop_digest.digest());
}
TEST(IdentifiabilityPaintOpDigestTest, SkPathDigestStability) {
StudyParticipationRaii study_participation_raii;
// These 2 SkPath objects will have different internal generation IDs. We
// can't use empty paths as in that case the internal SkPathRefs (which holds
// the ID) for each SkPath would be the same global "empty" SkPathRef, so we
// make the SkPaths non-empty.
SkPath path1;
path1.rLineTo(1.0f, 0.0f);
SkPath path2;
path2.rLineTo(1.0f, 0.0f);
IdentifiabilityPaintOpDigest identifiability_paintop_digest1(kSize);
IdentifiabilityPaintOpDigest identifiability_paintop_digest2(kSize);
auto paint_record1 = sk_make_sp<cc::PaintRecord>();
auto paint_record2 = sk_make_sp<cc::PaintRecord>();
paint_record1->push<cc::DrawPathOp>(path1, cc::PaintFlags());
paint_record2->push<cc::DrawPathOp>(path2, cc::PaintFlags());
identifiability_paintop_digest1.MaybeUpdateDigest(paint_record1,
/*num_ops_to_visit=*/1);
identifiability_paintop_digest2.MaybeUpdateDigest(paint_record2,
/*num_ops_to_visit=*/1);
EXPECT_EQ(identifiability_paintop_digest1.digest(),
identifiability_paintop_digest2.digest());
EXPECT_EQ(UINT64_C(154630961637231072),
identifiability_paintop_digest1.digest());
}
TEST(IdentifiabilityPaintOpDigestTest, PaintShaderStability) {
StudyParticipationRaii study_participation_raii;
SkPath path;
path.rLineTo(1.0f, 0.0f);
IdentifiabilityPaintOpDigest identifiability_paintop_digest1(kSize);
IdentifiabilityPaintOpDigest identifiability_paintop_digest2(kSize);
auto paint_record1 = sk_make_sp<cc::PaintRecord>();
auto paint_record2 = sk_make_sp<cc::PaintRecord>();
auto paint_record_shader = sk_make_sp<cc::PaintRecord>();
paint_record_shader->push<cc::ScaleOp>(2.0f, 2.0f);
const SkRect tile = SkRect::MakeWH(100, 100);
// These 2 shaders will have different internal generation IDs.
cc::PaintFlags paint_flags1;
paint_flags1.setShader(cc::PaintShader::MakePaintRecord(
paint_record_shader, tile, SkTileMode::kClamp, SkTileMode::kClamp,
nullptr));
cc::PaintFlags paint_flags2;
paint_flags2.setShader(cc::PaintShader::MakePaintRecord(
paint_record_shader, tile, SkTileMode::kClamp, SkTileMode::kClamp,
nullptr));
paint_record1->push<cc::DrawPathOp>(path, paint_flags1);
paint_record2->push<cc::DrawPathOp>(path, paint_flags2);
identifiability_paintop_digest1.MaybeUpdateDigest(paint_record1,
/*num_ops_to_visit=*/1);
identifiability_paintop_digest2.MaybeUpdateDigest(paint_record2,
/*num_ops_to_visit=*/1);
EXPECT_EQ(identifiability_paintop_digest1.digest(),
identifiability_paintop_digest2.digest());
EXPECT_EQ(UINT64_C(4893847605848349986),
identifiability_paintop_digest1.digest());
}
} // namespace
} // namespace blink
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment