Skip to content

[AI Task] [Tizen.Multimedia.Recorder] Make GetSupportedVideoResolutions cache thread-safe via Lazy<T>#7712

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

[AI Task] [Tizen.Multimedia.Recorder] Make GetSupportedVideoResolutions cache thread-safe via Lazy<T>#7712
JoonghyunCho wants to merge 2 commits into
mainfrom
ai-task/issue-7641

Conversation

@JoonghyunCho

Copy link
Copy Markdown
Member

Summary

VideoRecorder.GetSupportedVideoResolutions(CameraDevice) cached its results in two static fields (_frontResolutions / _rearResolutions) using a non-atomic read → null-check → assign (?? (x = ...)) pattern. With no synchronization, two concurrent callers could each run LoadVideoResolutions, which opens a native Camera handle. Tizen camera resources are single-owner, so the second open can fail with NotSupportedException / InvalidOperationException, and the first caller's result may be lost (overwritten cache write).

This change converts both caches to Lazy<IEnumerable<Size>>, whose default LazyThreadSafetyMode.ExecutionAndPublication guarantees the factory runs exactly once even under concurrent access. This also aligns with the existing convention in the sibling Recorder.Capabilities.cs, which already uses Lazy<Capabilities>.

Changes

  • src/Tizen.Multimedia.Recorder/Recorder/VideoRecorder.Capabilities.cs
    • _frontResolutions / _rearResolutions: IEnumerable<Size>static readonly Lazy<IEnumerable<Size>> initialized with a LoadVideoResolutions(...) factory.
    • GetSupportedVideoResolutions: return _frontResolutions.Value / _rearResolutions.Value via a switch over CameraDevice; the default arm keeps the original Debug.Fail + direct-load fallback.

Mode

Refactoring

Verification

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

Fixes #7641

…hread-safe via Lazy<T>

Convert the inline-assigned static _frontResolutions/_rearResolutions
caches to Lazy<IEnumerable<Size>>. The previous read/null-check/assign
pattern was non-atomic, so two concurrent callers could each run
LoadVideoResolutions and open a native Camera handle twice. Lazy<T>'s
default ExecutionAndPublication mode guarantees a single execution.

Public API signature and success-path behavior are unchanged.

Fixes #7641

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 refactors the lazy initialization of camera resolutions in VideoRecorder.Capabilities.cs to use Lazy<IEnumerable> and simplifies the retrieval logic with a switch statement. However, using Lazy can permanently cache transient exceptions thrown during camera initialization (e.g., if the camera is temporarily busy). It is recommended to use a thread-safe, lock-based initialization pattern instead, ensuring that transient failures do not permanently poison the cache and subsequent attempts can retry the initialization.

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 +29 to +33
private static readonly Lazy<IEnumerable<Size>> _frontResolutions =
new Lazy<IEnumerable<Size>>(() => LoadVideoResolutions(CameraDevice.Front));

private static readonly Lazy<IEnumerable<Size>> _rearResolutions =
new Lazy<IEnumerable<Size>>(() => LoadVideoResolutions(CameraDevice.Rear));

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 Lazy<T> with the default constructor (which uses LazyThreadSafetyMode.ExecutionAndPublication) will cache any exception thrown during initialization.

Since LoadVideoResolutions instantiates a Camera object, it can throw transient exceptions (e.g., if the camera is temporarily busy or in use by another process). If an exception is thrown, the Lazy<T> instance will permanently cache that exception, causing all subsequent calls to GetSupportedVideoResolutions to fail even if the camera becomes available later.

To prevent transient failures from permanently poisoning the cache, we should use a lock-based initialization pattern instead of Lazy<T>.

        private static IEnumerable<Size> _frontResolutions;
        private static readonly object _frontLock = new object();

        private static IEnumerable<Size> _rearResolutions;
        private static readonly object _rearLock = new object();

Comment on lines +81 to 90
switch (device)
{
return _rearResolutions ?? (_rearResolutions = LoadVideoResolutions(CameraDevice.Rear));
case CameraDevice.Front:
return _frontResolutions.Value;
case CameraDevice.Rear:
return _rearResolutions.Value;
default:
Debug.Fail($"No cache for {device}.");
return LoadVideoResolutions(device);
}

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

Update the resolution retrieval logic to use a simple lock-based initialization. This ensures thread safety and guarantees that if LoadVideoResolutions throws a transient exception, the fields remain null so that subsequent calls can retry the initialization.

            switch (device)
            {
                case CameraDevice.Front:
                    lock (_frontLock)
                    {
                        return _frontResolutions ?? (_frontResolutions = LoadVideoResolutions(CameraDevice.Front));
                    }
                case CameraDevice.Rear:
                    lock (_rearLock)
                    {
                        return _rearResolutions ?? (_rearResolutions = LoadVideoResolutions(CameraDevice.Rear));
                    }
                default:
                    Debug.Fail($"No cache for {device}.");
                    return LoadVideoResolutions(device);
            }

@JoonghyunCho

Copy link
Copy Markdown
Member Author

🤖 [AI Review]

Reviewed — no findings.

Scope checked:

  • Confirmed Lazy<IEnumerable<Size>> with the default ExecutionAndPublication mode makes GetSupportedVideoResolutions thread-safe by guaranteeing a single factory execution per device.
  • Verified the new switch preserves the original Front/Rear branch behavior, including the default arm Debug.Fail plus the uncached LoadVideoResolutions(device) fallback.
  • Confirmed the public GetSupportedVideoResolutions(CameraDevice) signature is unchanged, so there is no API or documentation impact.
  • Checked that the dominant failure path (NotSupportedException for an unsupported feature) is deterministic, so the default Lazy<T> exception caching is acceptable here.

No 🔴 critical issues, no 🟡 suggestions to flag.


Automated review — final merge decision rests with human reviewers.

Replace Lazy<T> resolution cache with lock-based lazy initialization so a
transient failure in LoadVideoResolutions (e.g. camera busy) is not permanently
cached and subsequent calls can retry.

Applied-Human-Comments: 3448300022,3448300024
@JoonghyunCho

Copy link
Copy Markdown
Member Author

🤖 [AI Review]
Addressed review feedback in commit 2921234. Summary: replaced the Lazy<T> resolution cache with lock-based lazy initialization so a transient failure in LoadVideoResolutions (e.g. camera busy) is no longer permanently cached, allowing subsequent calls to retry.

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

Projects

None yet

1 participant