Skip to content

[AI Task] [Tizen.Multimedia.Remoting] Cache Enum.GetValues for MediaController capability enums#7713

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

[AI Task] [Tizen.Multimedia.Remoting] Cache Enum.GetValues for MediaController capability enums#7713
JoonghyunCho wants to merge 2 commits into
mainfrom
ai-task/issue-7642

Conversation

@JoonghyunCho

Copy link
Copy Markdown
Member

Summary

The MediaController capability / display-mode conversion helpers called Enum.GetValues(typeof(T)) on every invocation. The non-generic overload returns an Array, so each call allocates a fresh array and boxes every enum value. One of the sites (PlaybackCapabilityUpdated) is a hot path that fires on every server-side playback-capability change (multiple times per second during media transitions/scrubbing).

This change replaces those calls with static readonly caches populated once via the strongly-typed generic Enum.GetValues<T>() overload (.NET 5+), eliminating the per-call array allocation and boxing.

Changes

  • src/Tizen.Multimedia.Remoting/MediaController/MediaController.cs
    • Add s_allPlaybackActions static cache; use it in GetPlaybackCapabilities (was line 542).
  • src/Tizen.Multimedia.Remoting/MediaController/MediaController.Events.cs
    • Use the shared s_allPlaybackActions cache in CreatePlaybackCapabilityUpdatedEventArgs (was line 207).
  • src/Tizen.Multimedia.Remoting/MediaController/MediaControlEnums.cs
    • Add s_allDisplayModes / s_allDisplayRotations static caches in EnumExtensions; use them in the ToPublicList helpers (was lines 531, 595).

Mode

Refactoring

Verification

  • Build: passed (dotnet build Tizen.Multimedia.Remoting — 0 errors; pre-existing warnings only)
  • Tests: N/A (no unit-test project covers this path; enumeration order and results are unchanged)
  • Benchmark: skipped (sdb device unavailable in CI environment — manual verification required)

Fixes #7642

…capability enums

Replace per-call Enum.GetValues(typeof(T)), which returns a non-generic
Array (boxing each enum value and allocating a new array on every call),
with static readonly caches populated once via the generic
Enum.GetValues<T>() overload. Applied to the four capability/display-mode
conversion sites in MediaController.

- s_allPlaybackActions shared by GetPlaybackCapabilities and the
  PlaybackCapabilityUpdated event handler (a per-server-change hot path).
- s_allDisplayModes / s_allDisplayRotations for the DisplayMode and
  DisplayRotation ToPublicList helpers.

Enumeration order and results are unchanged; public API is unaffected.

Fixes #7642

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@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 optimizes performance by caching enum values in static read-only arrays instead of repeatedly calling Enum.GetValues. However, using Enum.GetValues<T>() on the [Flags] enums MediaControlNativeDisplayMode and MediaControlNativeDisplayRotation introduces a correctness bug. Because these enums contain an All member, it evaluates to true in HasFlag checks and results in duplicate entries in the returned lists. It is recommended to explicitly define the arrays of individual flags to avoid this issue.

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.

Comment on lines +381 to +385
private static readonly MediaControlNativeDisplayMode[] s_allDisplayModes
= Enum.GetValues<MediaControlNativeDisplayMode>();

private static readonly MediaControlNativeDisplayRotation[] s_allDisplayRotations
= Enum.GetValues<MediaControlNativeDisplayRotation>();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Using Enum.GetValues<T>() on MediaControlNativeDisplayMode and MediaControlNativeDisplayRotation introduces a correctness bug because these are [Flags] enums that define an All member (which is a bitwise combination of other members).

The Bug

  1. Enum.GetValues<T>() returns all defined constants, including All.
  2. In ToPublicList(), the loop checks modes.HasFlag(mode). If modes has all flags set, modes.HasFlag(All) evaluates to true.
  3. ToPublic() is then called with All. Since All is not handled in the switch statement of ToPublic(), it falls back to the default value (MediaControlDisplayMode.LetterBox or Rotation.Rotate0), resulting in duplicate entries in the returned list (e.g., LetterBox or Rotate0 added twice).

Solution

Explicitly define the arrays of individual flags. This completely resolves the duplicate element bug and also improves performance by avoiding reflection (Enum.GetValues) at startup.

        private static readonly MediaControlNativeDisplayMode[] s_allDisplayModes = new[]
        {
            MediaControlNativeDisplayMode.LetterBox,
            MediaControlNativeDisplayMode.OriginSize,
            MediaControlNativeDisplayMode.FullScreen,
            MediaControlNativeDisplayMode.CroppedFull
        };

        private static readonly MediaControlNativeDisplayRotation[] s_allDisplayRotations = new[]
        {
            MediaControlNativeDisplayRotation.Rotate0,
            MediaControlNativeDisplayRotation.Rotate90,
            MediaControlNativeDisplayRotation.Rotate180,
            MediaControlNativeDisplayRotation.Rotate270
        };

@JoonghyunCho

Copy link
Copy Markdown
Member Author

🤖 [AI Review]

Reviewed — no findings.

Scope checked:

  • Confirmed the three new private static readonly enum caches (s_allDisplayModes, s_allDisplayRotations, s_allPlaybackActions) yield the same values and order as the replaced Enum.GetValues(typeof(...)) calls, so the HasFlag iteration results are preserved.
  • Verified every touched member is internal/private; no public API signature or documentation surface is affected.
  • Confirmed s_allPlaybackActions (declared in MediaController.cs) resolves correctly from the MediaController.Events.cs partial-class file.
  • Confirmed the static field initializers run via the CLR type initializer, so the new caches add no thread-safety race.

No 🔴 critical issues, no 🟡 suggestions to flag.


Automated review — final merge decision rests with human reviewers.

Replace Enum.GetValues<T>() with explicit per-flag arrays for the display mode
and rotation capability enums. Enum.GetValues included the aggregate All member,
causing ToPublicList to emit a duplicate entry (and call ToPublic with All) when
all flags were set.

Applied-Human-Comments: 3448302429
@JoonghyunCho

Copy link
Copy Markdown
Member Author

🤖 [AI Review]
Addressed review feedback in commit b0a9918. Summary: replaced Enum.GetValues<T>() with explicit per-flag arrays for the display mode and rotation capability enums, so the aggregate All member is no longer iterated in ToPublicList (which previously produced a duplicate entry when all flags were set).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

1 participant