Address review comments

Clean up style
Fix references in class summaries
Combine Where+FirstOrDefault queries
Break up large method, long lines
Add validation on file extension
Apply test naming conventions
Extract mock of Movie class, comment on why not mocking interface

Co-authored-by: Cody Robibero <cody@robibe.ro>
Co-authored-by: Claus Vium <cvium@users.noreply.github.com>

MediaBrowser.MediaEncoding/Encoder/MediaEncoder.cs

@@ -548,6 +548,15 @@ namespace MediaBrowser.MediaEncoding.Encoder
                 throw new ArgumentNullException(nameof(inputPath));
             }
 
+            if (string.IsNullOrEmpty(outputExtension))
+            {
+                outputExtension = ".jpg";
+            }
+            else if (outputExtension[0] != '.')
+            {
+                outputExtension = "." + outputExtension;
+            }
+
             var tempExtractPath = Path.Combine(_configurationManager.ApplicationPaths.TempDirectory, Guid.NewGuid() + outputExtension);
             Directory.CreateDirectory(Path.GetDirectoryName(tempExtractPath));
 

MediaBrowser.MediaEncoding/Probing/ProbeResultNormalizer.cs

@@ -582,8 +582,8 @@ namespace MediaBrowser.MediaEncoding.Probing
         /// <returns>MediaAttachments.</returns>
         private MediaAttachment GetMediaAttachment(MediaStreamInfo streamInfo)
         {
-            if (!string.Equals(streamInfo.CodecType, "attachment", StringComparison.OrdinalIgnoreCase) &&
-                !(streamInfo.Disposition != null && streamInfo.Disposition.GetValueOrDefault("attached_pic") == 1))
+            if (!string.Equals(streamInfo.CodecType, "attachment", StringComparison.OrdinalIgnoreCase)
+                && streamInfo.Disposition?.GetValueOrDefault("attached_pic") != 1)
             {
                 return null;
             }

MediaBrowser.Providers/MediaInfo/EmbeddedImageProvider.cs

@@ -1,5 +1,3 @@
-#pragma warning disable CS1591
-
 using System;
 using System.Collections.Generic;
 using System.IO;
@@ -19,7 +17,7 @@ using MediaBrowser.Model.Net;
 namespace MediaBrowser.Providers.MediaInfo
 {
     /// <summary>
-    /// Uses ffmpeg to extract embedded images.
+    /// Uses <see cref="IMediaEncoder"/> to extract embedded images.
     /// </summary>
     public class EmbeddedImageProvider : IDynamicImageProvider, IHasOrder
     {
@@ -46,6 +44,10 @@ namespace MediaBrowser.Providers.MediaInfo
 
         private readonly IMediaEncoder _mediaEncoder;
 
+        /// <summary>
+        /// Initializes a new instance of the <see cref="EmbeddedImageProvider"/> class.
+        /// </summary>
+        /// <param name="mediaEncoder">The media encoder for extracting attached/embedded images.</param>
         public EmbeddedImageProvider(IMediaEncoder mediaEncoder)
         {
             _mediaEncoder = mediaEncoder;
@@ -65,13 +67,13 @@ namespace MediaBrowser.Providers.MediaInfo
             {
                 if (item is Episode)
                 {
-                    return new List<ImageType>
+                    return new[]
                     {
                         ImageType.Primary,
                     };
                 }
 
-                return new List<ImageType>
+                return new[]
                 {
                     ImageType.Primary,
                     ImageType.Backdrop,
@@ -79,7 +81,7 @@ namespace MediaBrowser.Providers.MediaInfo
                 };
             }
 
-            return new List<ImageType>();
+            return Array.Empty<ImageType>();
         }
 
         /// <inheritdoc />
@@ -114,47 +116,20 @@ namespace MediaBrowser.Providers.MediaInfo
             };
 
             // Try attachments first
-            var attachmentSources = item.GetMediaSources(false).SelectMany(source => source.MediaAttachments).ToList();
-            var attachmentStream = attachmentSources
-                .Where(attachment => !string.IsNullOrEmpty(attachment.FileName))
-                .FirstOrDefault(attachment => imageFileNames.Any(name => attachment.FileName.Contains(name, StringComparison.OrdinalIgnoreCase)));
+            var attachmentStream = item.GetMediaSources(false)
+                .SelectMany(source => source.MediaAttachments)
+                .FirstOrDefault(attachment => !string.IsNullOrEmpty(attachment.FileName)
+                    && imageFileNames.Any(name => attachment.FileName.Contains(name, StringComparison.OrdinalIgnoreCase)));
 
             if (attachmentStream != null)
             {
-                var extension = string.IsNullOrEmpty(attachmentStream.MimeType) ?
-                    Path.GetExtension(attachmentStream.FileName) :
-                    MimeTypes.ToExtension(attachmentStream.MimeType);
-
-                if (string.IsNullOrEmpty(extension))
-                {
-                    extension = ".jpg";
-                }
-
-                string extractedAttachmentPath = await _mediaEncoder.ExtractVideoImage(item.Path, item.Container, mediaSource, null, attachmentStream.Index, extension, cancellationToken).ConfigureAwait(false);
-
-                ImageFormat format = extension switch
-                {
-                    ".bmp" => ImageFormat.Bmp,
-                    ".gif" => ImageFormat.Gif,
-                    ".jpg" => ImageFormat.Jpg,
-                    ".png" => ImageFormat.Png,
-                    ".webp" => ImageFormat.Webp,
-                    _ => ImageFormat.Jpg
-                };
-
-                return new DynamicImageResponse
-                {
-                    Format = format,
-                    HasImage = true,
-                    Path = extractedAttachmentPath,
-                    Protocol = MediaProtocol.File
-                };
+                return await ExtractAttachment(item, cancellationToken, attachmentStream, mediaSource);
             }
 
             // Fall back to EmbeddedImage streams
             var imageStreams = item.GetMediaStreams().FindAll(i => i.Type == MediaStreamType.EmbeddedImage);
 
-            if (!imageStreams.Any())
+            if (imageStreams.Count == 0)
             {
                 // Can't extract if we don't have any EmbeddedImage streams
                 return new DynamicImageResponse { HasImage = false };
@@ -162,8 +137,8 @@ namespace MediaBrowser.Providers.MediaInfo
 
             // Extract first stream containing an element of imageFileNames
             var imageStream = imageStreams
-                .Where(stream => !string.IsNullOrEmpty(stream.Comment))
-                .FirstOrDefault(stream => imageFileNames.Any(name => stream.Comment.Contains(name, StringComparison.OrdinalIgnoreCase)));
+                .FirstOrDefault(stream => !string.IsNullOrEmpty(stream.Comment)
+                    && imageFileNames.Any(name => stream.Comment.Contains(name, StringComparison.OrdinalIgnoreCase)));
 
             // Primary type only: default to first image if none found by label
             if (imageStream == null)
@@ -179,7 +154,9 @@ namespace MediaBrowser.Providers.MediaInfo
                 }
             }
 
-            string extractedImagePath = await _mediaEncoder.ExtractVideoImage(item.Path, item.Container, mediaSource, imageStream, imageStream.Index, "jpg", cancellationToken).ConfigureAwait(false);
+            string extractedImagePath =
+                await _mediaEncoder.ExtractVideoImage(item.Path, item.Container, mediaSource, imageStream, imageStream.Index, ".jpg", cancellationToken)
+                    .ConfigureAwait(false);
 
             return new DynamicImageResponse
             {
@@ -190,6 +167,40 @@ namespace MediaBrowser.Providers.MediaInfo
             };
         }
 
+        private async Task<DynamicImageResponse> ExtractAttachment(Video item, CancellationToken cancellationToken, MediaAttachment attachmentStream, MediaSourceInfo mediaSource)
+        {
+            var extension = string.IsNullOrEmpty(attachmentStream.MimeType)
+                ? Path.GetExtension(attachmentStream.FileName)
+                : MimeTypes.ToExtension(attachmentStream.MimeType);
+
+            if (string.IsNullOrEmpty(extension))
+            {
+                extension = ".jpg";
+            }
+
+            string extractedAttachmentPath =
+                await _mediaEncoder.ExtractVideoImage(item.Path, item.Container, mediaSource, null, attachmentStream.Index, extension, cancellationToken)
+                    .ConfigureAwait(false);
+
+            ImageFormat format = extension switch
+            {
+                ".bmp" => ImageFormat.Bmp,
+                ".gif" => ImageFormat.Gif,
+                ".jpg" => ImageFormat.Jpg,
+                ".png" => ImageFormat.Png,
+                ".webp" => ImageFormat.Webp,
+                _ => ImageFormat.Jpg
+            };
+
+            return new DynamicImageResponse
+            {
+                Format = format,
+                HasImage = true,
+                Path = extractedAttachmentPath,
+                Protocol = MediaProtocol.File
+            };
+        }
+
         /// <inheritdoc />
         public bool Supports(BaseItem item)
         {

MediaBrowser.Providers/MediaInfo/VideoImageProvider.cs

@@ -1,6 +1,3 @@
-#nullable enable
-#pragma warning disable CS1591
-
 using System;
 using System.Collections.Generic;
 using System.Linq;
@@ -18,13 +15,18 @@ using Microsoft.Extensions.Logging;
 namespace MediaBrowser.Providers.MediaInfo
 {
     /// <summary>
-    /// Uses ffmpeg to create still images from the main video.
+    /// Uses <see cref="IMediaEncoder"/> to create still images from the main video.
     /// </summary>
     public class VideoImageProvider : IDynamicImageProvider, IHasOrder
     {
         private readonly IMediaEncoder _mediaEncoder;
         private readonly ILogger<VideoImageProvider> _logger;
 
+        /// <summary>
+        /// Initializes a new instance of the <see cref="VideoImageProvider"/> class.
+        /// </summary>
+        /// <param name="mediaEncoder">The media encoder for capturing images.</param>
+        /// <param name="logger">The logger.</param>
         public VideoImageProvider(IMediaEncoder mediaEncoder, ILogger<VideoImageProvider> logger)
         {
             _mediaEncoder = mediaEncoder;

tests/Jellyfin.Providers.Tests/MediaInfo/EmbeddedImageProviderTests.cs

@@ -17,38 +17,37 @@ namespace Jellyfin.Providers.Tests.MediaInfo
 {
     public class EmbeddedImageProviderTests
     {
-        public static TheoryData<BaseItem> GetSupportedImages_Empty_TestData =>
-            new ()
+        private static TheoryData<BaseItem> GetSupportedImages_UnsupportedBaseItems_ReturnsEmpty_TestData()
+        {
+            return new ()
             {
                 new AudioBook(),
                 new BoxSet(),
                 new Series(),
                 new Season(),
             };
-
-        public static TheoryData<BaseItem, IEnumerable<ImageType>> GetSupportedImages_Populated_TestData =>
-            new TheoryData<BaseItem, IEnumerable<ImageType>>
-            {
-                { new Episode(), new List<ImageType> { ImageType.Primary } },
-                { new Movie(), new List<ImageType> { ImageType.Logo, ImageType.Backdrop, ImageType.Primary } },
-            };
-
-        private EmbeddedImageProvider GetEmbeddedImageProvider(IMediaEncoder? mediaEncoder)
-        {
-            return new EmbeddedImageProvider(mediaEncoder);
         }
 
         [Theory]
-        [MemberData(nameof(GetSupportedImages_Empty_TestData))]
-        public void GetSupportedImages_Empty(BaseItem item)
+        [MemberData(nameof(GetSupportedImages_UnsupportedBaseItems_ReturnsEmpty_TestData))]
+        public void GetSupportedImages_UnsupportedBaseItems_ReturnsEmpty(BaseItem item)
         {
             var embeddedImageProvider = GetEmbeddedImageProvider(null);
-            Assert.False(embeddedImageProvider.GetSupportedImages(item).Any());
+            Assert.Empty(embeddedImageProvider.GetSupportedImages(item));
+        }
+
+        private static TheoryData<BaseItem, IEnumerable<ImageType>> GetSupportedImages_SupportedBaseItems_ReturnsPopulated_TestData()
+        {
+            return new TheoryData<BaseItem, IEnumerable<ImageType>>
+            {
+                { new Episode(), new List<ImageType> { ImageType.Primary } },
+                { new Movie(), new List<ImageType> { ImageType.Logo, ImageType.Backdrop, ImageType.Primary } },
+            };
         }
 
         [Theory]
-        [MemberData(nameof(GetSupportedImages_Populated_TestData))]
-        public void GetSupportedImages_Populated(BaseItem item, IEnumerable<ImageType> expected)
+        [MemberData(nameof(GetSupportedImages_SupportedBaseItems_ReturnsPopulated_TestData))]
+        public void GetSupportedImages_SupportedBaseItems_ReturnsPopulated(BaseItem item, IEnumerable<ImageType> expected)
         {
             var embeddedImageProvider = GetEmbeddedImageProvider(null);
             var actual = embeddedImageProvider.GetSupportedImages(item);
@@ -56,56 +55,34 @@ namespace Jellyfin.Providers.Tests.MediaInfo
         }
 
         [Fact]
-        public async void GetImage_Empty_NoStreams()
+        public async void GetImage_InputWithNoStreams_ReturnsNoImage()
         {
             var embeddedImageProvider = GetEmbeddedImageProvider(null);
 
-            var input = new Mock<Movie>();
-            input.Setup(movie => movie.GetMediaSources(It.IsAny<bool>()))
-                .Returns(new List<MediaSourceInfo>());
-            input.Setup(movie => movie.GetMediaStreams())
-                .Returns(new List<MediaStream>());
+            var input = GetMovie(new List<MediaAttachment>(), new List<MediaStream>());
 
-            var actual = await embeddedImageProvider.GetImage(input.Object, ImageType.Primary, CancellationToken.None);
+            var actual = await embeddedImageProvider.GetImage(input, ImageType.Primary, CancellationToken.None);
             Assert.NotNull(actual);
             Assert.False(actual.HasImage);
         }
 
         [Fact]
-        public async void GetImage_Empty_NoLabeledAttachments()
+        public async void GetImage_InputWithUnlabeledAttachments_ReturnsNoImage()
         {
             var embeddedImageProvider = GetEmbeddedImageProvider(null);
 
-            var input = new Mock<Movie>();
             // add an attachment without a filename - has a list to look through but finds nothing
-            input.Setup(movie => movie.GetMediaSources(It.IsAny<bool>()))
-                .Returns(new List<MediaSourceInfo> { new () { MediaAttachments = new List<MediaAttachment> { new () } } });
-            input.Setup(movie => movie.GetMediaStreams())
-                .Returns(new List<MediaStream>());
+            var input = GetMovie(
+                new List<MediaAttachment> { new () },
+                new List<MediaStream>());
 
-            var actual = await embeddedImageProvider.GetImage(input.Object, ImageType.Primary, CancellationToken.None);
+            var actual = await embeddedImageProvider.GetImage(input, ImageType.Primary, CancellationToken.None);
             Assert.NotNull(actual);
             Assert.False(actual.HasImage);
         }
 
         [Fact]
-        public async void GetImage_Empty_NoEmbeddedLabeledBackdrop()
-        {
-            var embeddedImageProvider = GetEmbeddedImageProvider(null);
-
-            var input = new Mock<Movie>();
-            input.Setup(movie => movie.GetMediaSources(It.IsAny<bool>()))
-                .Returns(new List<MediaSourceInfo>());
-            input.Setup(movie => movie.GetMediaStreams())
-                .Returns(new List<MediaStream> { new () { Type = MediaStreamType.EmbeddedImage } });
-
-            var actual = await embeddedImageProvider.GetImage(input.Object, ImageType.Backdrop, CancellationToken.None);
-            Assert.NotNull(actual);
-            Assert.False(actual.HasImage);
-        }
-
-        [Fact]
-        public async void GetImage_Attached()
+        public async void GetImage_InputWithLabeledAttachments_ReturnsCorrectSelection()
         {
             // first tests file extension detection, second uses mimetype, third defaults to jpg
             MediaAttachment sampleAttachment1 = new () { FileName = "clearlogo.png", Index = 1 };
@@ -124,25 +101,23 @@ namespace Jellyfin.Providers.Tests.MediaInfo
                 .Returns(Task.FromResult(targetPath3));
             var embeddedImageProvider = GetEmbeddedImageProvider(mediaEncoder.Object);
 
-            var input = new Mock<Movie>();
-            input.Setup(movie => movie.GetMediaSources(It.IsAny<bool>()))
-                .Returns(new List<MediaSourceInfo> { new () { MediaAttachments = new List<MediaAttachment> { sampleAttachment1, sampleAttachment2, sampleAttachment3 } } });
-            input.Setup(movie => movie.GetMediaStreams())
-                .Returns(new List<MediaStream>());
+            var input = GetMovie(
+                new List<MediaAttachment> { sampleAttachment1, sampleAttachment2, sampleAttachment3 },
+                new List<MediaStream>());
 
-            var actualLogo = await embeddedImageProvider.GetImage(input.Object, ImageType.Logo, CancellationToken.None);
+            var actualLogo = await embeddedImageProvider.GetImage(input, ImageType.Logo, CancellationToken.None);
             Assert.NotNull(actualLogo);
             Assert.True(actualLogo.HasImage);
             Assert.Equal(targetPath1, actualLogo.Path);
             Assert.Equal(ImageFormat.Png, actualLogo.Format);
 
-            var actualBackdrop = await embeddedImageProvider.GetImage(input.Object, ImageType.Backdrop, CancellationToken.None);
+            var actualBackdrop = await embeddedImageProvider.GetImage(input, ImageType.Backdrop, CancellationToken.None);
             Assert.NotNull(actualBackdrop);
             Assert.True(actualBackdrop.HasImage);
             Assert.Equal(targetPath2, actualBackdrop.Path);
             Assert.Equal(ImageFormat.Bmp, actualBackdrop.Format);
 
-            var actualPrimary = await embeddedImageProvider.GetImage(input.Object, ImageType.Primary, CancellationToken.None);
+            var actualPrimary = await embeddedImageProvider.GetImage(input, ImageType.Primary, CancellationToken.None);
             Assert.NotNull(actualPrimary);
             Assert.True(actualPrimary.HasImage);
             Assert.Equal(targetPath3, actualPrimary.Path);
@@ -150,23 +125,35 @@ namespace Jellyfin.Providers.Tests.MediaInfo
         }
 
         [Fact]
-        public async void GetImage_EmbeddedDefault()
+        public async void GetImage_InputWithUnlabeledEmbeddedImages_BackdropReturnsNoImage()
+        {
+            var embeddedImageProvider = GetEmbeddedImageProvider(null);
+
+            var input = GetMovie(
+                new List<MediaAttachment>(),
+                new List<MediaStream> { new () { Type = MediaStreamType.EmbeddedImage } });
+
+            var actual = await embeddedImageProvider.GetImage(input, ImageType.Backdrop, CancellationToken.None);
+            Assert.NotNull(actual);
+            Assert.False(actual.HasImage);
+        }
+
+        [Fact]
+        public async void GetImage_InputWithUnlabeledEmbeddedImages_PrimaryReturnsImage()
         {
             MediaStream sampleStream = new () { Type = MediaStreamType.EmbeddedImage, Index = 1 };
             string targetPath = "path";
 
             var mediaEncoder = new Mock<IMediaEncoder>(MockBehavior.Strict);
-            mediaEncoder.Setup(encoder => encoder.ExtractVideoImage(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<MediaSourceInfo>(), sampleStream, 1, "jpg", CancellationToken.None))
+            mediaEncoder.Setup(encoder => encoder.ExtractVideoImage(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<MediaSourceInfo>(), sampleStream, 1, ".jpg", CancellationToken.None))
                 .Returns(Task.FromResult(targetPath));
             var embeddedImageProvider = GetEmbeddedImageProvider(mediaEncoder.Object);
 
-            var input = new Mock<Movie>();
-            input.Setup(movie => movie.GetMediaSources(It.IsAny<bool>()))
-                .Returns(new List<MediaSourceInfo>());
-            input.Setup(movie => movie.GetMediaStreams())
-                .Returns(new List<MediaStream>() { sampleStream });
+            var input = GetMovie(
+                new List<MediaAttachment>(),
+                new List<MediaStream> { sampleStream });
 
-            var actual = await embeddedImageProvider.GetImage(input.Object, ImageType.Primary, CancellationToken.None);
+            var actual = await embeddedImageProvider.GetImage(input, ImageType.Primary, CancellationToken.None);
             Assert.NotNull(actual);
             Assert.True(actual.HasImage);
             Assert.Equal(targetPath, actual.Path);
@@ -174,7 +161,7 @@ namespace Jellyfin.Providers.Tests.MediaInfo
         }
 
         [Fact]
-        public async void GetImage_EmbeddedSelection()
+        public async void GetImage_InputWithLabeledEmbeddedImages_ReturnsCorrectSelection()
         {
             // primary is second stream to ensure it's not defaulting, backdrop is first
             MediaStream sampleStream1 = new () { Type = MediaStreamType.EmbeddedImage, Index = 1, Comment = "backdrop" };
@@ -183,29 +170,47 @@ namespace Jellyfin.Providers.Tests.MediaInfo
             string targetPath2 = "path2.jpg";
 
             var mediaEncoder = new Mock<IMediaEncoder>(MockBehavior.Strict);
-            mediaEncoder.Setup(encoder => encoder.ExtractVideoImage(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<MediaSourceInfo>(), sampleStream1, 1, "jpg", CancellationToken.None))
+            mediaEncoder.Setup(encoder => encoder.ExtractVideoImage(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<MediaSourceInfo>(), sampleStream1, 1, ".jpg", CancellationToken.None))
                 .Returns(Task.FromResult(targetPath1));
-            mediaEncoder.Setup(encoder => encoder.ExtractVideoImage(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<MediaSourceInfo>(), sampleStream2, 2, "jpg", CancellationToken.None))
+            mediaEncoder.Setup(encoder => encoder.ExtractVideoImage(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<MediaSourceInfo>(), sampleStream2, 2, ".jpg", CancellationToken.None))
                 .Returns(Task.FromResult(targetPath2));
             var embeddedImageProvider = GetEmbeddedImageProvider(mediaEncoder.Object);
 
-            var input = new Mock<Movie>();
-            input.Setup(movie => movie.GetMediaSources(It.IsAny<bool>()))
-                .Returns(new List<MediaSourceInfo>());
-            input.Setup(movie => movie.GetMediaStreams())
-                .Returns(new List<MediaStream> { sampleStream1, sampleStream2 });
+            var input = GetMovie(
+                new List<MediaAttachment>(),
+                new List<MediaStream> { sampleStream1, sampleStream2 });
 
-            var actualPrimary = await embeddedImageProvider.GetImage(input.Object, ImageType.Primary, CancellationToken.None);
+            var actualPrimary = await embeddedImageProvider.GetImage(input, ImageType.Primary, CancellationToken.None);
             Assert.NotNull(actualPrimary);
             Assert.True(actualPrimary.HasImage);
             Assert.Equal(targetPath2, actualPrimary.Path);
             Assert.Equal(ImageFormat.Jpg, actualPrimary.Format);
 
-            var actualBackdrop = await embeddedImageProvider.GetImage(input.Object, ImageType.Backdrop, CancellationToken.None);
+            var actualBackdrop = await embeddedImageProvider.GetImage(input, ImageType.Backdrop, CancellationToken.None);
             Assert.NotNull(actualBackdrop);
             Assert.True(actualBackdrop.HasImage);
             Assert.Equal(targetPath1, actualBackdrop.Path);
             Assert.Equal(ImageFormat.Jpg, actualBackdrop.Format);
         }
+
+        private static EmbeddedImageProvider GetEmbeddedImageProvider(IMediaEncoder? mediaEncoder)
+        {
+            return new EmbeddedImageProvider(mediaEncoder);
+        }
+
+        private static Movie GetMovie(List<MediaAttachment> mediaAttachments, List<MediaStream> mediaStreams)
+        {
+            // Mocking IMediaSourceManager GetMediaAttachments and GetMediaStreams instead of mocking Movie works, but
+            // has concurrency problems between this and VideoImageProviderTests due to BaseItem.MediaSourceManager
+            // being static
+            var movie = new Mock<Movie>();
+
+            movie.Setup(item => item.GetMediaSources(It.IsAny<bool>()))
+                .Returns(new List<MediaSourceInfo> { new () { MediaAttachments = mediaAttachments } } );
+            movie.Setup(item => item.GetMediaStreams())
+                .Returns(mediaStreams);
+
+            return movie.Object;
+        }
     }
 }

tests/Jellyfin.Providers.Tests/MediaInfo/VideoImageProviderTests.cs

@@ -16,56 +16,51 @@ namespace Jellyfin.Providers.Tests.MediaInfo
 {
     public class VideoImageProviderTests
     {
-        private VideoImageProvider GetVideoImageProvider(IMediaEncoder? mediaEncoder)
-        {
-            // strict to ensure this isn't accidentally used where a prepared mock is intended
-            mediaEncoder ??= new Mock<IMediaEncoder>(MockBehavior.Strict).Object;
-            return new VideoImageProvider(mediaEncoder, new NullLogger<VideoImageProvider>());
-        }
-
         [Fact]
-        public async void GetImage_Empty_IsPlaceholder()
+        public async void GetImage_InputIsPlaceholder_ReturnsNoImage()
         {
             var videoImageProvider = GetVideoImageProvider(null);
 
-            var input = new Mock<Movie>();
-            input.Object.IsPlaceHolder = true;
+            var input = new Movie
+            {
+                IsPlaceHolder = true
+            };
 
-            var actual = await videoImageProvider.GetImage(input.Object, ImageType.Primary, CancellationToken.None);
+            var actual = await videoImageProvider.GetImage(input, ImageType.Primary, CancellationToken.None);
             Assert.NotNull(actual);
             Assert.False(actual.HasImage);
         }
 
         [Fact]
-        public async void GetImage_Empty_NoDefaultVideoStream()
+        public async void GetImage_NoDefaultVideoStream_ReturnsNoImage()
         {
             var videoImageProvider = GetVideoImageProvider(null);
 
-            var input = new Mock<Movie>();
+            var input = new Movie
+            {
+                DefaultVideoStreamIndex = null
+            };
 
-            var actual = await videoImageProvider.GetImage(input.Object, ImageType.Primary, CancellationToken.None);
+            var actual = await videoImageProvider.GetImage(input, ImageType.Primary, CancellationToken.None);
             Assert.NotNull(actual);
             Assert.False(actual.HasImage);
         }
 
         [Fact]
-        public async void GetImage_Empty_DefaultSet_NoVideoStream()
+        public async void GetImage_DefaultSetButNoVideoStream_ReturnsNoImage()
         {
             var videoImageProvider = GetVideoImageProvider(null);
 
-            var input = new Mock<Movie>();
-            input.Setup(movie => movie.GetMediaStreams())
-                .Returns(new List<MediaStream>());
             // set a default index but don't put anything there (invalid input, but provider shouldn't break)
-            input.Object.DefaultVideoStreamIndex = 1;
+            var input = GetMovie(0, null, new List<MediaStream>());
 
-            var actual = await videoImageProvider.GetImage(input.Object, ImageType.Primary, CancellationToken.None);
+            var actual = await videoImageProvider.GetImage(input, ImageType.Primary, CancellationToken.None);
             Assert.NotNull(actual);
             Assert.False(actual.HasImage);
         }
 
         [Fact]
-        public async void GetImage_Extract_DefaultStream()
+        public async void GetImage_DefaultSetMultipleVideoStreams_ReturnsDefaultStreamImage()
         {
             MediaStream firstStream = new () { Type = MediaStreamType.Video, Index = 0 };
             MediaStream targetStream = new () { Type = MediaStreamType.Video, Index = 1 };
@@ -78,14 +73,9 @@ namespace Jellyfin.Providers.Tests.MediaInfo
                 .Returns(Task.FromResult(targetPath));
             var videoImageProvider = GetVideoImageProvider(mediaEncoder.Object);
 
-            var input = new Mock<Movie>();
-            input.Setup(movie => movie.GetDefaultVideoStream())
-                .Returns(targetStream);
-            input.Setup(movie => movie.GetMediaStreams())
-                .Returns(new List<MediaStream>() { firstStream, targetStream });
-            input.Object.DefaultVideoStreamIndex = 1;
+            var input = GetMovie(1, targetStream, new List<MediaStream> { firstStream, targetStream } );
 
-            var actual = await videoImageProvider.GetImage(input.Object, ImageType.Primary, CancellationToken.None);
+            var actual = await videoImageProvider.GetImage(input, ImageType.Primary, CancellationToken.None);
             Assert.NotNull(actual);
             Assert.True(actual.HasImage);
             Assert.Equal(targetPath, actual.Path);
@@ -93,7 +83,7 @@ namespace Jellyfin.Providers.Tests.MediaInfo
         }
 
         [Fact]
-        public async void GetImage_Extract_FallbackToFirstVideoStream()
+        public async void GetImage_InvalidDefaultSingleVideoStream_ReturnsFirstVideoStreamImage()
         {
             MediaStream targetStream = new () { Type = MediaStreamType.Video, Index = 0 };
             string targetPath = "path.jpg";
@@ -103,13 +93,10 @@ namespace Jellyfin.Providers.Tests.MediaInfo
                 .Returns(Task.FromResult(targetPath));
             var videoImageProvider = GetVideoImageProvider(mediaEncoder.Object);
 
-            var input = new Mock<Movie>();
-            input.Setup(movie => movie.GetMediaStreams())
-                .Returns(new List<MediaStream>() { targetStream });
-            // default must be set, ensure a stream is still found if not pointed at a video
-            input.Object.DefaultVideoStreamIndex = 5;
+            // provide query results for default (empty) and all streams (populated)
+            var input = GetMovie(5, null, new List<MediaStream> { targetStream });
 
-            var actual = await videoImageProvider.GetImage(input.Object, ImageType.Primary, CancellationToken.None);
+            var actual = await videoImageProvider.GetImage(input, ImageType.Primary, CancellationToken.None);
             Assert.NotNull(actual);
             Assert.True(actual.HasImage);
             Assert.Equal(targetPath, actual.Path);
@@ -117,10 +104,12 @@ namespace Jellyfin.Providers.Tests.MediaInfo
         }
 
         [Fact]
-        public async void GetImage_Time_Default()
+        public async void GetImage_NoTimeSpanSet_CallsEncoderWithDefaultTime()
         {
             MediaStream targetStream = new () { Type = MediaStreamType.Video, Index = 0 };
 
+            // use a callback to catch the actual value
+            // provides more information on failure than verifying a specific input was called on the mock
             TimeSpan? actualTimeSpan = null;
             var mediaEncoder = new Mock<IMediaEncoder>(MockBehavior.Strict);
             mediaEncoder.Setup(encoder => encoder.ExtractVideoImage(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<MediaSourceInfo>(), It.IsAny<MediaStream>(), It.IsAny<Video3DFormat?>(), It.IsAny<TimeSpan?>(), CancellationToken.None))
@@ -128,20 +117,16 @@ namespace Jellyfin.Providers.Tests.MediaInfo
                 .Returns(Task.FromResult("path"));
             var videoImageProvider = GetVideoImageProvider(mediaEncoder.Object);
 
-            var input = new Mock<Movie>();
-            input.Setup(movie => movie.GetMediaStreams())
-                .Returns(new List<MediaStream>() { targetStream });
-            // default must be set
-            input.Object.DefaultVideoStreamIndex = 0;
+            var input = GetMovie(0, targetStream, new List<MediaStream> { targetStream });
 
             // not testing return, just verifying what gets requested for time span
-            await videoImageProvider.GetImage(input.Object, ImageType.Primary, CancellationToken.None);
+            await videoImageProvider.GetImage(input, ImageType.Primary, CancellationToken.None);
 
             Assert.Equal(TimeSpan.FromSeconds(10), actualTimeSpan);
         }
 
         [Fact]
-        public async void GetImage_Time_Calculated()
+        public async void GetImage_TimeSpanSet_CallsEncoderWithCalculatedTime()
         {
             MediaStream targetStream = new () { Type = MediaStreamType.Video, Index = 0 };
 
@@ -152,17 +137,40 @@ namespace Jellyfin.Providers.Tests.MediaInfo
                 .Returns(Task.FromResult("path"));
             var videoImageProvider = GetVideoImageProvider(mediaEncoder.Object);
 
-            var input = new Mock<Movie>();
-            input.Setup(movie => movie.GetMediaStreams())
-                .Returns(new List<MediaStream>() { targetStream });
-            // default must be set
-            input.Object.DefaultVideoStreamIndex = 0;
-            input.Object.RunTimeTicks = 5000;
+            var input = GetMovie(0, targetStream, new List<MediaStream> { targetStream });
+            input.RunTimeTicks = 5000;
 
             // not testing return, just verifying what gets requested for time span
-            await videoImageProvider.GetImage(input.Object, ImageType.Primary, CancellationToken.None);
+            await videoImageProvider.GetImage(input, ImageType.Primary, CancellationToken.None);
 
             Assert.Equal(TimeSpan.FromTicks(500), actualTimeSpan);
         }
+
+        private static VideoImageProvider GetVideoImageProvider(IMediaEncoder? mediaEncoder)
+        {
+            // strict to ensure this isn't accidentally used where a prepared mock is intended
+            mediaEncoder ??= new Mock<IMediaEncoder>(MockBehavior.Strict).Object;
+            return new VideoImageProvider(mediaEncoder, new NullLogger<VideoImageProvider>());
+        }
+
+        private static Movie GetMovie(int defaultVideoStreamIndex, MediaStream? defaultStream, List<MediaStream> mediaStreams)
+        {
+            // Mocking IMediaSourceManager GetMediaStreams instead of mocking Movie works, but has concurrency problems
+            // between this and EmbeddedImageProviderTests due to BaseItem.MediaSourceManager being static
+            var movie = new Mock<Movie>
+            {
+                Object =
+                {
+                    DefaultVideoStreamIndex = defaultVideoStreamIndex
+                }
+            };
+
+            movie.Setup(item => item.GetDefaultVideoStream())
+                .Returns(defaultStream!);
+            movie.Setup(item => item.GetMediaStreams())
+                .Returns(mediaStreams);
+
+            return movie.Object;
+        }
     }
 }