Merge pull request #2007 from shimat/fix/window-trackbar-refactor-2005

Replace Window.ShowImages two-array overload with tuples; clean up Window/CvTrackbar

Author: shimat

src/OpenCvSharp/Modules/highgui/CvTrackbar.cs

@@ -1,14 +1,10 @@
-using OpenCvSharp.Internal;
-
 namespace OpenCvSharp;
 
 /// <summary>
 /// Trackbar that is shown on an OpenCV Window
 /// </summary>
 public sealed class CvTrackbar
 {
-    private readonly int result;
-
     #region Properties
 
     /// <summary>
@@ -35,40 +31,24 @@ public int Pos
         set => Cv2.SetTrackbarPos(TrackbarName, WindowName, value);
     }
 
-    /// <summary>
-    /// Result value of cv::createTrackbar
-    /// </summary>
-    public int Result => result;
-
     #endregion
 
     #region Init
 
-    /// <summary>
-    /// Constructor (value=0, max=100)
-    /// </summary>
-    /// <param name="name">Trackbar name</param>
-    /// <param name="window">Window name</param>
-    /// <param name="callback">Callback handler</param>
-    internal CvTrackbar(string name, string window, TrackbarCallback callback)
-        : this(name, window, 0, 100, callback)
-    {
-    }
-
     /// <summary>
     /// Constructor
     /// </summary>
     /// <param name="trackbarName">Trackbar name</param>
     /// <param name="windowName">Window name</param>
+    /// <param name="callback">Callback handler</param>
     /// <param name="initialPos">Initial slider position</param>
     /// <param name="max">The upper limit of the range this trackbar is working with. </param>
-    /// <param name="callback">Callback handler</param>
-    internal CvTrackbar(string trackbarName, string windowName, int initialPos, int max, TrackbarCallback callback)
+    internal CvTrackbar(string trackbarName, string windowName, TrackbarCallback callback, int initialPos = 0, int max = 100)
     {
         if (string.IsNullOrEmpty(trackbarName))
-            throw new ArgumentNullException(nameof(trackbarName));
+            throw new ArgumentException("Null or empty trackbar name.", nameof(trackbarName));
         if (string.IsNullOrEmpty(windowName))
-            throw new ArgumentNullException(nameof(windowName));
+            throw new ArgumentException("Null or empty window name.", nameof(windowName));
 
         Callback = callback ?? throw new ArgumentNullException(nameof(callback));
         TrackbarName = trackbarName;
@@ -78,13 +58,12 @@ internal CvTrackbar(string trackbarName, string windowName, int initialPos, int
         // Cv2.CreateTrackbar roots the native-shaped delegate for the window's lifetime, so no
         // GCHandle is needed here.
         TrackbarCallbackNative callbackNative = (pos, _) => callback(pos);
-        result = Cv2.CreateTrackbar(trackbarName, windowName, max, callbackNative);
+        var result = Cv2.CreateTrackbar(trackbarName, windowName, max, callbackNative);
+        if (result == 0)
+            throw new OpenCvSharpException("Failed to create CvTrackbar.");
 
         // Set initial trackbar position
         Cv2.SetTrackbarPos(trackbarName, windowName, initialPos);
-
-        if (result == 0)
-            throw new OpenCvSharpException("Failed to create CvTrackbar.");
     }
 
     #endregion
@@ -96,8 +75,7 @@ internal CvTrackbar(string trackbarName, string windowName, int initialPos, int
     /// <param name="maxVal">New maximum position.</param>
     public void SetMax(int maxVal)
     {
-        NativeMethods.HandleException(
-            NativeMethods.highgui_setTrackbarMax(TrackbarName, WindowName, maxVal));
+        Cv2.SetTrackbarMax(TrackbarName, WindowName, maxVal);
     }
 
     /// <summary>
@@ -107,7 +85,6 @@ public void SetMax(int maxVal)
     /// <param name="minVal">New minimum position.</param>
     public void SetMin(int minVal)
     {
-        NativeMethods.HandleException(
-            NativeMethods.highgui_setTrackbarMin(TrackbarName, WindowName, minVal));
+        Cv2.SetTrackbarMin(TrackbarName, WindowName, minVal);
     }
 }

src/OpenCvSharp/Modules/highgui/Window.cs

@@ -1,3 +1,5 @@
+using System.Collections.Concurrent;
+using System.Threading;
 using OpenCvSharp.Internal;
 
 // ReSharper disable UnusedMember.Local
@@ -11,10 +13,10 @@ public sealed class Window : IDisposable
 {
     #region Field
 
-    internal static Dictionary<string, Window> Windows = new();
-    private static uint windowCount;
+    internal static readonly ConcurrentDictionary<string, Window> Windows = new();
+    private static int windowCount;
 
-    private string name;
+    private readonly string name;
     private Mat? image;
     // ReSharper disable once IdentifierTypo
     private readonly Dictionary<string, CvTrackbar> trackbars;
@@ -31,36 +33,6 @@ public Window()
     {
     }
 
-    /// <summary>
-    /// Creates a window
-    /// </summary>
-    /// <param name="name">Name of the window which is used as window identifier and appears in the window caption. </param>
-    public Window(string name)
-        : this(name, null, WindowFlags.AutoSize)
-    {
-    }
-
-    /// <summary>
-    /// Creates a window
-    /// </summary>
-    /// <param name="name">Name of the window which is used as window identifier and appears in the window caption. </param>
-    /// <param name="flags">Flags of the window. Currently the only supported flag is WindowMode.AutoSize.
-    /// If it is set, window size is automatically adjusted to fit the displayed image (see cvShowImage), while user can not change the window size manually. </param>
-    public Window(string name, WindowFlags flags = WindowFlags.AutoSize)
-        : this(name, null, flags)
-    {
-    }
-
-    /// <summary>
-    /// Creates a window
-    /// </summary>
-    /// <param name="name">Name of the window which is used as window identifier and appears in the window caption. </param>
-    /// <param name="image">Image to be shown.</param>
-    public Window(string name, Mat image)
-        : this(name, image ?? throw new ArgumentNullException(nameof(image)), WindowFlags.AutoSize)
-    {
-    }
-
     /// <summary>
     /// Creates a window
     /// </summary>
@@ -82,8 +54,7 @@ public Window(string name, Mat? image = null, WindowFlags flags = WindowFlags.Au
 
         trackbars = new Dictionary<string, CvTrackbar>();
 
-        if (!Windows.ContainsKey(name))
-            Windows.Add(name, this);
+        Windows[name] = this;
     }
 
     /// <summary>
@@ -92,7 +63,7 @@ public Window(string name, Mat? image = null, WindowFlags flags = WindowFlags.Au
     /// <returns></returns>
     private static string DefaultName()
     {
-        return $"window{windowCount++}";
+        return $"window{Interlocked.Increment(ref windowCount) - 1}";
     }
 
     /// <summary>
@@ -109,7 +80,9 @@ public void Dispose()
             return;
         IsDisposed = true;
 
-        Windows.Remove(name);
+        // Only remove the map entry if it still points at this instance: another Window may have
+        // since reused the same name (see the constructor), and that instance's entry must survive.
+        ((ICollection<KeyValuePair<string, Window>>) Windows).Remove(new KeyValuePair<string, Window>(name, this));
         trackbars.Clear();
 
         // Destroying the window also releases OpenCV's references to its mouse/trackbar
@@ -150,11 +123,7 @@ public Mat? Image
     /// <summary>
     /// Gets window name
     /// </summary>
-    public string Name
-    {
-        get => name;
-        private set => name = value;
-    }
+    public string Name => name;
 
     #endregion
 
@@ -169,7 +138,7 @@ public string Name
     public CvTrackbar CreateTrackbar(string trackbarName, TrackbarCallback callback)
     {
         var trackbar = new CvTrackbar(trackbarName, name, callback);
-        trackbars.Add(trackbarName, trackbar);
+        trackbars[trackbarName] = trackbar;
         return trackbar;
     }
 
@@ -183,8 +152,8 @@ public CvTrackbar CreateTrackbar(string trackbarName, TrackbarCallback callback)
     /// <returns></returns>
     public CvTrackbar CreateTrackbar(string trackbarName, int initialPos, int max, TrackbarCallback callback)
     {
-        var trackbar = new CvTrackbar(trackbarName, name, initialPos, max, callback);
-        trackbars.Add(trackbarName, trackbar);
+        var trackbar = new CvTrackbar(trackbarName, name, callback, initialPos, max);
+        trackbars[trackbarName] = trackbar;
         return trackbar;
     }
 
@@ -290,52 +259,36 @@ public static void ShowImages(params Mat[] images)
     {
         if (images is null)
             throw new ArgumentNullException(nameof(images));
-        if (images.Length == 0)
-            return;
 
-        var windows = new List<Window>();
-        foreach (var img in images)
-        {
-            windows.Add(new Window { Image = img });
-        }
-
-        WaitKey();
-
-        foreach (var w in windows)
-        {
-            w.Close();
-        }
+        ShowImagesAndWaitKey(images.Select(img => new Window { Image = img }));
     }
 
     /// <summary>
-    ///
+    /// Shows each of the given (title, image) pairs in its own window.
+    /// Pairing each title with its image in a single tuple makes a length mismatch
+    /// between titles and images structurally impossible.
     /// </summary>
-    /// <param name="images"></param>
-    /// <param name="names"></param>
-    public static void ShowImages(IEnumerable<Mat> images, IEnumerable<string> names)
+    /// <param name="images">Pairs of window title and image to display</param>
+    public static void ShowImages(params (string Title, Mat Image)[] images)
     {
         if (images is null)
             throw new ArgumentNullException(nameof(images));
-        if (names is null)
-            throw new ArgumentNullException(nameof(names));
 
-        var imagesArray = images.ToArray();
-        var namesArray = names.ToArray();
+        ShowImagesAndWaitKey(images.Select(t => new Window(t.Title, image: t.Image)));
+    }
 
-        if (imagesArray.Length == 0)
+    /// <summary>
+    /// Opens the given windows, blocks until a key is pressed, then closes them all.
+    /// </summary>
+    private static void ShowImagesAndWaitKey(IEnumerable<Window> windows)
+    {
+        var windowList = windows.ToList();
+        if (windowList.Count == 0)
             return;
-        if (namesArray.Length < imagesArray.Length)
-            throw new ArgumentException("names.Length < images.Length");
 
-        var windows = new List<Window>();
-        for (var i = 0; i < imagesArray.Length; i++)
-        {
-            windows.Add(new Window(namesArray[i], image: imagesArray[i]));
-        }
-
-        Cv2.WaitKey();
+        WaitKey();
 
-        foreach (var w in windows)
+        foreach (var w in windowList)
         {
             w.Close();
         }

test/OpenCvSharp.Tests/TestBase.cs

@@ -74,11 +74,11 @@ protected static void ShowImagesWhenDebugMode(params Mat[] mats)
         }
     }
 
-    protected static void ShowImagesWhenDebugMode(IEnumerable<Mat> mats, IEnumerable<string> names)
+    protected static void ShowImagesWhenDebugMode(params (string Title, Mat Image)[] images)
     {
         if (Debugger.IsAttached)
         {
-            Window.ShowImages(mats, names);
+            Window.ShowImages(images);
         }
     }
 }

test/OpenCvSharp.Tests/highgui/TrackbarTest.cs

@@ -70,8 +70,8 @@ public void RunTest()
 
         for (;;)
         {
-            openCloseTrackbar.Callback.DynamicInvoke(0, null);
-            erodeDilateTrackbar.Callback.DynamicInvoke(0, null);
+            openCloseTrackbar.Callback(0);
+            erodeDilateTrackbar.Callback(0);
 
             var key = Cv2.WaitKey();
 

test/OpenCvSharp.Tests/imgproc/ImgProcTest.cs

@@ -725,7 +725,7 @@ public void HoughLinesP()
                 Cv2.Line(view, line.P1, line.P2, Scalar.Red);
             }
 
-            Window.ShowImages([src, binary, view], ["src", "binary", "lines"]);
+            Window.ShowImages(("src", src), ("binary", binary), ("lines", view));
         }
     }
 

test/OpenCvSharp.Tests/ximgproc/XimgProcTest.cs

@@ -35,7 +35,7 @@ public void Sauvola()
                 5, 0.5,
                 LocalBinarizationMethods.Sauvola,
                 r);
-            ShowImagesWhenDebugMode([dst], [$"r={r}"]);
+            ShowImagesWhenDebugMode(($"r={r}", dst));
         }
     }