-
Notifications
You must be signed in to change notification settings - Fork 5
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
Capture pixel screenshots implementation on iOS #99
Capture pixel screenshots implementation on iOS #99
Conversation
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.
Added assertions and a comment, looks good. May be worth adding information about why it's safe to render on a background thread (you mentioned that it was tricky to find out whether it's OK to do it on a background thread or not)
// To handle cases where no window is available, an assertion is included here. | ||
// TODO: Add a log to inform the Rust layer if the screenshot log emission fails, allowing it | ||
// to proceed to the next screenshot (if any). | ||
assertionFailure("no window to take screenshot of") |
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.
If that’s the case might be better to send an empty data object back for now, wdyt? @Augustyniak
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.
Sure, makes sense - so if we send a screenshot log without screen
field set then you will handle it somehow on the frontend layer?
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.
or do you want me to somehow annotated such log with an extra attribute?
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.
We'll handle in the fe
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.
cool, let me make a change in your PR in here
e43ff93
to
29d6bcc
Compare
let layer = window.layer | ||
let bounds = UIScreen.main.bounds.size | ||
|
||
self.queue.async { [weak self] in |
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.
@Reflejo for my android-side understanding: would these two operations happen on separate threads? are any of these new worker threads or do we re-use existing ones?
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.
similarly, those these threads get spawned even when the runtime flag session_replay.screenshots.enabled
is off?
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.
Queues do not map 1:1 to threads, we actually use a default queue for the target here meaning this won't create an additional thread.
The lines that are under the DispatchQueue.main.async
but above self.queue.async
will be dispatched to the main thread (basically just to access the layer object) then the self.queue.async
closure runs that in our serial queue mentioned before
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.
I'm more confused than before? then what's the diff between queue and thread? what does default mean?
Example of a capture
Takes about 500ms running on device