Skip to content

[AI Task] [Tizen.Multimedia.Remoting] Remove triple enumeration in ScreenMirroring.SendGenericMouseEvent - #7738

Open
JoonghyunCho wants to merge 2 commits into
mainfrom
ai-task/issue-7643
Open

[AI Task] [Tizen.Multimedia.Remoting] Remove triple enumeration in ScreenMirroring.SendGenericMouseEvent#7738
JoonghyunCho wants to merge 2 commits into
mainfrom
ai-task/issue-7643

Conversation

@JoonghyunCho

Copy link
Copy Markdown
Member

Summary

ScreenMirroring.SendGenericMouseEvent enumerated its IEnumerable<UibcMouseInfo> input three times (Any()Count()foreach) on the UIBC high-frequency mouse-input path, and threw the semantically wrong ArgumentNullException for an empty collection (while an actual null input crashed with NullReferenceException).

This change materializes the input exactly once and uses the correct argument-validation exception types, with no public API signature change.

Changes

  • src/Tizen.Multimedia.Remoting/ScreenMirroring/ScreenMirroring.cs
    • SendGenericMouseEvent now validates null input up front via ArgumentNullException.ThrowIfNull.
    • The input sequence is materialized once (as IReadOnlyList<UibcMouseInfo> fast path, ToArray() fallback), removing the Any() + Count() + foreach triple enumeration — deferred LINQ sequences are now executed a single time.
    • Empty input now throws ArgumentException (was ArgumentNullException).
    • Added the <exception cref="ArgumentException"> XML doc tag; marshalling and the native call are unchanged.

Mode

Refactoring

Verification

  • Build: passed (dotnet build on Tizen.Multimedia.Remoting — 0 errors)
  • Tests: N/A
  • Benchmark: skipped (sdb error: no device attached — sdb devices lists no targets)

Fixes #7643

🤖 Generated with Claude Code

…seEvent (Fixes #7643)

- Materialize the input IEnumerable<UibcMouseInfo> once (IReadOnlyList fast
  path, ToArray fallback) instead of enumerating it three times via
  Any() + Count() + foreach.
- Throw ArgumentNullException for null input (previously
  NullReferenceException from Any()).
- Throw ArgumentException for empty input instead of the semantically
  wrong ArgumentNullException.
- Document the new ArgumentException in the XML doc comment.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@JoonghyunCho

Copy link
Copy Markdown
Member Author

⚠️ Benchmark skipped: Encountered an issue with sdb device connection/deployment/execution, so the benchmark could not be run. Manual benchmark verification is required. Error: sdb devices reports no attached devices (List of devices attached — empty).

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the SendGenericMouseEvent method in ScreenMirroring.cs to prevent multiple enumerations of uibcMouseInfos, introduce proper null validation, and fix a potential indexing bug by replacing the foreach loop with a standard for loop. The reviewer provided valuable feedback pointing out a potential memory leak during unmanaged memory allocation and suggested a refactoring to ensure proper cleanup using a try-finally block, alongside simplifying pointer arithmetic.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

var size = Marshal.SizeOf(typeof(Native.UibcMouse));
IntPtr unmanagedUibcMouse = Marshal.AllocHGlobal(size * uibcMouseInfosSize);
for (i = 0; i < uibcMouseInfosSize; i++)
for (int i = 0; i < uibcMouseInfosSize; i++)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Memory Leak and Pointer Arithmetic Improvement

There is a potential memory leak in the allocation of unmanaged memory, and the pointer arithmetic can be significantly simplified.

  1. Memory Leak:
    If Marshal.AllocHGlobal(Marshal.SizeOf(uibcObject)) throws an exception (such as OutOfMemoryException), or if Marshal.StructureToPtr throws an exception, the previously allocated unmanagedUibcMouse memory will be leaked because the try block has not been entered yet.

  2. Pointer Arithmetic:
    Instead of checking IntPtr.Size == 4 and performing manual pointer arithmetic with ToInt32() or ToInt64(), you can use IntPtr.Add or the + operator, which is cleaner, safer, and cross-platform.

Here is a suggested refactoring to address both issues:

            var size = Marshal.SizeOf(typeof(Native.UibcMouse));
            IntPtr unmanagedUibcMouse = IntPtr.Zero;
            IntPtr unmanagedUibcObject = IntPtr.Zero;

            try
            {
                unmanagedUibcMouse = Marshal.AllocHGlobal(size * uibcMouseInfosSize);
                for (int i = 0; i < uibcMouseInfosSize; i++)
                {
                    IntPtr unmanagedUibcMousePtr = unmanagedUibcMouse + (i * size);
                    Marshal.StructureToPtr(uibcMouse[i], unmanagedUibcMousePtr, false);
                }

                Native.UibcMouseEvent uibcObject;
                uibcObject.size = uibcMouseInfosSize;
                uibcObject.type = type;
                uibcObject.uibcMouse = unmanagedUibcMouse;

                unmanagedUibcObject = Marshal.AllocHGlobal(Marshal.SizeOf(uibcObject));
                Marshal.StructureToPtr(uibcObject, unmanagedUibcObject, false);

                Native.SendGenericMouseEvent(Handle, unmanagedUibcObject).ThrowIfError("Failed to send generic mouse event");
            }
            finally
            {
                if (unmanagedUibcMouse != IntPtr.Zero)
                {
                    Marshal.FreeHGlobal(unmanagedUibcMouse);
                }
                if (unmanagedUibcObject != IntPtr.Zero)
                {
                    Marshal.FreeHGlobal(unmanagedUibcObject);
                }
            }

@JoonghyunCho

Copy link
Copy Markdown
Member Author

🤖 [AI Review]

Reviewed — no findings.

Scope checked:

  • Verified the single materialization (uibcMouseInfos as IReadOnlyList<UibcMouseInfo> ?? uibcMouseInfos.ToArray()) removes all three prior enumerations (Any(), Count(), foreach) without changing element order or count semantics.
  • ArgumentNullException.ThrowIfNull is valid here — Tizen.Multimedia.Remoting targets net8.0.
  • Empty input now throws ArgumentException instead of the previously incorrect ArgumentNullException; the new <exception cref="ArgumentException"> doc tag matches, and the member is [EditorBrowsable(Never)].
  • Confirmed the rewritten index-based loop assigns id/x/y to the same slots as the old i++ side-effect pattern, and the second marshaling loop's re-scoped int i keeps identical bounds.
  • Null check moved ahead of ValidateState: only affects exception precedence for a null argument on a disposed/invalid-state instance, which is standard guard ordering.

No 🔴 critical issues, no 🟡 suggestions to flag.


Automated review — final merge decision rests with human reviewers.

Fix potential native memory leak in SendGenericMouseEvent by allocating
unmanaged buffers inside try/finally, and replace 32/64-bit pointer
arithmetic branching with IntPtr addition

Applied-Human-Comments: 3524738875
@JoonghyunCho

Copy link
Copy Markdown
Member Author

🤖 [AI Review]
Addressed review feedback in commit 9261cda. Summary: moved the unmanaged buffer allocations in SendGenericMouseEvent inside a try/finally so unmanagedUibcMouse can no longer leak if a subsequent allocation or StructureToPtr throws, and replaced the 32/64-bit pointer arithmetic branching with IntPtr addition.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days

@github-actions github-actions Bot added the Stale label Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

1 participant