-
Notifications
You must be signed in to change notification settings - Fork 3
feat: Android double masking with frame drop #342
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
Conversation
* andrey/nonstandard-views: Attached dialog sample space use Canvas for certain types of windows consolidate in one view sample of adding floating views add floating button test: Add UI and logic to evaluate boolean flags (#305)
…ed-mask2 * andrey/do-not-send-duplicate-windows: Unit tests Volatile address feedback Fix feedback Recycle bitmap early fix size bug no message
* main: doc: Add using ldMask in readme. (#311) chore: release main (#312) feat: take transformed coordinates, which are more precise in animation (#309) chore: release main (#307) fix(SEC-7530): update react-server-dom-webpack to 19.0.1 (#310) # Conflicts: # sdk/@launchdarkly/observability-android/lib/src/main/kotlin/com/launchdarkly/observability/replay/capture/CaptureSource.kt # sdk/@launchdarkly/observability-android/lib/src/main/kotlin/com/launchdarkly/observability/replay/masking/ComposeMaskTarget.kt # sdk/@launchdarkly/observability-android/lib/src/main/kotlin/com/launchdarkly/observability/replay/masking/Mask.kt # sdk/@launchdarkly/observability-android/lib/src/main/kotlin/com/launchdarkly/observability/replay/masking/NativeMaskTarget.kt
* main: feat: Limit accumulating canvas buffer (#322) chore: Fix existing RRwebGraphQLReplayLogExporterTest tests (#321) feat: sanitize URLs + semantic conventions for header attributes (#317) refactor: introduce granular ObservabilityOptions (#323) refactor: OY11-846 - Add Session Replay plugin (#313) chore: upgrade react-server-dom-webpack to 19.0.3 (#320) chore: release main (#319) feat: enhance Web Vitals telemetry with semantic attributes (#316) chore: release main (#318) fix: Android - Remove Disk Buffering (#315) chore: readme update with real examples (#314)
…bservability-sdk into andrey/fix-masking-offset * 'andrey/fix-masking-offset' of github.com:launchdarkly/observability-sdk: feat: Graphql client memory optimization (#325)
* main: docs: Update readme with options refactor (#326)
* main: feat: Gzip compression for Graphql request body (#328)
* main: feat: Android SR Identify support (#330)
* main: test: O11Y-908 - Add Android CI workflows (#337) chore: release main (#335) feat: publish umd for broser environments (#334) chore: release main (#333) feat: Pause and resume replay capture on app background/foreground (#329) fix: Fix compose coordinate offset. (#331) chore: release main (#324) fix(highlight.run): correct privacy masking for empty strings (#332)
* main: chore: Android SR - cleanup privacy settings (#341)
This reverts commit feaa22f.
...y-android/lib/src/main/kotlin/com/launchdarkly/observability/replay/capture/CaptureSource.kt
Show resolved
Hide resolved
...y-android/lib/src/main/kotlin/com/launchdarkly/observability/replay/capture/CaptureSource.kt
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| // if need to draw something on base bitmap additionally | ||
| if (captureResults.size > 1 || afterMasks.isNotEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Condition check always true causing unnecessary canvas creation
Low Severity
The condition afterMasks.isNotEmpty() always evaluates to true because afterMasks is created from captureResults at line 141 with the same size, and execution only reaches this point when captureResults contains at least one element. This causes unnecessary Canvas creation and drawMasks calls even when there are no actual masks to apply. The intent, as indicated by the comment "if need to draw something on base bitmap additionally", appears to be checking if there are masks that need drawing, which would require checking if any inner lists contain masks (e.g., afterMasks.any { it?.isNotEmpty() == true }).
| continuation.resume(Unit) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bitmap memory leak on coroutine cancellation during frame wait
Low Severity
The second Choreographer.postFrameCallback wait (lines 132-139) occurs after bitmaps have been captured into captureResults but before the withContext(default) block that handles cleanup via recycleCaptureResults. If the coroutine is cancelled during this frame wait, the captured Bitmap objects are orphaned without being recycled. Since Android bitmaps hold native memory that requires explicit recycling, this causes a native memory leak. The suspendCancellableCoroutine lacks an invokeOnCancellation handler to recycle the bitmaps when cancellation occurs at this point.
| } | ||
|
|
||
| private val maskIntRect = Rect() | ||
| private val path = Path() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shared mutable Path causes thread-safety race condition
Medium Severity
The path field is now a shared member variable instead of being created locally within each drawMasks call as it was in the original code. Since MaskApplier is a single instance on CaptureSource and drawMasks executes on Dispatchers.Default (a multi-threaded dispatcher), concurrent capture operations can modify the same path object simultaneously. The drawMask method calls reset(), moveTo(), lineTo(), and close() on this shared path before drawing, which can corrupt path data when interleaved with another concurrent call, resulting in incorrect mask rendering.
Summary
Introduces double masking and if number of masks changed the frame being dropped.
How did you test this change?
Are there any deployment considerations?
Note
Double-pass masking with frame synchronization and stability check
MaskApplier.mergeMasksMap; drops capture if masks are unstable or counts differMaskApplierto render masks (pre/post paints) and centralize merge/draw logic; removes inlined drawing fromCaptureSourceChoreographertwice, recycles bitmaps safely, and guards with tiled signature check.ldMask()to ZIP and CVV fields (remove from header), small import/format changesWritten by Cursor Bugbot for commit 319c9cc. This will update automatically on new commits. Configure here.