Commit 704b0c4e authored by Chris Fredrickson's avatar Chris Fredrickson Committed by Commit Bot

Revert "Include image data when reporting UKM from canvas readbacks."

This reverts commit 5bf5f652.

Reason for revert: This cl caused a perf regression in the rendering.mobile and blink_perf.canvas benchmarks.

Bug: 1092434,1087234,973801

Original change's description:
> Include image data when reporting UKM from canvas readbacks.
> 
> This is a reland of https://crrev.com/c/2207979, but which takes extra steps to avoid performance regressions. In particular:
> * No additional overhead is incurred if the user is not in the identifiability study.
> * The overhead of constructing the pixmap is posted to another thread, rather than holding up the current thread.
> 
> Bug: 973801,1087234
> Change-Id: I7701c622e4d146504b1ea57d0f2047aa2a98ed9f
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2224820
> Reviewed-by: Fernando Serboncini <fserb@chromium.org>
> Reviewed-by: Asanka Herath <asanka@chromium.org>
> Commit-Queue: Chris Fredrickson <cfredric@google.com>
> Cr-Commit-Position: refs/heads/master@{#775558}

TBR=fserb@chromium.org,asanka@chromium.org,cfredric@google.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 973801, 1087234
Change-Id: I5bc8452692562f8b82af0c3b0903327b86744cc9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2236336Reviewed-by: default avatarFernando Serboncini <fserb@chromium.org>
Commit-Queue: Chris Fredrickson <cfredric@google.com>
Cr-Commit-Position: refs/heads/master@{#776536}
parent 67a4e891
...@@ -8,9 +8,6 @@ ...@@ -8,9 +8,6 @@
#include "base/metrics/histogram_functions.h" #include "base/metrics/histogram_functions.h"
#include "base/metrics/histogram_macros.h" #include "base/metrics/histogram_macros.h"
#include "build/build_config.h" #include "build/build_config.h"
#include "third_party/blink/public/common/privacy_budget/identifiability_metric_builder.h"
#include "third_party/blink/public/common/privacy_budget/identifiability_metrics.h"
#include "third_party/blink/public/common/privacy_budget/identifiability_study_participation.h"
#include "third_party/blink/public/platform/platform.h" #include "third_party/blink/public/platform/platform.h"
#include "third_party/blink/public/platform/task_type.h" #include "third_party/blink/public/platform/task_type.h"
#include "third_party/blink/renderer/core/dom/document.h" #include "third_party/blink/renderer/core/dom/document.h"
...@@ -149,7 +146,6 @@ CanvasAsyncBlobCreator::CanvasAsyncBlobCreator( ...@@ -149,7 +146,6 @@ CanvasAsyncBlobCreator::CanvasAsyncBlobCreator(
ToBlobFunctionType function_type, ToBlobFunctionType function_type,
base::TimeTicks start_time, base::TimeTicks start_time,
ExecutionContext* context, ExecutionContext* context,
base::Optional<UkmParameters> ukm_params,
ScriptPromiseResolver* resolver) ScriptPromiseResolver* resolver)
: CanvasAsyncBlobCreator(image, : CanvasAsyncBlobCreator(image,
options, options,
...@@ -157,7 +153,6 @@ CanvasAsyncBlobCreator::CanvasAsyncBlobCreator( ...@@ -157,7 +153,6 @@ CanvasAsyncBlobCreator::CanvasAsyncBlobCreator(
nullptr, nullptr,
start_time, start_time,
context, context,
ukm_params,
resolver) {} resolver) {}
CanvasAsyncBlobCreator::CanvasAsyncBlobCreator( CanvasAsyncBlobCreator::CanvasAsyncBlobCreator(
...@@ -167,7 +162,6 @@ CanvasAsyncBlobCreator::CanvasAsyncBlobCreator( ...@@ -167,7 +162,6 @@ CanvasAsyncBlobCreator::CanvasAsyncBlobCreator(
V8BlobCallback* callback, V8BlobCallback* callback,
base::TimeTicks start_time, base::TimeTicks start_time,
ExecutionContext* context, ExecutionContext* context,
base::Optional<UkmParameters> ukm_params,
ScriptPromiseResolver* resolver) ScriptPromiseResolver* resolver)
: fail_encoder_initialization_for_test_(false), : fail_encoder_initialization_for_test_(false),
enforce_idle_encoding_for_test_(false), enforce_idle_encoding_for_test_(false),
...@@ -178,7 +172,6 @@ CanvasAsyncBlobCreator::CanvasAsyncBlobCreator( ...@@ -178,7 +172,6 @@ CanvasAsyncBlobCreator::CanvasAsyncBlobCreator(
start_time_(start_time), start_time_(start_time),
static_bitmap_image_loaded_(false), static_bitmap_image_loaded_(false),
callback_(callback), callback_(callback),
ukm_params_(ukm_params),
script_promise_resolver_(resolver) { script_promise_resolver_(resolver) {
DCHECK(image); DCHECK(image);
DCHECK(context); DCHECK(context);
...@@ -475,8 +468,6 @@ void CanvasAsyncBlobCreator::CreateBlobAndReturnResult() { ...@@ -475,8 +468,6 @@ void CanvasAsyncBlobCreator::CreateBlobAndReturnResult() {
WrapPersistent(result_blob))); WrapPersistent(result_blob)));
} }
RecordIdentifiabilityMetric();
RecordScaledDurationHistogram(mime_type_, RecordScaledDurationHistogram(mime_type_,
base::TimeTicks::Now() - start_time_, base::TimeTicks::Now() - start_time_,
image_->width(), image_->height()); image_->width(), image_->height());
...@@ -484,33 +475,6 @@ void CanvasAsyncBlobCreator::CreateBlobAndReturnResult() { ...@@ -484,33 +475,6 @@ void CanvasAsyncBlobCreator::CreateBlobAndReturnResult() {
Dispose(); Dispose();
} }
void CanvasAsyncBlobCreator::RecordIdentifiabilityMetric() {
if (!ukm_params_.has_value() || !IsUserInIdentifiabilityStudy())
return;
// Creating this ImageDataBuffer has some overhead, namely getting the SkImage
// and computing the pixmap.
context_->GetTaskRunner(TaskType::kInternalDefault)
->PostTask(
FROM_HERE,
WTF::Bind(
[](scoped_refptr<StaticBitmapImage> image,
UkmParameters ukm_params) {
std::unique_ptr<ImageDataBuffer> data_buffer =
ImageDataBuffer::Create(image);
if (!data_buffer)
return;
blink::IdentifiabilityMetricBuilder(ukm_params.source_id)
.Set(blink::IdentifiableSurface::FromTypeAndInput(
blink::IdentifiableSurface::Type::kCanvasReadback,
0),
blink::IdentifiabilityDigestOfBytes(
base::make_span(data_buffer->Pixels(),
data_buffer->ComputeByteSize())))
.Record(ukm_params.ukm_recorder);
},
image_, ukm_params_.value()));
}
void CanvasAsyncBlobCreator::CreateNullAndReturnResult() { void CanvasAsyncBlobCreator::CreateNullAndReturnResult() {
RecordIdleTaskStatusHistogram(idle_task_status_); RecordIdleTaskStatusHistogram(idle_task_status_);
if (function_type_ == kHTMLCanvasToBlobCallback) { if (function_type_ == kHTMLCanvasToBlobCallback) {
......
...@@ -8,14 +8,11 @@ ...@@ -8,14 +8,11 @@
#include <memory> #include <memory>
#include "base/location.h" #include "base/location.h"
#include "base/optional.h"
#include "base/single_thread_task_runner.h" #include "base/single_thread_task_runner.h"
#include "services/metrics/public/cpp/ukm_recorder.h"
#include "third_party/blink/renderer/bindings/core/v8/script_promise_resolver.h" #include "third_party/blink/renderer/bindings/core/v8/script_promise_resolver.h"
#include "third_party/blink/renderer/bindings/core/v8/v8_blob_callback.h" #include "third_party/blink/renderer/bindings/core/v8/v8_blob_callback.h"
#include "third_party/blink/renderer/bindings/core/v8/v8_image_encode_options.h" #include "third_party/blink/renderer/bindings/core/v8/v8_image_encode_options.h"
#include "third_party/blink/renderer/core/core_export.h" #include "third_party/blink/renderer/core/core_export.h"
#include "third_party/blink/renderer/core/html/canvas/ukm_parameters.h"
#include "third_party/blink/renderer/core/typed_arrays/dom_typed_array.h" #include "third_party/blink/renderer/core/typed_arrays/dom_typed_array.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/graphics/graphics_types.h" #include "third_party/blink/renderer/platform/graphics/graphics_types.h"
...@@ -63,7 +60,6 @@ class CORE_EXPORT CanvasAsyncBlobCreator ...@@ -63,7 +60,6 @@ class CORE_EXPORT CanvasAsyncBlobCreator
ToBlobFunctionType function_type, ToBlobFunctionType function_type,
base::TimeTicks start_time, base::TimeTicks start_time,
ExecutionContext*, ExecutionContext*,
base::Optional<UkmParameters> ukm_params,
ScriptPromiseResolver*); ScriptPromiseResolver*);
CanvasAsyncBlobCreator(scoped_refptr<StaticBitmapImage>, CanvasAsyncBlobCreator(scoped_refptr<StaticBitmapImage>,
const ImageEncodeOptions*, const ImageEncodeOptions*,
...@@ -71,7 +67,6 @@ class CORE_EXPORT CanvasAsyncBlobCreator ...@@ -71,7 +67,6 @@ class CORE_EXPORT CanvasAsyncBlobCreator
V8BlobCallback*, V8BlobCallback*,
base::TimeTicks start_time, base::TimeTicks start_time,
ExecutionContext*, ExecutionContext*,
base::Optional<UkmParameters> ukm_params,
ScriptPromiseResolver* = nullptr); ScriptPromiseResolver* = nullptr);
virtual ~CanvasAsyncBlobCreator(); virtual ~CanvasAsyncBlobCreator();
...@@ -137,8 +132,6 @@ class CORE_EXPORT CanvasAsyncBlobCreator ...@@ -137,8 +132,6 @@ class CORE_EXPORT CanvasAsyncBlobCreator
// Used for HTMLCanvasElement only // Used for HTMLCanvasElement only
Member<V8BlobCallback> callback_; Member<V8BlobCallback> callback_;
base::Optional<UkmParameters> ukm_params_;
// Used for OffscreenCanvas only // Used for OffscreenCanvas only
Member<ScriptPromiseResolver> script_promise_resolver_; Member<ScriptPromiseResolver> script_promise_resolver_;
...@@ -154,8 +147,6 @@ class CORE_EXPORT CanvasAsyncBlobCreator ...@@ -154,8 +147,6 @@ class CORE_EXPORT CanvasAsyncBlobCreator
void IdleTaskStartTimeoutEvent(double quality); void IdleTaskStartTimeoutEvent(double quality);
void IdleTaskCompleteTimeoutEvent(); void IdleTaskCompleteTimeoutEvent();
void RecordIdentifiabilityMetric();
}; };
} // namespace blink } // namespace blink
......
...@@ -35,7 +35,6 @@ class MockCanvasAsyncBlobCreator : public CanvasAsyncBlobCreator { ...@@ -35,7 +35,6 @@ class MockCanvasAsyncBlobCreator : public CanvasAsyncBlobCreator {
nullptr, nullptr,
base::TimeTicks(), base::TimeTicks(),
document->GetExecutionContext(), document->GetExecutionContext(),
base::make_optional<UkmParameters>(),
nullptr) { nullptr) {
if (fail_encoder_initialization) if (fail_encoder_initialization)
fail_encoder_initialization_for_test_ = true; fail_encoder_initialization_for_test_ = true;
...@@ -130,6 +129,7 @@ class CanvasAsyncBlobCreatorTest : public PageTestBase { ...@@ -130,6 +129,7 @@ class CanvasAsyncBlobCreatorTest : public PageTestBase {
void TearDown() override; void TearDown() override;
private: private:
Persistent<MockCanvasAsyncBlobCreator> async_blob_creator_; Persistent<MockCanvasAsyncBlobCreator> async_blob_creator_;
}; };
...@@ -264,8 +264,7 @@ TEST_F(CanvasAsyncBlobCreatorTest, ColorManagedConvertToBlob) { ...@@ -264,8 +264,7 @@ TEST_F(CanvasAsyncBlobCreatorTest, ColorManagedConvertToBlob) {
kDisplayP3ImageColorSpaceName, kDisplayP3ImageColorSpaceName,
kRec2020ImageColorSpaceName}; kRec2020ImageColorSpaceName};
std::list<String> blob_pixel_formats = { std::list<String> blob_pixel_formats = {
kRGBA8ImagePixelFormatName, kRGBA8ImagePixelFormatName, kRGBA16ImagePixelFormatName,
kRGBA16ImagePixelFormatName,
}; };
// Maximum differences are both observed locally with // Maximum differences are both observed locally with
...@@ -296,8 +295,7 @@ TEST_F(CanvasAsyncBlobCreatorTest, ColorManagedConvertToBlob) { ...@@ -296,8 +295,7 @@ TEST_F(CanvasAsyncBlobCreatorTest, ColorManagedConvertToBlob) {
source_bitmap_image, options, source_bitmap_image, options,
CanvasAsyncBlobCreator::ToBlobFunctionType:: CanvasAsyncBlobCreator::ToBlobFunctionType::
kHTMLCanvasConvertToBlobPromise, kHTMLCanvasConvertToBlobPromise,
base::TimeTicks(), GetFrame().DomWindow(), base::TimeTicks(), GetFrame().DomWindow(), nullptr);
base::make_optional<UkmParameters>(), nullptr);
ASSERT_TRUE(async_blob_creator->EncodeImageForConvertToBlobTest()); ASSERT_TRUE(async_blob_creator->EncodeImageForConvertToBlobTest());
sk_sp<SkData> sk_data = SkData::MakeWithCopy( sk_sp<SkData> sk_data = SkData::MakeWithCopy(
...@@ -328,4 +326,4 @@ TEST_F(CanvasAsyncBlobCreatorTest, ColorManagedConvertToBlob) { ...@@ -328,4 +326,4 @@ TEST_F(CanvasAsyncBlobCreatorTest, ColorManagedConvertToBlob) {
} }
} }
} }
} // namespace blink }
...@@ -329,7 +329,7 @@ ScriptPromise CanvasRenderingContextHost::convertToBlob( ...@@ -329,7 +329,7 @@ ScriptPromise CanvasRenderingContextHost::convertToBlob(
} }
auto* async_creator = MakeGarbageCollected<CanvasAsyncBlobCreator>( auto* async_creator = MakeGarbageCollected<CanvasAsyncBlobCreator>(
image_bitmap, options, function_type, start_time, image_bitmap, options, function_type, start_time,
ExecutionContext::From(script_state), ukm_params_, resolver); ExecutionContext::From(script_state), resolver);
async_creator->ScheduleAsyncBlobCreation(options->quality()); async_creator->ScheduleAsyncBlobCreation(options->quality());
return resolver->Promise(); return resolver->Promise();
} }
......
...@@ -377,6 +377,12 @@ ScriptPromise HTMLCanvasElement::convertToBlob( ...@@ -377,6 +377,12 @@ ScriptPromise HTMLCanvasElement::convertToBlob(
ScriptState* script_state, ScriptState* script_state,
const ImageEncodeOptions* options, const ImageEncodeOptions* options,
ExceptionState& exception_state) { ExceptionState& exception_state) {
RecordIdentifiabilityMetric(
blink::IdentifiableSurface::FromTypeAndInput(
blink::IdentifiableSurface::Type::kCanvasReadback,
context_ ? context_->GetContextType()
: CanvasRenderingContext::kContextTypeUnknown),
0);
return CanvasRenderingContextHost::convertToBlob(script_state, options, return CanvasRenderingContextHost::convertToBlob(script_state, options,
exception_state); exception_state);
} }
...@@ -964,8 +970,7 @@ String HTMLCanvasElement::ToDataURLInternal( ...@@ -964,8 +970,7 @@ String HTMLCanvasElement::ToDataURLInternal(
RecordIdentifiabilityMetric( RecordIdentifiabilityMetric(
blink::IdentifiableSurface::FromTypeAndInput( blink::IdentifiableSurface::FromTypeAndInput(
blink::IdentifiableSurface::Type::kCanvasReadback, final_digest), blink::IdentifiableSurface::Type::kCanvasReadback, final_digest),
blink::IdentifiabilityDigestOfBytes(base::make_span( blink::IdentifiabilityDigestOfBytes(data_url.Span8()));
data_buffer->Pixels(), data_buffer->ComputeByteSize())));
return data_url; return data_url;
} }
...@@ -1032,11 +1037,16 @@ void HTMLCanvasElement::toBlob(V8BlobCallback* callback, ...@@ -1032,11 +1037,16 @@ void HTMLCanvasElement::toBlob(V8BlobCallback* callback,
async_creator = MakeGarbageCollected<CanvasAsyncBlobCreator>( async_creator = MakeGarbageCollected<CanvasAsyncBlobCreator>(
image_bitmap, options, image_bitmap, options,
CanvasAsyncBlobCreator::kHTMLCanvasToBlobCallback, callback, start_time, CanvasAsyncBlobCreator::kHTMLCanvasToBlobCallback, callback, start_time,
GetExecutionContext(), GetExecutionContext());
base::make_optional<UkmParameters>(
{GetDocument().UkmRecorder(), GetDocument().UkmSourceID()}));
} }
RecordIdentifiabilityMetric(
blink::IdentifiableSurface::FromTypeAndInput(
blink::IdentifiableSurface::Type::kCanvasReadback,
context_ ? context_->GetContextType()
: CanvasRenderingContext::kContextTypeUnknown),
0);
if (async_creator) { if (async_creator) {
async_creator->ScheduleAsyncBlobCreation(quality); async_creator->ScheduleAsyncBlobCreation(quality);
} else { } else {
......
...@@ -60,7 +60,6 @@ class PLATFORM_EXPORT ImageDataBuffer { ...@@ -60,7 +60,6 @@ class PLATFORM_EXPORT ImageDataBuffer {
const IntSize& size() const { return size_; } const IntSize& size() const { return size_; }
int Height() const { return size_.Height(); } int Height() const { return size_.Height(); }
int Width() const { return size_.Width(); } int Width() const { return size_.Width(); }
size_t ComputeByteSize() const { return pixmap_.computeByteSize(); }
private: private:
ImageDataBuffer(const IntSize&, ImageDataBuffer(const IntSize&,
......
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