fix: Screenshot Capture

CHANGELOG.md

@@ -23,6 +23,12 @@
   customize it by creating your own variant. The user feedback feature allows your users to provide feedback in form
   of a written message that can optionally have a screenshot attached. Read more about it ([here](https://docs.sentry.io/product/user-feedback/)). ([#2220](https://github.com/getsentry/sentry-unity/pull/2220))
 
+### Fixes
+
+- The SDK now waits for 'End of Frame' before capturing a screenshot. This should address any blank or malformed 
+  screenshots previously attached to events. The SDK now also only captures one screenshot for the first error event in
+  each individual frame. ([#2240](https://github.com/getsentry/sentry-unity/pull/2240)) 
+
 ### Dependencies
 
 - Bump Java SDK from v8.14.0 to v8.17.0 ([#2218](https://github.com/getsentry/sentry-unity/pull/2218), [#2223](https://github.com/getsentry/sentry-unity/pull/2223), [#2238](https://github.com/getsentry/sentry-unity/pull/2238))

samples/unity-of-bugs/Assets/Resources/Sentry/SentryOptions.asset

@@ -28,10 +28,10 @@ MonoBehaviour:
   <ReleaseOverride>k__BackingField: 
   <EnvironmentOverride>k__BackingField: 
   <AttachStacktrace>k__BackingField: 1
-  <AttachScreenshot>k__BackingField: 0
+  <AttachScreenshot>k__BackingField: 1
   <ScreenshotQuality>k__BackingField: 1
   <ScreenshotCompression>k__BackingField: 75
-  <AttachViewHierarchy>k__BackingField: 0
+  <AttachViewHierarchy>k__BackingField: 1
   <MaxViewHierarchyRootObjects>k__BackingField: 100
   <MaxViewHierarchyObjectChildCount>k__BackingField: 20
   <MaxViewHierarchyDepth>k__BackingField: 10

src/Sentry.Unity/ScreenshotEventProcessor.cs

@@ -1,64 +1,66 @@
+using System;
+using System.Collections;
 using Sentry.Extensibility;
-using Sentry.Unity.Integrations;
+using Sentry.Internal;
 using UnityEngine;
 
 namespace Sentry.Unity;
 
-public class ScreenshotEventProcessor : ISentryEventProcessorWithHint
+public class ScreenshotEventProcessor : ISentryEventProcessor
 {
     private readonly SentryUnityOptions _options;
-    private readonly IApplication _application;
-    public ScreenshotEventProcessor(SentryUnityOptions sentryOptions) : this(sentryOptions, null) { }
+    private readonly ISentryMonoBehaviour _sentryMonoBehaviour;
+    private bool _isCapturingScreenshot;
 
-    internal ScreenshotEventProcessor(SentryUnityOptions sentryOptions, IApplication? application)
+    internal Func<SentryUnityOptions, byte[]> ScreenshotCaptureFunction = SentryScreenshot.Capture;
+    internal Action<SentryId, SentryAttachment> AttachmentCaptureFunction = (eventId, attachment) =>
+        ((Hub)Sentry.SentrySdk.CurrentHub).CaptureAttachment(eventId, attachment);
+
+    public ScreenshotEventProcessor(SentryUnityOptions sentryOptions) : this(sentryOptions, SentryMonoBehaviour.Instance) { }
+
+    internal ScreenshotEventProcessor(SentryUnityOptions sentryOptions, ISentryMonoBehaviour sentryMonoBehaviour)
     {
         _options = sentryOptions;
-        _application = application ?? ApplicationAdapter.Instance;
+        _sentryMonoBehaviour = sentryMonoBehaviour;
     }
 
-    public SentryEvent? Process(SentryEvent @event)
+    public SentryEvent Process(SentryEvent @event)
     {
+        // Only ever capture one screenshot per frame
+        if (!_isCapturingScreenshot)
+        {
+            _isCapturingScreenshot = true;
+            _sentryMonoBehaviour.StartCoroutine(CaptureScreenshotCoroutine(@event.EventId));
+        }
         return @event;
     }
 
-    public SentryEvent? Process(SentryEvent @event, SentryHint hint)
+    internal IEnumerator CaptureScreenshotCoroutine(SentryId eventId)
     {
-        // save event id
-        // wait for end of frame
-        // check if last id is event it
-        // send screenshot
+        _options.LogDebug("Screenshot capture triggered. Waiting for End of Frame.");
 
-        // add workitem: screentshot for ID xxx
-        // sdk integration checking for work: if ID got sent, follow up with screenshot
+        yield return new WaitForEndOfFrame();
 
-        if (!MainThreadData.IsMainThread())
+        try
         {
-            _options.DiagnosticLogger?.LogDebug("Screenshot capture skipped. Can't capture screenshots on other than the main thread.");
-            return @event;
-        }
+            var screenshotBytes = ScreenshotCaptureFunction(_options);
+            var attachment = new SentryAttachment(
+                    AttachmentType.Default,
+                    new ByteAttachmentContent(screenshotBytes),
+                    "screenshot.jpg",
+                    "image/jpeg");
 
-        if (_options.BeforeCaptureScreenshotInternal?.Invoke() is not false)
-        {
-            if (_application.IsEditor)
-            {
-                _options.DiagnosticLogger?.LogInfo("Screenshot capture skipped. Capturing screenshots it not supported in the Editor");
-                return @event;
-            }
+            _options.LogDebug("Screenshot captured for event {0}", eventId);
 
-            if (Screen.width == 0 || Screen.height == 0)
-            {
-                _options.DiagnosticLogger?.LogWarning("Can't capture screenshots on a screen with a resolution of '{0}x{1}'.", Screen.width, Screen.height);
-            }
-            else
-            {
-                hint.AddAttachment(SentryScreenshot.Capture(_options), "screenshot.jpg", contentType: "image/jpeg");
-            }
+            AttachmentCaptureFunction(eventId, attachment);
         }
-        else
+        catch (Exception e)
         {
-            _options.DiagnosticLogger?.LogInfo("Screenshot capture skipped by BeforeAttachScreenshot callback.");
+            _options.LogError(e, "Failed to capture screenshot.");
+        }
+        finally
+        {
+            _isCapturingScreenshot = false;
         }
-
-        return @event;
     }
 }

src/Sentry.Unity/SentryMonoBehaviour.cs

@@ -1,4 +1,5 @@
 using System;
+using System.Collections;
 using Sentry.Unity.Integrations;
 using UnityEngine;
 
@@ -7,6 +8,7 @@ namespace Sentry.Unity;
 internal interface ISentryMonoBehaviour
 {
     event Action? ApplicationResuming;
+    public Coroutine StartCoroutine(IEnumerator routine);
 }
 
 /// <summary>

src/Sentry.Unity/SentrySdk.cs

@@ -1,7 +1,6 @@
 using System;
 using System.ComponentModel;
 using Sentry.Extensibility;
-using Sentry.Unity.NativeUtils;
 
 namespace Sentry.Unity;
 

src/Sentry.Unity/SentryUnitySdk.cs

@@ -2,6 +2,7 @@
 using System.IO;
 using System.Threading.Tasks;
 using Sentry.Extensibility;
+using Sentry.Internal;
 using Sentry.Unity.Integrations;
 using UnityEngine;
 
@@ -126,6 +127,26 @@ public void CaptureFeedback(string message, string? email, string? name, bool ad
         Sentry.SentrySdk.CurrentHub.CaptureFeedback(message, email, name, hint: hint);
     }
 
+    internal void CaptureAttachment(SentryId eventId, SentryAttachment attachment)
+    {
+        try
+        {
+            if (Sentry.SentrySdk.CurrentHub is Hub hub)
+            {
+                hub.CaptureAttachment(eventId, attachment);
+                _options.LogDebug("Attachment captured for event {0}", eventId);
+            }
+            else
+            {
+                _options.LogError("Capturing the attachment failed due to the current hub.");
+            }
+        }
+        catch (Exception ex)
+        {
+            _options.DiagnosticLogger?.LogError(ex, "Failed to capture attachment for event {0}", eventId);
+        }
+    }
+
     internal static void SetUpWindowsPlayerCaching(SentryUnitySdk unitySdk, SentryUnityOptions options)
     {
         // On Windows-Standalone, we disable cache dir in case multiple app instances run over the same path.

test/Sentry.Unity.Tests/ScreenshotEventProcessorTests.cs

@@ -1,93 +1,89 @@
-using System.Threading;
+using System.Collections;
 using NUnit.Framework;
 using Sentry.Unity.Tests.Stubs;
+using UnityEngine;
+using UnityEngine.TestTools;
 
 namespace Sentry.Unity.Tests;
 
 public class ScreenshotEventProcessorTests
 {
-    private class Fixture
+    [Test]
+    public void Process_FirstCallInAFrame_StartsCoroutine()
     {
-        public SentryUnityOptions Options = new() { AttachScreenshot = true };
-        public TestApplication TestApplication = new();
-
-        public ScreenshotEventProcessor GetSut() => new(Options, TestApplication);
-    }
+        var sentryMonoBehaviour = GetTestMonoBehaviour();
+        var screenshotProcessor = new ScreenshotEventProcessor(new SentryUnityOptions(), sentryMonoBehaviour);
 
-    private Fixture _fixture = null!;
+        screenshotProcessor.Process(new SentryEvent());
 
-    [SetUp]
-    public void SetUp() => _fixture = new Fixture();
+        Assert.IsTrue(sentryMonoBehaviour.StartCoroutineCalled);
+    }
 
-    [TearDown]
-    public void TearDown()
+    [UnityTest]
+    public IEnumerator Process_ExecutesCoroutine_CapturesScreenshotAndCapturesAttachment()
     {
-        if (SentrySdk.IsEnabled)
+        var sentryMonoBehaviour = GetTestMonoBehaviour();
+        var screenshotProcessor = new ScreenshotEventProcessor(new SentryUnityOptions(), sentryMonoBehaviour);
+
+        var capturedEventId = SentryId.Empty;
+        SentryAttachment? capturedAttachment = null;
+        screenshotProcessor.AttachmentCaptureFunction = (eventId, attachment) =>
         {
-            SentrySdk.Close();
-        }
-    }
+            capturedEventId = eventId;
+            capturedAttachment = attachment;
+        };
 
-    [Test]
-    public void Process_IsMainThread_AddsScreenshotToHint()
-    {
-        _fixture.TestApplication.IsEditor = false;
-        var sut = _fixture.GetSut();
-        var sentryEvent = new SentryEvent();
-        var hint = new SentryHint();
+        var eventId = SentryId.Create();
+        var sentryEvent = new SentryEvent(eventId: eventId);
+
+        screenshotProcessor.Process(sentryEvent);
 
-        sut.Process(sentryEvent, hint);
+        // Wait for the coroutine to complete
+        yield return null;
 
-        Assert.AreEqual(1, hint.Attachments.Count);
+        Assert.IsTrue(sentryMonoBehaviour.StartCoroutineCalled);
+        Assert.AreEqual(eventId, capturedEventId);
+        Assert.NotNull(capturedAttachment); // Sanity check
+        Assert.AreEqual("screenshot.jpg", capturedAttachment!.FileName);
+        Assert.AreEqual("image/jpeg", capturedAttachment.ContentType);
+        Assert.AreEqual(AttachmentType.Default, capturedAttachment.Type);
     }
 
-    [Test]
-    public void Process_IsNonMainThread_DoesNotAddScreenshotToHint()
+    [UnityTest]
+    public IEnumerator Process_CalledMultipleTimesQuickly_OnlyExecutesScreenshotCaptureOnce()
     {
-        var sut = _fixture.GetSut();
-        var sentryEvent = new SentryEvent();
-        var hint = new SentryHint();
+        var sentryMonoBehaviour = GetTestMonoBehaviour();
+        var screenshotProcessor = new ScreenshotEventProcessor(new SentryUnityOptions(), sentryMonoBehaviour);
 
-        new Thread(() =>
+        var screenshotCaptureCallCount = 0;
+        screenshotProcessor.ScreenshotCaptureFunction = _ =>
         {
-            Thread.CurrentThread.IsBackground = true;
-            var stream = sut.Process(sentryEvent, hint);
+            screenshotCaptureCallCount++;
+            return [0];
+        };
 
-            Assert.AreEqual(0, hint.Attachments.Count);
-        }).Start();
-    }
+        var attachmentCaptureCallCount = 0;
+        screenshotProcessor.AttachmentCaptureFunction = (_, _) =>
+        {
+            attachmentCaptureCallCount++;
+        };
 
-    [Test]
-    [TestCase(true)]
-    [TestCase(false)]
-    public void Process_BeforeCaptureScreenshotCallbackProvided_RespectsScreenshotCaptureDecision(bool captureScreenshot)
-    {
-        _fixture.TestApplication.IsEditor = false;
-        _fixture.Options.SetBeforeCaptureScreenshot(() => captureScreenshot);
-        var sut = _fixture.GetSut();
-        var sentryEvent = new SentryEvent();
-        var hint = new SentryHint();
+        // Process multiple events quickly (before any coroutine can complete)
+        screenshotProcessor.Process(new SentryEvent());
+        screenshotProcessor.Process(new SentryEvent());
+        screenshotProcessor.Process(new SentryEvent());
 
-        sut.Process(sentryEvent, hint);
+        // Wait for the coroutine to complete
+        yield return null;
 
-        Assert.AreEqual(captureScreenshot ? 1 : 0, hint.Attachments.Count);
+        Assert.AreEqual(1, screenshotCaptureCallCount);
+        Assert.AreEqual(1, attachmentCaptureCallCount);
     }
 
-    [Test]
-    [TestCase(true, 0)]
-    [TestCase(false, 1)]
-    public void Process_InEditorEnvironment_DoesNotCaptureScreenshot(bool isEditor, int expectedAttachmentCount)
+    private static TestSentryMonoBehaviour GetTestMonoBehaviour()
     {
-        // Arrange
-        _fixture.TestApplication.IsEditor = isEditor;
-        var sut = _fixture.GetSut();
-        var sentryEvent = new SentryEvent();
-        var hint = new SentryHint();
-
-        // Act
-        sut.Process(sentryEvent, hint);
-
-        // Assert
-        Assert.AreEqual(expectedAttachmentCount, hint.Attachments.Count);
+        var gameObject = new GameObject("ScreenshotProcessorTest");
+        var behaviour = gameObject.AddComponent<TestSentryMonoBehaviour>();
+        return behaviour;
     }
 }

test/Sentry.Unity.Tests/SentryMonoBehaviourTests.cs

@@ -1,6 +1,8 @@
+using System.Collections;
 using NUnit.Framework;
 using Sentry.Unity.Tests.Stubs;
 using UnityEngine;
+using UnityEngine.TestTools;
 
 namespace Sentry.Unity.Tests;
 
@@ -78,4 +80,5 @@ public void UpdatePauseStatus_ResumedTwice_ApplicationResumingInvokedOnlyOnce()
 
         Assert.AreEqual(1, counter);
     }
+
 }

test/Sentry.Unity.Tests/Stubs/SentryTestMonoBehaviour.cs

@@ -0,0 +1,19 @@
+using System;
+using System.Collections;
+using UnityEngine;
+
+namespace Sentry.Unity.Tests.Stubs;
+
+internal class TestSentryMonoBehaviour : MonoBehaviour, ISentryMonoBehaviour
+{
+    public event System.Action? ApplicationResuming;
+    public void ResumeApplication() => ApplicationResuming?.Invoke();
+
+    public bool StartCoroutineCalled { get; private set; }
+
+    public new Coroutine StartCoroutine(IEnumerator routine)
+    {
+        StartCoroutineCalled = true;
+        return base.StartCoroutine(routine);
+    }
+}

test/Sentry.Unity.Tests/Stubs/TestHub.cs

@@ -187,4 +187,9 @@ public void CaptureFeedback(SentryFeedback feedback, Action<Scope> configureScop
     {
         throw new NotImplementedException();
     }
+
+    public bool CaptureAttachment(SentryId eventId, SentryAttachment attachment)
+    {
+        throw new NotImplementedException();
+    }
 }

test/Sentry.Unity.Tests/TraceGenerationIntegrationTests.cs

@@ -93,11 +93,4 @@ public void TraceGeneration_OnActiveSceneChange_GeneratesNewTrace()
 
         Assert.AreNotEqual(initialPropagationContext, scope.PropagationContext);
     }
-
-    internal class TestSentryMonoBehaviour : ISentryMonoBehaviour
-    {
-        public event Action? ApplicationResuming;
-
-        public void ResumeApplication() => ApplicationResuming?.Invoke();
-    }
 }