Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref: Session replay performance for SwiftUI #4419

Merged
merged 10 commits into from
Oct 10, 2024
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ via the option `swizzleClassNameExclude`.
### Improvements

- Serializing profile on a BG Thread (#4377) to avoid potentially slightly blocking the main thread.
- Session Replay performance for SwiftUI (#4419)

## 8.38.0-beta.1

Expand Down
6 changes: 6 additions & 0 deletions Sources/Swift/Tools/SentryViewPhotographer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -45,18 +45,24 @@ class SentryViewPhotographer: NSObject, SentryViewScreenshotProvider {
dispatchQueue.dispatchAsync {
let screenshot = UIGraphicsImageRenderer(size: imageSize, format: .init(for: .init(displayScale: 1))).image { context in

let imageRect = CGRect(origin: .zero, size: imageSize)
context.cgContext.addRect(CGRect(origin: CGPoint.zero, size: imageSize))
context.cgContext.clip(using: .evenOdd)
UIColor.blue.setStroke()

context.cgContext.interpolationQuality = .none
image.draw(at: .zero)

var latestRegion: RedactRegion?
for region in redact {
let rect = CGRect(origin: CGPoint.zero, size: region.size)
var transform = region.transform
let path = CGPath(rect: rect, transform: &transform)

defer { latestRegion = region }

guard region != latestRegion && imageRect.contains(path.boundingBoxOfPath) else { continue }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m: It seems like this speeds up the performance significantly. Maybe it's worth adding a comment for this, cause we don't have a test to validate that we do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do have a test to validate this.
Its the only test added to this PR


switch region.type {
case .redact, .redactSwiftUI:
(region.color ?? UIImageHelper.averageColor(of: context.currentImage, at: rect.applying(region.transform))).setFill()
Expand Down
6 changes: 5 additions & 1 deletion Sources/Swift/Tools/UIRedactBuilder.swift
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ enum RedactRegionType {
case redactSwiftUI
}

struct RedactRegion {
struct RedactRegion: Equatable {
let size: CGSize
let transform: CGAffineTransform
let type: RedactRegionType
Expand All @@ -39,6 +39,10 @@ struct RedactRegion {
self.type = type
self.color = color
}

static func == (lhs: RedactRegion, rhs: RedactRegion) -> Bool {
lhs.size == rhs.size && lhs.transform == rhs.transform && lhs.type == rhs.type
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

h: I guess you added this so you can compare the regions easily here:

guard region != latestRegion && imageRect.contains(path.boundingBoxOfPath) else { continue }

I'm not a fan of this cause the equals here doesn't include the color. This can be very confusing. I would prefer adding a helper method or something different if you only want to check if to redact regions are the same without considering the color.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we can do that.

}
}

class UIRedactBuilder {
Expand Down
15 changes: 15 additions & 0 deletions Tests/SentryTests/SentryViewPhotographerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,21 @@ class SentryViewPhotographerTests: XCTestCase {
assertColor(pixel2, .white)
}

func testSkipSameRegion() throws {
let label1 = UILabel(frame: CGRect(x: 0, y: 0, width: 50, height: 25))
label1.text = "Test"
label1.textColor = .red

let label2 = UILabel(frame: CGRect(x: 0, y: 0, width: 50, height: 25))
label2.text = "Test"
label2.textColor = .green

let image = try XCTUnwrap(prepare(views: [label1, label2]))
let pixel1 = color(at: CGPoint(x: 10, y: 10), in: image)

assertColor(pixel1, .green)
}

private func assertColor(_ color1: UIColor, _ color2: UIColor) {
let sRGBColor1 = color1.cgColor.converted(to: CGColorSpace(name: CGColorSpace.sRGB)!, intent: .defaultIntent, options: nil)
let sRGBColor2 = color2.cgColor.converted(to: CGColorSpace(name: CGColorSpace.sRGB)!, intent: .defaultIntent, options: nil)
Expand Down
Loading